drivers/resctrl/mpam_resctrl.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
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.
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.
Fixes: 6789fb99282c ("arm_mpam: resctrl: Add CDP emulation")
Signed-off-by: Zeng Heng <zengheng4@huawei.com>
---
drivers/resctrl/mpam_resctrl.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
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;
if (enable) {
if (mpam_partid_max < 1)
--
2.25.1
Hi Zeng,
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.
>
> 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.
>
> Fixes: 6789fb99282c ("arm_mpam: resctrl: Add CDP emulation")
> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> ---
> drivers/resctrl/mpam_resctrl.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> 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;
Isn't it more that the existing checks are correct (if not totally necessary) and guard against
resctrl_arch_set_cdp_enabled() being called with RDT_RESOURCE_MBA but that re-enable on unmount is missing?
Does just adding this check make sense? As already commented in the function we assume that
resctrl_arch_set_cdp_enabled() is only called with enable=false on error or unmount.
--- a/drivers/resctrl/mpam_resctrl.c
+++ b/drivers/resctrl/mpam_resctrl.c
@@ -224,6 +224,9 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level rid, bool enable)
mpam_resctrl_controls[RDT_RESOURCE_MBA].class)
mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = true;
+ if (!cdp_enabled && mpam_resctrl_controls[RDT_RESOURCE_MBA].class)
+ mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = true;
+
if (enable) {
if (mpam_partid_max < 1)
return -EINVAL;
Thanks,
Ben
>
> if (enable) {
> if (mpam_partid_max < 1)
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.
> Isn't it more that the existing checks are correct (if not totally necessary) and guard against
> resctrl_arch_set_cdp_enabled() being called with RDT_RESOURCE_MBA but that re-enable on unmount is missing?
> Does just adding this check make sense? As already commented in the function we assume that
> resctrl_arch_set_cdp_enabled() is only called with enable=false on error or unmount.
>
> --- a/drivers/resctrl/mpam_resctrl.c
> +++ b/drivers/resctrl/mpam_resctrl.c
> @@ -224,6 +224,9 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level rid, bool enable)
> mpam_resctrl_controls[RDT_RESOURCE_MBA].class)
> mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = true;
>
> + if (!cdp_enabled && mpam_resctrl_controls[RDT_RESOURCE_MBA].class)
> + mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = true;
> +
> if (enable) {
> if (mpam_partid_max < 1)
> return -EINVAL;
I'll add a comment above the original code quoted above describing what it does, and
take your hunk here, (with a comment that its for umount):
-------------%<-------------
arm_mpam: resctrl: Fix MBA CDP alloc_capable handling on unmount
The code to set MBA's alloc_capable to true appears to be trying to
restore alloc_capable on unmount. This can never work because
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.
The alloc_capable setting in resctrl_arch_set_cdp_enabled() is to
re-enable MBA if the caller opts in to separate control values using
CDP for this resource. This doesn't happen today.
Add a comment to describe this.
However a bug remains where MBA allocation is permanently disabled after
the mount with CDP option. Remounting without CDP cannot restore the MBA
partition capability.
Add a check to re-enable MBA when CDP is disabled, which happens on
unmount.
---
@@ -220,10 +220,18 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level rid, bool
enable)
if (cdp_enabled && !mpam_resctrl_controls[RDT_RESOURCE_MBA].cdp_enabled)
mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = false;
+ /*
+ * if resctrl has attempted to enable CDP on MBA, re-enable MBA as two
+ * configurations will provided so there is no aliasing problem.
+ */
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;
+ /* On unmount when CDP is disabled, re-enable MBA */
+ if (!cdp_enabled && mpam_resctrl_controls[RDT_RESOURCE_MBA].class)
+ mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = true;
+
if (enable) {
if (mpam_partid_max < 1)
return -EINVAL;
-------------%<-------------
Ben - do you want a Co-developed by for this?
While I'm at it:
Reviewed-by: James Morse <james.morse@arm.com>
Thanks,
James
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
Hi James,
On 4/17/26 17: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.
>
>
>> Isn't it more that the existing checks are correct (if not totally necessary) and guard against
>> resctrl_arch_set_cdp_enabled() being called with RDT_RESOURCE_MBA but that re-enable on unmount is missing?
>> Does just adding this check make sense? As already commented in the function we assume that
>> resctrl_arch_set_cdp_enabled() is only called with enable=false on error or unmount.
>>
>> --- a/drivers/resctrl/mpam_resctrl.c
>> +++ b/drivers/resctrl/mpam_resctrl.c
>> @@ -224,6 +224,9 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level rid, bool enable)
>> mpam_resctrl_controls[RDT_RESOURCE_MBA].class)
>> mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = true;
>>
>> + if (!cdp_enabled && mpam_resctrl_controls[RDT_RESOURCE_MBA].class)
>> + mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = true;
>> +
>> if (enable) {
>> if (mpam_partid_max < 1)
>> return -EINVAL;
>
>
> I'll add a comment above the original code quoted above describing what it does, and
> take your hunk here, (with a comment that its for umount):
> -------------%<-------------
> arm_mpam: resctrl: Fix MBA CDP alloc_capable handling on unmount
>
> The code to set MBA's alloc_capable to true appears to be trying to
> restore alloc_capable on unmount. This can never work because
> 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.
>
> The alloc_capable setting in resctrl_arch_set_cdp_enabled() is to
> re-enable MBA if the caller opts in to separate control values using
> CDP for this resource. This doesn't happen today.
>
> Add a comment to describe this.
>
> However a bug remains where MBA allocation is permanently disabled after
> the mount with CDP option. Remounting without CDP cannot restore the MBA
> partition capability.
>
> Add a check to re-enable MBA when CDP is disabled, which happens on
> unmount.
> ---
> @@ -220,10 +220,18 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level rid, bool
> enable)
> if (cdp_enabled && !mpam_resctrl_controls[RDT_RESOURCE_MBA].cdp_enabled)
> mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = false;
>
> + /*
> + * if resctrl has attempted to enable CDP on MBA, re-enable MBA as two
if -> If> + * configurations will provided so there is no aliasing problem.
will -> will be> + */
> 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;
>
> + /* On unmount when CDP is disabled, re-enable MBA */
> + if (!cdp_enabled && mpam_resctrl_controls[RDT_RESOURCE_MBA].class)
> + mpam_resctrl_controls[RDT_RESOURCE_MBA].resctrl_res.alloc_capable = true;
> +
> if (enable) {
> if (mpam_partid_max < 1)
> return -EINVAL;
> -------------%<-------------
>
> Ben - do you want a Co-developed by for this?
No need.
The additional comments and updated commit message make sense to me.
Reviewed-by: Ben Horgan <ben.horgan@arm.com>
Thanks,
Ben
>
> While I'm at it:
> Reviewed-by: James Morse <james.morse@arm.com>
>
>
> Thanks,
>
> James
© 2016 - 2026 Red Hat, Inc.