Re: [PATCH v11 17/31] x86/resctrl: Find and enable usable telemetry events

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

Hi Tony,

On 10/6/25 12:58 PM, Luck, Tony wrote:
> On Fri, Oct 03, 2025 at 04:52:01PM -0700, Reinette Chatre wrote:
>> On 9/25/25 1:03 PM, Tony Luck wrote:
>>> @@ -114,12 +116,44 @@ static struct event_group *known_perf_event_groups[] = {
>>>  	for (_peg = (_grp); _peg < &_grp[ARRAY_SIZE(_grp)]; _peg++)	\
>>>  		if ((*_peg)->pfg)
>>>  
>>> -/* Stub for now */
>>> -static bool enable_events(struct event_group *e, struct pmt_feature_group *p)
>>> +static bool skip_telem_region(struct telemetry_region *tr, struct event_group *e)
>>>  {
>>> +	if (tr->guid != e->guid)
>>> +		return true;
>>> +	if (tr->plat_info.package_id >= topology_max_packages()) {
>>> +		pr_warn("Bad package %u in guid 0x%x\n", tr->plat_info.package_id,
>>> +			tr->guid);
>>> +		return true;
>>> +	}
>>
>> I have not encountered any mention of the possibility that packages may differ
>> in which telemetry region types they support. For example, could it be possible for package
>> A to have usable regions of the PERF type but package B doesn't? From what I can tell
>> INTEL_PMT_TELEMETRY supports layouts where this can be possible. If I understand correctly
>> this implementation will create event files for these domains but when the user attempts to
>> read the data it will fail. Can this work add some snippet about possibility of this
>> scenario and if/how it is supported?
> 
> Yes, this is architecturally possible. But I do not expect that systems will
> be built that do this. You are right that such a system will create files that
> always return "Unavailable" when read.
> 
> Is it sufficient to document this in the commit message?
> 
> I don't feel that it would be worthwhile to suppress creation of these files for
> a "can't happen" situation. I'm not sure that doing so would be significantly
> better from a user interface perspective. Users would get slightly more notice
> (-ENOENT when trying to open the file). But the code would require
> architecture calls from file system code to check which files need to be created
> separately for each domain.

I think it is sufficient to document this in the commit message to help create
confidence in robustness in support of different scenarios. I have not encountered such
a system but could this scenario be similar to one where a two socket system supports MBM
but only one socket has memory populated? I do not know what reading the counter MSR will
return in this case though.

> 
>>
>>> +	if (tr->size != e->mmio_size) {
>>> +		pr_warn("MMIO space wrong size (%zu bytes) for guid 0x%x. Expected %zu bytes.\n",
>>> +			tr->size, e->guid, e->mmio_size);
>>> +		return true;
>>> +	}
>>> +
>>>  	return false;
>>>  }
>>>  
>>> +static bool enable_events(struct event_group *e, struct pmt_feature_group *p)
>>> +{
>>> +	bool usable_events = false;
>>> +
>>> +	for (int i = 0; i < p->count; i++) {
>>> +		if (skip_telem_region(&p->regions[i], e))
>>> +			continue;
>>> +		usable_events = true;
>>
>> A previous concern [1] was why this loop does not break out at this point. I think it will 
>> help to make this clear if marking a telemetry region as unusable (mark_telem_region_unusable())
>> is done in this patch. Doing so makes the "usable" and "unusable" distinction in one
>> patch while making clear that the loop needs to complete.
> 
> Ok. I'll pull mark_telem_region_unusable() into this patch.

Thank you.


Reinette