Re: [PATCH v2 24/29] arm_mpam: Track bandwidth counter state for overflow and power management

James Morse
2025-10-13T16:29:21Z

Hi Ben,

On 12/09/2025 16:55, Ben Horgan wrote:
> On 9/10/25 21:43, James Morse wrote:
>> Bandwidth counters need to run continuously to correctly reflect the
>> bandwidth.
>>
>> The value read may be lower than the previous value read in the case
>> of overflow and when the hardware is reset due to CPU hotplug.
>>
>> Add struct mbwu_state to track the bandwidth counter to allow overflow
>> and power management to be handled.

>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index 1543c33c5d6a..eeb62ed94520 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
>> @@ -990,20 +992,32 @@ static void write_msmon_ctl_flt_vals(struct mon_read *m, u32 ctl_val,
>> mpam_write_monsel_reg(msc, CFG_MBWU_CTL, ctl_val);
>> mpam_write_monsel_reg(msc, MBWU, 0);
>> mpam_write_monsel_reg(msc, CFG_MBWU_CTL, ctl_val | MSMON_CFG_x_CTL_EN);
>> +
>> + mbwu_state = &m->ris->mbwu_state[m->ctx->mon];
>> + if (mbwu_state)
>> + mbwu_state->prev_val = 0;

> What's the if condition doing here?

Yes, that looks like cruft....
It took the address of an array element - how could it be null?!

> The below could make more sense but I don't think you can get here if
> the allocation fails.

Heh ... only because __allocate_component_cfg() has lost the error value.
Without the outer/inner locking stuff, its feasible for __allocate_component_cfg() to
return the error value directly.

With that fixed, and ignoring a bogus ctx->mon value - I agree you can't get a case where
this needs checking.

I think this was originally testing if the array had been allocated, and its been folded
wrongly at some point in the past. I assume I kept those bogus tests around as I saw it
blow up with nonsense num_mbwu_mon - which is something I'll retest.

>> +
>> break;
>> default:
>> return;
>> }
>> }

>> @@ -2106,6 +2227,35 @@ static int __allocate_component_cfg(struct mpam_component *comp)
>> return -ENOMEM;
>> init_garbage(comp->cfg);
>>
>> + list_for_each_entry(vmsc, &comp->vmsc, comp_list) {
>> + if (!vmsc->props.num_mbwu_mon)
>> + continue;
>> +
>> + msc = vmsc->msc;
>> + list_for_each_entry(ris, &vmsc->ris, vmsc_list) {
>> + if (!ris->props.num_mbwu_mon)
>> + continue;
>> +
>> + mbwu_state = kcalloc(ris->props.num_mbwu_mon,
>> + sizeof(*ris->mbwu_state),
>> + GFP_KERNEL);
>> + if (!mbwu_state) {
>> + __destroy_component_cfg(comp);
>> + err = -ENOMEM;
>> + break;
>> + }
>> +
>> + if (mpam_mon_sel_lock(msc)) {
>> + init_garbage(mbwu_state);
>> + ris->mbwu_state = mbwu_state;
>> + mpam_mon_sel_unlock(msc);
>> + }
>
> The if statement is confusing now that mpam_mon_sel_lock()
> unconditionally returns true.

Sure, but this and the __must_check means all the paths that use this must be able to
return an error.

This is a churn-or-not trade-off for the inclusion of the firmware-backed support.
I'd prefer it to be hard to add code-paths that are going to create a lot of work when
that comes - especially as folk are promising platforms that need this in the coming months.

Thanks,

James