Re: [PATCH v17 13/32] x86,fs/resctrl: Add an architectural hook called for each mount

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 1/7/26 4:16 PM, Luck, Tony wrote:
> 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.

ok, that is fair.

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

Thank you for considering. This is getting difficult to read.

> 	// 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()

Thank you for considering the options. Placing DO_ONCE_SLEEPABLE() in
rdt_get_tree() is fine by me.

Reinette