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

[Ben Horgan <ben.horgan@arm.com>]

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