[v2,2/3] selftests/resctrl: Simplify cleanup in ctrl-c handler

Message ID 3a6639cd338a706bd2ed341aa6c717e1e79cc1f0.1708596015.git.maciej.wieczor-retman@intel.com
State New
Headers
Series selftests/resctrl: Simplify test cleanup functions |

Commit Message

Maciej Wieczor-Retman Feb. 22, 2024, 10:04 a.m. UTC
  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

Ilpo Järvinen Feb. 22, 2024, 10:12 a.m. UTC | #1
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.
  
Ilpo Järvinen Feb. 22, 2024, 10:13 a.m. UTC | #2
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.
  
Maciej Wieczor-Retman Feb. 22, 2024, 10:51 a.m. UTC | #3
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.
>
  

Patch

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;