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(-)
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)
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
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
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
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
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
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)
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
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
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);
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)))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
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
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; }
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
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
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
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
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
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(¶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; }
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
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
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]);
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
linux-kselftest-mirror@lists.linaro.org
-
Reinette Chatre -
Shaopeng Tan -
tan.shaopeng＠fujitsu.com