lkml.org 
[lkml]   [2022]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate
    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?

    Thanks.

    -Fenghua

    \
     
     \ /
      Last update: 2022-05-18 18:38    [W:2.229 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site