[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4525e857-c52a-4e5d-bd74-120f66a707e3@intel.com>
Date: Wed, 7 Jan 2026 11:33:26 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: "Luck, Tony" <tony.luck@...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
Hi Tony,
On 1/7/26 10:05 AM, Luck, Tony wrote:
> On Wed, Jan 07, 2026 at 09:29:27AM -0800, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 1/5/26 12:15 PM, Luck, Tony wrote:
>>>> Ok, if it works and passes testing, I could wait for you to send me an updated
>>>> patch and drop this one.
>>>
>>> Building and testing now.
>>>
>>> Reinette: When originally developing this you suggested that rdt_get_tree()
>>> should call resctrl_arch_pre_mount() on *every* mount (to make it generally
>>> useful should future changes need something to be done in architecture code
>>> on each mount).
>>
>> I'm digging through the history just to refresh on why I made that comment. From what I can
>> tell this work always called the AET init on every mount attempt. One difference is that during
>> v2 it did so by taking some extra locks before doing so, but still did the AET init before
>> resctrl's "resctrl_mounted" check. The move to current spot (before extra locks) was made in v3,
>> and looking at v2 comments I could just find a request to use a generic resctrl_arch_* helper in
>> fs code instead of the arch specific rdt_get_intel_aet_mount() called from fs code.
>
> I should stop relying on my memory and check the actual history. You are
> right. The call from rdt_get_tree() has moved, and changed name, but all
> versions called it every time.
>
>>>
>>> That flexibility isn't needed for enumerating telemetry events. Boris' suggestion
>>> to use DO_ONCE_SLEEPABLE() would revert to what I had in some earlier
>>> version where rdt_get_tree() only calls this hook on first mount.
>>
>> I think I am missing something here - even the original RFC calls the AET init on
>> every mount. Which version are you referring to? I am also missing why DO_ONCE_SLEEPABLE()
>> requires a flow change.
>>
>>>
>>> Are you OK with this? Or do you still think that the hook should be called on
>>> every mount?
>>
>> To be specific, the current implementation calls the resctrl_arch_pre_mount() hook on
>> every mount *attempt*. For the hook to be called on every mount it should be after the
>> resctrl_mounted check. This would change resctrl_arch_pre_mount() to be called with
>> rdtgroup_mutex held though but that seems trouble since resctrl_arch_pre_mount() currently
>> follows lock ordering of domain_list_lock then rdtgroup_mutex to match lock ordering
>> during resctrl init.
>
> Yes, the call was moved before any locks obtained because of lock
> ordering issues with domain_list_lock.
>
> A better summary of the change is that the "only once" logic is being
> moved from open-coded using atomic operations in resctrl_arch_pre_mount()
> to using DO_ONCE_SLEEPABLE() in rdt_get_tree().
One thing about DO_ONCE_SLEEPABLE() that is unexpected to me is that it disables the static
key in a workqueue that seems unnecessary. Original motivation for the workqueue on which DO_ONCE()
is based (per commit a48e42920ff3 ("net: introduce new macro net_get_random_once")) was to support
calling code from atomic sections. Looks like DO_ONCE_SLEEPABLE() copied this implementation. It switched
the spinlock to mutex but the changelog (62c07983bef9 ("once: add DO_ONCE_SLOW() for sleepable contexts")) does
not mention revisiting the workqueue. Looks like deferring it to workqueue does make it easier to not have
to worry whether helper is called with hotplug lock held or not though.
Do you see any issue with deferring the disable of the static key in the resctrl usage? Since
resctrl_arch_pre_mount() is called without any locks in resctrl control it now relies on fs code
to not have rdt_get_tree() called concurrently and thus risk resctrl_arch_pre_mount() called before
static key is disabled? I just want to make sure here since from what I can tell this makes resctrl the
first user of this helper apart from code for which this helper was created and there may be implicit
assumptions that resctrl does not adhere to.
Reinette
Powered by blists - more mailing lists