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

[Zeng Heng <zengheng4@huawei.com>]

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.

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);

Similarly, in resctrl_arch_get_config() requires the same treatment to
ensure consistency.


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;
> +}