Re: [PATCH next] arm_mpam: Fix MBA alloc_capable handling

[Zeng Heng]

Hi James, Ben:

On 2026/4/18 0:07, James Morse wrote:
> Hi Zeng, Ben,
>
> On 17/04/2026 10:52, Ben Horgan wrote:
> > On 4/13/26 10:00, Zeng Heng wrote:
> > > The resctrl_arch_set_cdp_enabled() is never invoked with RDT_RESOURCE_MBA
> > > as the rid parameter. Consequently,
> > > mpam_resctrl_controls[RDT_RESOURCE_MBA].cdp_enabled always remains false,
> > > making it unreliable for conditional checks.
> > >
> > > The original logic uses this field to determine MBA state, causing a bug
> > > where MBA allocation is permanently disabled after the mount with CDP
> > > option. Remounting without CDP cannot restore the MBA partition capability.
> >
> > Ah, I see. Thanks for spotting this and sending a patch.
>
> This shape of bug is likely - I've not had access to a platform with bandwidth
> monitors!
>
>
> > > Fix by using the cdp_enabled global parameter directly. Enable MBA
> > > partition only when CDP is disabled and the MBA resource class is
> > > present. Disable MBA partition otherwise.
>
> > > diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
> > > index a9938006d0e6..161fb8905e28 100644
> > > --- a/drivers/resctrl/mpam_resctrl.c
> > > +++ b/drivers/resctrl/mpam_resctrl.c
> > > @@ -217,12 +217,10 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level rid, bool enable)
> > >   		l3->mon.num_rmid = resctrl_arch_system_num_rmid_idx();
> > >   
> > >   	/* The mbw_max feature can't hide cdp as it's a per-partid maximum. */
> > > -	if (cdp_enabled && !mpam_resctrl_controls[RDT_RESOURCE_MBA].cdp_enabled)
> > > -		mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = false;
> > > -
> > > -	if (mpam_resctrl_controls[RDT_RESOURCE_MBA].cdp_enabled &&
> > > -	    mpam_resctrl_controls[RDT_RESOURCE_MBA].class)
> > > +	if (!cdp_enabled && mpam_resctrl_controls[RDT_RESOURCE_MBA].class)
> > >   		mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = true;
> > > +	else
> > > +		mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = false;
>
> My reading of the original:
> |	if (mpam_resctrl_controls[RDT_RESOURCE_MBA].cdp_enabled &&
> |	    mpam_resctrl_controls[RDT_RESOURCE_MBA].class)
> |		mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = true;
>
> is that it was trying to do this reset - but never running. (as Zeng's commit message
> describes).
>
> I've spoken to Ben - and the intent here is a bit more complicated...
>
> if resctrl does start calling:
> | resctrl_arch_set_cdp_enabled(RDT_RESOURCE_MBA, true)
>
> then we expect to receive two configurations, one for code and one for data, so the
> aliasing controls problem goes away. The above original code is re-enabling MBA in this
> case. It needs to ignore the global 'cdp_enabled' to be immune to the order of
> resctrl_arch_set_cdp_enabled() calls.
>
> While this might seem odd, as resctrl doesn't do this today - it is a valid enum value,
> and this is in the woolly "are we Xeon shaped" area.
>
> I think long term we need a list of non-aliasing controls (min and max etc) which need
> disabling when CDP enabled. But re-enabling them when called explicitly makes sense as
> we have to handle those enum value anyway.

The above comments and explanations look reasonable to me. Thanks to
all for the thorough review.


Thanks for your time,
Zeng Heng