[PATCH v3 0/5] Some improvements of resctrl selftest

[PATCH v3 0/5] Some improvements of resctrl selftest

[Shaopeng Tan]

Hello,

The aim of this patch series is to improve the resctrl selftest.
Without these fixes, some unnecessary processing will be executed
and test results will be confusing. 
There is no behavior change in test themselves.

[patch 1] Make write_schemata() run to set up shemata with 100% allocation
	  on first run in MBM test.
[patch 2] The MBA test result message is always output as "ok",
	  make output message to be "not ok" if MBA check result is failed.
[patch 3] When a child process is created by fork(), the buffer of the 
	  parent process is also copied. Flush the buffer before
	  executing fork().
[patch 4] Add a signal handler to cleanup properly before exiting the 
	  parent process, if there is an error occurs after creating 
	  a child process with fork() in the CAT test.
[patch 5] Before exiting each test CMT/CAT/MBM/MBA, clear test result 
	  files function cat/cmt/mbm/mba_test_cleanup() are called
	  twice. Delete once.

This patch series is based on Linux v6.1-rc3

Difference from v2:
Moved [PATCH v2 3/4] to the last and insert patch 4 before it.
[patch 1] Fixed the typo miss in the changelog and initialized
	  *p(resctrl_val_param) before use it. And since there was no 
	  MBM processing in write_schemata(), added it.
[patch 4] A signal handler is introduced in this patch. With this patch, 
	  patch 5 clear duplicate code cat/cmt/mbm/mba_test_cleanup() 
	  without falling into the indicated trap.
	  https://lore.kernel.org/lkml/bdb19cf6-dd4b-2042-7cda-7f6108e543aa@intel.com/

Pervious versions of this series:
[v1] https://lore.kernel.org/lkml/20220914015147.3071025-1-tan.shaopeng@jp.fujitsu.com/
[v2] https://lore.kernel.org/lkml/20221005013933.1486054-1-tan.shaopeng@jp.fujitsu.com/

Shaopeng Tan (5):
  selftests/resctrl: Fix set up schemata with 100% allocation on first
    run in MBM test
  selftests/resctrl: Return MBA check result and make it to output
    message
  selftests/resctrl: Flush stdout file buffer before executing fork()
  selftests/resctrl: Cleanup properly when an error occurs in CAT test.
  selftests/resctrl: Remove duplicate codes that clear each test result
    file

 tools/testing/selftests/resctrl/cat_test.c    | 29 +++++++++++++------
 tools/testing/selftests/resctrl/mba_test.c    |  8 ++---
 tools/testing/selftests/resctrl/mbm_test.c    | 13 +++++----
 .../testing/selftests/resctrl/resctrl_tests.c |  4 ---
 tools/testing/selftests/resctrl/resctrl_val.c |  1 +
 tools/testing/selftests/resctrl/resctrlfs.c   |  5 +++-
 6 files changed, 36 insertions(+), 24 deletions(-)

-- 
2.27.0

[PATCH v3 1/5] selftests/resctrl: Fix set up schemata with 100% allocation on first run in MBM test

[Shaopeng Tan]

There is a comment "Set up shemata with 100% allocation on the first run"
in function mbm_setup(), but there is an increment bug and the condition
"num_of_runs == 0" will never be met and write_schemata() will never be
called to set schemata to 100%. Even if write_schemata() is called in MBM
test, since it is not supported for MBM test it does not set the schemata.
This is currently fine because resctrl_val_parm->mum_resctrlfs is always 1
and umount/mount will be run in each test to set the schemata to 100%.

To support the usage when MBM test does not unmount/remount resctrl
filesystem before the test starts, fix to call write_schemata() and
set schemata properly when the function is called for the first time.

Also, remove static local variable 'num_of_runs' because this is not
needed as there is resctrl_val_param->num_of_runs which should be used
instead like in cat_setup().

Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
 tools/testing/selftests/resctrl/mbm_test.c  | 13 +++++++------
 tools/testing/selftests/resctrl/resctrlfs.c |  4 +++-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 8392e5c55ed0..6d550f012829 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -89,23 +89,24 @@ static int check_results(int span)
 static int mbm_setup(int num, ...)
 {
 	struct resctrl_val_param *p;
-	static int num_of_runs;
 	va_list param;
 	int ret = 0;
 
-	/* Run NUM_OF_RUNS times */
-	if (num_of_runs++ >= NUM_OF_RUNS)
-		return -1;
-
 	va_start(param, num);
 	p = va_arg(param, struct resctrl_val_param *);
 	va_end(param);
 
+	/* Run NUM_OF_RUNS times */
+	if (p->num_of_runs >= NUM_OF_RUNS)
+		return -1;
+
 	/* Set up shemata with 100% allocation on the first run. */
-	if (num_of_runs == 0)
+	if (p->num_of_runs == 0)
 		ret = write_schemata(p->ctrlgrp, "100", p->cpu_no,
 				     p->resctrl_val);
 
+	p->num_of_runs++;
+
 	return ret;
 }
 
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 6f543e470ad4..8546bc9f1786 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -498,6 +498,7 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
 	FILE *fp;
 
 	if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) &&
+	    strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) &&
 	    strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) &&
 	    strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
 		return -ENOENT;
