[PATCH 0/5] selftests/resctrl: Some improvements of resctrl selftest

[PATCH 0/5] selftests/resctrl: Some improvements of resctrl selftest

[Shaopeng Tan]

Hello,

The aim of this patch series is to improve the resctrl selftest.
The first three patches clear redundant code. 
The last two patches are bug fixes. Without the two fixes, 
some unnecessary processing will be executed and test results 
will be confusing. There is no behavior change in test themselves.
[patch 1] Because the default schemata is 100% , in MBM test
          it is not necessary to reset schemata by write_schemata().
[patch 2] Delete CMT-related processing in write_schemata() which is 
	  not called by CMT.
[patch 3] Before exiting each test CMT/CAT/MBM/MBA, clear test result 
	  files function cat/cmt/mbm/mba_test_cleanup() are called twice. 
	  Delete once.
[patch 4] If there is an exception occurs after creating a child 
	  process with fork() in the CAT test, kill the child process 
	  before terminating the parent process.
[patch 5] When a child process is created by fork(), the buffer of the 
	  parent process is also copied. Flush the buffer before executing fork().

This patch series is based on Linux v6.0-rc5

Shaopeng Tan (5):
  selftests/resctrl: Clear unused initalization code in MBM tests
  selftests/resctrl: Clear unused common codes called by CAT/MBA tests
  testing/selftests: Remove duplicate codes that clear each test result
    file
  selftests/resctrl: Kill the child process before exiting the parent
    process if an exception occurs
  selftests/resctrl: Flush stdout file buffer before executing fork()

 tools/testing/selftests/resctrl/cat_test.c    | 17 +++++++++--------
 tools/testing/selftests/resctrl/cmt_test.c    |  2 --
 tools/testing/selftests/resctrl/mba_test.c    |  2 --
 tools/testing/selftests/resctrl/mbm_test.c    | 19 ++++++-------------
 tools/testing/selftests/resctrl/resctrl_val.c |  1 +
 tools/testing/selftests/resctrl/resctrlfs.c   |  7 +++----
 6 files changed, 19 insertions(+), 29 deletions(-)

-- 
2.27.0

[PATCH 1/5] selftests/resctrl: Clear unused initalization code in MBM tests

[Shaopeng Tan]

There is a comment "Set up shemata with 100% allocation on the first run"
in function mbm_setup(), but the condition "num_of_runs == 0" will
never be met and write_schemata() will never be called to set schemata
to 100%.

Since umount/mount resctrl file system is run on each resctrl test,
at the same time the default schemata will also be set to 100%.

Clear unused initialization code in MBM test, such as CMT test.

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

diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 8392e5c55ed0..38a3b3ad1c76 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -89,24 +89,19 @@ 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);
 
-	/* Set up shemata with 100% allocation on the first run. */
-	if (num_of_runs == 0)
-		ret = write_schemata(p->ctrlgrp, "100", p->cpu_no,
-				     p->resctrl_val);
+	/* Run NUM_OF_RUNS times */
+	if (p->num_of_runs >= NUM_OF_RUNS)
+		return -1;
+
+	p->num_of_runs++;
 
-	return ret;
+	return 0;
 }
 
 void mbm_test_cleanup(void)
-- 
2.27.0

[PATCH] 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 fail.

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

This patch is based on Linux v6.0-rc5

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 93ffacb416df..e7dfeb697e5e 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

[PATCH 2/5] selftests/resctrl: Clear unused common codes called by CAT/MBA tests

[Shaopeng Tan]

In CAT/MBA(allocation) tests, function write_schemata() is used to
change the percentage of schemata.
In CMT/MBM(monitoring) tests schemata only need to be set 100% once,
and the default value of schemata is 100% which is set by executing
mount/umout resctrl filesystem.
In addition, write_schemata() was not currently called from CMT.

Clean up unused CMT-related processing in function write_schemata().

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

diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 6f543e470ad4..349dce00472f 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -498,8 +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, CAT_STR, sizeof(CAT_STR)) &&
-	    strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
+	    strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
 		return -ENOENT;
 
 	if (!schemata) {
@@ -520,8 +519,7 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
 	else
 		sprintf(controlgroup, "%s/schemata", RESCTRL_PATH);
 
-	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) ||
-	    !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
+	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
 		sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata);
 	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)))
 		sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata);
-- 
2.27.0

