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(-)
--
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 false.
Make output message to be "not ok" if MBA check result is failed.
Reviewed-by: Reinette Chatre <[email protected]>
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 1a1bdb6180cf..5d14802add4d 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -51,7 +51,7 @@ static int mba_setup(int num, ...)
return 0;
}
-static void show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
+static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
{
int allocation, runs;
bool failed = false;
@@ -97,6 +97,8 @@ static void show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
failed ? "Fail:" : "Pass:");
if (failed)
ksft_print_msg("At least one test failed\n");
+
+ return failed;
}
static int check_results(void)
@@ -132,9 +134,7 @@ static int check_results(void)
fclose(fp);
- show_mba_info(bw_imc, bw_resc);
-
- return 0;
+ return show_mba_info(bw_imc, bw_resc);
}
void mba_test_cleanup(void)
--
2.27.0
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 <[email protected]>
---
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;
}
--
2.27.0
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 <[email protected]>
---
tools/testing/selftests/resctrl/resctrl_tests.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index df0d8d8526fc..8732cf736528 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -88,7 +88,6 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
ksft_test_result(!res, "MBM: bw change\n");
if ((get_vendor() == ARCH_INTEL) && res)
ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
- mbm_test_cleanup();
}
static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
@@ -107,7 +106,6 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
sprintf(benchmark_cmd[1], "%d", span);
res = mba_schemata_change(cpu_no, bw_report, benchmark_cmd);
ksft_test_result(!res, "MBA: schemata change\n");
- mba_test_cleanup();
}
static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
@@ -126,7 +124,6 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
ksft_test_result(!res, "CMT: test\n");
if ((get_vendor() == ARCH_INTEL) && res)
ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
- cmt_test_cleanup();
}
static void run_cat_test(int cpu_no, int no_of_bits)
@@ -142,7 +139,6 @@ static void run_cat_test(int cpu_no, int no_of_bits)
res = cat_perf_miss_val(cpu_no, no_of_bits, "L3");
ksft_test_result(!res, "CAT: test\n");
- cat_test_cleanup();
}
int main(int argc, char **argv)
--
2.27.0
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 <[email protected]>
> ---
> 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/[email protected]/
- search for "please be careful".
Reinette
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 <[email protected]>
> ---
> 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
Hi Shaopeng,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on shuah-kselftest/next]
[also build test WARNING on linus/master v6.0 next-20221010]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Shaopeng-Tan/Some-improvements-of-resctrl-selftest/20221005-094642
base: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce:
# https://github.com/intel-lab-lkp/linux/commit/6e383feff6f15ba9a8c35fcb3fd284172a29971e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Shaopeng-Tan/Some-improvements-of-resctrl-selftest/20221005-094642
git checkout 6e383feff6f15ba9a8c35fcb3fd284172a29971e
make O=/tmp/kselftest headers
make O=/tmp/kselftest -C tools/testing/selftests
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
mbm_test.c: In function 'mbm_setup':
>> mbm_test.c:96:14: warning: 'p' is used uninitialized [-Wuninitialized]
96 | if (p->num_of_runs >= NUM_OF_RUNS)
| ~^~~~~~~~~~~~~
--
0-DAY CI Kernel Test Service
https://01.org/lkp