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

[Reinette Chatre]

Hi Tony,

On 6/10/26 10:24 AM, Luck, Tony wrote:
>> 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.
> 
> pmt_module is NULL if CONFIG_INTEL_PMT_TELEMETRY=y ... i.e. built-in to the kernel.
> 
> In that case it obviously can't go away, and doesn't need module_get()/module_put().
> There's no special case for this. try_module_get() takes a fault on NULL dereference.
> 
> When CONFIG_INTEL_PMT_TELEMETRY=m then the get/put should stop it going away
> (I tried rmmod while resctrl mounted and it fails to remove as expected).

Have you tried "unbind" via sysfs?

Even so, my comment was in response to the code you shared. Let me paste it
back and highlight what I meant with the "many pmt_module checks":

> /*
>  * 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) {

Here is an "if (pmt_module)" check ... can it ever be false? If so
then the rest of this function becomes very confusing (more below ...)

> 		if (!try_module_get(pmt_module))
> 			return false;
> 	}
> 
> 	ret = aet_get_events();

aet_get_events() can thus seemingly be called when pmt_module is unset?

> 
> 	if (!ret) {
> 		if (pmt_module)

Can pmt_module be unset here?

> 			module_put(pmt_module);
> 	} else {
> 		pmt_in_use = true;

So pmt_in_use could be true if pmt_module is unset? Confusing, no?

> 	}
> 
> 	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)

So this implies that pmt_module could be unset here ... if that is the
case then that means that the PMT module disappeared while resctrl was
mounted which is exactly what this work aims to prevent, no?

> 		module_put(pmt_module);
> 	pmt_in_use = false;

... if pmt_module was unset then how could pmt_in_use ever be true?

> }

Reinette