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

[Andreas Hindborg <a.hindborg@kernel.org>]

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

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

As I said, I'm fine with whatever name, but I'd appreciate if someone
else chime in, so we don't have to change the name too many times.


Best regards,
Andreas Hindborg