[PATCH 3/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 are printed by ksft_print_msg() and then temporary result
files are cleaned by function cmt/cat/mbm/mba_test_cleanup().
However, before running ksft_print_msg(), 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/cat_test.c | 1 -
 tools/testing/selftests/resctrl/cmt_test.c | 2 --
 tools/testing/selftests/resctrl/mba_test.c | 2 --
 tools/testing/selftests/resctrl/mbm_test.c | 2 --
 4 files changed, 7 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index 1c5e90c63254..d1134f66469f 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -221,7 +221,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 		kill(bm_pid, SIGKILL);
 	}
 
-	cat_test_cleanup();
 	if (bm_pid)
 		umount_resctrlfs();
 
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index 8968e36db99d..b3b17fb876f4 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -139,7 +139,5 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
 	if (ret)
 		return ret;
 
-	cmt_test_cleanup();
-
 	return 0;
 }
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 1a1bdb6180cf..93ffacb416df 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -166,7 +166,5 @@ int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd)
 	if (ret)
 		return ret;
 
-	mba_test_cleanup();
-
 	return 0;
 }
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 38a3b3ad1c76..a453db4c221f 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -134,7 +134,5 @@ int mbm_bw_change(int span, int cpu_no, char *bw_report, char **benchmark_cmd)
 	if (ret)
 		return ret;
 
-	mbm_test_cleanup();
-
 	return 0;
 }
-- 
2.27.0

[PATCH 4/5] selftests/resctrl: Kill the child process before exiting the parent process if an exception occurs

[Shaopeng Tan]

After creating a child process with fork() in CAT test, if there is
an exception occurs, the parent process will be terminated immediately,
but the child process will not be killed and umount_resctrlfs() will not
be called.

When fork() is used in CMT/MBA/MBM tests.
If an exception occurs, before parent process exiting,
the child process is killed and umount_resctrlfs() is called.

Kill the child process before exiting the parent process
if an exception occurs in CAT test, like in CMT/MBA/MBM tests.

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

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
index d1134f66469f..f62da445acbb 100644
--- a/tools/testing/selftests/resctrl/cat_test.c
+++ b/tools/testing/selftests/resctrl/cat_test.c
@@ -186,11 +186,11 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 
 	ret = cat_val(&param);
 	if (ret)
-		return ret;
+		goto out;
 
 	ret = check_results(&param);
 	if (ret)
-		return ret;
+		goto out;
 
 	if (bm_pid == 0) {
 		/* Tell parent that child is ready */
@@ -200,7 +200,8 @@ 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;
+			ret = errno;
+			goto out;
 		}
 
 		close(pipefd[1]);
@@ -218,11 +219,11 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
 			}
 		}
 		close(pipefd[0]);
-		kill(bm_pid, SIGKILL);
 	}
 
-	if (bm_pid)
-		umount_resctrlfs();
+out:
+	kill(bm_pid, SIGKILL);
+	umount_resctrlfs();
 
-	return 0;
+	return ret;
 }
-- 
2.27.0

[PATCH 5/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().

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 f62da445acbb..69d17f5f4606 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 349dce00472f..a8cea64de65e 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -674,6 +674,7 @@ int filter_dmesg(void)
 		perror("pipe");
 		return ret;
 	}
+	fflush(stdout);
 	pid = fork();
 	if (pid == 0) {
 		close(pipefds[0]);
-- 
2.27.0

Re: [PATCH 1/5] selftests/resctrl: Clear unused initalization code in MBM tests

[Reinette Chatre]

Hi Shaopeng,

(typo in Subject: initalization -> initialization)

On 9/13/2022 6:51 PM, Shaopeng Tan wrote:
> There is a comment "Set up shemata with 100% allocation on the first run"
> in function mbm_setup(), but the condition "num_of_runs == 0" will
> never be met and write_schemata() will never be called to set schemata
> to 100%.

Thanks for catching this.

> 
> Since umount/mount resctrl file system is run on each resctrl test,
> at the same time the default schemata will also be set to 100%.

This is the case when a test is run with struct
resctrl_val_param->mum_resctrlfs == 1, but if the test is run with
struct
resctrl_val_param->mum_resctrlfs == 0 then resctrl filesystem will
not be remounted.

I do think that this setup function should support both cases.

> 
> Clear unused initialization code in MBM test, such as CMT test.

Could the initialization code be fixed instead to increment
the number of runs later, similar to cat_setup()?

> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  tools/testing/selftests/resctrl/mbm_test.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index 8392e5c55ed0..38a3b3ad1c76 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -89,24 +89,19 @@ 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);
>  
> -	/* Set up shemata with 100% allocation on the first run. */
> -	if (num_of_runs == 0)
> -		ret = write_schemata(p->ctrlgrp, "100", p->cpu_no,
> -				     p->resctrl_val);
> +	/* Run NUM_OF_RUNS times */
> +	if (p->num_of_runs >= NUM_OF_RUNS)
> +		return -1;

