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 12:25 PM, Luck, Tony wrote:
> On Wed, Jan 07, 2026 at 11:33:26AM -0800, Reinette Chatre wrote:
>>> 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,
> 
> The deferred reset of the static key does seem unnecessary.
> 
> But it looks like DO_ONCE_SLEEPABLE() is still correct. If there are
> multiple parallel calls before the static key is reset, then the 2nd and
> subsequent instance will block on mutex_lock(&once_mutex) in
> __do_once_sleepable_start(). When it is their turn, they will find that
> "*done" is true, so resctrl_arch_pre_mount() will not be called again.

Ah - I see. Thank you. So in addition to the static key there is the "done"
variable that is protected with the mutex and protects against a second call
before static key can be disabled.

> 
> Side note: This global "once_mutex" means that any other subsystem using
> DO_ONCE_SLEEPABLE() would be blocked waiting for resctrl_arch_pre_mount()
> to complete. Same is true for DO_ONCE() where parallel calls from
> different subsystems would be serialized by the "once_lock" spinlock.

oh, good catch.

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