Re: [PATCH] rust: sync: add lazy initialization methods to SetOnce

[Alice Ryhl <aliceryhl@google.com>]

On Tue, May 12, 2026 at 12:42:47PM +0200, Andreas Hindborg wrote:
> Andreas Hindborg <a.hindborg@kernel.org> writes:
> 
> > "Alice Ryhl" <aliceryhl@google.com> writes:
> >
> >> On Tue, May 12, 2026 at 10:39:57AM +0200, Andreas Hindborg wrote:
> >>> Andreas Hindborg <a.hindborg@kernel.org> writes:
> >>>
> >>> > "Alice Ryhl" <aliceryhl@google.com> writes:
> >>> >
> >>> >> On Mon, Feb 16, 2026 at 11:26:11AM +0000, Alice Ryhl wrote:
> >>> >>> On Mon, Feb 16, 2026 at 12:10:16PM +0100, Andreas Hindborg wrote:
> >>> >>> > "Alice Ryhl" <aliceryhl@google.com> writes:
> >>> >>> >
> >>> >>> > > On Sun, Feb 15, 2026 at 09:27:17PM +0100, Andreas Hindborg wrote:
> >>> >>> > >> Add methods to get a reference to the contained value or populate the
> >>> >>> > >> SetOnce if empty. The new `as_ref_or_populate` method accepts a value
> >>> >>> > >> directly, while `as_ref_or_populate_with` accepts a fallible closure,
> >>> >>> > >> allowing for lazy initialization that may fail. Both methods spin-wait
> >>> >>> > >> if another thread is concurrently initializing the container.
> >>> >>> > >>
> >>> >>> > >> Also add `populate_with` which takes a fallible closure and serves as
> >>> >>> > >> the implementation basis for the other populate methods.
> >>> >>> > >>
> >>> >>> > >> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> >>> >>> > >
> >>> >>> > >> +    /// Get a reference to the contained object, or populate the [`SetOnce`]
> >>> >>> > >> +    /// with the value returned by `callable` and return a reference to that
> >>> >>> > >> +    /// object.
> >>> >>> > >> +    pub fn as_ref_or_populate_with(&self, callable: impl FnOnce() -> Result<T>) -> Result<&T> {
> >>> >>> > >> +        if !self.populate_with(callable)? {
> >>> >>> > >> +            while self.init.load(Acquire) != 2 {
> >>> >>> > >> +                core::hint::spin_loop();
> >>> >>> > >> +            }
> >>> >>> > >
> >>> >>> > > We should not be implementing our own spinlocks.
> >>> >>> >
> >>> >>> > That is a great proverb. I'd be happy to receive a suggestion on an
> >>> >>> > alternate approach for this particular context.
> >>> >>>
> >>> >>> You can add a spinlock to SetOnce. Like I mentioned previously [1],
> >>> >>> support for waiting will require the addition of extra fields.
> >>> >
> >>> > Thanks, I'll be sure to take a look again.
> >>>
> >>> I took a look at this again. I think the structure will be less
> >>> efficient if we use a spin lock.
> >>>
> >>> Initialization is now
> >>>  - cmpxchg lock relaxed
> >>>  - store pointer
> >>>  - store lock release
> >>>
> >>> With a spin lock it will be
> >>>  - lock acquire
> >>>  - test pointer
> >>>  - store pointer
> >>>  - lock release
> >>>
> >>> All the other tests for initialization is now:
> >>>  - load lock acquire
> >>>  - load pointer
> >>>
> >>> They will become
> >>>  - lock acquire
> >>>  - load pointer
> >>>  - test pointer
> >>>  - lock release
> >>>
> >>>
> >>> bit_spinlock does not make this any better. It just gives us 64 locks in
> >>> a u64. It does not help us store state of the data structure
> >>> (empty/populated).
> >>>
> >>> Do we want a less efficient data structure in order to gain benefits of
> >>> lockdep and friends?
> >>
> >> I'm not just talking about lockdep. Your current implementation is wrong
> >> in several other ways, for example:
> >>
> >> 1. Spinlocks must disable preemption.
> >
> > That is an easy fix.
> >
> >> 2. It doesn't fall back to a mutex under PREEMPT_RT.
> >
> > I don't know how to solve that, but I'm sure there is a way.
> >
> >>
> >> And probably lots of other things. By using the kernel spinlock, you do
> >> not have to worry about the long list of things spinlocks have to get
> >> right.
> >
> > Nah, can't be that many things. But I get your point.

And what about the quirks added to spinlocks in the future?

I'm not willing to put my name on a manual spinlock implementation.

> So, when messing around with this `SpinLock` business, I run into a
> problem. `SetOnce::new` is `const` and has to be for the use in
> `module_param` as well as for the use I have in `rnull` where I use
> `SetOnce` in global scope:
> 
>   static SHARED_TAG_SET: SetOnce<Arc<TagSet<NullBlkDevice>>> = SetOnce::new();
> 
> I could use `global_lock` instead, but I'd rather not have the unsafe
> initializer in my driver.
> 
> We could split `SetOnce` and `OnceLock` as you have previously
> suggested, but that would still require some kind of unsafe to
> initialize a global `OnceLock`.
> 
> Based on these observations, I think I should either drop this patch
> entirely and use `global_lock!`, which I'd rather not, or we should find
> a way to open code the spin lock in a way that is compatible with the
> kernel in general.

So it's true that putting synchronization primitives in global variables
is a real problem we don't have amazing solutions to right now. There
are various ways we can work around it. I came up with one workaround,
which is a single unsafe block to ensure that the global is initialized
in `init()`. You've come up with another workaround, which is a manual
spinlock.

My opinion is that the manual spinlock is far more problematic than the
unsafe block.

And I also think that as a principle we should avoid coming up with
several different work-arounds for the same problem.

> We could yank the spinning functionality out of `SetOnce` into `Atomic`.
> An `Atomic` with the ability to spin until a certain value is observed
> could be a nice primitive. Or it could spin until a `Fn(T) -> bool` on
> the observed value returns true.
> 
> Any alternatives?

Why do you need waiting for SHARED_TAG_SET to begin with? If you make
sure to initialize it as the very first thing in your module init, then
all other uses can just `unwrap()` the `SetOnce` because you know the
value is already there.

Alice