Message-ID: <aV73RiFGJvYxAeo2@agluck-desk3>
Date: Wed, 7 Jan 2026 16:16:06 -0800
From: "Luck, Tony" <tony.luck@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
CC: Borislav Petkov <bp@...en8.de>, Fenghua Yu <fenghuay@...dia.com>,
	"Wieczor-Retman, Maciej" <maciej.wieczor-retman@...el.com>, Peter Newman
	<peternewman@...gle.com>, James Morse <james.morse@....com>, Babu Moger
	<babu.moger@....com>, Drew Fustini <dfustini@...libre.com>, Dave Martin
	<Dave.Martin@....com>, "Chen, Yu C" <yu.c.chen@...el.com>, "x86@...nel.org"
	<x86@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "patches@...ts.linux.dev"
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v17 13/32] x86,fs/resctrl: Add an architectural hook
 called for each mount

On Wed, Jan 07, 2026 at 03:09:24PM -0800, Reinette Chatre wrote:
> Hi Tony,
> 
> On 1/7/26 2:27 PM, Luck, Tony wrote:
> > On Wed, Jan 07, 2026 at 02:09:35PM -0800, Reinette Chatre wrote:
> >> Hi Tony,
> >>> If these DO_ONCE macros are ever used heavily in run-time code, it might
> >>> be better for once_lock and once_mutex to be statically defined in each
> >>> invocation of the DO_ONCE() and DO_ONCE_SLEEPABLE() macros. But the fact
> >>> that the static key protects the spinlock/mutex from being called may
> >>> mean that it is practically hard to hit problems.
> >>
> >> Which problems do you have in mind? One problem I see is that since these "once"
> >> functions are globally forced to be serialized this may cause unnecessary delays,
> >> for example during initialization. I do not think this impacts the resctrl intended
> >> usage since resctrl_arch_pre_mount() is not called during initialization and is
> >> already ok with delays (it is on a "slow" path).
> > 
> > Reinette
> > 
> > Yes. Unnecessary delays due to serialization. But that only happens if
> > the first call to a DO_ONCE*() instance overlaps with another first
> > call. It might be quite hard to hit that during boot unless there are
> > many uses of DO_ONCE*()
> > 
> > Looking at this some more, DO_ONCE() is overkill for mounting resctrl. The
> > static key part is there so that DO_ONCE*() can be safely used in some
> > hot code path without adding overhead of checking some "bool done" type
> > variable and branching around it.  I don't see anyone except validation
> > executing resctrl mounts at multiple times per second.
> > 
> > But it does make the code easier to read with a single line with obvious
> > meaning instead of multiple lines with declarations, initializations,
> > and if () conditions.
> 
> I am ok with using DO_ONCE_SLEEPABLE(). The next question (perhaps nitpicking?) is
> if it is resctrl fs or the arch's decision to use this. That is, whether the flow is
> something like below where the arch decides:
> 	arch/x86/kernel/cpu/resctrl/core.c:
> 		void resctrl_arch_pre_mount(void)
> 		{
> 			DO_ONCE_SLEEPABLE(aet_specific_call);
> 		}

The AET code in resctrl_arch_pre_mount() includes building the domains.
That needs the domain_list_lock mutex and domain_add_cpu_mon() which are
both static in core.c. So either they need to be unstatic'd and added
to "internal.h", or that part of the code needs to stay in core.c

Opinion on making these available to intel_aet.c? I'm not a fan.

Keeping it in core.c means finding out if intel_aet_get_events()
succeeded or not. DO_ONCE_SLEEPABLE() doesn't return the return value
of the called function. It just returns true/false to say if it called
the function.

So with this approach I have:

void resctrl_arch_pre_mount(void)
{
	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_PERF_PKG].r_resctrl;
	int cpu;

	if (!DO_ONCE_SLEEPABLE(intel_aet_get_events))
		return;

	// intel_aet_get_events() sets mon_capable if it succeeds
	if (!r->mon_capable)
		return;

	/*
	 * Late discovery of telemetry events means the domains for the
	 * resource were not built. Do that now.
	 */
	cpus_read_lock();
	mutex_lock(&domain_list_lock);
	rdt_mon_capable = true;
	for_each_online_cpu(cpu)
		domain_add_cpu_mon(cpu, r);
	mutex_unlock(&domain_list_lock);
	cpus_read_unlock();
}

It does reduce by one the number of stubs. intel_aet_add_debugfs() can
be static in intel_aet.c

> 	fs/resctrl/rdtgroup.c:
> 		static int rdt_get_tree(struct fs_context *fc)
> 		{
> 			...
> 			resctrl_arch_pre_mount();
> 			...
> 		}
> 
> or something like below where resctrl fs dictates the function can only be called once:
> 
> 	arch/x86/kernel/cpu/resctrl/core.c:
> 		void resctrl_arch_pre_mount(void)
> 		{
> 			/* AET specific code */
This is the minimal change from my current series. So my laziness factor
leans toward it.

> 		}
> 
> 	fs/resctrl/rdtgroup.c:
> 		static int rdt_get_tree(struct fs_context *fc)
> 		{
> 			...
> 			DO_ONCE_SLEEPABLE(resctrl_arch_pre_mount);
> 			...
> 		}
> 
> It looks to me as though the first option creates opportunity for better isolation
> of AET code into arch/x86/kernel/cpu/resctrl/intel_aet.c, specifically, it needs fewer AET
> stubs in arch/x86/kernel/cpu/resctrl/internal.h. I do not envision resctrl fs needing
> to call resctrl_arch_pre_mount() multiple times but the safe pattern appears to be to
> place DO_ONCE* in a helper function to ensure that only one static key is ever created.
> 
> While the first option allows more flexibility to the arch that should not be a reason though
> since this is internal and we can always change to better accommodate arch requirements.
> The question here is just what is best for AET support. What do you think?

The current usage for resctrl_arch_pre_mount() is that it only needs to
be called once. As you say, that could be changed if a new requirement
appears. But the simpler approach today is to put the
DO_ONCE_SLEEPABLE() into rdt_get_tree()

> Reinette

-Tony

Powered by blists - more mailing lists