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

[Luck, Tony]

On Thu, Jun 11, 2026 at 03:27:16PM -0700, Luck, Tony wrote:
> On Thu, Jun 11, 2026 at 02:22:23PM -0700, Reinette Chatre wrote:
> > Hi Tony,
> > 
> > On 6/11/26 11:01 AM, Luck, Tony wrote:
> > > With renamed variable, and comment. Is this clearer?
> > > 
> > > 
> > > /*
> > >  * intel_aet_register_enumeration() provides the value of "THIS_MODULE"
> > >  * from the the pmt_telemetry module. Note that this is NULL when
> > >  * CONFIG_INTEL_PMT_TELEMETRY=y so it serves as a flag to decide whether
> > >  * try_module_get() and module_put() need to be called.
> > >  */
> > > static struct module *pmt_is_a_module;
> > > 
> > 
> > Using a single variable to contain both data and state has caused problem in
> > the past. Could it be helpful to create two utilities, for example get_pmt()
> > and put_pmt() although naming is not at issue at the moment, that contains
> > and makes obvious these "is INTEL_PMT_TELEMETRY a module or not" checks? These would
> > then be no-op when INTEL_PMT_TELEMETRY=y. This would simplify the code in the
> > callers by containing this logic and not rely on callers doing this check so
> > many times.
> 
> I can code this up to see how it looks.

Eye-strain "if (...)" are gone. Code is now:

--- internal.h ---

#ifdef CONFIG_INTEL_PMT_TELEMETRY_MODULE
static inline bool intel_aet_try_module_get(struct module *mod)
{
	return try_module_get(mod);
}
static inline void intel_aet_module_put(struct module *mod)
{
	module_put(mod);
}
#else
static inline bool intel_aet_try_module_get(struct module *mod) { return true; }
static inline void intel_aet_module_put(struct module *mod) { }
#endif

--- intel_aet.c ---

bool intel_aet_pre_mount(void)
{
	bool ret;

	guard(mutex)(&aet_register_lock);
	if (!get_feature || !put_feature)
		return false;

	if (!intel_aet_try_module_get(pmt_module))
		return false;

	ret = aet_get_events();

	if (!ret)
		intel_aet_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) {
		struct event_group *e = *peg;

		if (e->pfg) {
			for (int j = 0; j < e->num_events; j++)
				resctrl_disable_mon_event(e->evts[j].id);
			put_feature(e->pfg);
			e->pfg = NULL;
		}
	}
	intel_aet_module_put(pmt_module);
	pmt_in_use = false;
}

-Tony