Message-ID: <aOQIMQsgBOta0PRP@agluck-desk3>
Date: Mon, 6 Oct 2025 11:19:29 -0700
From: "Luck, Tony" <tony.luck@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
CC: 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>, Chen Yu
	<yu.c.chen@...el.com>, <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v11 14/31] x86/resctrl: Discover hardware telemetry events

On Fri, Oct 03, 2025 at 04:35:11PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 9/25/25 1:03 PM, Tony Luck wrote:
> > Each CPU collects data for telemetry events that it sends to the nearest
> > telemetry event aggregator either when the value of IA32_PQR_ASSOC.RMID
> 
> Please note that one of the "Touchups" done during merge of [1] was to
> use full names for registers in descriptions. Considering this,
> "IA32_PQR_ASSOC.RMID" -> "MSR_IA32_PQR_ASSOC.RMID
> 
> (also please make same change in cover letter)

Will do.

> 
> > changes, or when a two millisecond timer expires.
> > 
> 
> ...
> 
> > +
> > +/**
> > + * struct event_group - All information about a group of telemetry events.
> > + * @pfg:		Points to the aggregated telemetry space information
> > + *			returned by the intel_pmt_get_regions_by_feature()
> > + *			call to the INTEL_PMT_TELEMETRY driver that contains
> > + *			data for all telemetry regions of a specific type.
> > + *			Valid if the system supports the event group.
> > + *			NULL otherwise.
> > + * @guid:		Unique number per XML description file.
> > + */
> > +struct event_group {
> > +	/* Data fields for additional structures to manage this group. */
> > +	struct pmt_feature_group	*pfg;
> > +
> > +	/* Remaining fields initialized from XML file. */
> > +	u32				guid;
> > +};
> 
> 
> ...
> 
> > +
> > +/*
> > + * Make a request to the INTEL_PMT_TELEMETRY driver for a copy of the
> > + * pmt_feature_group for a specific feature. If there is one, the returned
> > + * structure has an array of telemetry_region structures. Each describes
> > + * one telemetry aggregator.
> > + * Try to use every telemetry aggregator with a known guid.
> 
> The guid is associated with struct event_group and every telemetry region has
> its own guid. It is not clear to me why the guid is not associated with pmt_feature_group.
> To me this implies that a pmt_feature_group my contain telemetry regions that have
> different guid.
> 
> This is not fully apparent in this patch but as this code evolves I do not think
> the scenario where telemetry regions have different supported (by resctrl) guid is handled
> by this enumeration.
> If I understand correctly, all telemetry regions of a given pmt_feature_group will be
> matched against a single supported guid at a time and all telemetry regions with that
> guid will be considered usable and any other considered unusable without further processing
> of that pmt_feature_group. If there are more than one matching guid supported by resctrl
> then only events of the first one will be enumerated?
> 
> > + */
> > +static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
> > +			    unsigned int num_evg)
> > +{
> > +	struct pmt_feature_group *p __free(intel_pmt_put_feature_group) = NULL;
> > +	struct event_group **peg;
> > +	bool ret;
> > +
> > +	p = intel_pmt_get_regions_by_feature(feature);
> > +
> > +	if (IS_ERR_OR_NULL(p))
> > +		return false;
> > +
> > +	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
> > +		ret = enable_events(*peg, p);
> > +		if (ret) {
> > +			(*peg)->pfg = no_free_ptr(p);
> > +			return true;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}

Perhaps David wants to cope with a future system that supports multiple
guids?

You are right that my code will not handle this. It will just enable
the first recognised guid and ignore any others.

How about this. Take an extra reference on any pmt_feature_group
structures that include a known guid (to keep the accounting right
when intel_aet_exit() is called). This simplifies the function so
I don't need the __free() handler that confuses checkpatch.pl :-)


/*
 * Make a request to the INTEL_PMT_TELEMETRY driver for a copy of the
 * pmt_feature_group for a specific feature. If there is one, the returned
 * structure has an array of telemetry_region structures, each element of
 * the array describes one telemetry aggregator.
 * A single pmt_feature_group may include multiple different guids.
 * Try to use every telemetry aggregator with a known guid.
 */
static bool get_pmt_feature(enum pmt_feature_id feature, struct event_group **evgs,
			    unsigned int num_evg)
{
	struct pmt_feature_group *p = intel_pmt_get_regions_by_feature(feature);
	struct event_group **peg;
	bool ret = false;

	if (IS_ERR_OR_NULL(p))
		return false;

	for (peg = evgs; peg < &evgs[num_evg]; peg++) {
		if (enable_events(*peg, p)) {
			kref_get(&p->kref);
			(*peg)->pfg = no_free_ptr(p);
			ret = true;
		}
	}
	intel_pmt_put_feature_group(p);

	return ret;
}

> Reinette
> 
> 
> [1] https://lore.kernel.org/all/175793566119.709179.8448328033383658699.tip-bot2@tip-bot2/

Powered by blists - more mailing lists