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

[Luck, Tony]

On Wed, Jun 10, 2026 at 10:58:26AM -0700, Reinette Chatre wrote:
> 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?

I need to resolve the unbind issue. Looks like I may have to extend the
registration interface to pass a pointer to the device so I can call
get_device() to prevent such an unbind operation while the file system
is mounted in addition to try_get_module() to stop the module from being
unloaded.

Or talk David into marking the driver with "device_driver::suppress_bind_attrs"
so that unbind isn't available for the user.

But that's separate from the issue of whether all those "if (pmt_module)" tests are needed.

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

Yes. pmt_module is NULL when the kernel is built with CONFIG_INTEL_PMT_TELEMETRY=y

I.e. the value of THIS_MODULE in the pmt_telemetry code is NULL when it
isn't a module, but is built-in to the kernel.

> 
> > 		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?

If it was NULL earlier (because CONFIG_INTEL_PMT_TELEMETRY=y) then it
will be NULL forever. If it was non-NULL above, then it must still be
non-NULL here because changes are protected but the aet_register_lock
mutex.
> 

> > 			module_put(pmt_module);
> > 	} else {
> > 		pmt_in_use = true;
> 
> So pmt_in_use could be true if pmt_module is unset? Confusing, no?

I'm using "pmt", it just isn't a module. I'm using the built-in kernel
copy of the code.

Do you have a better suggestion for the name of "pmt_in_use" that makes
it more obvious that it doesn't refer to the loaded state of the module?

Or maybe change the "pmt_module" variable name? Suggestions welcome.

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

As above. If CONFIG_INTEL_PMT_TELEMETRY=y then pmt_module is always NULL.
The module didn't disappear, there never was a module. It was built-in
the whole time.

> 
> > 		module_put(pmt_module);
> > 	pmt_in_use = false;
> 
> ... if pmt_module was unset then how could pmt_in_use ever be true?

It can be set in the case where the kernel was built with CONFIG_INTEL_PMT_TELEMETRY=y
> 
> > }
> 
> Reinette

-Tony