@@ -523,7 +524,8 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
 	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) ||
 	    !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
 		sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata);
-	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)))
+	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) ||
+	    !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)))
 		sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata);
 
 	fp = fopen(controlgroup, "w");
-- 
2.27.0

Re: [PATCH v3 1/5] selftests/resctrl: Fix set up schemata with 100% allocation on first run in MBM test

[Reinette Chatre]

Hi Shaopeng,

On 11/1/2022 2:43 AM, Shaopeng Tan wrote:
> There is a comment "Set up shemata with 100% allocation on the first run"
> in function mbm_setup(), but there is an increment bug and the condition
> "num_of_runs == 0" will never be met and write_schemata() will never be
> called to set schemata to 100%. Even if write_schemata() is called in MBM
> test, since it is not supported for MBM test it does not set the schemata.
> This is currently fine because resctrl_val_parm->mum_resctrlfs is always 1
> and umount/mount will be run in each test to set the schemata to 100%.
> 
> To support the usage when MBM test does not unmount/remount resctrl
> filesystem before the test starts, fix to call write_schemata() and
> set schemata properly when the function is called for the first time.
> 
> Also, remove static local variable 'num_of_runs' because this is not
> needed as there is resctrl_val_param->num_of_runs which should be used
> instead like in cat_setup().
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>

Thank you very much.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

[PATCH v3 2/5] selftests/resctrl: Return MBA check result and make it to output message

[Shaopeng Tan]

Since MBA check result is not returned, the MBA test result message
is always output as "ok" regardless of whether the MBA check result is
true or false.

Make output message to be "not ok" if MBA check result is failed.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
 tools/testing/selftests/resctrl/mba_test.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 1a1bdb6180cf..5d14802add4d 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -51,7 +51,7 @@ static int mba_setup(int num, ...)
 	return 0;
 }
 
-static void show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
+static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
 {
 	int allocation, runs;
 	bool failed = false;
@@ -97,6 +97,8 @@ static void show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
 		       failed ? "Fail:" : "Pass:");
 	if (failed)
 		ksft_print_msg("At least one test failed\n");
+
+	return failed;
 }
 
 static int check_results(void)
@@ -132,9 +134,7 @@ static int check_results(void)
 
 	fclose(fp);
 
-	show_mba_info(bw_imc, bw_resc);
-
-	return 0;
+	return show_mba_info(bw_imc, bw_resc);
 }
 
 void mba_test_cleanup(void)
-- 
2.27.0

Re: [PATCH v3 2/5] selftests/resctrl: Return MBA check result and make it to output message

[Shuah Khan]

On 11/1/22 03:43, Shaopeng Tan wrote:
> Since MBA check result is not returned, the MBA test result message
> is always output as "ok" regardless of whether the MBA check result is
> true or false.
> 
> Make output message to be "not ok" if MBA check result is failed.
> 
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>   tools/testing/selftests/resctrl/mba_test.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
> index 1a1bdb6180cf..5d14802add4d 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -51,7 +51,7 @@ static int mba_setup(int num, ...)
>   	return 0;
>   }
>   
> -static void show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
> +static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
>   {
>   	int allocation, runs;
>   	bool failed = false;
> @@ -97,6 +97,8 @@ static void show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
>   		       failed ? "Fail:" : "Pass:");
>   	if (failed)
>   		ksft_print_msg("At least one test failed\n");
> +
> +	return failed;

Rename "failed" to "ret" - naming the variable "failed" is confusing.

>   }
>   
>   static int check_results(void)
> @@ -132,9 +134,7 @@ static int check_results(void)
>   
>   	fclose(fp);
>   
> -	show_mba_info(bw_imc, bw_resc);
> -
> -	return 0;
> +	return show_mba_info(bw_imc, bw_resc);
>   }
>   
>   void mba_test_cleanup(void)

With that change,

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

[PATCH v3 3/5] selftests/resctrl: Flush stdout file buffer before executing fork()

[Shaopeng Tan]

When a process has buffered output, a child process created by fork()
will also copy buffered output. When using kselftest framework,
the output (resctrl test result message) will be printed multiple times.

Add fflush() to flush out the buffered output before executing fork().

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
 tools/testing/selftests/resctrl/cat_test.c    | 1 +
 tools/testing/selftests/resctrl/resctrl_val.c | 1 +
 tools/testing/selftests/resctrl/resctrlfs.c   | 1 +
 3 files changed, 3 insertions(+)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 1c5e90c63254..6a8306b0a109 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -167,6 +167,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 		return errno;
 	}
 
+	fflush(stdout);
 	bm_pid = fork();
 
 	/* Set param values for child thread which will be allocated bitmask
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index b32b96356ec7..6948843bf995 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -629,6 +629,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
 	 * Fork to start benchmark, save child's pid so that it can be killed
 	 * when needed
 	 */
+	fflush(stdout);
 	bm_pid = fork();
 	if (bm_pid == -1) {
 		perror("# Unable to fork");
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 8546bc9f1786..d95688298469 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -678,6 +678,7 @@ int filter_dmesg(void)
 		perror("pipe");
 		return ret;
 	}
