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)
--
2.27.0Hi 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
© 2016 - 2026 Red Hat, Inc.