[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <89e640fc-6bd7-40b1-8968-ca1a85005f4c@intel.com>
Date: Mon, 6 Oct 2025 14:33:00 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: "Luck, Tony" <tony.luck@...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
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
Powered by blists - more mailing lists