Message-ID: <20251218113014.00002691@huawei.com>
Date: Thu, 18 Dec 2025 11:30:14 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: James Morse <james.morse@....com>
CC: <linux-kernel@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>, D
 Scott Phillips OS <scott@...amperecomputing.com>,
	<carl@...amperecomputing.com>, <lcherian@...vell.com>,
	<bobo.shaobowang@...wei.com>, <tan.shaopeng@...itsu.com>,
	<baolin.wang@...ux.alibaba.com>, Jamie Iles <quic_jiles@...cinc.com>, "Xin
 Hao" <xhao@...ux.alibaba.com>, <peternewman@...gle.com>,
	<dfustini@...libre.com>, <amitsinght@...vell.com>, David Hildenbrand
	<david@...nel.org>, Dave Martin <dave.martin@....com>, Koba Ko
	<kobak@...dia.com>, Shanker Donthineni <sdonthineni@...dia.com>,
	<fenghuay@...dia.com>, <baisheng.gao@...soc.com>, Gavin Shan
	<gshan@...hat.com>, Ben Horgan <ben.horgan@....com>, <rohit.mathew@....com>,
	<reinette.chatre@...el.com>, Punit Agrawal <punit.agrawal@....qualcomm.com>
Subject: Re: [RFC PATCH 07/38] arm_mpam: resctrl: Add boilerplate cpuhp and
 domain allocation

On Fri, 5 Dec 2025 21:58:30 +0000
James Morse <james.morse@....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@....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.

> +
> +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) {
	}

> +
> +		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.

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


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

Powered by blists - more mailing lists