[v2,2/3] selftests/resctrl: Simplify cleanup in ctrl-c handler
Commit Message
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(-)
Comments
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.
>
@@ -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);
@@ -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;
}
@@ -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;