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] 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] 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-rc7
Difference from v1: [patch 1] Make write_schemata() always be called, and use resctrl_val_param->num_of_runs instead of static num_of_runs. [patch 2] Add Reviewed-by tag. [patch 3] Remove cat/cmt/mbm/mba_test_cleanup() from run_cmt/mbm/mba_test() and modify changelog. [patch 4] Add Reviewed-by tag.
Notice that I dropped the one patch from v1 in this series ("[PATCH 4/5] selftests/resctrl: Kill the child process before exiting the parent process if an exception occurs"). This is because the bug will take some time to fix, I will submit it separately in the future.
Shaopeng Tan (4): selftests/resctrl: Fix set up shemata with 100% allocation on first run in MBM test. selftests/resctrl: Return MBA check result and make it to output message selftests/resctrl: Remove duplicate codes that clear each test result file selftests/resctrl: Flush stdout file buffer before executing fork()
tools/testing/selftests/resctrl/cat_test.c | 1 + tools/testing/selftests/resctrl/mba_test.c | 8 ++++---- tools/testing/selftests/resctrl/mbm_test.c | 6 +++--- tools/testing/selftests/resctrl/resctrl_tests.c | 4 ---- tools/testing/selftests/resctrl/resctrl_val.c | 1 + tools/testing/selftests/resctrl/resctrlfs.c | 1 + 6 files changed, 10 insertions(+), 11 deletions(-)
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%. This is currently fine because resctl_val_parm->num_resctrlfs is always 1 and umount/mount will be run in each test to set the schemata to 100%.
To make mbm_setup() future code-change proof, fix to call write-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 resctl_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 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index 8392e5c55ed0..4a54be314402 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -89,12 +89,11 @@ 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) + if (p->num_of_runs >= NUM_OF_RUNS) return -1;
va_start(param, num); @@ -102,9 +101,10 @@ static int mbm_setup(int num, ...) va_end(param);
/* 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; }
Hi Shaopeng,
I understand there is a typo in the code you are modifying but I do not think that justifies the same typo in the subject: shemata -> schemata
On 10/4/2022 6:39 PM, 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%. This is currently fine because resctl_val_parm->num_resctrlfs is always 1 and umount/mount will be run
resctl_val_parm -> resctrl_val_param num_resctrlfs -> mum_resctrlfs
in each test to set the schemata to 100%.
To make mbm_setup() future code-change proof, fix to call
You could be more specific by indicating that this change will support the usage when the test does not unmount/remount resctrl before the test.
write-schemata() properly when the function is called for the first time.
write-schemata() -> write_schemata()
Also, remove static local variable 'num_of_runs' because this is not needed as there is resctl_val_param->num_of_runs which should be used
resctl_val_param -> resctrl_val_param
instead like in cat_setup().
With the move to using a member from the struct I think it would make the code easier to understand if num_of_runs is explicitly initialized. That is indeed the norm when looking at the other call sites.
Signed-off-by: Shaopeng Tan tan.shaopeng@jp.fujitsu.com
tools/testing/selftests/resctrl/mbm_test.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index 8392e5c55ed0..4a54be314402 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -89,12 +89,11 @@ static int check_results(int span) static int mbm_setup(int num, ...) { struct resctrl_val_param *p;
p is defined here ...
- static int num_of_runs; va_list param; int ret = 0;
/* Run NUM_OF_RUNS times */
- if (num_of_runs++ >= NUM_OF_RUNS)
- if (p->num_of_runs >= NUM_OF_RUNS)
p is used here _before_ it is initialized in the code that follows.
return -1;va_start(param, num); @@ -102,9 +101,10 @@ static int mbm_setup(int num, ...) va_end(param); /* 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; }
Reinette
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)
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)
Hi Shaopeng,
On 10/4/2022 6:39 PM, 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();
} 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)
I think this is the right direction ... but you fell into the trap that I warned you about in https://lore.kernel.org/lkml/bdb19cf6-dd4b-2042-7cda-7f6108e543aa@intel.com/ - search for "please be careful".
Reinette
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 6f543e470ad4..c7447cd2a25f 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -676,6 +676,7 @@ int filter_dmesg(void) perror("pipe"); return ret; } + fflush(stdout); pid = fork(); if (pid == 0) { close(pipefds[0]);
linux-kselftest-mirror@lists.linaro.org
-
Reinette Chatre -
Shaopeng Tan