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

I guess I could just always initialize it. I'll do that for now and then
drop this patch.

My thought was that I did not want to initialize it at all, if it is not
going to be used. I think that is how the corresponding C code is
designed in the C null block driver.

I don't _need_ to build the Rust driver the same as the C one in this
particular case, but now it is annoying me slightly that we don't have a
safe way to implement similar logic.

I guess one option I could go for if I really want to lazy init with
safe code is to move the global out of global scope and into the module
struct. Then it can be initialized with a spin lock in it and I can have
something like `OnceLock`.

Best regards,
Andreas Hindborg