| Message ID | 8c4fcfb6b4e38a0f0e400be88ecf1af0d20e12e7.1708434017.git.maciej.wieczor-retman@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Simplify test cleanup functions | expand |
On Tue, 20 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 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> > --- > tools/testing/selftests/resctrl/resctrl.h | 2 ++ > .../testing/selftests/resctrl/resctrl_tests.c | 20 +++++++++---------- > tools/testing/selftests/resctrl/resctrl_val.c | 2 +- > 3 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > index 0f49df4961ea..79b45cbeb628 100644 > --- a/tools/testing/selftests/resctrl/resctrl.h > +++ b/tools/testing/selftests/resctrl/resctrl.h > @@ -128,6 +128,8 @@ extern pid_t bm_pid, ppid; > > extern char llc_occup_path[1024]; > > +extern struct resctrl_test current_test; Why this is not just a pointer? > + > int get_vendor(void); > bool check_resctrlfs_support(void); > int filter_dmesg(void); > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > index 75fc49ba3efb..b17f7401892c 100644 > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > @@ -14,6 +14,12 @@ > static volatile int sink_target; > volatile int *value_sink = &sink_target; > > +/* > + * Set during test preparation for the cleanup function pointer used in > + * ctrl-c sa_sigaction > + */ > +struct resctrl_test current_test; > + > static struct resctrl_test *resctrl_tests[] = { > &mbm_test, > &mba_test, > @@ -75,18 +81,12 @@ 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(); > -} This should be removed from resctrl.h too. > - > -static int test_prepare(void) > +static int test_prepare(const struct resctrl_test *test) > { > int res; > > + current_test = *test; I'd prefer to keep this internal to signal handling functions so that either the struct resctrl_test or just the cleanup handler is passed to signal_handler_register(). It'd also allow current_test (or the cleanup function ptr) to be static.
On 2024-02-20 at 15:45:23 +0200, Ilpo Järvinen wrote: >On Tue, 20 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 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> >> --- >> tools/testing/selftests/resctrl/resctrl.h | 2 ++ >> .../testing/selftests/resctrl/resctrl_tests.c | 20 +++++++++---------- >> tools/testing/selftests/resctrl/resctrl_val.c | 2 +- >> 3 files changed, 13 insertions(+), 11 deletions(-) >> >> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h >> index 0f49df4961ea..79b45cbeb628 100644 >> --- a/tools/testing/selftests/resctrl/resctrl.h >> +++ b/tools/testing/selftests/resctrl/resctrl.h >> @@ -128,6 +128,8 @@ extern pid_t bm_pid, ppid; >> >> extern char llc_occup_path[1024]; >> >> +extern struct resctrl_test current_test; > >Why this is not just a pointer? I tried making this as a pointer but the 'test' in test_prepare() is of type 'const struct resctrl_test *' and there are warnings about dropping the const modifier. > >> + >> int get_vendor(void); >> bool check_resctrlfs_support(void); >> int filter_dmesg(void); >> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c >> index 75fc49ba3efb..b17f7401892c 100644 >> --- a/tools/testing/selftests/resctrl/resctrl_tests.c >> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c >> @@ -14,6 +14,12 @@ >> static volatile int sink_target; >> volatile int *value_sink = &sink_target; >> >> +/* >> + * Set during test preparation for the cleanup function pointer used in >> + * ctrl-c sa_sigaction >> + */ >> +struct resctrl_test current_test; >> + >> static struct resctrl_test *resctrl_tests[] = { >> &mbm_test, >> &mba_test, >> @@ -75,18 +81,12 @@ 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(); >> -} > >This should be removed from resctrl.h too. Thanks, forgot that it was there too. > >> - >> -static int test_prepare(void) >> +static int test_prepare(const struct resctrl_test *test) >> { >> int res; >> >> + current_test = *test; > >I'd prefer to keep this internal to signal handling functions so that >either the struct resctrl_test or just the cleanup handler is passed >to signal_handler_register(). Okay, would moving this assignment to signal_handler_register() be okay then? > >It'd also allow current_test (or the cleanup function ptr) to be static. > >-- > i. >
On Tue, 20 Feb 2024, Maciej Wieczor-Retman wrote: > On 2024-02-20 at 15:45:23 +0200, Ilpo Järvinen wrote: > >On Tue, 20 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 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> > >> --- > >> tools/testing/selftests/resctrl/resctrl.h | 2 ++ > >> .../testing/selftests/resctrl/resctrl_tests.c | 20 +++++++++---------- > >> tools/testing/selftests/resctrl/resctrl_val.c | 2 +- > >> 3 files changed, 13 insertions(+), 11 deletions(-) > >> > >> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > >> index 0f49df4961ea..79b45cbeb628 100644 > >> --- a/tools/testing/selftests/resctrl/resctrl.h > >> +++ b/tools/testing/selftests/resctrl/resctrl.h > >> @@ -128,6 +128,8 @@ extern pid_t bm_pid, ppid; > >> > >> extern char llc_occup_path[1024]; > >> > >> +extern struct resctrl_test current_test; > > > >Why this is not just a pointer? > > I tried making this as a pointer but the 'test' in test_prepare() is of type > 'const struct resctrl_test *' and there are warnings about dropping the const > modifier. Why cannot the pointer be const too? The signal handler is not supposed to modify the contents of the resctrl_test struct. There are two types of constness in C. One (the most commonly used one) relates to immutability of the contents of the struct the pointer (or char *) points to while the other enforces the pointer itself to remain immutable. Usually, the former is what is useful and it's what you get when you naturally write "const struct". > >> + current_test = *test; > > > >I'd prefer to keep this internal to signal handling functions so that > >either the struct resctrl_test or just the cleanup handler is passed > >to signal_handler_register(). > > Okay, would moving this assignment to signal_handler_register() be okay then? Yes, that's what I'm after here. Lets keep it internal to the signal handling code.
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 0f49df4961ea..79b45cbeb628 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -128,6 +128,8 @@ extern pid_t bm_pid, ppid; extern char llc_occup_path[1024]; +extern struct resctrl_test current_test; + int get_vendor(void); bool check_resctrlfs_support(void); int filter_dmesg(void); diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 75fc49ba3efb..b17f7401892c 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -14,6 +14,12 @@ static volatile int sink_target; volatile int *value_sink = &sink_target; +/* + * Set during test preparation for the cleanup function pointer used in + * ctrl-c sa_sigaction + */ +struct resctrl_test current_test; + static struct resctrl_test *resctrl_tests[] = { &mbm_test, &mba_test, @@ -75,18 +81,12 @@ 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; + current_test = *test; + res = signal_handler_register(); if (res) { ksft_print_msg("Failed to register signal handler\n"); @@ -130,7 +130,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..db6aac33ced1 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -472,7 +472,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);
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 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> --- tools/testing/selftests/resctrl/resctrl.h | 2 ++ .../testing/selftests/resctrl/resctrl_tests.c | 20 +++++++++---------- tools/testing/selftests/resctrl/resctrl_val.c | 2 +- 3 files changed, 13 insertions(+), 11 deletions(-)