Re: [PATCH v4 18/41] arm_mpam: resctrl: Implement helpers to update configuration

[Zeng Heng <zengheng4@huawei.com>]

Hi Ben,

On 2026/2/16 22:23, Ben Horgan wrote:
> Hi Zeng,
> 
> On 2/14/26 10:39, Zeng Heng wrote:
>> Hi Ben,
>>
>> On 2026/2/4 5:43, Ben Horgan wrote:
>>> From: James Morse <james.morse@arm.com>
>>>
>>> resctrl has two helpers for updating the configuration.
>>> resctrl_arch_update_one() updates a single value, and is used by the
>>> software-controller to apply feedback to the bandwidth controls, it
>>> has to
>>> be called on one of the CPUs in the resctrl:domain.
>>>
>>> resctrl_arch_update_domains() copies multiple staged configurations,
>>> it can
>>> be called from anywhere.
>>>
>>> Both helpers should update any changes to the underlying hardware.
>>>
>>> Implement resctrl_arch_update_domains() to use
>>> resctrl_arch_update_one(). Neither need to be called on a specific CPU as
>>> the mpam driver will send IPIs as needed.
>>>
>>> Tested-by: Gavin Shan <gshan@redhat.com>
>>> Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
>>> Tested-by: Peter Newman <peternewman@google.com>
>>> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>>> Signed-off-by: James Morse <james.morse@arm.com>
>>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>>> ---
>>> Changes since rfc:
>>> list_for_each_entry -> list_for_each_entry_rcu
>>> return 0
>>> Restrict scope of local variables
>>>
>>> Changes since v2:
>>> whitespace fix
>>> ---
>>>    drivers/resctrl/mpam_resctrl.c | 70 ++++++++++++++++++++++++++++++++++
>>>    1 file changed, 70 insertions(+)
>>>
>>> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/
>>> mpam_resctrl.c
>>> index ecf00386edca..48d047510089 100644
>>> --- a/drivers/resctrl/mpam_resctrl.c
>>> +++ b/drivers/resctrl/mpam_resctrl.c
>>> @@ -212,6 +212,76 @@ u32 resctrl_arch_get_config(struct rdt_resource
>>> *r, struct rdt_ctrl_domain *d,
>>>        }
>>>    }
>>>    +int resctrl_arch_update_one(struct rdt_resource *r, struct
>>> rdt_ctrl_domain *d,
>>> +                u32 closid, enum resctrl_conf_type t, u32 cfg_val)
>>> +{
>>> +    u32 partid;
>>> +    struct mpam_config cfg;
>>> +    struct mpam_props *cprops;
>>> +    struct mpam_resctrl_res *res;
>>> +    struct mpam_resctrl_dom *dom;
>>> +
>>> +    lockdep_assert_cpus_held();
>>> +    lockdep_assert_irqs_enabled();
>>> +
>>> +    /*
>>> +     * No need to check the CPU as mpam_apply_config() doesn't care, and
>>> +     * resctrl_arch_update_domains() relies on this.
>>> +     */
>>> +    res = container_of(r, struct mpam_resctrl_res, resctrl_res);
>>> +    dom = container_of(d, struct mpam_resctrl_dom, resctrl_ctrl_dom);
>>> +    cprops = &res->class->props;
>>> +
>>> +    partid = resctrl_get_config_index(closid, t);
>>
>>
>> As a victim, I must admit I cannot verify this feedback on my local
>> Kunpeng environment since MB functionality is not yet supported by the
>> driver. However, after careful consideration, I believe this is worth
>> bringing up for discussion.
> 
> Thank you for thinking and finding problems beyond your platform.
> 
>>
>> Regarding the MB configuration flow, the partid conversion should
>> include the mpam_resctrl_hide_cdp() condition check. Here's the
>> rationale:
>>
>> After resctrl parsing schemata update, MB configuration is set via
>> parse_bw() or rdtgroup_init_mba(), which stores the updated
>> configuration in dom->staged_config[CDP_NONE]. If the MB configuration
>> update directly uses t = CDP_NONE, it would result in MB obtaining the
>> wrong partid and cfg[partid].
>>
>> The specific fix would be like:
>>
>> -       partid = resctrl_get_config_index(closid, t);
>> +       if (mpam_resctrl_hide_cdp(r->rid))
>> +        /* The configuration of CDP_DATA is same as the CDP_CODE one. */
>> +               partid = resctrl_get_config_index(closid, CDP_DATA);
>> +       else
>> +               partid = resctrl_get_config_index(closid, t);
> 
> The CDP emulation support is added later in the series in patch 20, Add
> CDP emulation. However, I think you have spotted an actual problem. With
> hidden CDP the cfg is first found with
> 
> resctrl_get_config_index(closid, t) when CDP_NONE
> but then the setting does use CDP_CODE and CDP_DATA.
> 
> CDP in general is proving to be quite tricky.
> 
>>
>> Similarly, in resctrl_arch_get_config() requires the same treatment to
>> ensure consistency.
> 
> I don't see the problem here but maybe I'm missing something?
> 
> Isn't this handled by:
> 
> 	/*
> 	 * When CDP is enabled, but the resource doesn't support it,
> 	 * the control is cloned across both partids.
> 	 * Pick one at random to read:
> 	 */
> 	if (mpam_resctrl_hide_cdp(r->rid))
> 		type = CDP_DATA;
> 
> I think we could do similar in resctrl_arch_update_one()
> 
>>
>>


Yes, I noticed that handling has already been added to
resctrl_arch_get_config(), and I have updated the comments on v5
accordingly.


Thanks,
Zeng Heng