You seem to be fixing two bugs in this patch, the first is described in the
commit message and the second is to use p->num_of_runs instead of the
local num_of_runs. Although, after a quick look I cannot see if 
struct resctrl_val_param->num_of_runs is used anywhere. Could you
please add description of these changes to the changelog?

> +
> +	p->num_of_runs++;
>  
> -	return ret;
> +	return 0;
>  }
>  
>  void mbm_test_cleanup(void)

Thank you

Reinette

Re: [PATCH 2/5] selftests/resctrl: Clear unused common codes called by CAT/MBA tests

[Reinette Chatre]

Hi Shaopeng,

On 9/13/2022 6:51 PM, Shaopeng Tan wrote:
> In CAT/MBA(allocation) tests, function write_schemata() is used to
> change the percentage of schemata.
> In CMT/MBM(monitoring) tests schemata only need to be set 100% once,
> and the default value of schemata is 100% which is set by executing
> mount/umout resctrl filesystem.
> In addition, write_schemata() was not currently called from CMT.

While this is all accurate it is not clear to me that this
justifies the removal of the support for changing the
schemata as part of a CMT test. 

From what I can tell write_schemata() is a a generic function that
currently supports all possible tests. If a later update needs to use
this for a CMT test then it should work.

I do not see any harm in leaving these checks.

> 
> Clean up unused CMT-related processing in function write_schemata().
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  tools/testing/selftests/resctrl/resctrlfs.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 6f543e470ad4..349dce00472f 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -498,8 +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, CAT_STR, sizeof(CAT_STR)) &&
> -	    strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
> +	    strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
>  		return -ENOENT;
>  
>  	if (!schemata) {
> @@ -520,8 +519,7 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
>  	else
>  		sprintf(controlgroup, "%s/schemata", RESCTRL_PATH);
>  
> -	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) ||
> -	    !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
> +	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
>  		sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata);
>  	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)))
>  		sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata);

Reinette

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

[Reinette Chatre]

Hi Shaopeng,

On 9/13/2022 6:51 PM, Shaopeng Tan wrote:
> Before exiting each test function(run_cmt/cat/mbm/mba_test()),
> test results are printed by ksft_print_msg() and then temporary result
> files are cleaned by function cmt/cat/mbm/mba_test_cleanup().
> However, before running ksft_print_msg(), function

before -> after?

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

Another good catch, thank you.

> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  tools/testing/selftests/resctrl/cat_test.c | 1 -
>  tools/testing/selftests/resctrl/cmt_test.c | 2 --
>  tools/testing/selftests/resctrl/mba_test.c | 2 --
>  tools/testing/selftests/resctrl/mbm_test.c | 2 --
>  4 files changed, 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 1c5e90c63254..d1134f66469f 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -221,7 +221,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>  		kill(bm_pid, SIGKILL);
>  	}
>  
> -	cat_test_cleanup();
>  	if (bm_pid)
>  		umount_resctrlfs();
>  

It makes it much easier to understand code if it is symmetrical. Since the files are
created within cat_perf_miss_val() I think it would be better to perform the cleanup
in the same function. So, keep this cleanup but remove the call from run_cat_test()
instead.

Similar for the cleanups below ... could you please keep them and instead
remove the duplicate cleanup from run_cmt/mbm/mba_test() instead?

When you do so, please be careful since it seems that there is (another!) bug where
the cleanup is not done if the test fails.

> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index 8968e36db99d..b3b17fb876f4 100644
> --- a/tools/testing/selftests/resctrl/cmt_test.c
> +++ b/tools/testing/selftests/resctrl/cmt_test.c
> @@ -139,7 +139,5 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
>  	if (ret)
>  		return ret;
>  
> -	cmt_test_cleanup();
> -
>  	return 0;
>  }
> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
> index 1a1bdb6180cf..93ffacb416df 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -166,7 +166,5 @@ int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd)
>  	if (ret)
>  		return ret;
>  
> -	mba_test_cleanup();
> -
>  	return 0;
>  }
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index 38a3b3ad1c76..a453db4c221f 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -134,7 +134,5 @@ int mbm_bw_change(int span, int cpu_no, char *bw_report, char **benchmark_cmd)
>  	if (ret)
>  		return ret;
>  
> -	mbm_test_cleanup();
> -
>  	return 0;
>  }

