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

Ben Horgan
2026-01-07T15:19:59Z

Hi Jonathan,

On 1/6/26 14:01, Jonathan Cameron wrote:
> 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).

I'll keep it friendly and drop 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.

Done.

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

This is synchronized by the wait_event() in mpam_resctrl_setup() but I
realize now that this is only added later in the series. I'll consider
bringing that earlier in the series.

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

I've updated to this already based on your comments on the rfc.

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

Thanks,

Ben