+	fflush(stdout);
 	pid = fork();
 	if (pid == 0) {
 		close(pipefds[0]);
-- 
2.27.0

Re: [PATCH v3 3/5] selftests/resctrl: Flush stdout file buffer before executing fork()

[Shuah Khan]

On 11/1/22 03:43, Shaopeng Tan wrote:
> When a process has buffered output, a child process created by fork()
> will also copy buffered output. When using kselftest framework,
> the output (resctrl test result message) will be printed multiple times.
> 
> Add fflush() to flush out the buffered output before executing fork().
> 
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>   tools/testing/selftests/resctrl/cat_test.c    | 1 +
>   tools/testing/selftests/resctrl/resctrl_val.c | 1 +
>   tools/testing/selftests/resctrl/resctrlfs.c   | 1 +
>   3 files changed, 3 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 1c5e90c63254..6a8306b0a109 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -167,6 +167,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>   		return errno;
>   	}
>   
> +	fflush(stdout);
>   	bm_pid = fork();
>   
>   	/* Set param values for child thread which will be allocated bitmask
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index b32b96356ec7..6948843bf995 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -629,6 +629,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
>   	 * Fork to start benchmark, save child's pid so that it can be killed
>   	 * when needed
>   	 */
> +	fflush(stdout);
>   	bm_pid = fork();
>   	if (bm_pid == -1) {
>   		perror("# Unable to fork");
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 8546bc9f1786..d95688298469 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -678,6 +678,7 @@ int filter_dmesg(void)
>   		perror("pipe");
>   		return ret;
>   	}
> +	fflush(stdout);
>   	pid = fork();
>   	if (pid == 0) {
>   		close(pipefds[0]);

Good find. Looks good to me.

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

[PATCH v3 4/5] selftests/resctrl: Cleanup properly when an error occurs in CAT test

[Shaopeng Tan]

After creating a child process with fork() in CAT test, if there is
an error occurs or such as a SIGINT signal is received, the parent
process will be terminated immediately, but the child process will not
be killed and also umount_resctrlfs() will not be called.

Add a signal handler like other tests to kill child process, umount
resctrlfs, cleanup result files, etc. when an error occurs.

Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
 tools/testing/selftests/resctrl/cat_test.c | 28 +++++++++++++++-------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 6a8306b0a109..5f81817f4366 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -98,12 +98,21 @@ void cat_test_cleanup(void)
 	remove(RESULT_FILE_NAME2);
 }
 
+static void ctrl_handler(int signo)
+{
+	kill(bm_pid, SIGKILL);
+	umount_resctrlfs();
+	tests_cleanup();
+	ksft_print_msg("Ending\n\n");
+
+	exit(EXIT_SUCCESS);
+}
+
 int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 {
 	unsigned long l_mask, l_mask_1;
 	int ret, pipefd[2], sibling_cpu_no;
 	char pipe_message;
-	pid_t bm_pid;
 
 	cache_size = 0;
 
@@ -181,17 +190,19 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 		strcpy(param.filename, RESULT_FILE_NAME1);
 		param.num_of_runs = 0;
 		param.cpu_no = sibling_cpu_no;
+	} else {
+		/* set up ctrl-c handler */
+		if (signal(SIGINT, ctrl_handler) == SIG_ERR ||
+		    signal(SIGHUP, ctrl_handler) == SIG_ERR ||
+		    signal(SIGTERM, ctrl_handler) == SIG_ERR)
+			printf("Failed to catch SIGNAL!\n");
 	}
 
 	remove(param.filename);
 
 	ret = cat_val(&param);
-	if (ret)
-		return ret;
-
-	ret = check_results(&param);
-	if (ret)
-		return ret;
+	if (ret == 0)
+		ret = check_results(&param);
 
 	if (bm_pid == 0) {
 		/* Tell parent that child is ready */
@@ -201,7 +212,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 		    sizeof(pipe_message)) {
 			close(pipefd[1]);
 			perror("# failed signaling parent process");
-			return errno;
 		}
 
 		close(pipefd[1]);
@@ -226,5 +236,5 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 	if (bm_pid)
 		umount_resctrlfs();
 
-	return 0;
+	return ret;
 }
-- 
2.27.0

Re: [PATCH v3 4/5] selftests/resctrl: Cleanup properly when an error occurs in CAT test

[Shuah Khan]

