Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate

[Stephane Eranian <eranian@google.com>]

On Wed, May 18, 2022 at 7:36 PM Fenghua Yu <fenghua.yu@intel.com> wrote:
>
> Hi, Babu,
>
> On Tue, May 17, 2022 at 11:10:40AM -0700, Reinette Chatre wrote:
> > On 5/17/2022 10:27 AM, Fenghua Yu wrote:
> > > On Tue, May 17, 2022 at 09:49:22AM -0700, Reinette Chatre wrote:
> > >> On 5/17/2022 9:33 AM, Fenghua Yu wrote:
> > >>> On Mon, May 16, 2022 at 05:12:34PM -0700, Stephane Eranian wrote:
> > >>>> AMD supports cbm with no bits set as reflected in rdt_init_res_defs_amd() by:
> > >>> ...
> > >>>> @@ -107,6 +107,10 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
> > >>>>          first_bit = find_first_bit(&val, cbm_len);
> > >>>>          zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
> > >>>>
> > >>>> +        /* no need to check bits if arch supports no bits set */
> > >>>> +        if (r->cache.arch_has_empty_bitmaps && val == 0)
> > >>>> +                goto done;
> > >>>> +
> > >>>>          /* Are non-contiguous bitmaps allowed? */
> > >>>>          if (!r->cache.arch_has_sparse_bitmaps &&
> > >>>>              (find_next_bit(&val, cbm_len, zero_bit) < cbm_len)) {
> > >>>> @@ -119,7 +123,7 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
> > >>>>                                      r->cache.min_cbm_bits);
> > >>>>                  return false;
> > >>>>          }
> > >>>> -
> > >>>> +done:
> > >>>>          *data = val;
> > >>>>          return true;
> > >>>>  }
> > >>>
> > >>> Isn't it AMD supports 0 minimal CBM bits? Then should set its min_cbm_bits as 0.
> > >>> Is the following patch a better fix? I don't have AMD machine and cannot
> > >>> test the patch.
> > >>>
> > >>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > >>> index 6055d05af4cc..031d77dd982d 100644
> > >>> --- a/arch/x86/kernel/cpu/resctrl/core.c
> > >>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > >>> @@ -909,6 +909,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;
> > >>
> > >> That is actually what Stephane's V1 [1] did and I proposed that
> > >> he fixes it with (almost) what he has in V2 (I think the check
> > >> can be moved earlier before any bits are searched for).
> > >>
> > >> The reasons why I proposed this change are:
> > >> - min_cbm_bits is a value that is exposed to user space and from the
> > >>   time AMD was supported this has always been 1 for those systems. I
> > >>   do not know how user space uses this value and unless I can be certain
> > >>   making this 0 will not affect user space I would prefer not to
> > >>   make such a change.
> > >
> > > But a user visible mismatch is created by the V2 patch:
> >
> > No. V2 does not create a user visible change, it restores original behavior.
> >
> > > User queries min_cbm_bits and finds it is 1 but turns out 0 can be written
> > > to the schemata.
> > >
> > > Is it an acceptable behavior? Shouldn't user read right min_cbm_bits (0)
> > > on AMD?
> >
> > Original AMD enabling set min_cbm_bits as 1 while also supporting 0 to
> > be written to schemata file. Supporting 0 to be written to schemata file
> > was unintentionally broken during one of the MPAM prep patches. This
> > patch fixes that.
> >
> > You are proposing a change to original AMD support that impacts user
> > space API while I find fixing to restore original behavior to
> > be the safest option. Perhaps Babu could pick the preferred solution
> > and I would step aside if you prefer to go with (an improved) V1.
>
> Is it OK for you to set min_cbm_bits to 0 on AMD?
>
I agree with Fenghua here. The proper fix is my v1 which sets the
min_cbm_bits to the value it should have set to from the beginning.
This field is exposed via resctrl fs and it is still fine if the value
changes. Any app is supposed to read the content of the file and not
hardcode
any value or pre-suppose it is always 1.