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

[Zeng Heng]

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@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()
>
> >


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