Re: [PATCH v7 11/14] mpam,x86/resctrl: Resolve INTEL_PMT_TELEMETRY symbols at runtime

[Reinette Chatre]

Hi Tony,

On 6/10/26 9:34 AM, Luck, Tony wrote:
> On Wed, Jun 10, 2026 at 09:21:04AM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 6/10/26 8:49 AM, Luck, Tony wrote:
>>> On Wed, Jun 10, 2026 at 08:27:15AM -0700, Reinette Chatre wrote:
>>>> On 6/9/26 5:08 PM, Luck, Tony wrote:
>>>>> On Mon, Jun 08, 2026 at 04:25:51PM -0700, Reinette Chatre wrote:
>>
>>>>>>> +void intel_aet_unmount(void)
>>>>>>>  {
>>>>>>>  	struct event_group **peg;
>>>>>>>  
>>>>>>> +	guard(mutex)(&aet_register_lock);
>>>>>>
>>>>>> Could this get a short-circuit to make behavior on AMD obvious?
>>>>>
>>>>> Like this?
>>>>>
>>>>> 	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
>>>>> 		return;
>>>>>
>>>>> Same in intel_aet_pre_mount() for symmetry??
>>>>
>>>> I do not see that this requires to be architecture specific. This capability
>>>> introduces AET register()/unregister() and pre_mount()/unmount().
>>>> pre_mount() already does no-op if (essentially) nothing is registered. Could
>>>> unmount() similarly be a no-op if (essentially) nothing is registered?
>>>
>>> Ah, I see. Maybe I just need a better name for my "have_pmt_hold" flag
>>> so it can be used as the fast exit indicator in intel_aet_unmount().
>>>
>>> Perhaps "pmt_in_use"?
>>>
>>> Then:
>>>
>>> 	if (!pmt_in_use)
>>> 		return;
>>
>> Something like this. It is not obvious to me how the rest of unmount will look
>> with this.
>> Ideally it will be something symmetrical to pre_mount() to make the
>> flow and short-circuit of the flow obvious.
> 
> With updates applied, mount/umount look like this:
> 
> /*
>  * Track whether pmt_telemetry enumeration succeeded during mount for use
>  * during unmount.
>  */
> static bool pmt_in_use;
> 
> bool intel_aet_pre_mount(void)
> {
> 	bool ret;
> 
> 	guard(mutex)(&aet_register_lock);
> 	if (!get_feature || !put_feature)
> 		return false;
> 
> 	if (pmt_module) {
> 		if (!try_module_get(pmt_module))
> 			return false;
> 	}
> 
> 	ret = aet_get_events();
> 
> 	if (!ret) {
> 		if (pmt_module)
> 			module_put(pmt_module);
> 	} else {
> 		pmt_in_use = true;
> 	}
> 
> 	return ret;
> }
> 
> void intel_aet_unmount(void)
> {
> 	struct event_group **peg;
> 
> 	guard(mutex)(&aet_register_lock);
> 	if (!pmt_in_use)
> 		return;
> 
> 	for_each_event_group(peg) {
> 		if ((*peg)->pfg) {
> 			struct event_group *e = *peg;
> 
> 			for (int j = 0; j < e->num_events; j++)
> 				resctrl_disable_mon_event(e->evts[j].id);
> 			put_feature((*peg)->pfg);
> 			(*peg)->pfg = NULL;
> 		}
> 	}
> 	if (pmt_module)
> 		module_put(pmt_module);
> 	pmt_in_use = false;
> }

I see many "if (pmt_module)" checks ... intel_aet_pre_mount() even calls it twice. Are they all necessary?
It creates the impression that the PMT module can be yanked from AET at any time, something which
intel_aet_unregister_enumeration() seems to allow. I was hoping that there can be some guarantee
that if PMT is available during pre_mount() it will continue to be available at least until
unmount() completes.

Reinette