Thank you

Reinette

Re: [PATCH 4/5] selftests/resctrl: Kill the child process before exiting the parent process if an exception occurs

[Reinette Chatre]

Hi Shaopeng,

On 9/13/2022 6:51 PM, Shaopeng Tan wrote:

...

> @@ -218,11 +219,11 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>  			}
>  		}
>  		close(pipefd[0]);
> -		kill(bm_pid, SIGKILL);
>  	}
>  
> -	if (bm_pid)
> -		umount_resctrlfs();
> +out:
> +	kill(bm_pid, SIGKILL);
> +	umount_resctrlfs();
>  

From what I can tell both parent and child will now run this code. So
both will attempt to unmount resctrl fs and the child will attempt
to kill PID 0?

Reinette

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

[Reinette Chatre]

Hi Shaopeng,

On my side this patch arrived as an unnumbered sixth patch forming
part of a five patch series.

On 9/13/2022 6:51 PM, 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 fail.

fail -> false?

I think it should be either succeed/fail or true/false.

> 
> Make output message to be "not ok" if MBA check result is failed.
> 
> This patch is based on Linux v6.0-rc5

This should not be part of the changelog but instead be below the "---".

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

Thank you very much for catching this. The fix looks good,
I only have nitpicks about the changelog.

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

Reinette

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

[Reinette Chatre]

On 9/13/2022 6:51 PM, 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().
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>

Thank you Shaopeng.

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

Reinette

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

[tan.shaopeng]

Hi Reinette,

Thanks for your advice.

> On my side this patch arrived as an unnumbered sixth patch forming part of a
> five patch series.

In next version, I will add this patch into patch series.

> On 9/13/2022 6:51 PM, 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 fail.
> 
> fail -> false?

It is false.

> I think it should be either succeed/fail or true/false.
> 
> >
> > Make output message to be "not ok" if MBA check result is failed.
> >
> > This patch is based on Linux v6.0-rc5
> 
> This should not be part of the changelog but instead be below the "---".

Thanks.

> >
> > Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> > ---
> 
> Thank you very much for catching this. The fix looks good, I only have nitpicks
> about the changelog.
> 
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Thanks.

Best Regards,
Shaopeng

RE: [PATCH 1/5] selftests/resctrl: Clear unused initalization code in MBM tests

[tan.shaopeng]

Hi Reinette,

> (typo in Subject: initalization -> initialization)

Thanks.

> On 9/13/2022 6:51 PM, Shaopeng Tan wrote:
> > There is a comment "Set up shemata with 100% allocation on the first run"
> > in function mbm_setup(), but the condition "num_of_runs == 0" will
> > never be met and write_schemata() will never be called to set schemata
> > to 100%.
> 
> Thanks for catching this.
> 
> >
> > Since umount/mount resctrl file system is run on each resctrl test, at
> > the same time the default schemata will also be set to 100%.
> 
> This is the case when a test is run with struct
> resctrl_val_param->mum_resctrlfs == 1, but if the test is run with struct
> resctrl_val_param->mum_resctrlfs == 0 then resctrl filesystem will not be
> remounted.
> 
> I do think that this setup function should support both cases.

In mbm test(mbm_test.c), resctrl_val_param.mum_resctrlfs is set to 1 and never be changed,
and umount/mount resctrl file system is always executed.
So it is not necessary to run "if (num_of_runs == 0)".

> >
> > Clear unused initialization code in MBM test, such as CMT test.
> 
> Could the initialization code be fixed instead to increment the number of runs
> later, similar to cat_setup()?

In cat test(cat_test.c), resctrl_val_param.mum_resctrlfs is set to 0,
and cat test need to reset schemata by write_schemata().
MBM and CMT are monitoring test, and their resctrl_val_param.mum_resctrlfs is set to 1,
I think it is better to make mbm_setup() similar to cmt_setup() .

> >
> > Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> > ---
> >  tools/testing/selftests/resctrl/mbm_test.c | 17 ++++++-----------
> >  1 file changed, 6 insertions(+), 11 deletions(-)
> >
> > diff --git a/tools/testing/selftests/resctrl/mbm_test.c
> > b/tools/testing/selftests/resctrl/mbm_test.c
> > index 8392e5c55ed0..38a3b3ad1c76 100644
> > --- a/tools/testing/selftests/resctrl/mbm_test.c
> > +++ b/tools/testing/selftests/resctrl/mbm_test.c
> > @@ -89,24 +89,19 @@ 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);
> >
> > -	/* Set up shemata with 100% allocation on the first run. */
> > -	if (num_of_runs == 0)
> > -		ret = write_schemata(p->ctrlgrp, "100", p->cpu_no,
> > -				     p->resctrl_val);
> > +	/* Run NUM_OF_RUNS times */
> > +	if (p->num_of_runs >= NUM_OF_RUNS)
> > +		return -1;
> 
> You seem to be fixing two bugs in this patch, the first is described in the
> commit message and the second is to use p->num_of_runs instead of the
> local num_of_runs. Although, after a quick look I cannot see if struct
> resctrl_val_param->num_of_runs is used anywhere. Could you please add
> description of these changes to the changelog?

