[PATCH] x86/resctrl: fix min_cbm_bits for AMD

[PATCH] x86/resctrl: fix min_cbm_bits for AMD

[Stephane Eranian]

AMD supports cbm with no bits set as reflected in rdt_init_res_defs_amd() by:

	r->cache.arch_has_empty_bitmaps = true;

However given the unified code in cbm_validate(), checking for

    val == 0 && !arch_has_empty_bitmaps

is not enough because you also have in cbm_validate():
	if ((zero_bit - first_bit) < r->cache.min_cbm_bits)

Default value for if r->cache.min_cbm_bits = 1

Leading to:

$ cd /sys/fs/resctrl
$ mkdir foo
$ cd foo
$ echo L3:0=0 > schemata
-bash: echo: write error: Invalid argument

Patch initializes r->cache.min_cbm_bits to 0 for AMD.

Signed-off-by: Stephane Eranian <eranian@google.com>
---
 arch/x86/kernel/cpu/resctrl/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index bb1c3f5f60c8..14782361ebb7 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -897,6 +897,7 @@ static __init void rdt_init_res_defs_amd(void)
 			r->cache.arch_has_sparse_bitmaps = true;
 			r->cache.arch_has_empty_bitmaps = true;
 			r->cache.arch_has_per_cpu_cfg = true;
