[PATCH 0/3] selftests/resctrl: Simplify test cleanup functions

[PATCH 0/3] selftests/resctrl: Simplify test cleanup functions

[Maciej Wieczor-Retman]

Cleaning up after tests is implemented separately for individual tests
and called at the end of each test execution. Since these functions are
very similar and a more generalized test framework was introduced a
function pointer in the resctrl_test struct can be used to reduce the
amount of function calls.

These functions are also all called in the ctrl-c handler because the
handler isn't aware which test is currently running. Since the handler
is implemented with a sigaction no function parameters can be passed
there but information about what test is currently running can be passed
with a global variable.

Maciej Wieczor-Retman (3):
  selftests/resctrl: Add cleanup function to test framework
  selftests/resctrl: Simplify cleanup in ctrl-c handler
  selftests/resctrl: Move cleanups out of individual tests

 tools/testing/selftests/resctrl/cat_test.c    |  5 ++---
 tools/testing/selftests/resctrl/cmt_test.c    |  4 ++--
 tools/testing/selftests/resctrl/mba_test.c    |  5 ++---
 tools/testing/selftests/resctrl/mbm_test.c    |  5 ++---
 tools/testing/selftests/resctrl/resctrl.h     |  8 +++----
 .../testing/selftests/resctrl/resctrl_tests.c | 22 ++++++++++---------
 tools/testing/selftests/resctrl/resctrl_val.c |  2 +-
 7 files changed, 25 insertions(+), 26 deletions(-)

-- 
2.43.2

[PATCH 1/3] selftests/resctrl: Add cleanup function to test framework

[Maciej Wieczor-Retman]

Resctrl selftests use very similar functions to cleanup after
themselves. This creates a lot of code duplication. Also not being
hooked to the test framework means that ctrl-c handler isn't aware of
what test is currently running and executes all cleanups even though
only one is needed.