On 11/1/22 03:43, Shaopeng Tan wrote:
> After creating a child process with fork() in CAT test, if there is
> an error occurs or such as a SIGINT signal is received, the parent
> process will be terminated immediately, but the child process will not
> be killed and also umount_resctrlfs() will not be called.
> 
> Add a signal handler like other tests to kill child process, umount
> resctrlfs, cleanup result files, etc. when an error occurs.
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>   tools/testing/selftests/resctrl/cat_test.c | 28 +++++++++++++++-------
>   1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 6a8306b0a109..5f81817f4366 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -98,12 +98,21 @@ void cat_test_cleanup(void)
>   	remove(RESULT_FILE_NAME2);
>   }
>   
> +static void ctrl_handler(int signo)
> +{
> +	kill(bm_pid, SIGKILL);
> +	umount_resctrlfs();
> +	tests_cleanup();
> +	ksft_print_msg("Ending\n\n");

Is there a reason to print this message? Remove it unless it serves
a purpose.

> +
> +	exit(EXIT_SUCCESS);
> +}
> +
>   int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>   {
>   	unsigned long l_mask, l_mask_1;
>   	int ret, pipefd[2], sibling_cpu_no;
>   	char pipe_message;
> -	pid_t bm_pid;

Odd. bm_pid is used below - why remove it here?

>   
>   	cache_size = 0;
>   
> @@ -181,17 +190,19 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>   		strcpy(param.filename, RESULT_FILE_NAME1);
>   		param.num_of_runs = 0;
>   		param.cpu_no = sibling_cpu_no;
> +	} else {
> +		/* set up ctrl-c handler */
> +		if (signal(SIGINT, ctrl_handler) == SIG_ERR ||
> +		    signal(SIGHUP, ctrl_handler) == SIG_ERR ||
> +		    signal(SIGTERM, ctrl_handler) == SIG_ERR)
> +			printf("Failed to catch SIGNAL!\n");

Is perror() more appropriate here?

>   	}
>   
>   	remove(param.filename);
>   
>   	ret = cat_val(&param);
> -	if (ret)
> -		return ret;
> -
> -	ret = check_results(&param);
> -	if (ret)
> -		return ret;
> +	if (ret == 0)
> +		ret = check_results(&param);

Why not use a goto in error case to do umount_resctrlfs() instead of changing
the conditionals?

>   
>   	if (bm_pid == 0) {
>   		/* Tell parent that child is ready */
> @@ -201,7 +212,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>   		    sizeof(pipe_message)) {
>   			close(pipefd[1]);
>   			perror("# failed signaling parent process");
> -			return errno;
>   		}
>   
>   		close(pipefd[1]);
> @@ -226,5 +236,5 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>   	if (bm_pid)
>   		umount_resctrlfs();
>   
> -	return 0;
> +	return ret;
>   }


With these changes made:

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

Re: [PATCH v3 4/5] selftests/resctrl: Cleanup properly when an error occurs in CAT test

[Reinette Chatre]

Hi Shaopeng and Shuah,

On 11/2/2022 2:41 AM, Shuah Khan wrote:
> On 11/1/22 03:43, Shaopeng Tan wrote:
>> After creating a child process with fork() in CAT test, if there is
>> an error occurs or such as a SIGINT signal is received, the parent
>> process will be terminated immediately, but the child process will not
>> be killed and also umount_resctrlfs() will not be called.
>>
>> Add a signal handler like other tests to kill child process, umount
>> resctrlfs, cleanup result files, etc. when an error occurs.
>>
>> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
>> ---
>>   tools/testing/selftests/resctrl/cat_test.c | 28 +++++++++++++++-------
>>   1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>> index 6a8306b0a109..5f81817f4366 100644
>> --- a/tools/testing/selftests/resctrl/cat_test.c
>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>> @@ -98,12 +98,21 @@ void cat_test_cleanup(void)
>>       remove(RESULT_FILE_NAME2);
>>   }
>>   +static void ctrl_handler(int signo)
>> +{
>> +    kill(bm_pid, SIGKILL);
>> +    umount_resctrlfs();
>> +    tests_cleanup();
>> +    ksft_print_msg("Ending\n\n");
> 
> Is there a reason to print this message? Remove it unless it serves
> a purpose.

This function appears to be a duplicate of existing
resctrl_val.c:ctrlc_handler(). Could the duplication be avoided
instead of refining the copy?

> 
>> +
>> +    exit(EXIT_SUCCESS);
>> +}
>> +
>>   int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>>   {
>>       unsigned long l_mask, l_mask_1;
>>       int ret, pipefd[2], sibling_cpu_no;
>>       char pipe_message;
>> -    pid_t bm_pid;
> 
> Odd. bm_pid is used below - why remove it here?

There is a global bm_pid in resctrl_val.c that is made available
via extern in resctrl.h. This is what causes this code to still
compile but I would also like to learn why moving to that is
desired as a change here. I expect such a big change to get a
mention in the commit message.

> 
>>         cache_size = 0;
>>   @@ -181,17 +190,19 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>>           strcpy(param.filename, RESULT_FILE_NAME1);
>>           param.num_of_runs = 0;
>>           param.cpu_no = sibling_cpu_no;
>> +    } else {
>> +        /* set up ctrl-c handler */
>> +        if (signal(SIGINT, ctrl_handler) == SIG_ERR ||
>> +            signal(SIGHUP, ctrl_handler) == SIG_ERR ||
>> +            signal(SIGTERM, ctrl_handler) == SIG_ERR)
>> +            printf("Failed to catch SIGNAL!\n");
> 
> Is perror() more appropriate here?

Should we be using signal() at all? "man signal" reads:
"WARNING: the behavior of signal() varies across UNIX versions,
and has also varied historically across different versions of Linux.
Avoid its use: use sigaction(2) instead."

"Failed to catch SIGNAL" also seems unclear to me. This is
where a signal handler is set up, the signal for which the handler
is installed has not arrived.


> 
>>       }
>>         remove(param.filename);
>>         ret = cat_val(&param);
>> -    if (ret)
>> -        return ret;
>> -
>> -    ret = check_results(&param);
>> -    if (ret)
>> -        return ret;
>> +    if (ret == 0)
>> +        ret = check_results(&param);
> 
> Why not use a goto in error case to do umount_resctrlfs() instead of changing
> the conditionals?

