Message-ID: <fba9db1e-4840-432e-87ff-12819381ff41@intel.com>
Date: Fri, 18 Apr 2025 22:14:33 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>, Fenghua Yu <fenghuay@...dia.com>, "Maciej
 Wieczor-Retman" <maciej.wieczor-retman@...el.com>, Peter Newman
	<peternewman@...gle.com>, James Morse <james.morse@....com>, Babu Moger
	<babu.moger@....com>, Drew Fustini <dfustini@...libre.com>, Dave Martin
	<Dave.Martin@....com>, Anil Keshavamurthy <anil.s.keshavamurthy@...el.com>
CC: <linux-kernel@...r.kernel.org>, <patches@...ts.linux.dev>
Subject: Re: [PATCH v3 19/26] x86/resctrl: Sanity check telemetry RMID values

Hi Tony,

On 4/7/25 4:40 PM, Tony Luck wrote:
> There are three values of interest:
> 1) The number of RMIDs supported by the CPU core. This is enumerated by
>    CPUID leaf 0xF. Linux saves the value in boot_cpu_data.x86_cache_max_rmid.
> 2) The number of counter registers in each telemetry region. This is
>    described in the XML file for the region. Linux hard codes it into
>    the struct telem_entry..num_rmids field.

Syntax telem_entry::num_rmids can be used for a member.

> 3) The maximum number of RMIDs that can be tracked simultaneously for
>    a telemetry region. This is provided in the structures received from
>    the intel_pmt_get_regions_by_feature() calls.

Is (2) and (3) not required to be the same? If not, how does resctrl know
which counter/RMID is being tracked?

> 
> Print appropriate warnings if these values do not match.

As mentioned in cover letter I do not think that just printing a warning
is sufficient. It really becomes a trial-and-error guessing game for user
space to know which monitor group supports telemetry events.

> 
> TODO: Need a better UI. The number of implemented counters can be
> different per telemetry region.
> 
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 31 +++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index 67a1245858dc..0bcbac326bee 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -13,6 +13,7 @@
>  
>  #include <linux/cpu.h>
>  #include <linux/cleanup.h>
> +#include <linux/minmax.h>

Please sort includes alphabetically.

>  #include "fake_intel_aet_features.h"
>  #include <linux/intel_vsec.h>
>  #include <linux/resctrl.h>
> @@ -51,6 +52,7 @@ struct pmt_event {
>   * @last_overflow_tstamp_off:	Offset of overflow timestamp
>   * @last_update_tstamp_off:	Offset of last update timestamp
>   * @active:			Marks this group as active on this system
> + * @rmid_warned:		Set to stop multiple rmid sanity warnings

rmid -> RMID. 

I find the description unclear on how to interact with this member. How about
something like:
	True if user space have been warned about number of RMIDs used by
	different resources not matching.

>   * @num_events:			Size of @evts array
>   * @evts:			Telemetry events in this group
>   */
> @@ -63,6 +65,7 @@ struct telem_entry {
>  	int	last_overflow_tstamp_off;
>  	int	last_update_tstamp_off;
>  	bool	active;
> +	bool	rmid_warned;
>  	int	num_events;
>  	struct pmt_event evts[];
>  };
> @@ -84,6 +87,33 @@ static struct telem_entry *telem_entry[] = {
>  	NULL
>  };
>  
> +static void rmid_sanity_check(struct telemetry_region *tr, struct telem_entry *tentry)
> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_PERF_PKG].r_resctrl;
> +	int system_rmids = boot_cpu_data.x86_cache_max_rmid + 1;

It is not clear what "system_rmids" should represent here. Is it, as changelog states,
maximum supported by CPU core, or is it maximum supported by L3 resource, which is the
maximum number of monitor groups that can be created.

We see in rdt_get_mon_l3_config() that:
	r->num_rmid = (boot_cpu_data.x86_cache_max_rmid + 1) / snc_nodes_per_l3_cache;

This makes me wonder how this feature behaves on SNC systems?

> +
> +	if (tentry->rmid_warned)
> +		return;
> +
> +	if (tentry->num_rmids != system_rmids) {
> +		pr_info("Telemetry region %s has %d RMIDs system supports %d\n",

Is pr_info() intended to be pr_warn()?
The message self could do with a comma?

> +			tentry->name, tentry->num_rmids, system_rmids);
> +		tentry->rmid_warned = true;
> +	}

Could you please add comments about consequences of when this is encountered?

> +
> +	if (tr->num_rmids < tentry->num_rmids) {
> +		pr_info("Telemetry region %s only supports %d simultaneous RMIDS\n",
> +			tentry->name, tr->num_rmids);
> +		tentry->rmid_warned = true;
> +	}

I am still trying to get used to all the data structures. From what I can tell, the
offset of counter is obtained from struct telem_entry. If struct telem_entry thus
thinks there are more RMIDs than what the region supports, would this not cause
memory reads to exceed what region supports?

Could you please add comments about consequences of when this is encountered?

> +
> +	/* info/PKG_PERF_MON/num_rmids reports number of guaranteed counters */
> +	if (!r->num_rmid)
> +		r->num_rmid = tr->num_rmids;
> +	else
> +		r->num_rmid = min((u32)r->num_rmid, tr->num_rmids);
> +}

As I mentioned in response to previous version it may be possible to move
resctrl_mon_resource_init() to rdt_get_tree() to be done after these RMID
counts are discovered. When doing so it is possible to size the available
RMIDs used on system to be supported by all resources.

> +
>  /*
>   * Scan a feature group looking for guids recognized
>   * and update the per-package counts of known groups.
> @@ -109,6 +139,7 @@ static bool count_events(struct pkg_info *pkg, int max_pkgs, struct pmt_feature_
>  					pr_warn_once("MMIO region for guid 0x%x too small\n", tr->guid);
>  					continue;
>  				}
> +				rmid_sanity_check(tr, *tentry);
>  				found = true;
>  				(*tentry)->active = true;
>  				pkg[tr->plat_info.package_id].count++;

Reinette

Powered by blists - more mailing lists