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@xxxxxxxxxx? 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@xxxxxxxxxx>
> ---
>  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