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

[Ben Horgan]

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@xxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Tested-by: Shaopeng Tan <tan.shaopeng@xxxxxxxxxxxxxx>
>> Tested-by: Peter Newman <peternewman@xxxxxxxxxx>
>> Reviewed-by: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
>> Signed-off-by: James Morse <james.morse@xxxxxxx>
>> Signed-off-by: Ben Horgan <ben.horgan@xxxxxxx>
>> ---
>> 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()

> 
> 
> Best regards,
> and happy Lunar New Year!
> 
> Zeng Heng
> 
> 
>> +    if (!r->alloc_capable || partid >= resctrl_arch_get_num_closid(r)) {
>> +        pr_debug("Not alloc capable or computed PARTID out of range\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    /*
>> +     * Copy the current config to avoid clearing other resources when
>> the
>> +     * same component is exposed multiple times through resctrl.
>> +     */
>> +    cfg = dom->ctrl_comp->cfg[partid]; > +
>> +    switch (r->rid) {
>> +    case RDT_RESOURCE_L2:
>> +    case RDT_RESOURCE_L3:
>> +        cfg.cpbm = cfg_val;
>> +        mpam_set_feature(mpam_feat_cpor_part, &cfg);
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>> +    return mpam_apply_config(dom->ctrl_comp, partid, &cfg);
>> +}
>> +
>> +int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
>> +{
>> +    int err;
>> +    struct rdt_ctrl_domain *d;
>> +
>> +    lockdep_assert_cpus_held();
>> +    lockdep_assert_irqs_enabled();
>> +
>> +    list_for_each_entry_rcu(d, &r->ctrl_domains, hdr.list) {
>> +        for (enum resctrl_conf_type t = 0; t < CDP_NUM_TYPES; t++) {
>> +            struct resctrl_staged_config *cfg = &d->staged_config[t];
>> +
>> +            if (!cfg->have_new_ctrl)
>> +                continue;
>> +
>> +            err = resctrl_arch_update_one(r, d, closid, t,
>> +                              cfg->new_ctrl);
>> +            if (err)
>> +                return err;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}


Thanks,

Ben