My understanding is the code that follows is needed to
synchronize the exits between the parent and child. It is the parent
that will run umount_resctrlfs() and it should only do so
after the child is done. A goto by the parent may thus cause
umount_resctrlfs() to be run while the child still relies on it while
a goto by the child may cause the parent not to receive the message
that the child is complete.

Reinette

RE: [PATCH v3 4/5] selftests/resctrl: Cleanup properly when an error occurs in CAT test

[Shaopeng Tan (Fujitsu)]

Hi Reinette and Shuah,

> On 11/2/2022 2:41 AM, Shuah Khan wrote:
> > On 11/1/22 03:43, Shaopeng Tan wrote:
> >> After creating a child process with fork() in CAT test, if there is
> >> an error occurs or such as a SIGINT signal is received, the parent
> >> process will be terminated immediately, but the child process will
> >> not be killed and also umount_resctrlfs() will not be called.
> >>
> >> Add a signal handler like other tests to kill child process, umount
> >> resctrlfs, cleanup result files, etc. when an error occurs.
> >>
> >> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> >> ---
> >>   tools/testing/selftests/resctrl/cat_test.c | 28
> >> +++++++++++++++-------
> >>   1 file changed, 19 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/resctrl/cat_test.c
> >> b/tools/testing/selftests/resctrl/cat_test.c
> >> index 6a8306b0a109..5f81817f4366 100644
> >> --- a/tools/testing/selftests/resctrl/cat_test.c
> >> +++ b/tools/testing/selftests/resctrl/cat_test.c
> >> @@ -98,12 +98,21 @@ void cat_test_cleanup(void)
> >>       remove(RESULT_FILE_NAME2);
> >>   }
> >>   +static void ctrl_handler(int signo)
> >> +{
> >> +    kill(bm_pid, SIGKILL);
> >> +    umount_resctrlfs();
> >> +    tests_cleanup();
> >> +    ksft_print_msg("Ending\n\n");
> >
> > Is there a reason to print this message? Remove it unless it serves a
> > purpose.
> 
> This function appears to be a duplicate of existing resctrl_val.c:ctrlc_handler().
> Could the duplication be avoided instead of refining the copy?

Yes, it's a duplicate of existing resctrl_val.c:ctrlc_handler().
I will try to use resctrl_val.c:ctrlc_handler() in next version patch series.

> >> +
> >> +    exit(EXIT_SUCCESS);
> >> +}
> >> +
> >>   int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
> >>   {
> >>       unsigned long l_mask, l_mask_1;
> >>       int ret, pipefd[2], sibling_cpu_no;
> >>       char pipe_message;
> >> -    pid_t bm_pid;
> >
> > Odd. bm_pid is used below - why remove it here?
> 
> There is a global bm_pid in resctrl_val.c that is made available via extern in
> resctrl.h. This is what causes this code to still compile but I would also like to
> learn why moving to that is desired as a change here. I expect such a big
> change to get a mention in the commit message.

Yes. I used global bm_pid, since bm_pid is used in ctrl_handler() before this function cat_perf_miss_val().
I will add a description to commit message.

> >>         cache_size = 0;
> >>   @@ -181,17 +190,19 @@ int cat_perf_miss_val(int cpu_no, int n, char
> >> *cache_type)
> >>           strcpy(param.filename, RESULT_FILE_NAME1);
> >>           param.num_of_runs = 0;
> >>           param.cpu_no = sibling_cpu_no;
> >> +    } else {
> >> +        /* set up ctrl-c handler */
> >> +        if (signal(SIGINT, ctrl_handler) == SIG_ERR ||
> >> +            signal(SIGHUP, ctrl_handler) == SIG_ERR ||
> >> +            signal(SIGTERM, ctrl_handler) == SIG_ERR)
> >> +            printf("Failed to catch SIGNAL!\n");
> >
> > Is perror() more appropriate here?
> 
> Should we be using signal() at all? "man signal" reads:
> "WARNING: the behavior of signal() varies across UNIX versions, and has also
> varied historically across different versions of Linux.
> Avoid its use: use sigaction(2) instead."
> 
> "Failed to catch SIGNAL" also seems unclear to me. This is where a signal
> handler is set up, the signal for which the handler is installed has not arrived.

I will use sigaction(2) and perror().

> >>       }
> >>         remove(param.filename);
> >>         ret = cat_val(&param);
> >> -    if (ret)
> >> -        return ret;
> >> -
> >> -    ret = check_results(&param);
> >> -    if (ret)
> >> -        return ret;
> >> +    if (ret == 0)
> >> +        ret = check_results(&param);
> >
> > Why not use a goto in error case to do umount_resctrlfs() instead of
> > changing the conditionals?
> 
> My understanding is the code that follows is needed to synchronize the exits
> between the parent and child. It is the parent that will run umount_resctrlfs()
> and it should only do so after the child is done. A goto by the parent may thus
> cause
> umount_resctrlfs() to be run while the child still relies on it while a goto by the
> child may cause the parent not to receive the message that the child is
> complete.

Yes, the code that follows is needed to synchronize the exits between the parent and child.

