| Message ID | 20220914015147.3071025-2-tan.shaopeng@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Some improvements of resctrl selftest | expand |
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
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)
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(-)