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

[Andreas Hindborg]

Andreas Hindborg <a.hindborg@xxxxxxxxxx> writes:

> "Alice Ryhl" <aliceryhl@xxxxxxxxxx> writes:
>
>> On Tue, May 12, 2026 at 10:39:57AM +0200, Andreas Hindborg wrote:
>>> Andreas Hindborg <a.hindborg@xxxxxxxxxx> writes:
>>>
>>> > "Alice Ryhl" <aliceryhl@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> >>> > >
>>> >>> > >> +    /// 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.

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.

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?


Best regards,
Andreas Hindborg