Re: [PATCH v3 28/47] arm_mpam: resctrl: Add support for csu counters

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

Hi Reinette,

On 1/13/26 23:14, Reinette Chatre wrote:
> Hi Ben,
> 
> On 1/12/26 8:58 AM, Ben Horgan wrote:
>> From: James Morse <james.morse@arm.com>
>>
>> resctrl exposes a counter via a file named llc_occupancy. This isn't really
>> a counter as its value goes up and down, this is a snapshot of the cache
>> storage usage monitor.
>>
>> Add some picking code to find a cache as close as possible to the L3 that
>> supports the CSU monitor.
>>
>> If there is an L3, but it doesn't have any controls, force the L3 resource
>> to exist. The existing topology_matches_l3() and
>> mpam_resctrl_domain_hdr_init() code will ensure this looks like the L3,
>> even if the class belongs to a later cache.
>>
>> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> Co-developed-by: Dave Martin <dave.martin@arm.com>
>> Signed-off-by: Dave Martin <dave.martin@arm.com>
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>> ---
>> Changes since rfc:
>> Allow csu counters however many partid or pmg there are
>> else if -> if
>> reduce scope of local variables
>> drop has_csu
>>
>> Changes since v2:
>> return -> break so works for mbwu in later patch
>> add for_each_mpam_resctrl_mon
>> return error from mpam_resctrl_monitor_init(). It may fail when is abmc
>> allocation introduced in a later patch.
>> Squashed in patch from Dave Martin:
>> https://lore.kernel.org/lkml/20250820131621.54983-1-Dave.Martin@arm.com/
>> ---
>>  drivers/resctrl/mpam_internal.h |   6 ++
>>  drivers/resctrl/mpam_resctrl.c  | 173 +++++++++++++++++++++++++++++++-
>>  2 files changed, 174 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/resctrl/mpam_internal.h b/drivers/resctrl/mpam_internal.h
>> index f89ceaf7623d..21cc776e57aa 100644
>> --- a/drivers/resctrl/mpam_internal.h
>> +++ b/drivers/resctrl/mpam_internal.h
>> @@ -349,6 +349,12 @@ struct mpam_resctrl_res {
>>  	struct rdt_resource	resctrl_res;
>>  };
>>  
>> +struct mpam_resctrl_mon {
>> +	struct mpam_class	*class;
>> +
>> +	/* per-class data that resctrl needs will live here */
>> +};
>> +
>>  static inline int mpam_alloc_csu_mon(struct mpam_class *class)
>>  {
>>  	struct mpam_props *cprops = &class->props;
>> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
>> index 7402bf4293b6..5020a5faed96 100644
>> --- a/drivers/resctrl/mpam_resctrl.c
>> +++ b/drivers/resctrl/mpam_resctrl.c
>> @@ -37,6 +37,21 @@ static struct mpam_resctrl_res mpam_resctrl_controls[RDT_NUM_RESOURCES];
>>  /* The lock for modifying resctrl's domain lists from cpuhp callbacks. */
>>  static DEFINE_MUTEX(domain_list_lock);
>>  
>> +/*
>> + * The classes we've picked to map to resctrl events.
>> + * Resctrl believes all the worlds a Xeon, and these are all on the L3. This
>> + * array lets us find the actual class backing the event counters. e.g.
>> + * the only memory bandwidth counters may be on the memory controller, but to
>> + * make use of them, we pretend they are on L3.
>> + * Class pointer may be NULL.
>> + */
>> +static struct mpam_resctrl_mon mpam_resctrl_counters[QOS_NUM_EVENTS];
>> +
>> +#define for_each_mpam_resctrl_mon(mon, eventid)					\
>> +	for (eventid = 0, mon = &mpam_resctrl_counters[eventid];		\
>> +	     eventid < QOS_NUM_EVENTS;						\
>> +	     eventid++, mon = &mpam_resctrl_counters[eventid])
>> +
> 
> Reading the above loop and how it is used to call mpam_resctrl_monitor_init() for every event
> it looks like there is an implicit assumption that MPAM supports all events known to
> resctrl.
> 
> Please consider the most recent resctrl feature "telemetry monitoring" currently queued
> for inclusion: https://lore.kernel.org/lkml/20251217172121.12030-1-tony.luck@intel.com/
> 
> (You can find latest resctrl code queued for inclusion on the x86/cache branch of
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git)

I'll test against this.

> 
> New telemetry monitoring introduces several new events known to resctrl. Specifically, here
> is how enum resctrl_event_id looks at the moment:
> 
> /* Event IDs */                                                                 
> enum resctrl_event_id {                                                         
> 	/* Must match value of first event below */                             
> 	QOS_FIRST_EVENT			= 0x01,                                 
[...]

Thanks for bringing this to my attention. mpam_resctrl_monitor_init()
won't be called for all events known to resctrl as
mpam_resctrl_pick_counters() will only set a class for the 3 that MPAM
knows about. Still, it is probably best to restrict the iterator to the
relevant ones.

> };                                     
> 
> ...
> 
>> @@ -582,6 +677,57 @@ static int mpam_resctrl_pick_domain_id(int cpu, struct mpam_component *comp)
>>  	return comp->comp_id;
>>  }
>>  
>> +static int mpam_resctrl_monitor_init(struct mpam_resctrl_mon *mon,
>> +				     enum resctrl_event_id type)
>> +{
>> +	struct mpam_resctrl_res *res = &mpam_resctrl_controls[RDT_RESOURCE_L3];
>> +	struct rdt_resource *l3 = &res->resctrl_res;
>> +
>> +	lockdep_assert_cpus_held();
>> +
>> +	/* There also needs to be an L3 cache present */
>> +	if (get_cpu_cacheinfo_id(smp_processor_id(), 3) == -1)
>> +		return 0;
>> +
>> +	/*
>> +	 * If there are no MPAM resources on L3, force it into existence.
>> +	 * topology_matches_l3() already ensures this looks like the L3.
>> +	 * The domain-ids will be fixed up by mpam_resctrl_domain_hdr_init().
>> +	 */
>> +	if (!res->class) {
>> +		pr_warn_once("Faking L3 MSC to enable counters.\n");
>> +		res->class = mpam_resctrl_counters[type].class;
>> +	}
>> +
>> +	/* Called multiple times!, once per event type */
>> +	if (exposed_mon_capable) {
>> +		l3->mon_capable = true;
>> +
>> +		/* Setting name is necessary on monitor only platforms */
>> +		l3->name = "L3";
>> +		l3->mon_scope = RESCTRL_L3_CACHE;
>> +
>> +		resctrl_enable_mon_event(type);
> 
> btw, the telemetry work also changed this function prototype to be:
> 	bool resctrl_enable_mon_event(enum resctrl_event_id eventid, bool any_cpu,
> 				      unsigned int binary_bits, void *arch_priv);

I will update to use the new signature.

> 
> If I understand correctly resctrl_enable_mon_event() will be called for every event in 
> enum resctrl_event_id which now contains events that may not actually be supported. I think it
> may be safer to be specific in which events MPAM wants to enable.

As noted above, this only happens for the ones chosen
mpam_resctrl_pick_counters().

> 
>> +
>> +		/*
>> +		 * Unfortunately, num_rmid doesn't mean anything for
>> +		 * mpam, and its exposed to user-space!
>> +		 *
> 
> The idea of adding a per MON group "num_mon_groups" file has been floated a couple of
> times now. I have not heard any objections against doing something like this.
> https://lore.kernel.org/all/cbe665c2-fe83-e446-1696-7115c0f9fd76@arm.com/
> https://lore.kernel.org/lkml/46767ca7-1f1b-48e8-8ce6-be4b00d129f9@intel.com/

Hmm, I see now that 'num_rmid' is documented as an upper bound and so
neither 1 or mpam_pmg_max + 1 agree with the documentation.

"
"num_rmids":
		The number of RMIDs available. This is the
		upper bound for how many "CTRL_MON" + "MON"
		groups can be created.
"

So, if I understand correctly you're proposing setting
num_rmids = num_pmg * num_partids on arm platforms and that in the
interim this can then be used to calculate the num_pmg by calculating
num_closid/num_rmid but that a per CTRL_MON num_mon_groups should be
added to make this consistent across architectures?

> 
>> +		 * num-rmid is supposed to mean the minimum number of
>> +		 * monitoring groups that can exist simultaneously, including
>> +		 * the default monitoring group for each control group.
>> +		 *
>> +		 * For mpam, each control group has its own pmg/rmid space, so
>> +		 * it is not appropriate to advertise the whole rmid_idx space
>> +		 * here.  But the pmgs corresponding to the parent control
>> +		 * group can be allocated freely:
>> +		 */
>> +		l3->mon.num_rmid = mpam_pmg_max + 1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
> 
> Reinette
> 

I appreciate that you have shared this resctrl knowledge with me.

Thanks,

Ben