Your observation is right.
I will add description of num_of_runs to the changelog in the next version.

Best Regards,
Shaopeng

> > +
> > +	p->num_of_runs++;
> >
> > -	return ret;
> > +	return 0;
> >  }
> >
> >  void mbm_test_cleanup(void)
> 
> Thank you
> 
> Reinette

RE: [PATCH 2/5] selftests/resctrl: Clear unused common codes called by CAT/MBA tests

[tan.shaopeng]

Hi Reinette,

> On 9/13/2022 6:51 PM, Shaopeng Tan wrote:
> > In CAT/MBA(allocation) tests, function write_schemata() is used to
> > change the percentage of schemata.
> > In CMT/MBM(monitoring) tests schemata only need to be set 100% once,
> > and the default value of schemata is 100% which is set by executing
> > mount/umout resctrl filesystem.
> > In addition, write_schemata() was not currently called from CMT.
> 
> While this is all accurate it is not clear to me that this justifies the removal of the
> support for changing the schemata as part of a CMT test.
> 
> From what I can tell write_schemata() is a a generic function that currently
> supports all possible tests. If a later update needs to use this for a CMT test
> then it should work.
> 
> I do not see any harm in leaving these checks.

According to my research, whether clearing this code or not has no effect on the current program.
I cleared this code because it looks redundant. Because CMT test didn't call write_schemata().
I will remove this patch in the next version.

Best Regards,
Shaopeng

> >
> > Clean up unused CMT-related processing in function write_schemata().
> >
> > Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> > ---
> >  tools/testing/selftests/resctrl/resctrlfs.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c
> > b/tools/testing/selftests/resctrl/resctrlfs.c
> > index 6f543e470ad4..349dce00472f 100644
> > --- a/tools/testing/selftests/resctrl/resctrlfs.c
> > +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> > @@ -498,8 +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, CAT_STR, sizeof(CAT_STR)) &&
> > -	    strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
> > +	    strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
> >  		return -ENOENT;
> >
> >  	if (!schemata) {
> > @@ -520,8 +519,7 @@ int write_schemata(char *ctrlgrp, char *schemata, int
> cpu_no, char *resctrl_val)
> >  	else
> >  		sprintf(controlgroup, "%s/schemata", RESCTRL_PATH);
> >
> > -	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) ||
> > -	    !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
> > +	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
> >  		sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=',
> schemata);
> >  	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)))
> >  		sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=',
> schemata);
> 
> Reinette

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

[tan.shaopeng]

Hi Reinette,

> On 9/13/2022 6:51 PM, Shaopeng Tan wrote:
> > Before exiting each test function(run_cmt/cat/mbm/mba_test()),
> > test results are printed by ksft_print_msg() and then temporary result
> > files are cleaned by function cmt/cat/mbm/mba_test_cleanup().
> > However, before running ksft_print_msg(), function
> 
> before -> after?

I think it is "before".

> > 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.
> >
> 
> Another good catch, thank you.
> 
> > Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> > ---
> >  tools/testing/selftests/resctrl/cat_test.c | 1 -
> > tools/testing/selftests/resctrl/cmt_test.c | 2 --
> > tools/testing/selftests/resctrl/mba_test.c | 2 --
> > tools/testing/selftests/resctrl/mbm_test.c | 2 --
> >  4 files changed, 7 deletions(-)
> >
> > diff --git a/tools/testing/selftests/resctrl/cat_test.c
> > b/tools/testing/selftests/resctrl/cat_test.c
> > index 1c5e90c63254..d1134f66469f 100644
> > --- a/tools/testing/selftests/resctrl/cat_test.c
> > +++ b/tools/testing/selftests/resctrl/cat_test.c
> > @@ -221,7 +221,6 @@ int cat_perf_miss_val(int cpu_no, int n, char
> *cache_type)
> >  		kill(bm_pid, SIGKILL);
> >  	}
> >
> > -	cat_test_cleanup();
> >  	if (bm_pid)
> >  		umount_resctrlfs();
> >
> 
> It makes it much easier to understand code if it is symmetrical. Since the files
> are created within cat_perf_miss_val() I think it would be better to perform the
> cleanup in the same function. So, keep this cleanup but remove the call from
> run_cat_test() instead.
> 
> Similar for the cleanups below ... could you please keep them and instead
> remove the duplicate cleanup from run_cmt/mbm/mba_test() instead?
> 
> When you do so, please be careful since it seems that there is (another!) bug
> where the cleanup is not done if the test fails.

