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

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

[Andreas Hindborg]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> "Gary Guo" <gary@garyguo.net> writes:
>
>> On Sun Feb 15, 2026 at 8:27 PM GMT, 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.
>>
>> Hi Andreas, in an earlier call I mentioned that I'm working on getting SetOnce
>> to work with pin-init, the capability of which I think is a superset of you have
>> here.
>>
>> The API I have is
>>
>> impl<T> SetOnce<T> {
>>     pub fn init<E>(&self, init: impl Init<T, E>) -> Result<&T, InitError<E>>;
>>     pub fn pin_init<E>(self, Pin<&Self>, init: impl PinInit<T, E>) -> Result<&T, InitError<E>>;
>> }
>>
>> To achieve what you need with a function, you can simply write:
>>
>>     set_once.init(pin_init::init_scope(your_fn))
>>
>> The patch that implement the API is here:
>> https://github.com/nbdd0121/linux/commit/4aabdbcf20b11626c253f203745b1d55c37ab2ee
>> in tree
>> https://github.com/nbdd0121/linux/tree/lazy_revocable_nova_wip/
>>
>> which I haven't submitted to the list as the user side of this API isn't ready.
>
> I probably evicted that from cache.
>
> It looks like that could replace the `populate` in my patch? We would
> still need the synchronization in `as_ref_or_populate`, right? (or
> `as_ref_or_init` if you will)

Looks like this is not ready yet. I think we can move forward with the
current suggestion and then wire up pin-init when it lands.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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?

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

I personally have no opinion on the name. Is everyone in favor of
renaming to OnceLock?


Best regards,
Andreas Hindborg

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

[Alice Ryhl]

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

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

[Andreas Hindborg]

"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

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

[Andreas Hindborg]

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.

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

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

[Gary Guo]

On Tue May 12, 2026 at 9:07 AM BST, Andreas Hindborg wrote:
> Andreas Hindborg <a.hindborg@kernel.org> writes:
>
>> "Gary Guo" <gary@garyguo.net> writes:
>>
>>> On Sun Feb 15, 2026 at 8:27 PM GMT, 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.
>>>
>>> Hi Andreas, in an earlier call I mentioned that I'm working on getting SetOnce
>>> to work with pin-init, the capability of which I think is a superset of you have
>>> here.
>>>
>>> The API I have is
>>>
>>> impl<T> SetOnce<T> {
>>>     pub fn init<E>(&self, init: impl Init<T, E>) -> Result<&T, InitError<E>>;
>>>     pub fn pin_init<E>(self, Pin<&Self>, init: impl PinInit<T, E>) -> Result<&T, InitError<E>>;
>>> }
>>>
>>> To achieve what you need with a function, you can simply write:
>>>
>>>     set_once.init(pin_init::init_scope(your_fn))
>>>
>>> The patch that implement the API is here:
>>> https://github.com/nbdd0121/linux/commit/4aabdbcf20b11626c253f203745b1d55c37ab2ee
>>> in tree
>>> https://github.com/nbdd0121/linux/tree/lazy_revocable_nova_wip/
>>>
>>> which I haven't submitted to the list as the user side of this API isn't ready.
>>
>> I probably evicted that from cache.
>>
>> It looks like that could replace the `populate` in my patch? We would
>> still need the synchronization in `as_ref_or_populate`, right? (or
>> `as_ref_or_init` if you will)
>
> Looks like this is not ready yet. I think we can move forward with the
> current suggestion and then wire up pin-init when it lands.
>

Alvin's been carrying the patch in his series:
https://lore.kernel.org/rust-for-linux/20260326-b4-tyr-debugfs-v1-1-074badd18716@linux.dev/

Best,
Gary

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

[Alice Ryhl]

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

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

[Andreas Hindborg]

"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