Re: [PATCH V2 10/13] selftests/resctrl: Ensure measurements skip initialization of default benchmark

-- i. > --- > Changes since V1: > - Keep the fflush(stdout) before fork() to avoid duplicate messages. (Ilpo) > - Re-order in series to that the new behavior is introduced after issues > with existing behavior is addressed. > --- > tools/testing/selftests/resctrl/fill_buf.c | 15 -- > tools/testing/selftests/resctrl/resctrl.h | 1 - > tools/testing/selftests/resctrl/resctrl_val.c | 199 +++++------------- > 3 files changed, 50 insertions(+), 165 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c > index e4f1cea317f1..75956345cb12 100644 > --- a/tools/testing/selftests/resctrl/fill_buf.c > +++ b/tools/testing/selftests/resctrl/fill_buf.c > @@ -129,18 +129,3 @@ unsigned char *alloc_buffer(size_t buf_size, int memflush) > > return buf; > } > - > -int run_fill_buf(size_t buf_size, int memflush) > -{ > - unsigned char *buf; > - > - buf = alloc_buffer(buf_size, memflush); > - if (!buf) > - return -1; > - > - fill_cache_read(buf, buf_size, false); > - > - free(buf); > - > - return 0; > -} > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > index 98462752cb46..dc01dc75cba5 100644 > --- a/tools/testing/selftests/resctrl/resctrl.h > +++ b/tools/testing/selftests/resctrl/resctrl.h > @@ -168,7 +168,6 @@ int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu, > unsigned char *alloc_buffer(size_t buf_size, int memflush); > void mem_flush(unsigned char *buf, size_t buf_size); > void fill_cache_read(unsigned char *buf, size_t buf_size, bool once); > -int run_fill_buf(size_t buf_size, int memflush); > int initialize_read_mem_bw_imc(void); > int measure_read_mem_bw(const struct user_params *uparams, > struct resctrl_val_param *param, pid_t bm_pid); > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c > index 380c3f218d01..f025be5c7d82 100644 > --- a/tools/testing/selftests/resctrl/resctrl_val.c > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > @@ -373,7 +373,7 @@ static int get_mem_bw_resctrl(FILE *fp, unsigned long *mbm_total) > return 0; > } > > -static pid_t bm_pid, ppid; > +static pid_t bm_pid; > > void ctrlc_handler(int signum, siginfo_t *info, void *ptr) > { > @@ -431,13 +431,6 @@ void signal_handler_unregister(void) > } > } > > -static void parent_exit(pid_t ppid) > -{ > - kill(ppid, SIGKILL); > - umount_resctrlfs(); > - exit(EXIT_FAILURE); > -} > - > /* > * print_results_bw: the memory bandwidth results are stored in a file > * @filename: file that stores the results > @@ -535,56 +528,6 @@ int measure_read_mem_bw(const struct user_params *uparams, > return ret; > } > > -struct benchmark_info { > - const struct user_params *uparams; > - struct resctrl_val_param *param; > -}; > - > -/* > - * 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 > - */ > -static void run_benchmark(int signum, siginfo_t *info, void *ucontext) > -{ > - struct benchmark_info *benchmark_info; > - const struct user_params *uparams; > - struct resctrl_val_param *param; > - FILE *fp; > - int ret; > - > - benchmark_info = info->si_ptr; > - uparams = benchmark_info->uparams; > - param = benchmark_info->param; > - > - /* > - * Direct stdio of child to /dev/null, so that only parent writes to > - * stdio (console) > - */ > - fp = freopen("/dev/null", "w", stdout); > - if (!fp) { > - ksft_perror("Unable to direct benchmark status to /dev/null"); > - parent_exit(ppid); > - } > - > - if (param->fill_buf) { > - if (run_fill_buf(param->fill_buf->buf_size, > - param->fill_buf->memflush)) > - fprintf(stderr, "Error in running fill buffer\n"); > - } else if (uparams->benchmark_cmd[0]) { > - /* Execute specified benchmark */ > - ret = execvp(uparams->benchmark_cmd[0], (char **)uparams->benchmark_cmd); > - if (ret) > - ksft_perror("execvp"); > - } > - > - fclose(stdout); > - ksft_print_msg("Unable to run specified benchmark\n"); > - parent_exit(ppid); > -} > - > /* > * resctrl_val: execute benchmark and measure memory bandwidth on > * the benchmark > @@ -598,12 +541,11 @@ int resctrl_val(const struct resctrl_test *test, > const struct user_params *uparams, > struct resctrl_val_param *param) > { > - struct benchmark_info benchmark_info; > - struct sigaction sigact; > - int ret = 0, pipefd[2]; > - char pipe_message = 0; > - union sigval value; > + unsigned char *buf = NULL; > + cpu_set_t old_affinity; > int domain_id; > + int ret = 0; > + pid_t ppid; > > if (strcmp(param->filename, "") == 0) > sprintf(param->filename, "stdio"); > @@ -614,108 +556,65 @@ int resctrl_val(const struct resctrl_test *test, > return ret; > } > > - benchmark_info.uparams = uparams; > - benchmark_info.param = param; > - > - /* > - * If benchmark wasn't successfully started by child, then child should > - * kill parent, so save parent's pid > - */ > ppid = getpid(); > > - if (pipe(pipefd)) { > - ksft_perror("Unable to create pipe"); > + /* Taskset test to specified CPU. */ > + ret = taskset_benchmark(ppid, uparams->cpu, &old_affinity); > + if (ret) > + return ret; > > - return -1; > + /* Write test to specified control & monitoring group in resctrl FS. */ > + ret = write_bm_pid_to_resctrl(ppid, param->ctrlgrp, param->mongrp); > + if (ret) > + goto reset_affinity; > + > + if (param->init) { > + ret = param->init(param, domain_id); > + if (ret) > + goto reset_affinity; > } > > /* > - * Fork to start benchmark, save child's pid so that it can be killed > - * when needed > + * If not running user provided benchmark, run the default > + * "fill_buf". First phase of "fill_buf" is to prepare the > + * buffer that the benchmark will operate on. No measurements > + * are needed during this phase and prepared memory will be > + * passed to next part of benchmark via copy-on-write thus > + * no impact on the benchmark that relies on reading from > + * memory only. > */ > + if (param->fill_buf) { > + buf = alloc_buffer(param->fill_buf->buf_size, > + param->fill_buf->memflush); > + if (!buf) { > + ret = -ENOMEM; > + goto reset_affinity; > + } > + } > + > fflush(stdout); > bm_pid = fork(); > if (bm_pid == -1) { > + ret = -errno; > ksft_perror("Unable to fork"); > - > - return -1; > + goto free_buf; > } > > + /* > + * What needs to be measured runs in separate process until > + * terminated. > + */ > if (bm_pid == 0) { > - /* > - * Mask all signals except SIGUSR1, parent uses SIGUSR1 to > - * start benchmark > - */ > - sigfillset(&sigact.sa_mask); > - sigdelset(&sigact.sa_mask, SIGUSR1); > - > - sigact.sa_sigaction = run_benchmark; > - sigact.sa_flags = SA_SIGINFO; > - > - /* Register for "SIGUSR1" signal from parent */ > - if (sigaction(SIGUSR1, &sigact, NULL)) { > - ksft_perror("Can't register child for signal"); > - parent_exit(ppid); > - } > - > - /* Tell parent that child is ready */ > - close(pipefd[0]); > - pipe_message = 1; > - if (write(pipefd[1], &pipe_message, sizeof(pipe_message)) < > - sizeof(pipe_message)) { > - ksft_perror("Failed signaling parent process"); > - close(pipefd[1]); > - return -1; > - } > - close(pipefd[1]); > - > - /* Suspend child until delivery of "SIGUSR1" from parent */ > - sigsuspend(&sigact.sa_mask); > - > - ksft_perror("Child is done"); > - parent_exit(ppid); > + if (param->fill_buf) > + fill_cache_read(buf, param->fill_buf->buf_size, false); > + else if (uparams->benchmark_cmd[0]) > + execvp(uparams->benchmark_cmd[0], (char **)uparams->benchmark_cmd); > + exit(EXIT_SUCCESS); > } > > ksft_print_msg("Benchmark PID: %d\n", (int)bm_pid); > > - value.sival_ptr = (void *)&benchmark_info; > - > - /* Taskset benchmark to specified cpu */ > - ret = taskset_benchmark(bm_pid, uparams->cpu, NULL); > - if (ret) > - goto out; > - > - /* Write benchmark to specified control&monitoring grp in resctrl FS */ > - ret = write_bm_pid_to_resctrl(bm_pid, param->ctrlgrp, param->mongrp); > - if (ret) > - goto out; > - > - if (param->init) { > - ret = param->init(param, domain_id); > - if (ret) > - goto out; > - } > - > - /* Parent waits for child to be ready. */ > - close(pipefd[1]); > - while (pipe_message != 1) { > - if (read(pipefd[0], &pipe_message, sizeof(pipe_message)) < > - sizeof(pipe_message)) { > - ksft_perror("Failed reading message from child process"); > - close(pipefd[0]); > - goto out; > - } > - } > - close(pipefd[0]); > - > - /* Signal child to start benchmark */ > - if (sigqueue(bm_pid, SIGUSR1, value) == -1) { > - ksft_perror("sigqueue SIGUSR1 to child"); > - ret = -1; > - goto out; > - } > - > - /* Give benchmark enough time to fully run */ > + /* Give benchmark enough time to fully run. */ > sleep(1); > > /* Test runs until the callback setup() tells the test to stop. */ > @@ -733,8 +632,10 @@ int resctrl_val(const struct resctrl_test *test, > break; > } > > -out: > kill(bm_pid, SIGKILL); > - > +free_buf: > + free(buf); > +reset_affinity: > + taskset_restore(ppid, &old_affinity); > return ret; > } >