| Message ID | 3a6639cd338a706bd2ed341aa6c717e1e79cc1f0.1708596015.git.maciej.wieczor-retman@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Simplify test cleanup functions | expand |
On Thu, 22 Feb 2024, Maciej Wieczor-Retman wrote: > Ctrl-c handler isn't aware of what test is currently running. Because of > that it executes all cleanups even if they aren't necessary. Since the > ctrl-c handler uses the sa_sigaction system no parameters can be passed > to it as function arguments. > > Add a global variable to make ctrl-c handler aware of the currently run > test and only execute the correct cleanup callback. > > Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com> > --- > Changelog v2: > - Remove tests_cleanup() from resctrl.h. > - Make current_test a const pointer only inside resctrl_val.c. (Ilpo) > > tools/testing/selftests/resctrl/resctrl.h | 3 +-- > tools/testing/selftests/resctrl/resctrl_tests.c | 14 +++----------- > tools/testing/selftests/resctrl/resctrl_val.c | 6 ++++-- > 3 files changed, 8 insertions(+), 15 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > index 0f49df4961ea..826783b29c9d 100644 > --- a/tools/testing/selftests/resctrl/resctrl.h > +++ b/tools/testing/selftests/resctrl/resctrl.h > @@ -153,7 +153,6 @@ int resctrl_val(const struct resctrl_test *test, > const struct user_params *uparams, > const char * const *benchmark_cmd, > struct resctrl_val_param *param); > -void tests_cleanup(void); > void mbm_test_cleanup(void); > void mba_test_cleanup(void); > unsigned long create_bit_mask(unsigned int start, unsigned int len); > @@ -162,7 +161,7 @@ int get_full_cbm(const char *cache_type, unsigned long *mask); > int get_mask_no_shareable(const char *cache_type, unsigned long *mask); > int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size); > void ctrlc_handler(int signum, siginfo_t *info, void *ptr); > -int signal_handler_register(void); > +int signal_handler_register(const struct resctrl_test *test); > void signal_handler_unregister(void); > void cat_test_cleanup(void); > unsigned int count_bits(unsigned long n); > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > index 75fc49ba3efb..161f5365b4f0 100644 > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > @@ -75,19 +75,11 @@ static void cmd_help(void) > printf("\t-h: help\n"); > } > > -void tests_cleanup(void) > -{ > - mbm_test_cleanup(); > - mba_test_cleanup(); > - cmt_test_cleanup(); > - cat_test_cleanup(); > -} > - > -static int test_prepare(void) > +static int test_prepare(const struct resctrl_test *test) > { > int res; > > - res = signal_handler_register(); > + res = signal_handler_register(test); > if (res) { > ksft_print_msg("Failed to register signal handler\n"); > return res; > @@ -130,7 +122,7 @@ static void run_single_test(const struct resctrl_test *test, const struct user_p > > ksft_print_msg("Starting %s test ...\n", test->name); > > - if (test_prepare()) { > + if (test_prepare(test)) { > ksft_exit_fail_msg("Abnormal failure when preparing for the test\n"); > return; > } > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c > index 5a49f07a6c85..d572815436f3 100644 > --- a/tools/testing/selftests/resctrl/resctrl_val.c > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > @@ -62,6 +62,7 @@ struct imc_counter_config { > static char mbm_total_path[1024]; > static int imcs; > static struct imc_counter_config imc_counters_config[MAX_IMCS][2]; > +const struct resctrl_test *current_test; static const struct > void membw_initialize_perf_event_attr(int i, int j) > { > @@ -472,7 +473,7 @@ void ctrlc_handler(int signum, siginfo_t *info, void *ptr) > if (bm_pid) > kill(bm_pid, SIGKILL); > umount_resctrlfs(); > - tests_cleanup(); > + current_test->cleanup(); These calls should have if (current_test->cleanup()) guard. Isn't the non-contiguous already test w/o the cleanup function? Other than those two, this looked okay.
On Thu, 22 Feb 2024, Ilpo Järvinen wrote: > On Thu, 22 Feb 2024, Maciej Wieczor-Retman wrote: > > > Ctrl-c handler isn't aware of what test is currently running. Because of > > that it executes all cleanups even if they aren't necessary. Since the > > ctrl-c handler uses the sa_sigaction system no parameters can be passed > > to it as function arguments. > > > > Add a global variable to make ctrl-c handler aware of the currently run > > test and only execute the correct cleanup callback. > > > > Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com> > > --- > > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c > > index 5a49f07a6c85..d572815436f3 100644 > > --- a/tools/testing/selftests/resctrl/resctrl_val.c > > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > > @@ -62,6 +62,7 @@ struct imc_counter_config { > > static char mbm_total_path[1024]; > > static int imcs; > > static struct imc_counter_config imc_counters_config[MAX_IMCS][2]; > > +const struct resctrl_test *current_test; > > static const struct > > > void membw_initialize_perf_event_attr(int i, int j) > > { > > @@ -472,7 +473,7 @@ void ctrlc_handler(int signum, siginfo_t *info, void *ptr) > > if (bm_pid) > > kill(bm_pid, SIGKILL); > > umount_resctrlfs(); > > - tests_cleanup(); > > + current_test->cleanup(); > > These calls should have if (current_test->cleanup()) guard. Isn't the Err, if (current_test->cleanup) is what I meant. > non-contiguous already test w/o the cleanup function? > > Other than those two, this looked okay.
On 2024-02-22 at 12:12:44 +0200, Ilpo Järvinen wrote: >On Thu, 22 Feb 2024, Maciej Wieczor-Retman wrote: > >> Ctrl-c handler isn't aware of what test is currently running. Because of >> that it executes all cleanups even if they aren't necessary. Since the >> ctrl-c handler uses the sa_sigaction system no parameters can be passed >> to it as function arguments. >> >> Add a global variable to make ctrl-c handler aware of the currently run >> test and only execute the correct cleanup callback. >> >> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com> >> --- >> Changelog v2: >> - Remove tests_cleanup() from resctrl.h. >> - Make current_test a const pointer only inside resctrl_val.c. (Ilpo) >> >> tools/testing/selftests/resctrl/resctrl.h | 3 +-- >> tools/testing/selftests/resctrl/resctrl_tests.c | 14 +++----------- >> tools/testing/selftests/resctrl/resctrl_val.c | 6 ++++-- >> 3 files changed, 8 insertions(+), 15 deletions(-) >> >> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h >> index 0f49df4961ea..826783b29c9d 100644 >> --- a/tools/testing/selftests/resctrl/resctrl.h >> +++ b/tools/testing/selftests/resctrl/resctrl.h >> @@ -153,7 +153,6 @@ int resctrl_val(const struct resctrl_test *test, >> const struct user_params *uparams, >> const char * const *benchmark_cmd, >> struct resctrl_val_param *param); >> -void tests_cleanup(void); >> void mbm_test_cleanup(void); >> void mba_test_cleanup(void); >> unsigned long create_bit_mask(unsigned int start, unsigned int len); >> @@ -162,7 +161,7 @@ int get_full_cbm(const char *cache_type, unsigned long *mask); >> int get_mask_no_shareable(const char *cache_type, unsigned long *mask); >> int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size); >> void ctrlc_handler(int signum, siginfo_t *info, void *ptr); >> -int signal_handler_register(void); >> +int signal_handler_register(const struct resctrl_test *test); >> void signal_handler_unregister(void); >> void cat_test_cleanup(void); >> unsigned int count_bits(unsigned long n); >> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c >> index 75fc49ba3efb..161f5365b4f0 100644 >> --- a/tools/testing/selftests/resctrl/resctrl_tests.c >> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c >> @@ -75,19 +75,11 @@ static void cmd_help(void) >> printf("\t-h: help\n"); >> } >> >> -void tests_cleanup(void) >> -{ >> - mbm_test_cleanup(); >> - mba_test_cleanup(); >> - cmt_test_cleanup(); >> - cat_test_cleanup(); >> -} >> - >> -static int test_prepare(void) >> +static int test_prepare(const struct resctrl_test *test) >> { >> int res; >> >> - res = signal_handler_register(); >> + res = signal_handler_register(test); >> if (res) { >> ksft_print_msg("Failed to register signal handler\n"); >> return res; >> @@ -130,7 +122,7 @@ static void run_single_test(const struct resctrl_test *test, const struct user_p >> >> ksft_print_msg("Starting %s test ...\n", test->name); >> >> - if (test_prepare()) { >> + if (test_prepare(test)) { >> ksft_exit_fail_msg("Abnormal failure when preparing for the test\n"); >> return; >> } >> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c >> index 5a49f07a6c85..d572815436f3 100644 >> --- a/tools/testing/selftests/resctrl/resctrl_val.c >> +++ b/tools/testing/selftests/resctrl/resctrl_val.c >> @@ -62,6 +62,7 @@ struct imc_counter_config { >> static char mbm_total_path[1024]; >> static int imcs; >> static struct imc_counter_config imc_counters_config[MAX_IMCS][2]; >> +const struct resctrl_test *current_test; > >static const struct Okay, I'll add it. > >> void membw_initialize_perf_event_attr(int i, int j) >> { >> @@ -472,7 +473,7 @@ void ctrlc_handler(int signum, siginfo_t *info, void *ptr) >> if (bm_pid) >> kill(bm_pid, SIGKILL); >> umount_resctrlfs(); >> - tests_cleanup(); >> + current_test->cleanup(); > >These calls should have if (current_test->cleanup()) guard. Isn't the >non-contiguous already test w/o the cleanup function? Yes, I remembered to put the check in the main test function but I forgot to put the check here too, thanks. I'll just resend the corrected version today. > >Other than those two, this looked okay. > >-- > i. >
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 0f49df4961ea..826783b29c9d 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -153,7 +153,6 @@ int resctrl_val(const struct resctrl_test *test, const struct user_params *uparams, const char * const *benchmark_cmd, struct resctrl_val_param *param); -void tests_cleanup(void); void mbm_test_cleanup(void); void mba_test_cleanup(void); unsigned long create_bit_mask(unsigned int start, unsigned int len); @@ -162,7 +161,7 @@ int get_full_cbm(const char *cache_type, unsigned long *mask); int get_mask_no_shareable(const char *cache_type, unsigned long *mask); int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size); void ctrlc_handler(int signum, siginfo_t *info, void *ptr); -int signal_handler_register(void); +int signal_handler_register(const struct resctrl_test *test); void signal_handler_unregister(void); void cat_test_cleanup(void); unsigned int count_bits(unsigned long n); diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 75fc49ba3efb..161f5365b4f0 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -75,19 +75,11 @@ static void cmd_help(void) printf("\t-h: help\n"); } -void tests_cleanup(void) -{ - mbm_test_cleanup(); - mba_test_cleanup(); - cmt_test_cleanup(); - cat_test_cleanup(); -} - -static int test_prepare(void) +static int test_prepare(const struct resctrl_test *test) { int res; - res = signal_handler_register(); + res = signal_handler_register(test); if (res) { ksft_print_msg("Failed to register signal handler\n"); return res; @@ -130,7 +122,7 @@ static void run_single_test(const struct resctrl_test *test, const struct user_p ksft_print_msg("Starting %s test ...\n", test->name); - if (test_prepare()) { + if (test_prepare(test)) { ksft_exit_fail_msg("Abnormal failure when preparing for the test\n"); return; } diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index 5a49f07a6c85..d572815436f3 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -62,6 +62,7 @@ struct imc_counter_config { static char mbm_total_path[1024]; static int imcs; static struct imc_counter_config imc_counters_config[MAX_IMCS][2]; +const struct resctrl_test *current_test; void membw_initialize_perf_event_attr(int i, int j) { @@ -472,7 +473,7 @@ void ctrlc_handler(int signum, siginfo_t *info, void *ptr) if (bm_pid) kill(bm_pid, SIGKILL); umount_resctrlfs(); - tests_cleanup(); + current_test->cleanup(); ksft_print_msg("Ending\n\n"); exit(EXIT_SUCCESS); @@ -482,13 +483,14 @@ void ctrlc_handler(int signum, siginfo_t *info, void *ptr) * Register CTRL-C handler for parent, as it has to kill * child process before exiting. */ -int signal_handler_register(void) +int signal_handler_register(const struct resctrl_test *test) { struct sigaction sigact = {}; int ret = 0; bm_pid = 0; + current_test = test; sigact.sa_sigaction = ctrlc_handler; sigemptyset(&sigact.sa_mask); sigact.sa_flags = SA_SIGINFO;
Ctrl-c handler isn't aware of what test is currently running. Because of that it executes all cleanups even if they aren't necessary. Since the ctrl-c handler uses the sa_sigaction system no parameters can be passed to it as function arguments. Add a global variable to make ctrl-c handler aware of the currently run test and only execute the correct cleanup callback. Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com> --- Changelog v2: - Remove tests_cleanup() from resctrl.h. - Make current_test a const pointer only inside resctrl_val.c. (Ilpo) tools/testing/selftests/resctrl/resctrl.h | 3 +-- tools/testing/selftests/resctrl/resctrl_tests.c | 14 +++----------- tools/testing/selftests/resctrl/resctrl_val.c | 6 ++++-- 3 files changed, 8 insertions(+), 15 deletions(-)