Re: [PATCH v2 28/45] arm_mpam: resctrl: Pick classes for use as mbm counters

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Fri, 19 Dec 2025 18:11:30 +0000
Ben Horgan <ben.horgan@arm.com> wrote:

> From: James Morse <james.morse@arm.com>
> 
> resctrl has two types of counters, NUMA-local and global. MPAM has only
> bandwidth counters, but the position of the MSC may mean it counts
> NUMA-local, or global traffic.
> 
> But the topology information is not available.
> 
> Apply a heuristic: the L2 or L3 supports bandwidth monitors, these are
> probably NUMA-local. If the memory controller supports bandwidth monitors,
> they are probably global.
> 
> This also allows us to assert that we don't have the same class backing two
> different resctrl events.
> 
> Because the class or component backing the event may not be 'the L3', it is
> necessary for mpam_resctrl_get_domain_from_cpu() to search the monitor
> domains too. This matters the most for 'monitor only' systems, where 'the
> L3' control domains may be empty, and the ctrl_comp pointer NULL.
> 
> resctrl expects there to be enough monitors for every possible control and
> monitor group to have one. Such a system gets called 'free running' as the
> monitors can be programmed once and left running.  Any other platform will
> need to emulate ABMC.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
Hi Ben,

A few minor comments inline. + one question on a worrying sounding todo.

Jonathan

> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
> index 5fde610cc9d7..51caf3b82392 100644
> --- a/drivers/resctrl/mpam_resctrl.c
> +++ b/drivers/resctrl/mpam_resctrl.c



> @@ -925,6 +982,20 @@ static void mpam_resctrl_domain_insert(struct list_head *list,
>  	list_add_tail_rcu(&new->list, pos);
>  }
>  
> +static struct mpam_component *find_component(struct mpam_class *victim, int cpu)

This is a lovely generic sounding thing, but then the term victim comes in which
is very usecase specific.  Maybe something could have a better name? (either
function or avoid the victim naming).

> +{
> +	struct mpam_component *victim_comp;
> +
> +	guard(srcu)(&mpam_srcu);
> +	list_for_each_entry_srcu(victim_comp, &victim->components, class_list,
> +				 srcu_read_lock_held(&mpam_srcu)) {
> +		if (cpumask_test_cpu(cpu, &victim_comp->affinity))
> +			return victim_comp;
> +	}
> +
> +	return NULL;
> +}
> +
>  static struct mpam_resctrl_dom *
>  mpam_resctrl_alloc_domain(unsigned int cpu, struct mpam_resctrl_res *res)
>  {
> @@ -973,8 +1044,32 @@ mpam_resctrl_alloc_domain(unsigned int cpu, struct mpam_resctrl_res *res)
>  	}
>  
>  	if (exposed_mon_capable) {
> +		int i;
> +		struct mpam_component *mon_comp, *any_mon_comp;
> +
> +		/*
> +		 * Even if the monitor domain is backed by a different
> +		 * component, the L3 component IDs need to be used... only
> +		 * there may be no ctrl_comp for the L3.
> +		 * Search each event's class list for a component with
> +		 * overlapping CPUs and set up the dom->mon_comp array.
> +		 */
> +		for (i = 0; i < QOS_NUM_EVENTS; i++) {
For consistency with other loops (some of them anyway, I've not done
a detailed survey ;) I'd do
		for (int i = 0; ...
Probably bring scope of the mon_comp in here too.


> +			struct mpam_resctrl_mon *mon;
> +
> +			mon = &mpam_resctrl_counters[i];
> +			if (!mon->class)
> +				continue;       // dummy resource
> +
> +			mon_comp = find_component(mon->class, cpu);
> +			dom->mon_comp[i] = mon_comp;
> +			if (mon_comp)
> +				any_mon_comp = mon_comp;
> +		}
> +		WARN_ON_ONCE(!any_mon_comp);
> +
>  		mon_d = &dom->resctrl_mon_dom;
> -		mpam_resctrl_domain_hdr_init(cpu, ctrl_comp, &mon_d->hdr);
> +		mpam_resctrl_domain_hdr_init(cpu, any_mon_comp, &mon_d->hdr);
>  		mon_d->hdr.type = RESCTRL_MON_DOMAIN;
>  		mpam_resctrl_domain_insert(&r->mon_domains, &mon_d->hdr);
>  		err = resctrl_online_mon_domain(r, mon_d);
> @@ -996,6 +1091,39 @@ mpam_resctrl_alloc_domain(unsigned int cpu, struct mpam_resctrl_res *res)
>  	return dom;
>  }
>  
> +/*
> + * We know all the monitors are associated with the L3, even if there are no
> + * controls and therefore no control component. Find the cache-id for the CPU
> + * and use that to search for existing resctrl domains.
> + * This relies on mpam_resctrl_pick_domain_id() using the L3 cache-id
> + * for anything that is not a cache.
> + */
> +static struct mpam_resctrl_dom *mpam_resctrl_get_mon_domain_from_cpu(int cpu)
> +{
> +	u32 cache_id;
> +	struct rdt_mon_domain *mon_d;
> +	struct mpam_resctrl_dom *dom;
> +	struct mpam_resctrl_res *l3 = &mpam_resctrl_controls[RDT_RESOURCE_L3];
> +
> +	lockdep_assert_cpus_held();
> +
> +	if (!l3->class)
> +		return NULL;
> +	/* TODO: how does this order with cacheinfo updates under cpuhp? */

Considered a blocking todo or something that is future work to resolve if there
is an issue?

> +	cache_id = get_cpu_cacheinfo_id(cpu, 3);
> +	if (cache_id == ~0)
> +		return NULL;
> +
> +	list_for_each_entry_rcu(mon_d, &l3->resctrl_res.mon_domains, hdr.list) {
> +		dom = container_of(mon_d, struct mpam_resctrl_dom, resctrl_mon_dom);
Might as well move this under the condition.
I'm assuming no later patch needs dom for other reasons.

		if (mon_d->hdr.id == cache_id)
			return container_of(mon_d, struct mpam_resctrl_dom, resctrl_mon_dom);

> +
> +		if (mon_d->hdr.id == cache_id)
> +			return dom;
> +	}
> +
> +	return NULL;
> +}