Re: [PATCH v1 05/11] arm_mpam: Ensure MBWU counters are reset on restore

[Ben Horgan]

Hi Shaopeng,

On 7/16/26 01:26, Shaopeng Tan (Fujitsu) wrote:
> Hello Ben,
> 
>>>> When an MSC becomes inaccessible due to cpu offline CFG_MBWU_CTL is set to
>>>> zero in mpam_save_mbwu_state(). This is very likely to mean that the config
>>>> will mismatch when restoring and so the monitor will be reset. However, the
>>>> state may have been lost and so there are no guarantees. Ensure the reset
>>>> happens by setting the reset_on_next_read and remove the unnecessary writes
>>> >from mpam_save_mbwu_state().
>>>>
>>>> Fixes: 41e8a14950e1 ("arm_mpam: Track bandwidth counter state for power management")
>>>> Signed-off-by: Ben Horgan <ben.horgan@xxxxxxx>
>>>> ---
>>>>  drivers/resctrl/mpam_devices.c | 13 +++++++------
>>>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>>>> index b34e2a368516..222fc248067e 100644
>>>> --- a/drivers/resctrl/mpam_devices.c
>>>> +++ b/drivers/resctrl/mpam_devices.c
>>>> @@ -1648,10 +1648,13 @@ static int mpam_restore_mbwu_state(void *_ris)
>>>>          u64 val;
>>>>          struct mon_read mwbu_arg;
>>>>          struct mpam_msc_ris *ris = _ris;
>>>> +       struct msmon_mbwu_state *mbwu_state;
>>>>          struct mpam_msc *msc = ris->vmsc->msc;
>>>>          struct mpam_class *class = ris->vmsc->comp->class;
>>>>  
>>>>          for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>>>> +               mbwu_state = &ris->mbwu_state[i];
>>>> +
>>>>                  if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>>>>                          return -EIO;
>>>>  
>>>> @@ -1661,6 +1664,8 @@ static int mpam_restore_mbwu_state(void *_ris)
>>>>                          mwbu_arg.type = mpam_msmon_choose_counter(class);
>>>>                          mwbu_arg.val = &val;
>>>>  
>>>> +                       mbwu_state->reset_on_next_read = true;
>>>> +
>>>>                          mpam_mon_sel_unlock(msc);
>>>
>>>         for (i = 0; i < ris->props.num_mbwu_mon; i++) {
>>> +               mbwu_state = &ris->mbwu_state[i];
>>> +
>>>                 if (WARN_ON_ONCE(!mpam_mon_sel_lock(msc)))
>>>                         return -EIO;
>>>
>>> -               if (ris->mbwu_state[i].enabled) {
>>> +               if (mbwu_state->enabled) {  //this line might need refactoring
>>
>> I'm unclear on what you are trying to point out in this email. Please can you explain.
>>
>> Thanks,
>>
>> Ben
> 
> Inside the for loop, you introduced `mbwu_state = &ris->mbwu_state[i];` a few lines earlier, 
> and then used `mbwu_state->reset_on_next_read = true;` shortly after.
> Therefore, using mbwu_state->enabled and mbwu_state->cfg keeps 
> the whole block consistent and avoids repeating ris->mbwu_state[i] multiple times. 

Thanks for explaining. Indeed, continuing to use the ris->mbwu_state[i] once it's assigned to a
local variable does make this a bit messy.

Ben

> 
> Best regards,
> Shaopeng TAN
> 
>>>                         mwbu_arg.ris = ris;
>>> -                       mwbu_arg.ctx = &ris->mbwu_state[i].cfg;
>>> +                       mwbu_arg.ctx = &mbwu_state->cfg;  //and this line
>>>                         mwbu_arg.type = mpam_msmon_choose_counter(class);
>>>                         mwbu_arg.val = &val;
>>>
>>> +                       mbwu_state->reset_on_next_read = true;
>>> +
>>>                         mpam_mon_sel_unlock(msc);
>>>
>>>
>>> Best regards,
>>> Shaopeng TAN
>>>
>>>>                          __ris_msmon_read(&mwbu_arg);
>>>> @@ -1696,15 +1701,11 @@ static int mpam_save_mbwu_state(void *arg)
>>>>  
>>>>                  cur_flt = mpam_read_monsel_reg(msc, CFG_MBWU_FLT);
>>>>                  cur_ctl = mpam_read_monsel_reg(msc, CFG_MBWU_CTL);
>>>> -               mpam_write_monsel_reg(msc, CFG_MBWU_CTL, 0);
>>>>  
>>>> -               if (mpam_ris_has_mbwu_long_counter(ris)) {
>>>> +               if (mpam_ris_has_mbwu_long_counter(ris))
>>>>                          val = mpam_msc_read_mbwu_l(msc);
>>>> -                       mpam_msc_zero_mbwu_l(msc);
>>>> -               } else {
>>>> +               else
>>>>                          val = mpam_read_monsel_reg(msc, MBWU);
>>>> -                       mpam_write_monsel_reg(msc, MBWU, 0);
>>>> -               }
>>>>  
>>>>                  cfg->mon = i;
>>>>                  cfg->pmg = FIELD_GET(MSMON_CFG_x_FLT_PMG, cur_flt);
>>>> --
>>>> 2.43.0
>>
>>