> > @@ -201,7 +212,6 @@ int cat_perf_miss_val(int cpu_no, int n, char
> *cache_type)
> >  		    sizeof(pipe_message)) {
> >  			close(pipefd[1]);
> >  			perror("# failed signaling parent process");
> > -			return errno;
> 
> It looks like pipefd[1] will be closed twice if the write() failed.

This close(pipefd[1]); should also be removed.

> It does look strange to let the child continue to its infinite loop after the write()
> failed. I assume that it is because the parent will also be stuck and the new
> ctrl_handler() is expected to unblock both?

If a SIGNAL is received, ctrl_handler() will be called to kill the child process and exit parent process.
If no SIGNAL is received, the parent process will kill the child process. (by else{kill(bm_pid, SIGKILL);})

> Could you please add a comment to the code to clarify this flow?
I will add comments here.

Best regards,
Shaopeng Tan

Re: [PATCH v3 4/5] selftests/resctrl: Cleanup properly when an error occurs in CAT test

[Reinette Chatre]

Hi Shaopeng,

On 11/1/2022 2:43 AM, Shaopeng Tan wrote:
> After creating a child process with fork() in CAT test, if there is
> an error occurs or such as a SIGINT signal is received, the parent

I find the above hard to read. How about "..., if an error occurs or
a signal such as SIGINT is received, ..."

> process will be terminated immediately, but the child process will not
> be killed and also umount_resctrlfs() will not be called.
> 
> Add a signal handler like other tests to kill child process, umount
> resctrlfs, cleanup result files, etc. when an error occurs.
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  tools/testing/selftests/resctrl/cat_test.c | 28 +++++++++++++++-------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 

...

> @@ -201,7 +212,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>  		    sizeof(pipe_message)) {
>  			close(pipefd[1]);
>  			perror("# failed signaling parent process");
> -			return errno;

It looks like pipefd[1] will be closed twice if the write() failed.

It does look strange to let the child continue to its infinite loop
after the write() failed. I assume that it is because the parent will
also be stuck and the new ctrl_handler() is expected to unblock both?
Could you please add a comment to the code to clarify this flow?

Thank you very much

Reinette

[PATCH v3 5/5] selftests/resctrl: Remove duplicate codes that clear each test result file

[Shaopeng Tan]

Before exiting each test function(run_cmt/cat/mbm/mba_test()),
test results("ok","not ok") are printed by ksft_test_result() and then
temporary result files are cleaned by function
cmt/cat/mbm/mba_test_cleanup().
However, before running ksft_test_result(),
function cmt/cat/mbm/mba_test_cleanup()
has been run in each test function as follows:
  cmt_resctrl_val()
  cat_perf_miss_val()
  mba_schemata_change()
  mbm_bw_change()

Remove duplicate codes that clear each test result file.

Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
---
 tools/testing/selftests/resctrl/resctrl_tests.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index df0d8d8526fc..8732cf736528 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -88,7 +88,6 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
 	ksft_test_result(!res, "MBM: bw change\n");
 	if ((get_vendor() == ARCH_INTEL) && res)
 		ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
-	mbm_test_cleanup();
 }
 
 static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
@@ -107,7 +106,6 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
 		sprintf(benchmark_cmd[1], "%d", span);
 	res = mba_schemata_change(cpu_no, bw_report, benchmark_cmd);
 	ksft_test_result(!res, "MBA: schemata change\n");
-	mba_test_cleanup();
 }
 
 static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
@@ -126,7 +124,6 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
 	ksft_test_result(!res, "CMT: test\n");
 	if ((get_vendor() == ARCH_INTEL) && res)
 		ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
-	cmt_test_cleanup();
 }
 
 static void run_cat_test(int cpu_no, int no_of_bits)
@@ -142,7 +139,6 @@ static void run_cat_test(int cpu_no, int no_of_bits)
 
 	res = cat_perf_miss_val(cpu_no, no_of_bits, "L3");
 	ksft_test_result(!res, "CAT: test\n");
-	cat_test_cleanup();
 }
 
 int main(int argc, char **argv)
-- 
2.27.0

Re: [PATCH v3 5/5] selftests/resctrl: Remove duplicate codes that clear each test result file

[Shuah Khan]

On 11/1/22 03:43, Shaopeng Tan wrote:
> Before exiting each test function(run_cmt/cat/mbm/mba_test()),
> test results("ok","not ok") are printed by ksft_test_result() and then
> temporary result files are cleaned by function
> cmt/cat/mbm/mba_test_cleanup().
> However, before running ksft_test_result(),
> function cmt/cat/mbm/mba_test_cleanup()
> has been run in each test function as follows:
>    cmt_resctrl_val()
>    cat_perf_miss_val()
>    mba_schemata_change()
>    mbm_bw_change()
> 
> Remove duplicate codes that clear each test result file.

This isn't making much sense to me. Please include test report before
and after this change in the change log.

> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---


thanks,
-- Shuah

Re: [PATCH v3 5/5] selftests/resctrl: Remove duplicate codes that clear each test result file

[Reinette Chatre]

Hi Shaopeng,