+			r->cache.min_cbm_bits = 0;
 		} else if (r->rid == RDT_RESOURCE_MBA) {
 			hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
 			hw_res->msr_update = mba_wrmsr_amd;
-- 
2.36.0.550.gb090851708-goog

Re: [PATCH] x86/resctrl: fix min_cbm_bits for AMD

[Reinette Chatre]

+ x86 maintainers

Hi Stephane,

Thank you very much for catching this. While the fix is onto something
I would prefer the fix to be obvious and not a side effect of bit
checking in an empty bitmap.

When resubmitting, please ensure the subject starts with an uppercase letter.

Also, when resubmitting, could you please add x86@kernel.org? The resctrl
patches are routed upstream by the x86 architecture maintainers.

On 5/15/2022 10:50 PM, Stephane Eranian wrote:
> AMD supports cbm with no bits set as reflected in rdt_init_res_defs_amd() by:

Please watch the line lengths. Getting a pass from
scripts/checkpatch.pl would help.

> 
> 	r->cache.arch_has_empty_bitmaps = true;
> 

With the above the needed information is present. Changing min_cbm_bits
is not required.

> However given the unified code in cbm_validate(), checking for
> 
>     val == 0 && !arch_has_empty_bitmaps
> 
> is not enough because you also have in cbm_validate():
> 	if ((zero_bit - first_bit) < r->cache.min_cbm_bits)

You are correct, but that relies on checking of bits in a bitmap
where no bits are set so this fix relies on the failure cases of the earlier
find_first_bit() and find_next_zero_bit() to be used. I find that it
obscures the scenario being handled.

The code should be clear and to that end I think it would be simpler to
explicitly check that an empty bitmap is provided and not search
for bits at all when that is the case.

Something like this before the bit parsing starts:
	if (r->cache.arch_has_empty_bitmaps && val == 0)
		goto done;

	/* Skip bit parsing */

done:
	*data = val;
	return true;

> 
> Default value for if r->cache.min_cbm_bits = 1
> 
> Leading to:
> 
> $ cd /sys/fs/resctrl
> $ mkdir foo
> $ cd foo
> $ echo L3:0=0 > schemata
> -bash: echo: write error: Invalid argument
> 
> Patch initializes r->cache.min_cbm_bits to 0 for AMD.
> 

Seems like a Fixes tag is needed.
Fixes: 316e7f901f5a ("x86/resctrl: Add struct rdt_cache::arch_has_{sparse, empty}_bitmaps")

> Signed-off-by: Stephane Eranian <eranian@google.com>
> ---
>  arch/x86/kernel/cpu/resctrl/core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index bb1c3f5f60c8..14782361ebb7 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -897,6 +897,7 @@ static __init void rdt_init_res_defs_amd(void)
>  			r->cache.arch_has_sparse_bitmaps = true;
>  			r->cache.arch_has_empty_bitmaps = true;
>  			r->cache.arch_has_per_cpu_cfg = true;
> +			r->cache.min_cbm_bits = 0;
>  		} else if (r->rid == RDT_RESOURCE_MBA) {
>  			hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
>  			hw_res->msr_update = mba_wrmsr_amd;

Reinette

Re: [PATCH] x86/resctrl: fix min_cbm_bits for AMD

[Stephane Eranian]

On Mon, May 16, 2022 at 9:35 AM Reinette Chatre
<reinette.chatre@intel.com> wrote:
>
> + x86 maintainers
>
> Hi Stephane,
>
> Thank you very much for catching this. While the fix is onto something
> I would prefer the fix to be obvious and not a side effect of bit
> checking in an empty bitmap.
>
Are you asking for me to add a comment on the modified line or are you asking
for a change in cbm_validate()? There, I could add an empty_bitmask check to
that if.

> When resubmitting, please ensure the subject starts with an uppercase letter.
>
ok.

> Also, when resubmitting, could you please add x86@kernel.org? The resctrl
> patches are routed upstream by the x86 architecture maintainers.
>
ok.

> On 5/15/2022 10:50 PM, Stephane Eranian wrote:
> > AMD supports cbm with no bits set as reflected in rdt_init_res_defs_amd() by:
>
> Please watch the line lengths. Getting a pass from
> scripts/checkpatch.pl would help.
>
> >
> >       r->cache.arch_has_empty_bitmaps = true;
> >
>
> With the above the needed information is present. Changing min_cbm_bits
> is not required.
>
> > However given the unified code in cbm_validate(), checking for
> >
> >     val == 0 && !arch_has_empty_bitmaps
> >
> > is not enough because you also have in cbm_validate():
> >       if ((zero_bit - first_bit) < r->cache.min_cbm_bits)
>
> You are correct, but that relies on checking of bits in a bitmap
> where no bits are set so this fix relies on the failure cases of the earlier
> find_first_bit() and find_next_zero_bit() to be used. I find that it
> obscures the scenario being handled.
>
> The code should be clear and to that end I think it would be simpler to
> explicitly check that an empty bitmap is provided and not search
> for bits at all when that is the case.
>
> Something like this before the bit parsing starts:
>         if (r->cache.arch_has_empty_bitmaps && val == 0)
>                 goto done;
>
>         /* Skip bit parsing */
>
> done:
>         *data = val;
>         return true;
>
> >
> > Default value for if r->cache.min_cbm_bits = 1
> >
> > Leading to:
> >
> > $ cd /sys/fs/resctrl
> > $ mkdir foo
> > $ cd foo
> > $ echo L3:0=0 > schemata
> > -bash: echo: write error: Invalid argument
> >
> > Patch initializes r->cache.min_cbm_bits to 0 for AMD.
> >
>
> Seems like a Fixes tag is needed.
> Fixes: 316e7f901f5a ("x86/resctrl: Add struct rdt_cache::arch_has_{sparse, empty}_bitmaps")
>
> > Signed-off-by: Stephane Eranian <eranian@google.com>
> > ---
> >  arch/x86/kernel/cpu/resctrl/core.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > index bb1c3f5f60c8..14782361ebb7 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -897,6 +897,7 @@ static __init void rdt_init_res_defs_amd(void)
> >                       r->cache.arch_has_sparse_bitmaps = true;
> >                       r->cache.arch_has_empty_bitmaps = true;
> >                       r->cache.arch_has_per_cpu_cfg = true;
> > +                     r->cache.min_cbm_bits = 0;
> >               } else if (r->rid == RDT_RESOURCE_MBA) {
> >                       hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
> >                       hw_res->msr_update = mba_wrmsr_amd;
>
> Reinette

Re: [PATCH] x86/resctrl: fix min_cbm_bits for AMD

[Reinette Chatre]

Hi Stephane,

On 5/16/2022 12:07 PM, Stephane Eranian wrote:
> On Mon, May 16, 2022 at 9:35 AM Reinette Chatre
> <reinette.chatre@intel.com> wrote:

...

>> Thank you very much for catching this. While the fix is onto something
>> I would prefer the fix to be obvious and not a side effect of bit
>> checking in an empty bitmap.
>>
> Are you asking for me to add a comment on the modified line or are you asking
> for a change in cbm_validate()? There, I could add an empty_bitmask check to
> that if.

Please check my original response for inline comments to your patch.

I am asking for a change in cbm_validate() and my response did include some
sample code copied below. 

>> Something like this before the bit parsing starts:
>>         if (r->cache.arch_has_empty_bitmaps && val == 0)
>>                 goto done;
>>
>>         /* Skip bit parsing */
>>
>> done:
>>         *data = val;
>>         return true;
>>

Reinette