Re: [PATCH v11 14/31] x86/resctrl: Discover hardware telemetry events

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

Hi Tony,

On 10/6/25 11:19 AM, Luck, Tony wrote:
> 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:
>>> +
>>> +/*
>>> + * 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);

This is not clear to me ... would enable_events() still mark all telemetry_regions
that do not match the event_group's guid as unusable? It seems to me that if more
than one even_group refers to the same pmt_feature_group then the first one to match
will "win" and make the other event_group's telemetry regions unusable.

> 			(*peg)->pfg = no_free_ptr(p);
> 			ret = true;
> 		}
> 	}
> 	intel_pmt_put_feature_group(p);
> 
> 	return ret;
> }
> 

Reinette