Re: [PATCH v2 15/45] arm_mpam: resctrl: Add boilerplate cpuhp and domain allocation

Ben Horgan
2026-01-06T11:17:34Z

Hi Jonathan,

On 1/5/26 17:40, Jonathan Cameron wrote:
> On Fri, 19 Dec 2025 18:11:17 +0000
> Ben Horgan <ben.horgan@arm.com> wrote:
>
>> From: James Morse <james.morse@arm.com>
>>
>> resctrl has its own data structures to describe its resources. We can't use
>> these directly as we play tricks with the 'MBA' resource, picking the MPAM
>> controls or monitors that best apply. We may export the same component as
>> both L3 and MBA.
>>
>> Add mpam_resctrl_exports[] as the array of class->resctrl mappings we are
>> exporting, and add the cpuhp hooks that allocated and free the resctrl
>> domain structures.
>>
>> While we're here, plumb in a few other obvious things.
>>
>> CONFIG_ARM_CPU_RESCTRL is used to allow this code to be built even though
>> it can't yet be linked against resctrl.
>>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>> ---
>> Domain list is an rcu list
>> Add synchronize_rcu() to free the deleted element
>> Code flow simplification (Jonathan)
>
> Just trivial stuff and that one what do we loop over thing that
> we continued discussing in the RFC thread (I think you already
> tidied that up) Nothing here to stop:
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> (which is another way of saying I'm not planning to read it again :)

Yes, I've already made that change locally :) and just added your other
suggestions below.

>
>
>> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
>> new file mode 100644
>> index 000000000000..4beeeded00ff
>> --- /dev/null
>> +++ b/drivers/resctrl/mpam_resctrl.c
>
>> +
>> +static int mpam_resctrl_pick_domain_id(int cpu, struct mpam_component *comp)
>> +{
>> + struct mpam_class *class = comp->class;
>> +
>> + if (class->type == MPAM_CLASS_CACHE)
>> + return comp->comp_id;
>> +
>> + /* TODO: repaint domain ids to match the L3 domain ids */
>> + /*
>> + * Otherwise, expose the ID used by the firmware table code.
> Maybe more turns up in here. Otherwise, easily fits in single line comment.
>> + */
>> + return comp->comp_id;
>> +}
>
>> +static struct mpam_resctrl_dom *
>> +mpam_resctrl_get_domain_from_cpu(int cpu, struct mpam_resctrl_res *res)
>> +{
>> + struct mpam_resctrl_dom *dom;
>> + struct rdt_ctrl_domain *ctrl_d;
>> + struct rdt_resource *r = &res->resctrl_res;
>> +
>> + lockdep_assert_cpus_held();
>> +
>> + list_for_each_entry_rcu(ctrl_d, &r->ctrl_domains, hdr.list) {
>
> As a reminder, though I think you already changed this, discussion carried
> on wrt to the RFC version of this loop.
>
>> + dom = container_of(ctrl_d, struct mpam_resctrl_dom,
>> + resctrl_ctrl_dom);
>> +
>> + if (cpumask_test_cpu(cpu, &dom->ctrl_comp->affinity))
>> + return dom;
>> + }
>> +
>> + return NULL;
>> +}
>
>> +
>> +void mpam_resctrl_offline_cpu(unsigned int cpu)
>> +{
>> + resctrl_offline_cpu(cpu);
>> +
>> + guard(mutex)(&domain_list_lock);
>> + for (int i = 0; i < RDT_NUM_RESOURCES; i++) {
>> + struct mpam_resctrl_res *res;
>> + struct mpam_resctrl_dom *dom;
>> + struct rdt_mon_domain *mon_d;
>> + struct rdt_ctrl_domain *ctrl_d;
>> + bool ctrl_dom_empty, mon_dom_empty;
>> +
>> + res = &mpam_resctrl_controls[i];
>> + if (!res->class)
>> + continue; // dummy resource
>> +
>> + dom = mpam_resctrl_get_domain_from_cpu(cpu, res);
>> + if (WARN_ON_ONCE(!dom))
>> + continue;
>> +
>> + ctrl_dom_empty = true;
>> + if (exposed_alloc_capable) {
>> + ctrl_d = &dom->resctrl_ctrl_dom;
>> + ctrl_dom_empty = mpam_resctrl_offline_domain_hdr(cpu, &ctrl_d->hdr);
>> + if (ctrl_dom_empty)
>> + resctrl_offline_ctrl_domain(&res->resctrl_res, ctrl_d);
>> + }
> really small thing but I'd do
> } else {
> ctrl_dom_empty = true;
> }
> here to make it visually obvious why that is set to true. The initialize and override
> if we have more info pattern is to me less readable.
>
>> +
>> + mon_dom_empty = true;
>> + if (exposed_mon_capable) {
>> + mon_d = &dom->resctrl_mon_dom;
>> + mon_dom_empty = mpam_resctrl_offline_domain_hdr(cpu, &mon_d->hdr);
>> + if (mon_dom_empty)
>> + resctrl_offline_mon_domain(&res->resctrl_res, mon_d);
>> + }
> Similar for this one.
>> +
>> + if (ctrl_dom_empty && mon_dom_empty)
>> + kfree(dom);
>> + }
>> +}
>

--
Thanks,

Ben