Re: [PATCH v7 22/31] x86/resctrl: Read telemetry events

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 7/11/25 4:53 PM, Tony Luck wrote:
> The resctrl file system passes requests to read event monitor files to
> the architecture resctrl_arch_rmid_read() to collect values
> from hardware counters.
> 
> Use the resctrl resource to differentiate between calls to read legacy
> L3 events from the new telemetry events (which are attached to
> RDT_RESOURCE_PERF_PKG).
> 
> There may be multiple aggregators tracking each package, so scan all of
> them and add up all counters.
> 
> Enable the events marked as readable from any CPU providing an
> mon_evt::arch_priv pointer to the struct pmt_event for each
> event.
> 
> At run time when a user reads an event file the file system code
> provides the enum resctrl_event_id for the event and the arch_priv
> pointer that was supplied when the event was enabled.

The changelog ordering seems random. It starts by describing how reading of events are
handled and how counters are added when an event is read, then describes enabling the
events (this should happen before an event can be read?), then how data is passed when
reading an event (that should be followed by adding up the counters?).

I think it may help to clearly describe the phases involved. For example, start
with how events are enabled during enumeration/discovery, then how data is
passed during runtime when a user reads an event file, then how the
data is collected.

> 
> Resctrl now uses readq() so depends on X86_64. Update Kconfig.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
...

> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index f4bf0f2ccf26..bd6011a95d12 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -14,6 +14,7 @@
>  #include <linux/cleanup.h>
>  #include <linux/cpu.h>
>  #include <linux/intel_vsec.h>
> +#include <linux/io.h>
>  #include <linux/resctrl.h>
>  #include <linux/slab.h>
>  
> @@ -213,6 +214,13 @@ static int discover_events(struct event_group *e, struct pmt_feature_group *p)
>  
>  	list_add(&e->list, &active_event_groups);
>  

Should this addition be documented as "step 3"?

> +	for (int i = 0; i < e->num_events; i++) {
> +		enum resctrl_event_id eventid;
> +
> +		eventid = e->evts[i].id;
> +		resctrl_enable_mon_event(eventid, true, e->evts[i].bin_bits, &e->evts[i]);

Why is eventid needed? I think using e->evts[i].id makes it more obvious how
the parameters relate.

> +	}
> +
>  	return 0;
>  }
>  

Reinette