Thanks for your advices, I will remove the duplicate cleanups from run_cmt/mbm/mba_test().

Best Regards,
Shaopeng

> > diff --git a/tools/testing/selftests/resctrl/cmt_test.c
> > b/tools/testing/selftests/resctrl/cmt_test.c
> > index 8968e36db99d..b3b17fb876f4 100644
> > --- a/tools/testing/selftests/resctrl/cmt_test.c
> > +++ b/tools/testing/selftests/resctrl/cmt_test.c
> > @@ -139,7 +139,5 @@ int cmt_resctrl_val(int cpu_no, int n, char
> **benchmark_cmd)
> >  	if (ret)
> >  		return ret;
> >
> > -	cmt_test_cleanup();
> > -
> >  	return 0;
> >  }
> > diff --git a/tools/testing/selftests/resctrl/mba_test.c
> > b/tools/testing/selftests/resctrl/mba_test.c
> > index 1a1bdb6180cf..93ffacb416df 100644
> > --- a/tools/testing/selftests/resctrl/mba_test.c
> > +++ b/tools/testing/selftests/resctrl/mba_test.c
> > @@ -166,7 +166,5 @@ int mba_schemata_change(int cpu_no, char
> *bw_report, char **benchmark_cmd)
> >  	if (ret)
> >  		return ret;
> >
> > -	mba_test_cleanup();
> > -
> >  	return 0;
> >  }
> > diff --git a/tools/testing/selftests/resctrl/mbm_test.c
> > b/tools/testing/selftests/resctrl/mbm_test.c
> > index 38a3b3ad1c76..a453db4c221f 100644
> > --- a/tools/testing/selftests/resctrl/mbm_test.c
> > +++ b/tools/testing/selftests/resctrl/mbm_test.c
> > @@ -134,7 +134,5 @@ int mbm_bw_change(int span, int cpu_no, char
> *bw_report, char **benchmark_cmd)
> >  	if (ret)
> >  		return ret;
> >
> > -	mbm_test_cleanup();
> > -
> >  	return 0;
> >  }
> 
> Thank you
> 
> Reinette

RE: [PATCH 4/5] selftests/resctrl: Kill the child process before exiting the parent process if an exception occurs

[tan.shaopeng]

Hi Reinette,

> On 9/13/2022 6:51 PM, Shaopeng Tan wrote:
> 
> ...
> 
> > @@ -218,11 +219,11 @@ int cat_perf_miss_val(int cpu_no, int n, char
> *cache_type)
> >  			}
> >  		}
> >  		close(pipefd[0]);
> > -		kill(bm_pid, SIGKILL);
> >  	}
> >
> > -	if (bm_pid)
> > -		umount_resctrlfs();
> > +out:
> > +	kill(bm_pid, SIGKILL);
> > +	umount_resctrlfs();
> >
> 
> From what I can tell both parent and child will now run this code. So both will
> attempt to unmount resctrl fs and the child will attempt to kill PID 0?

Thanks for your advice. There are problems as you point out.
I think it is complicated if we fix this bug like MBM/MBA/CMT test using sigaction().
It seems this bug cannot be solved easily.
Please give me some time.

Best Regards,
Shaopeng

Re: [PATCH 1/5] selftests/resctrl: Clear unused initalization code in MBM tests

[Reinette Chatre]

Hi Shaopeng,

On 9/27/2022 2:01 AM, tan.shaopeng@fujitsu.com wrote:
>> On 9/13/2022 6:51 PM, Shaopeng Tan wrote:
>>> There is a comment "Set up shemata with 100% allocation on the first run"
>>> in function mbm_setup(), but the condition "num_of_runs == 0" will
>>> never be met and write_schemata() will never be called to set schemata
>>> to 100%.
>>
>> Thanks for catching this.
>>
>>>
>>> Since umount/mount resctrl file system is run on each resctrl test, at
>>> the same time the default schemata will also be set to 100%.
>>
>> This is the case when a test is run with struct
>> resctrl_val_param->mum_resctrlfs == 1, but if the test is run with struct
>> resctrl_val_param->mum_resctrlfs == 0 then resctrl filesystem will not be
>> remounted.
>>
>> I do think that this setup function should support both cases.
> 
> In mbm test(mbm_test.c), resctrl_val_param.mum_resctrlfs is set to 1 and never be changed,
> and umount/mount resctrl file system is always executed.
> So it is not necessary to run "if (num_of_runs == 0)".

