Re: [RFC PATCH 07/38] arm_mpam: resctrl: Add boilerplate cpuhp and domain allocation

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

Hi Jonathan,

On 12/18/25 11:30, Jonathan Cameron wrote:
> On Fri, 5 Dec 2025 21:58:30 +0000
> James Morse <james.morse@arm.com> wrote:
> 
>> 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>
> Hi,
> 
> A few code flow related comments. Fairly trivial stuff but I think
> some parts of this can be made more readable / maintainable with
> minor reorganization.
> 
> Jonathan
> 
> 
>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>> index 2996ad93fc3e..efaf7633bc35 100644
>> --- a/drivers/resctrl/mpam_devices.c
>> +++ b/drivers/resctrl/mpam_devices.c
> ...
> 
>> @@ -2516,6 +2522,12 @@ static void mpam_enable_once(void)
>>  	mutex_unlock(&mpam_list_lock);
>>  	cpus_read_unlock();
>>  
>> +	if (!err) {
>> +		err = mpam_resctrl_setup();
>> +		if (err)
>> +			pr_err("Failed to initialise resctrl: %d\n", err);
>> +	}
>> +
>>  	if (err) {
>>  		mpam_disable_reason = "Failed to enable.";
>>  		schedule_work(&mpam_broken_work);
> 
> I'd be tempted to move this to an error handling block via a goto
> making this bit
> 	if (err)
> 		goto err_disable_mpam;
> 
> 	err = mpam_resctrl_setup();
> 	if (err) {
> 		pr_err();
> 		goto err_dsiable_mpam;
> 	}
> 
> Up to you though. Personally I like all my good paths as straight line
> code with the errors handled in if (err) as that consistency really helps
> readability.  
> 
>> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
>> new file mode 100644
>> index 000000000000..320cebbd37ce
>> --- /dev/null
>> +++ b/drivers/resctrl/mpam_resctrl.c
>> @@ -0,0 +1,329 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2025 Arm Ltd.
>> +
>> +#define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__
>> +
>> +#include <linux/arm_mpam.h>
>> +#include <linux/cacheinfo.h>
>> +#include <linux/cpu.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/errno.h>
>> +#include <linux/list.h>
>> +#include <linux/printk.h>
>> +#include <linux/rculist.h>
>> +#include <linux/resctrl.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +
>> +#include <asm/mpam.h>
>> +
>> +#include "mpam_internal.h"
> 
> 
>> +static struct mpam_resctrl_dom *
>> +mpam_resctrl_alloc_domain(unsigned int cpu, struct mpam_resctrl_res *res)
>> +{
>> +	int err;
>> +	struct mpam_resctrl_dom *dom;
>> +	struct rdt_mon_domain *mon_d;
>> +	struct rdt_ctrl_domain *ctrl_d;
>> +	struct mpam_class *class = res->class;
>> +	struct mpam_component *comp_iter, *ctrl_comp;
>> +	struct rdt_resource *r = &res->resctrl_res;
>> +
>> +	lockdep_assert_held(&domain_list_lock);
>> +
>> +	ctrl_comp = NULL;
>> +	guard(srcu)(&mpam_srcu);
>> +	list_for_each_entry_srcu(comp_iter, &class->components, class_list,
>> +				 srcu_read_lock_held(&mpam_srcu)) {
>> +		if (cpumask_test_cpu(cpu, &comp_iter->affinity)) {
>> +			ctrl_comp = comp_iter;
>> +			break;
>> +		}
>> +	}
>> +
>> +	/* class has no component for this CPU */
>> +	if (WARN_ON_ONCE(!ctrl_comp))
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	dom = kzalloc_node(sizeof(*dom), GFP_KERNEL, cpu_to_node(cpu));
>> +	if (!dom)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	if (exposed_alloc_capable) {
>> +		dom->ctrl_comp = ctrl_comp;
>> +
>> +		ctrl_d = &dom->resctrl_ctrl_dom;
>> +		mpam_resctrl_domain_hdr_init(cpu, ctrl_comp, &ctrl_d->hdr);
>> +		ctrl_d->hdr.type = RESCTRL_CTRL_DOMAIN;
>> +		/* TODO: this list should be sorted */
>> +		list_add_tail_rcu(&ctrl_d->hdr.list, &r->ctrl_domains);
>> +		err = resctrl_online_ctrl_domain(r, ctrl_d);
>> +		if (err) {
>> +			dom = ERR_PTR(err);
>> +			goto offline_ctrl_domain;
>> +		}
>> +	} else {
>> +		pr_debug("Skipped control domain online - no controls\n");
>> +	}
>> +
>> +	if (exposed_mon_capable) {
>> +		mon_d = &dom->resctrl_mon_dom;
>> +		mpam_resctrl_domain_hdr_init(cpu, ctrl_comp, &mon_d->hdr);
>> +		mon_d->hdr.type = RESCTRL_MON_DOMAIN;
>> +		/* TODO: this list should be sorted */
>> +		list_add_tail_rcu(&mon_d->hdr.list, &r->mon_domains);
>> +		err = resctrl_online_mon_domain(r, mon_d);
>> +		if (err) {
>> +			dom = ERR_PTR(err);
>> +			goto offline_mon_hdr;
>> +		}
>> +	} else {
>> +		pr_debug("Skipped monitor domain online - no monitors\n");
>> +	}
>> +	goto out;
> 
> To keep flow simple, return here.  I thought maybe there was more stuff
> that was always done (added in later patches) but not seeing that.
> If there were then it would be a fairly strong indicator that a different
> code structure makes more sense - probably with some helper functions.

Makes sense.

> 
>> +
>> +offline_mon_hdr:
>> +	mpam_resctrl_offline_domain_hdr(cpu, &mon_d->hdr);
>> +offline_ctrl_domain:
>> +	resctrl_offline_ctrl_domain(r, ctrl_d);
>> +out:
>> +	return dom;
>> +}
>> +
>> +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;
>> +
>> +	lockdep_assert_cpus_held();
>> +
>> +	list_for_each_entry_rcu(ctrl_d, &res->resctrl_res.ctrl_domains,
>> +				hdr.list) {
>> +		dom = container_of(ctrl_d, struct mpam_resctrl_dom,
>> +				   resctrl_ctrl_dom);
> 
> I'm lazy so haven't checked for more code here in later patches, but
> if not, why not iterate the list to access the domain directly rather
> than jumping through the rdt_ctrl_domain?
> 
> Something along lines of:
> 	
> 	list_for_each_entry_rcu(dom, &res->resctrl_res.ctrl_domains,
> 				resctrl_ctrl_dom.hdr.list) {
> 	}
>

Unless I've misunderstood I don't think this works because it's not what
the fs/resctrl code expects.


>> +
>> +		if (cpumask_test_cpu(cpu, &dom->ctrl_comp->affinity))
>> +			return dom;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +int mpam_resctrl_online_cpu(unsigned int cpu)
>> +{
>> +	int i;
>> +	struct mpam_resctrl_dom *dom;
>> +	struct mpam_resctrl_res *res;
>> +
>> +	guard(mutex)(&domain_list_lock);
>> +	for (i = 0; i < RDT_NUM_RESOURCES; i++) {
> 
> I'd narrow the scope for dom and res to inside the loop.
> Maybe put the iterator in the for loop init (now considered
> acceptable in kernel code)
> 
> Similar applies in various other places.  No that important
> for functions that more or less just consist of a loop though.

I've done a bit of scope reducing here and in some other places.

> 
>> +		res = &mpam_resctrl_controls[i];
>> +		if (!res->class)
>> +			continue;	// dummy_resource;
>> +
>> +		dom = mpam_resctrl_get_domain_from_cpu(cpu, res);
>> +		if (!dom)
>> +			dom = mpam_resctrl_alloc_domain(cpu, res);
>> +		if (IS_ERR(dom))
>> +			return PTR_ERR(dom);
>> +	}
>> +
>> +	resctrl_online_cpu(cpu);
>> +
>> +	return 0;
>> +}
> 
>> +int mpam_resctrl_setup(void)
>> +{
>> +	int err = 0;
>> +	enum resctrl_res_level i;
>> +	struct mpam_resctrl_res *res;
>> +
>> +	cpus_read_lock();
>> +	for (i = 0; i < RDT_NUM_RESOURCES; i++) {
>> +		res = &mpam_resctrl_controls[i];
>> +		INIT_LIST_HEAD_RCU(&res->resctrl_res.ctrl_domains);
>> +		INIT_LIST_HEAD_RCU(&res->resctrl_res.mon_domains);
>> +		res->resctrl_res.rid = i;
>> +	}
>> +
>> +	/* TODO: pick MPAM classes to map to resctrl resources */
>> +
>> +	/* Initialise the resctrl structures from the classes */
>> +	for (i = 0; i < RDT_NUM_RESOURCES; i++) {
>> +		res = &mpam_resctrl_controls[i];
>> +		if (!res->class)
>> +			continue;	// dummy resource
>> +
>> +		err = mpam_resctrl_control_init(res, i);
>> +		if (err) {
>> +			pr_debug("Failed to initialise rid %u\n", i);
>> +			break;
>> +		}
>> +	}
>> +	cpus_read_unlock();
>> +
>> +	if (err || (!exposed_alloc_capable && !exposed_mon_capable)) {
>> +		if (err)
>> +			pr_debug("Internal error %d - resctrl not supported\n",
>> +				 err);
>> +		else
>> +			pr_debug("No alloc(%u) or monitor(%u) found - resctrl not supported\n",
>> +				 exposed_alloc_capable, exposed_mon_capable);
>> +		err = -EOPNOTSUPP;
> 
> return -EOPNOTSUPP; here to make the code flow simpler.
> Mind you nice to avoid eating err if it is set and the sharing here doesn't seem
> all that useful so perhaps just make this:
> 
> 	if (err) {
> 		pr_debug("Internal error %d - resctrl not supported\n", err);
> 		return err;
> 	}
> 
> 	if (!exposed_alloc_capable && !exposed_mon_capable) {
> 		pr_debug("No alloc(%u) or monitor(%u) found - resctrl not supported\n",
> 			 exposed_alloc_capable, exposed_mon_capable);
> 		return -EOPNOTSUPP;
> 	}

I've gone for the second option.

> 
> 
>> +	}
>> +
>> +	if (!err) {
>> +		if (!is_power_of_2(mpam_pmg_max + 1)) {
>> +			/*
>> +			 * If not all the partid*pmg values are valid indexes,
>> +			 * resctrl may allocate pmg that don't exist. This
>> +			 * should cause an error interrupt.
>> +			 */
>> +			pr_warn("Number of PMG is not a power of 2! resctrl may misbehave");
>> +		}
>> +
>> +		/* TODO: call resctrl_init() */
>> +	}
>> +
>> +	return err;
>> +}

Thanks,

Ben