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;
+
 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);
-- 
2.43.2

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.

-- 
 i.

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.
>

-- 
Kind regards
Maciej Wieczór-Retman

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.

-- 
 i.