Add a function pointer to the resctrl_test struct and attach to it
cleanup functions from individual tests.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
 tools/testing/selftests/resctrl/cat_test.c | 1 +
 tools/testing/selftests/resctrl/cmt_test.c | 1 +
 tools/testing/selftests/resctrl/mba_test.c | 1 +
 tools/testing/selftests/resctrl/mbm_test.c | 1 +
 tools/testing/selftests/resctrl/resctrl.h  | 2 ++
 5 files changed, 6 insertions(+)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 24af8310288a..2d2f69d3e5b7 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -299,4 +299,5 @@ struct resctrl_test l3_cat_test = {
 	.resource = "L3",
 	.feature_check = test_resource_feature_check,
 	.run_test = cat_run_test,
+	.cleanup = cat_test_cleanup,
 };
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index dd5ca343c469..32ddee87e43d 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -178,4 +178,5 @@ struct resctrl_test cmt_test = {
 	.resource = "L3",
 	.feature_check = cmt_feature_check,
 	.run_test = cmt_run_test,
+	.cleanup = cmt_test_cleanup,
 };
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index da256d2dbe5c..7cc4067ce930 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -180,4 +180,5 @@ struct resctrl_test mba_test = {
 	.vendor_specific = ARCH_INTEL,
 	.feature_check = mba_feature_check,
 	.run_test = mba_run_test,
+	.cleanup = mba_test_cleanup,
 };
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 34879e7b71a0..071e2d3808a7 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -150,4 +150,5 @@ struct resctrl_test mbm_test = {
 	.vendor_specific = ARCH_INTEL,
 	.feature_check = mbm_feature_check,
 	.run_test = mbm_run_test,
+	.cleanup = mbm_test_cleanup,
 };
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index c52eaf46f24d..0f49df4961ea 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -70,6 +70,7 @@ struct user_params {
  * @disabled:		Test is disabled
  * @feature_check:	Callback to check required resctrl features
  * @run_test:		Callback to run the test
+ * @cleanup:		Callback to cleanup after the test
  */
 struct resctrl_test {
 	const char	*name;
@@ -79,6 +80,7 @@ struct resctrl_test {
 	bool		(*feature_check)(const struct resctrl_test *test);
 	int		(*run_test)(const struct resctrl_test *test,
 				    const struct user_params *uparams);
+	void		(*cleanup)(void);
 };
 
 /*
-- 
2.43.2

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

[Maciej Wieczor-Retman]

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

[PATCH 3/3] selftests/resctrl: Move cleanups out of individual tests

[Maciej Wieczor-Retman]

Every test calls its cleanup function at the end of it's test function.
After the cleanup function pointer is added to the test framework this
can be simplified to executing the callback function at the end of the
generic test running function.

Make test cleanup functions static and call them from the end of
run_single_test() from the resctrl_test's cleanup function pointer.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
 tools/testing/selftests/resctrl/cat_test.c      | 4 +---
 tools/testing/selftests/resctrl/cmt_test.c      | 3 +--
 tools/testing/selftests/resctrl/mba_test.c      | 4 +---
 tools/testing/selftests/resctrl/mbm_test.c      | 4 +---
 tools/testing/selftests/resctrl/resctrl.h       | 4 ----
 tools/testing/selftests/resctrl/resctrl_tests.c | 2 ++
 6 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 2d2f69d3e5b7..ad5ebce65c07 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -128,7 +128,7 @@ static int check_results(struct resctrl_val_param *param, const char *cache_type
 	return fail;
 }
 
-void cat_test_cleanup(void)
+static void cat_test_cleanup(void)
 {
 	remove(RESULT_FILE_NAME);
 }
@@ -289,8 +289,6 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param
 	ret = check_results(&param, test->resource,
 			    cache_total_size, full_cache_mask, start_mask);
 out:
-	cat_test_cleanup();
-
 	return ret;
 }
 
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index 32ddee87e43d..c477f3c9635f 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -91,7 +91,7 @@ static int check_results(struct resctrl_val_param *param, size_t span, int no_of
 				 MAX_DIFF, MAX_DIFF_PERCENT, runs - 1, true);
 }
 
-void cmt_test_cleanup(void)
+static void cmt_test_cleanup(void)
 {
 	remove(RESULT_FILE_NAME);
 }
@@ -161,7 +161,6 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param
 		ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
 
 out:
-	cmt_test_cleanup();
 	free(span_str);
 
 	return ret;
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 7cc4067ce930..e4cd484941ec 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -137,7 +137,7 @@ static int check_results(void)
 	return show_mba_info(bw_imc, bw_resc);
 }
 
-void mba_test_cleanup(void)
+static void mba_test_cleanup(void)
 {
 	remove(RESULT_FILE_NAME);
 }
@@ -163,8 +163,6 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
 	ret = check_results();
 
 out:
-	mba_test_cleanup();
-
 	return ret;
 }
 
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 071e2d3808a7..405edd7df816 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -105,7 +105,7 @@ static int mbm_setup(const struct resctrl_test *test,
 	return ret;
 }
 
-void mbm_test_cleanup(void)
+static void mbm_test_cleanup(void)
 {
 	remove(RESULT_FILE_NAME);
 }
@@ -133,8 +133,6 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
 		ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
 
 out:
-	mbm_test_cleanup();
-
 	return ret;
 }
 
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 79b45cbeb628..c2a74e11a65e 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -156,8 +156,6 @@ int resctrl_val(const struct resctrl_test *test,
 		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);
 unsigned int count_contiguous_bits(unsigned long val, unsigned int *start);
 int get_full_cbm(const char *cache_type, unsigned long *mask);
@@ -166,9 +164,7 @@ 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);
 void signal_handler_unregister(void);
-void cat_test_cleanup(void);
 unsigned int count_bits(unsigned long n);
-void cmt_test_cleanup(void);
 
 void perf_event_attr_initialize(struct perf_event_attr *pea, __u64 config);
 void perf_event_initialize_read_format(struct perf_event_read *pe_read);
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index b17f7401892c..e01937e798ee 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -142,6 +142,8 @@ static void run_single_test(const struct resctrl_test *test, const struct user_p
 	}
 
 	ret = test->run_test(test, uparams);
+	if (test->cleanup)
+		test->cleanup();
 	ksft_test_result(!ret, "%s: test\n", test->name);
 
 cleanup:
-- 
2.43.2

Re: [PATCH 0/3] selftests/resctrl: Simplify test cleanup functions

[Maciej Wieczor-Retman]

I forgot to add to the cover letter that this series applies cleanly on both
kselftests/next and on top of my other series [1] currently in review.

[1] https://lore.kernel.org/all/cover.1708072203.git.maciej.wieczor-retman@intel.com/

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH 2/3] selftests/resctrl: Simplify cleanup in ctrl-c handler

[Ilpo Järvinen]

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.

Re: [PATCH 3/3] selftests/resctrl: Move cleanups out of individual tests

[Ilpo Järvinen]

On Tue, 20 Feb 2024, Maciej Wieczor-Retman wrote:

> Every test calls its cleanup function at the end of it's test function.
> After the cleanup function pointer is added to the test framework this
> can be simplified to executing the callback function at the end of the
> generic test running function.
> 
> Make test cleanup functions static and call them from the end of
> run_single_test() from the resctrl_test's cleanup function pointer.
> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
>  tools/testing/selftests/resctrl/cat_test.c      | 4 +---
>  tools/testing/selftests/resctrl/cmt_test.c      | 3 +--
>  tools/testing/selftests/resctrl/mba_test.c      | 4 +---
>  tools/testing/selftests/resctrl/mbm_test.c      | 4 +---
>  tools/testing/selftests/resctrl/resctrl.h       | 4 ----
>  tools/testing/selftests/resctrl/resctrl_tests.c | 2 ++
>  6 files changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 2d2f69d3e5b7..ad5ebce65c07 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -128,7 +128,7 @@ static int check_results(struct resctrl_val_param *param, const char *cache_type
>  	return fail;
>  }
>  
> -void cat_test_cleanup(void)
> +static void cat_test_cleanup(void)
>  {
>  	remove(RESULT_FILE_NAME);
>  }
> @@ -289,8 +289,6 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param
>  	ret = check_results(&param, test->resource,
>  			    cache_total_size, full_cache_mask, start_mask);
>  out:
> -	cat_test_cleanup();
> -

Any goto out can now become return ret; and the out label be then removed.


-- 
 i.

Re: [PATCH 2/3] selftests/resctrl: Simplify cleanup in ctrl-c handler

[Maciej Wieczor-Retman]

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

Re: [PATCH 3/3] selftests/resctrl: Move cleanups out of individual tests

[Maciej Wieczor-Retman]

On 2024-02-20 at 15:49:16 +0200, Ilpo Järvinen wrote:
>On Tue, 20 Feb 2024, Maciej Wieczor-Retman wrote:
>
>> Every test calls its cleanup function at the end of it's test function.
>> After the cleanup function pointer is added to the test framework this
>> can be simplified to executing the callback function at the end of the
>> generic test running function.
>> 
>> Make test cleanup functions static and call them from the end of
>> run_single_test() from the resctrl_test's cleanup function pointer.
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> ---
>>  tools/testing/selftests/resctrl/cat_test.c      | 4 +---
>>  tools/testing/selftests/resctrl/cmt_test.c      | 3 +--
>>  tools/testing/selftests/resctrl/mba_test.c      | 4 +---
>>  tools/testing/selftests/resctrl/mbm_test.c      | 4 +---
>>  tools/testing/selftests/resctrl/resctrl.h       | 4 ----
>>  tools/testing/selftests/resctrl/resctrl_tests.c | 2 ++
>>  6 files changed, 6 insertions(+), 15 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>> index 2d2f69d3e5b7..ad5ebce65c07 100644
>> --- a/tools/testing/selftests/resctrl/cat_test.c
>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>> @@ -128,7 +128,7 @@ static int check_results(struct resctrl_val_param *param, const char *cache_type
>>  	return fail;
>>  }
>>  
>> -void cat_test_cleanup(void)
>> +static void cat_test_cleanup(void)
>>  {
>>  	remove(RESULT_FILE_NAME);
>>  }
>> @@ -289,8 +289,6 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param
>>  	ret = check_results(&param, test->resource,
>>  			    cache_total_size, full_cache_mask, start_mask);
>>  out:
>> -	cat_test_cleanup();
>> -
>
>Any goto out can now become return ret; and the out label be then removed.

Right, thanks, I'll clean these up.

>
>
>-- 
> i.
>

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH 2/3] selftests/resctrl: Simplify cleanup in ctrl-c handler

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2444 bytes --]

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.