On 11/1/2022 2:43 AM, Shaopeng Tan wrote:
> Before exiting each test function(run_cmt/cat/mbm/mba_test()),
> test results("ok","not ok") are printed by ksft_test_result() and then
> temporary result files are cleaned by function
> cmt/cat/mbm/mba_test_cleanup().
> However, before running ksft_test_result(),
> function cmt/cat/mbm/mba_test_cleanup()
> has been run in each test function as follows:
>   cmt_resctrl_val()
>   cat_perf_miss_val()
>   mba_schemata_change()
>   mbm_bw_change()
> 
> Remove duplicate codes that clear each test result file.
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_tests.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index df0d8d8526fc..8732cf736528 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -88,7 +88,6 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
>  	ksft_test_result(!res, "MBM: bw change\n");
>  	if ((get_vendor() == ARCH_INTEL) && res)
>  		ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
> -	mbm_test_cleanup();
>  }
>  

From what I can tell this still seem to suffer from the problem where
the test files may not be cleaned. With the removal of mbm_test_cleanup()
the cleanup is now expected to be done in mbm_bw_change().

Note that:

mbm_bw_change()
{
	...

	ret = resctrl_val(benchmark_cmd, &param);
	if (ret)
		return ret;
	
	/* Test results stored in file */	

	ret = check_results(span);
	if (ret) 
		return ret; <== Return without cleaning test result file

	mbm_test_cleanup(); <== Test result file cleaned only when test passed.

	return 0;		
}



>  static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
> @@ -107,7 +106,6 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
>  		sprintf(benchmark_cmd[1], "%d", span);
>  	res = mba_schemata_change(cpu_no, bw_report, benchmark_cmd);
>  	ksft_test_result(!res, "MBA: schemata change\n");
> -	mba_test_cleanup();
>  }

mba_schemata_change() has the same pattern as mbm_bw_change() so test result
files may linger when test fails.

>  
>  static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
> @@ -126,7 +124,6 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
>  	ksft_test_result(!res, "CMT: test\n");
>  	if ((get_vendor() == ARCH_INTEL) && res)
>  		ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
> -	cmt_test_cleanup();
>  }

Same pattern again.

>  
>  static void run_cat_test(int cpu_no, int no_of_bits)
> @@ -142,7 +139,6 @@ static void run_cat_test(int cpu_no, int no_of_bits)
>  
>  	res = cat_perf_miss_val(cpu_no, no_of_bits, "L3");
>  	ksft_test_result(!res, "CAT: test\n");
> -	cat_test_cleanup();
>  }

Patch 4 makes this work. Thanks for doing that.

Reinette

RE: [PATCH v3 5/5] selftests/resctrl: Remove duplicate codes that clear each test result file

[Shaopeng Tan (Fujitsu)]

Hi Shuah and Reinette,

> On 11/1/2022 2:43 AM, Shaopeng Tan wrote:
> > Before exiting each test function(run_cmt/cat/mbm/mba_test()),
> > test results("ok","not ok") are printed by ksft_test_result() and then
> > temporary result files are cleaned by function
> > cmt/cat/mbm/mba_test_cleanup().
> > However, before running ksft_test_result(), function
> > cmt/cat/mbm/mba_test_cleanup() has been run in each test function as
> > follows:
> >    cmt_resctrl_val()
> >    cat_perf_miss_val()
> >    mba_schemata_change()
> >    mbm_bw_change()
> >
> > Remove duplicate codes that clear each test result file.
> 
> This isn't making much sense to me. Please include test report before and after
> this change in the change log.

With or without this patch, there is no effect on the result message.
These functions were executed twice, in brief, it runs as follows:
 - cmt/cat/mbm/mba_test_cleanup()
 - ksft_test_result()
 - cmt/cat/mbm/mba_test_cleanup()
So, I deleted once. 

> From what I can tell this still seem to suffer from the problem where the test
> files may not be cleaned. With the removal of mbm_test_cleanup() the cleanup
> is now expected to be done in mbm_bw_change().
> 
> Note that:
> 
> mbm_bw_change()
> {
> 	...
> 
> 	ret = resctrl_val(benchmark_cmd, &param);
> 	if (ret)
> 		return ret;
> 
> 	/* Test results stored in file */
> 
> 	ret = check_results(span);
> 	if (ret)
> 		return ret; <== Return without cleaning test result file
> 
> 	mbm_test_cleanup(); <== Test result file cleaned only when test
> passed.
> 
> 	return 0;
> }

I intend to avoid this problem through the following codes.

mbm_bw_change()
{
        ret = resctrl_val(benchmark_cmd, &param);
        if (ret)
-               return ret;
+               goto out;

        ret = check_results(span);
        if (ret)
-               return ret;
+               goto out;

+out:
        mbm_test_cleanup();

-       return 0;
+       return ret;
}


Best regards,
Shaopeng Tan

Re: [PATCH v3 5/5] selftests/resctrl: Remove duplicate codes that clear each test result file

[Reinette Chatre]

Hi Shaopeng,