This is true for the current usage. You could also add a warning here
("running test with stale config") if a future test sets mum_resctrlfs - but
with all the current output of the selftests a warning may be lost in the noise.

I think it would just be simpler to support both cases. Having the tests be more
robust is good.

Reinette

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

[Reinette Chatre]

Hi Shaopeng,

On 9/27/2022 2:01 AM, tan.shaopeng@fujitsu.com wrote:
> Hi Reinette,
> 
>> On 9/13/2022 6:51 PM, Shaopeng Tan wrote:
>>> Before exiting each test function(run_cmt/cat/mbm/mba_test()),
>>> test results are printed by ksft_print_msg() and then temporary result
>>> files are cleaned by function cmt/cat/mbm/mba_test_cleanup().
>>> However, before running ksft_print_msg(), function
>>
>> before -> after?
> 
> I think it is "before".

hmmm ... if cmt/cat/mbm/mba_test_cleanup() was run before
ksft_print_msg() then there would be no test results to print, no?
The current implementation runs cmt/cat/mbm/mba_test_cleanup()
after ksft_print_msg() ... albeit twice.

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

Reinette

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

[tan.shaopeng]

Hi Reinette,

> On 9/27/2022 2:01 AM, tan.shaopeng@fujitsu.com wrote:
> > Hi Reinette,
> >
> >> On 9/13/2022 6:51 PM, Shaopeng Tan wrote:
> >>> Before exiting each test function(run_cmt/cat/mbm/mba_test()),
> >>> test results are printed by ksft_print_msg() and then temporary
> >>> result files are cleaned by function cmt/cat/mbm/mba_test_cleanup().
> >>> However, before running ksft_print_msg(), function
> >>
> >> before -> after?
> >
> > I think it is "before".
> 
> hmmm ... if cmt/cat/mbm/mba_test_cleanup() was run before
> ksft_print_msg() then there would be no test results to print, no?
> The current implementation runs cmt/cat/mbm/mba_test_cleanup() after
> ksft_print_msg() ... albeit twice.


I am sorry I made a mistake in changelog.
It should be ksft_test_result() instead of ksft_print_msg().

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

An example of MBM test:
 resctrl_tests.c
  73 static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
  74                          int cpu_no, char *bw_report)
  75 {
  87         res = mbm_bw_change(span, cpu_no, bw_report, benchmark_cmd);
  88         ksft_test_result(!res, "MBM: bw change\n");
  89         if ((get_vendor() == ARCH_INTEL) && res)
  90                 ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
  91         mbm_test_cleanup();
  92 }
  
mbm_test.c
 117 int mbm_bw_change(int span, int cpu_no, char *bw_report, char **benchmark_cmd)
 118 {
 138         ret = check_results(span);
 142         mbm_test_cleanup();
 145 }
 
 50 static int check_results(int span)
 51 {
 82         ret = show_bw_info(bw_imc, bw_resc, span);
 87 }
* Test results saved in file RESULT_FILE_NAME are printed by ksft_print_msg() in function show_bw_info().

Best Regards,
Shaopeng

RE: [PATCH 1/5] selftests/resctrl: Clear unused initalization code in MBM tests

[tan.shaopeng]

Hi Reinette,

