| Message ID | 20230413072259.2089348-2-tan.shaopeng@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Some improvements of resctrl selftest | expand |
On Thu, 13 Apr 2023, Shaopeng Tan wrote: > There is a comment "Set up shemata with 100% allocation on the first run" s/shemata/schemata/ > 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%. Even if write_schemata() is called in MBM > test, since it is not supported for MBM test it does not set the schemata. > This is currently fine because resctrl_val_parm->mum_resctrlfs is always 1 > and umount/mount will be run in each test to set the schemata to 100%. > > To support the usage when MBM test does not unmount/remount resctrl > filesystem before the test starts, fix to call write_schemata() and > set 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 resctrl_val_param->num_of_runs which should be used > instead like in cat_setup(). > > Reviewed-by: Reinette Chatre <reinette.chatre@intel.com> > Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com> > --- > tools/testing/selftests/resctrl/mbm_test.c | 13 +++++++------ > tools/testing/selftests/resctrl/resctrlfs.c | 4 +++- > 2 files changed, 10 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c > index c9dfa54af42f..9b591c35310f 100644 > --- a/tools/testing/selftests/resctrl/mbm_test.c > +++ b/tools/testing/selftests/resctrl/mbm_test.c > @@ -89,23 +89,24 @@ 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 END_OF_TESTS; > - > va_start(param, num); > p = va_arg(param, struct resctrl_val_param *); > va_end(param); > > + /* Run NUM_OF_RUNS times */ > + if (p->num_of_runs >= NUM_OF_RUNS) > + return -1; This is not rebased correctly, negative return values mean now errors. To complete tests "normally", return END_OF_TESTS. Please see commit fa10366cc6f4 ("selftests/resctrl: Allow ->setup() to return errors") for future information. With the forementioned problems fixed: Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
On 4/14/2023 5:31 AM, Ilpo Järvinen wrote: > On Thu, 13 Apr 2023, Shaopeng Tan wrote: > >> There is a comment "Set up shemata with 100% allocation on the first run" > > s/shemata/schemata/ Having the typo may be ok since this is a verbatim (typo and all) copy of an existing comment. >> 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%. Even if write_schemata() is called in MBM >> test, since it is not supported for MBM test it does not set the schemata. >> This is currently fine because resctrl_val_parm->mum_resctrlfs is always 1 >> and umount/mount will be run in each test to set the schemata to 100%. >> >> To support the usage when MBM test does not unmount/remount resctrl >> filesystem before the test starts, fix to call write_schemata() and >> set 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 resctrl_val_param->num_of_runs which should be used >> instead like in cat_setup(). >> >> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com> >> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com> >> --- >> tools/testing/selftests/resctrl/mbm_test.c | 13 +++++++------ >> tools/testing/selftests/resctrl/resctrlfs.c | 4 +++- >> 2 files changed, 10 insertions(+), 7 deletions(-) >> >> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c >> index c9dfa54af42f..9b591c35310f 100644 >> --- a/tools/testing/selftests/resctrl/mbm_test.c >> +++ b/tools/testing/selftests/resctrl/mbm_test.c >> @@ -89,23 +89,24 @@ 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 END_OF_TESTS; >> - >> va_start(param, num); >> p = va_arg(param, struct resctrl_val_param *); >> va_end(param); >> >> + /* Run NUM_OF_RUNS times */ >> + if (p->num_of_runs >= NUM_OF_RUNS) >> + return -1; > > This is not rebased correctly, negative return values mean now errors. > To complete tests "normally", return END_OF_TESTS. Please see commit > fa10366cc6f4 ("selftests/resctrl: Allow ->setup() to return errors") for > future information. > > With the forementioned problems fixed: > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Thanks for catching this. Unfortunately this was just merged so a separate fix will be needed. Reinette
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index c9dfa54af42f..9b591c35310f 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -89,23 +89,24 @@ 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 END_OF_TESTS; - va_start(param, num); p = va_arg(param, struct resctrl_val_param *); va_end(param); + /* Run NUM_OF_RUNS times */ + if (p->num_of_runs >= NUM_OF_RUNS) + return -1; + /* 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; } diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index cc6cf49e3129..b31b1d9e41d1 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -498,6 +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, MBM_STR, sizeof(MBM_STR)) && strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) && strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) return -ENOENT; @@ -523,7 +524,8 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val) if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) || !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata); - if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) + if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) || + !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata); fp = fopen(controlgroup, "w");