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

[Alice Ryhl <aliceryhl@google.com>]

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.
2. It doesn't fall back to a mutex under PREEMPT_RT.

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.

> >> By the way, back then I suggested renaming it from OnceLock to SetOnce
> >> because you did not support waiting for the value to be populated, and
> >> you said you didn't need that. If you add that feature, then we should
> >> rename it back to OnceLock, or create a new type OnceLock for users who
> >> need that additional feature.
> >
> > That is fair. This is a different use case than the original one though.
> > I think we should keep this as one type for code reuse, but I am fine
> > with renaming to something that describe the usage better.
> 
> Please note that even though it could be added, we do not have a `wait`
> method now. What we have are blocking initializers.

You may have open-coded `wait` inside of `as_ref_or_populate_with`, but
you still have the functionality.

Alice

> I personally have no opinion on the name. Is everyone in favor of
> renaming to OnceLock?
> 
> 
> Best regards,
> Andreas Hindborg
> 
>