On 11/8/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote:
> Hi Shuah and Reinette,
> 
>> On 11/1/2022 2:43 AM, Shaopeng Tan wrote:
>>> Before exiting each test function(run_cmt/cat/mbm/mba_test()),
>>> test results("ok","not ok") are printed by ksft_test_result() and then
>>> temporary result files are cleaned by function
>>> cmt/cat/mbm/mba_test_cleanup().
>>> However, before running ksft_test_result(), function
>>> cmt/cat/mbm/mba_test_cleanup() has been run in each test function as
>>> follows:
>>>    cmt_resctrl_val()
>>>    cat_perf_miss_val()
>>>    mba_schemata_change()
>>>    mbm_bw_change()
>>>
>>> Remove duplicate codes that clear each test result file.
>>
>> This isn't making much sense to me. Please include test report before and after
>> this change in the change log.
> 
> With or without this patch, there is no effect on the result message.
> These functions were executed twice, in brief, it runs as follows:
>  - cmt/cat/mbm/mba_test_cleanup()
>  - ksft_test_result()
>  - cmt/cat/mbm/mba_test_cleanup()
> So, I deleted once. 
> 
>> From what I can tell this still seem to suffer from the problem where the test
>> files may not be cleaned. With the removal of mbm_test_cleanup() the cleanup
>> is now expected to be done in mbm_bw_change().
>>
>> Note that:
>>
>> mbm_bw_change()
>> {
>> 	...
>>
>> 	ret = resctrl_val(benchmark_cmd, &param);
>> 	if (ret)
>> 		return ret;
>>
>> 	/* Test results stored in file */
>>
>> 	ret = check_results(span);
>> 	if (ret)
>> 		return ret; <== Return without cleaning test result file
>>
>> 	mbm_test_cleanup(); <== Test result file cleaned only when test
>> passed.
>>
>> 	return 0;
>> }
> 
> I intend to avoid this problem through the following codes.
> 
> mbm_bw_change()
> {
>         ret = resctrl_val(benchmark_cmd, &param);
>         if (ret)
> -               return ret;
> +               goto out;
> 
>         ret = check_results(span);
>         if (ret)
> -               return ret;
> +               goto out;
> 
> +out:
>         mbm_test_cleanup();
> 
> -       return 0;
> +       return ret;
> }
> 

Yes, even though file removal may now encounter ENOENT this
does seem the most robust route and the possible error is ok
since mbm_test_cleanup() does not check the return code.
Could you please replicate this pattern to the other functions
(mba_schemata_change() and cmt_resctrl_val()) also?

Reinette

RE: [PATCH v3 5/5] selftests/resctrl: Remove duplicate codes that clear each test result file

[Shaopeng Tan (Fujitsu)]

Hi Reinette,

> On 11/8/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote:
> > Hi Shuah and Reinette,
> >
> >> On 11/1/2022 2:43 AM, Shaopeng Tan wrote:
> >>> Before exiting each test function(run_cmt/cat/mbm/mba_test()),
> >>> test results("ok","not ok") are printed by ksft_test_result() and
> >>> then temporary result files are cleaned by function
> >>> cmt/cat/mbm/mba_test_cleanup().
> >>> However, before running ksft_test_result(), function
> >>> cmt/cat/mbm/mba_test_cleanup() has been run in each test function as
> >>> follows:
> >>>    cmt_resctrl_val()
> >>>    cat_perf_miss_val()
> >>>    mba_schemata_change()
> >>>    mbm_bw_change()
> >>>
> >>> Remove duplicate codes that clear each test result file.
> >>
> >> This isn't making much sense to me. Please include test report before
> >> and after this change in the change log.
> >
> > With or without this patch, there is no effect on the result message.
> > These functions were executed twice, in brief, it runs as follows:
> >  - cmt/cat/mbm/mba_test_cleanup()
> >  - ksft_test_result()
> >  - cmt/cat/mbm/mba_test_cleanup()
> > So, I deleted once.
> >
> >> From what I can tell this still seem to suffer from the problem where
> >> the test files may not be cleaned. With the removal of
> >> mbm_test_cleanup() the cleanup is now expected to be done in
> mbm_bw_change().
> >>
> >> Note that:
> >>
> >> mbm_bw_change()
> >> {
> >> 	...
> >>
> >> 	ret = resctrl_val(benchmark_cmd, &param);
> >> 	if (ret)
> >> 		return ret;
> >>
> >> 	/* Test results stored in file */
> >>
> >> 	ret = check_results(span);
> >> 	if (ret)
> >> 		return ret; <== Return without cleaning test result file
> >>
> >> 	mbm_test_cleanup(); <== Test result file cleaned only when test
> >> passed.
> >>
> >> 	return 0;
> >> }
> >
> > I intend to avoid this problem through the following codes.
> >
> > mbm_bw_change()
> > {
> >         ret = resctrl_val(benchmark_cmd, &param);
> >         if (ret)
> > -               return ret;
> > +               goto out;
> >
> >         ret = check_results(span);
> >         if (ret)
> > -               return ret;
> > +               goto out;
> >
> > +out:
> >         mbm_test_cleanup();
> >
> > -       return 0;
> > +       return ret;
> > }
> >
> 
> Yes, even though file removal may now encounter ENOENT this does seem the
> most robust route and the possible error is ok since mbm_test_cleanup() does
> not check the return code.
> Could you please replicate this pattern to the other functions
> (mba_schemata_change() and cmt_resctrl_val()) also?

This is an example for MBM, I intended to replicate this pattern to other tests.

Best regard,
Shaopeng Tan