| Message ID | e527be8b871212823ff83f3800b6eecc2a75e455.1693213468.git.maciej.wieczor-retman@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Bug fix and optimizations | expand |
On Mon, 28 Aug 2023, Wieczor-Retman, Maciej wrote: > Resctrlfs.c file contains mostly functions that interact in some way > with resctrl FS entries while functions inside resctrl_val.c deal with > measurements and benchmarking. > > Run_benchmark() function is located in resctrlfs.c file even though it's > purpose is not interacting with the resctrl FS but to execute cache > checking logic. > > Move run_benchmark() to resctrl_val.c just before resctrl_val() function > that makes use of run_benchmark(). > > Remove return comment from kernel-doc since the function is type void. > > Changelog v2: > - Add dots at the end of patch msg sentences. > - Remove "Return: void" from run_benchmark() kernel-doc comment. > > Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Hi Maciej, On 8/28/2023 2:56 AM, Wieczor-Retman, Maciej wrote: > Resctrlfs.c file contains mostly functions that interact in some way > with resctrl FS entries while functions inside resctrl_val.c deal with > measurements and benchmarking. > > Run_benchmark() function is located in resctrlfs.c file even though it's > purpose is not interacting with the resctrl FS but to execute cache > checking logic. It looks like your editor may be automatically capitalize first words of sentences like Resctrlfs.c and Run_benchmark() above. Also please note that when using () to indicate a function it is not necessary to say it is a function. For example above can just be: "run_benchmark() is located ..." ... similarly you can just say "resctrlfs.c contains ...". > > Move run_benchmark() to resctrl_val.c just before resctrl_val() function > that makes use of run_benchmark(). > > Remove return comment from kernel-doc since the function is type void. > > Changelog v2: > - Add dots at the end of patch msg sentences. > - Remove "Return: void" from run_benchmark() kernel-doc comment. > same comment about changelog. > Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com> > --- > tools/testing/selftests/resctrl/resctrl_val.c | 50 ++++++++++++++++++ > tools/testing/selftests/resctrl/resctrlfs.c | 52 ------------------- > 2 files changed, 50 insertions(+), 52 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c > index f0f6c5f6e98b..5c8dc0a7bab9 100644 > --- a/tools/testing/selftests/resctrl/resctrl_val.c > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > @@ -621,6 +621,56 @@ measure_vals(struct resctrl_val_param *param, unsigned long *bw_resc_start) > return 0; > } > > +/* > + * run_benchmark - Run a specified benchmark or fill_buf (default benchmark) > + * in specified signal. Direct benchmark stdio to /dev/null. > + * @signum: signal number > + * @info: signal info > + * @ucontext: user context in signal handling > + */ > +void run_benchmark(int signum, siginfo_t *info, void *ucontext) Can run_benchmark() now be made static and its declaration removed from the header file? Reinette
Hi, On 2023-08-30 at 13:51:29 -0700, Reinette Chatre wrote: >Hi Maciej, > >On 8/28/2023 2:56 AM, Wieczor-Retman, Maciej wrote: >> Resctrlfs.c file contains mostly functions that interact in some way >> with resctrl FS entries while functions inside resctrl_val.c deal with >> measurements and benchmarking. >> >> Run_benchmark() function is located in resctrlfs.c file even though it's >> purpose is not interacting with the resctrl FS but to execute cache >> checking logic. > >It looks like your editor may be automatically capitalize first words >of sentences like Resctrlfs.c and Run_benchmark() above. I'll fix it. >Also please note that when using () to indicate a function it is not >necessary to say it is a function. For example above can just be: >"run_benchmark() is located ..." ... similarly you can just say >"resctrlfs.c contains ...". Thanks for the tip, will apply it from now on. >> >> Move run_benchmark() to resctrl_val.c just before resctrl_val() function >> that makes use of run_benchmark(). >> >> Remove return comment from kernel-doc since the function is type void. >> >> Changelog v2: >> - Add dots at the end of patch msg sentences. >> - Remove "Return: void" from run_benchmark() kernel-doc comment. >> > >same comment about changelog. It'll be fixed next time. >> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com> >> --- >> tools/testing/selftests/resctrl/resctrl_val.c | 50 ++++++++++++++++++ >> tools/testing/selftests/resctrl/resctrlfs.c | 52 ------------------- >> 2 files changed, 50 insertions(+), 52 deletions(-) >> >> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c >> index f0f6c5f6e98b..5c8dc0a7bab9 100644 >> --- a/tools/testing/selftests/resctrl/resctrl_val.c >> +++ b/tools/testing/selftests/resctrl/resctrl_val.c >> @@ -621,6 +621,56 @@ measure_vals(struct resctrl_val_param *param, unsigned long *bw_resc_start) >> return 0; >> } >> >> +/* >> + * run_benchmark - Run a specified benchmark or fill_buf (default benchmark) >> + * in specified signal. Direct benchmark stdio to /dev/null. >> + * @signum: signal number >> + * @info: signal info >> + * @ucontext: user context in signal handling >> + */ >> +void run_benchmark(int signum, siginfo_t *info, void *ucontext) > >Can run_benchmark() now be made static and its declaration removed from >the header file? Thanks for noticing. Yes, from my side it seems turning it into static is okay. I tried it out on Ilpo's branches that I know he's currently working on and there were no errors either.
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index f0f6c5f6e98b..5c8dc0a7bab9 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -621,6 +621,56 @@ measure_vals(struct resctrl_val_param *param, unsigned long *bw_resc_start) return 0; } +/* + * run_benchmark - Run a specified benchmark or fill_buf (default benchmark) + * in specified signal. Direct benchmark stdio to /dev/null. + * @signum: signal number + * @info: signal info + * @ucontext: user context in signal handling + */ +void run_benchmark(int signum, siginfo_t *info, void *ucontext) +{ + int operation, ret, memflush; + char **benchmark_cmd; + size_t span; + bool once; + FILE *fp; + + benchmark_cmd = info->si_ptr; + + /* + * Direct stdio of child to /dev/null, so that only parent writes to + * stdio (console) + */ + fp = freopen("/dev/null", "w", stdout); + if (!fp) + PARENT_EXIT("Unable to direct benchmark status to /dev/null"); + + if (strcmp(benchmark_cmd[0], "fill_buf") == 0) { + /* Execute default fill_buf benchmark */ + span = strtoul(benchmark_cmd[1], NULL, 10); + memflush = atoi(benchmark_cmd[2]); + operation = atoi(benchmark_cmd[3]); + if (!strcmp(benchmark_cmd[4], "true")) + once = true; + else if (!strcmp(benchmark_cmd[4], "false")) + once = false; + else + PARENT_EXIT("Invalid once parameter"); + + if (run_fill_buf(span, memflush, operation, once)) + fprintf(stderr, "Error in running fill buffer\n"); + } else { + /* Execute specified benchmark */ + ret = execvp(benchmark_cmd[0], benchmark_cmd); + if (ret) + perror("wrong\n"); + } + + fclose(stdout); + PARENT_EXIT("Unable to run specified benchmark"); +} + /* * resctrl_val: execute benchmark and measure memory bandwidth on * the benchmark diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 0f9644e5a25e..72dd8c3f655a 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -291,58 +291,6 @@ int taskset_benchmark(pid_t bm_pid, int cpu_no) return 0; } -/* - * run_benchmark - Run a specified benchmark or fill_buf (default benchmark) - * in specified signal. Direct benchmark stdio to /dev/null. - * @signum: signal number - * @info: signal info - * @ucontext: user context in signal handling - * - * Return: void - */ -void run_benchmark(int signum, siginfo_t *info, void *ucontext) -{ - int operation, ret, memflush; - char **benchmark_cmd; - size_t span; - bool once; - FILE *fp; - - benchmark_cmd = info->si_ptr; - - /* - * Direct stdio of child to /dev/null, so that only parent writes to - * stdio (console) - */ - fp = freopen("/dev/null", "w", stdout); - if (!fp) - PARENT_EXIT("Unable to direct benchmark status to /dev/null"); - - if (strcmp(benchmark_cmd[0], "fill_buf") == 0) { - /* Execute default fill_buf benchmark */ - span = strtoul(benchmark_cmd[1], NULL, 10); - memflush = atoi(benchmark_cmd[2]); - operation = atoi(benchmark_cmd[3]); - if (!strcmp(benchmark_cmd[4], "true")) - once = true; - else if (!strcmp(benchmark_cmd[4], "false")) - once = false; - else - PARENT_EXIT("Invalid once parameter"); - - if (run_fill_buf(span, memflush, operation, once)) - fprintf(stderr, "Error in running fill buffer\n"); - } else { - /* Execute specified benchmark */ - ret = execvp(benchmark_cmd[0], benchmark_cmd); - if (ret) - perror("wrong\n"); - } - - fclose(stdout); - PARENT_EXIT("Unable to run specified benchmark"); -} - /* * create_grp - Create a group only if one doesn't exist * @grp_name: Name of the group
Resctrlfs.c file contains mostly functions that interact in some way with resctrl FS entries while functions inside resctrl_val.c deal with measurements and benchmarking. Run_benchmark() function is located in resctrlfs.c file even though it's purpose is not interacting with the resctrl FS but to execute cache checking logic. Move run_benchmark() to resctrl_val.c just before resctrl_val() function that makes use of run_benchmark(). Remove return comment from kernel-doc since the function is type void. Changelog v2: - Add dots at the end of patch msg sentences. - Remove "Return: void" from run_benchmark() kernel-doc comment. Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com> --- tools/testing/selftests/resctrl/resctrl_val.c | 50 ++++++++++++++++++ tools/testing/selftests/resctrl/resctrlfs.c | 52 ------------------- 2 files changed, 50 insertions(+), 52 deletions(-)