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
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 <[email protected]>
---
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
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 <[email protected]>
---
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(¶m);
if (ret)
- return ret;
+ goto out;
ret = check_results(¶m);
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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 <[email protected]>
Thank you Shaopeng.
Reviewed-by: Reinette Chatre <[email protected]>
Reinette
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
Thank you very much for catching this. The fix looks good,
I only have nitpicks about the changelog.
Reviewed-by: Reinette Chatre <[email protected]>
Reinette
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 <[email protected]>
> > ---
>
> Thank you very much for catching this. The fix looks good, I only have nitpicks
> about the changelog.
>
> Reviewed-by: Reinette Chatre <[email protected]>
Thanks.
Best Regards,
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 <[email protected]>
> > ---
> > 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
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
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 <[email protected]>
> > ---
> > 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
Hi Shaopeng,
On 9/27/2022 2:01 AM, [email protected] 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
Hi Shaopeng,
On 9/27/2022 2:01 AM, [email protected] 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
Hi Reinette,
> On 9/27/2022 2:01 AM, [email protected] 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
Hi Reinette,
> On 9/27/2022 2:01 AM, [email protected] 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
Hi Shaopeng,
On 9/28/2022 10:28 PM, [email protected] wrote:
>> On 9/27/2022 2:01 AM, [email protected] 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
Hi Shaopeng,
On 9/28/2022 10:28 PM, [email protected] wrote:
>> On 9/27/2022 2:01 AM, [email protected] 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