> On 9/27/2022 2:01 AM, tan.shaopeng@fujitsu.com wrote:
> >> On 9/13/2022 6:51 PM, Shaopeng Tan wrote:
> >>> There is a comment "Set up shemata with 100% allocation on the first run"
> >>> in function mbm_setup(), but the condition "num_of_runs == 0" will
> >>> never be met and write_schemata() will never be called to set
> >>> schemata to 100%.
> >>
> >> Thanks for catching this.
> >>
> >>>
> >>> Since umount/mount resctrl file system is run on each resctrl test,
> >>> at the same time the default schemata will also be set to 100%.
> >>
> >> This is the case when a test is run with struct
> >> resctrl_val_param->mum_resctrlfs == 1, but if the test is run with
> >> struct resctrl_val_param->mum_resctrlfs == 0 then resctrl filesystem
> >> will not be remounted.
> >>
> >> I do think that this setup function should support both cases.
> >
> > In mbm test(mbm_test.c), resctrl_val_param.mum_resctrlfs is set to 1
> > and never be changed, and umount/mount resctrl file system is always
> executed.
> > So it is not necessary to run "if (num_of_runs == 0)".
> 
> This is true for the current usage. You could also add a warning here ("running
> test with stale config") if a future test sets mum_resctrlfs - but with all the
> current output of the selftests a warning may be lost in the noise.
> 
> I think it would just be simpler to support both cases. Having the tests be more
> robust is good.

I understand that mum_resctrlfs should support both cases(0&1).

However, "num_of_runs++" is executed before "if (num_of_runs == 0)",
So write_schemata() is never executed regardless of mum_rectrlfs is 0 or 1.

97         if (num_of_runs++ >= NUM_OF_RUNS)
105         if (num_of_runs == 0)
106                 ret = write_schemata(p->ctrlgrp, "100", p->cpu_no,

I will fix this in the next version

Best Regards,
Shaopeng

Re: [PATCH 1/5] selftests/resctrl: Clear unused initalization code in MBM tests

[Reinette Chatre]

Hi Shaopeng,

On 9/28/2022 10:28 PM, tan.shaopeng@fujitsu.com wrote:
>> On 9/27/2022 2:01 AM, tan.shaopeng@fujitsu.com wrote:
>>>> On 9/13/2022 6:51 PM, Shaopeng Tan wrote:
>>>>> There is a comment "Set up shemata with 100% allocation on the first run"
>>>>> in function mbm_setup(), but the condition "num_of_runs == 0" will
>>>>> never be met and write_schemata() will never be called to set
>>>>> schemata to 100%.
>>>>
>>>> Thanks for catching this.
>>>>
>>>>>
>>>>> Since umount/mount resctrl file system is run on each resctrl test,
>>>>> at the same time the default schemata will also be set to 100%.
>>>>
>>>> This is the case when a test is run with struct
>>>> resctrl_val_param->mum_resctrlfs == 1, but if the test is run with
>>>> struct resctrl_val_param->mum_resctrlfs == 0 then resctrl filesystem
>>>> will not be remounted.
>>>>
>>>> I do think that this setup function should support both cases.
>>>
>>> In mbm test(mbm_test.c), resctrl_val_param.mum_resctrlfs is set to 1
>>> and never be changed, and umount/mount resctrl file system is always
>> executed.
>>> So it is not necessary to run "if (num_of_runs == 0)".
>>
>> This is true for the current usage. You could also add a warning here ("running
>> test with stale config") if a future test sets mum_resctrlfs - but with all the
>> current output of the selftests a warning may be lost in the noise.
>>
>> I think it would just be simpler to support both cases. Having the tests be more
>> robust is good.
> 
> I understand that mum_resctrlfs should support both cases(0&1).
> 
> However, "num_of_runs++" is executed before "if (num_of_runs == 0)",
> So write_schemata() is never executed regardless of mum_rectrlfs is 0 or 1.
> 
> 97         if (num_of_runs++ >= NUM_OF_RUNS)
> 105         if (num_of_runs == 0)
> 106                 ret = write_schemata(p->ctrlgrp, "100", p->cpu_no,
> 
> I will fix this in the next version
> 

Thank you very much.

Reinette

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

[Reinette Chatre]

Hi Shaopeng,

On 9/28/2022 10:28 PM, tan.shaopeng@fujitsu.com wrote:
>> On 9/27/2022 2:01 AM, tan.shaopeng@fujitsu.com wrote:
>>>> On 9/13/2022 6:51 PM, Shaopeng Tan wrote:
>>>>> Before exiting each test function(run_cmt/cat/mbm/mba_test()),
>>>>> test results are printed by ksft_print_msg() and then temporary
>>>>> result files are cleaned by function cmt/cat/mbm/mba_test_cleanup().
>>>>> However, before running ksft_print_msg(), function
>>>>
>>>> before -> after?
>>>
>>> I think it is "before".
>>
>> hmmm ... if cmt/cat/mbm/mba_test_cleanup() was run before
>> ksft_print_msg() then there would be no test results to print, no?
>> The current implementation runs cmt/cat/mbm/mba_test_cleanup() after
>> ksft_print_msg() ... albeit twice.
> 
> 
> I am sorry I made a mistake in changelog.
> It should be ksft_test_result() instead of ksft_print_msg().
> 
> Changelog:
> 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 is clear, thank you very much.

Reinette