Messages in this thread | | | From | Stephane Eranian <> | Date | Mon, 16 May 2022 12:07:31 -0700 | Subject | Re: [PATCH] x86/resctrl: fix min_cbm_bits for AMD |
| |
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
| |