Messages in this thread | | | Date | Tue, 17 May 2022 10:27:00 -0700 | From | Fenghua Yu <> | Subject | Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate |
| |
Hi, Reinette,
On Tue, May 17, 2022 at 09:49:22AM -0700, Reinette Chatre wrote: > Hi Fenghua, > > On 5/17/2022 9:33 AM, Fenghua Yu wrote: > > Hi, Eranian, > > > > 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: 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?
Without the V2 patch, at least min_cbm_bits and writing to the schemata are matched: only 1 about above bits can be searched and written.
By setting min_cbm_bits=0, it reflects the right value and user can see the value as 0 and set schemata as 0 as well. Seems all match each other.
> - this fix restores original behavior that was changed in the patch noted > in the Fixes link. > > - this fix itself relies on math on error returns of bit checking on an empty > bitmap. I find that hides what the code does and this fix is more obvious. > You can see this feedback in my response to V1. > > - a fix like the above snippet is incomplete. To be appropriate > the initialization of rdt_resources_all[] needs to be changed to > not initialize min_cbm_bits anymore and move the platform specific bits > to rdt_init_res_defs_amd() and rdt_init_res_defs_intel() respectively.
Maybe that's better.
> > > Reinette > > [1] https://lore.kernel.org/lkml/20220516055055.2734840-1-eranian@google.com/
Thanks.
-Fenghua
| |