Re: [PATCH 2/3] rust: sync: add SetOnce::try_get_or_populate()

["Alexandre Courbot" <acourbot@nvidia.com>]

On Fri Jul 31, 2026 at 9:52 PM JST, Alice Ryhl wrote:
> On Thu, Jul 30, 2026 at 11:48:15PM +0900, Alexandre Courbot wrote:
>> On Thu Jul 30, 2026 at 6:11 PM JST, Alice Ryhl wrote:
>> > On Wed, Jul 29, 2026 at 11:05:55PM +0900, Alexandre Courbot wrote:
>> >> On Wed Jul 22, 2026 at 6:16 PM JST, Alice Ryhl wrote:
>> >> > +    where
>> >> > +        B: lock::Backend,
>> >> > +        F: FnOnce() -> Result<T, E>,
>> >> > +    {
>> >> > +        if let Some(value) = self.as_ref() {
>> >> > +            return Ok(value);
>> >> > +        }
>> >> > +
>> >> > +        let mut to_insert = f()?;
>> >> 
>> >> This means that `f` can run more than once for a given `SetOnce`, which
>> >> can lead to problems depending on `f`'s' side-effects.
>> >> 
>> >> In the GEM shmem case, we would create a second `SGTableMap`, and since
>> >> `SGTableMap` assumes it is the sole owner, the last instance to drop
>> >> would create a use-after-free.
>> >> 
>> >> Now this sounds more like a problem with `SGTableMap`, but if we cannot
>> >> avoid calling `f` at least twice then I think it would help if this was
>> >> documented.
>> >
>> > Hmm ... this is the behavior I want in Binder. Actually creating the
>> > value is an allocation, but my lock is a spinlock so I cannot invoke f()
>> > under the lock. This means that each caller will make their own
>> > allocation, and then we throw away any extras if there are concurrent
>> > callers.
>> >
>> > If GEM shmem wants f() invoked under the lock, then that's just a
>> > different operation than the one Binder wants.
>> >
>> > Do you think we should add both?
>> 
>> Possibly... but this would introduce a potential footgun that sleeps
>> while holding a spinlock, unless we limit the run-f-under-lock version
>> to work only with mutexes.
>> 
>> Maybe the proper solution is to fix `SGTableMap` so it supports multiple
>> instantiations. It is, after all, a bit footgunny on its own.
>> 
>> In any case, the current versions should warn users about the fact that
>> `f` can be called more than once imho.
>
> We could extend the naming scheme:
>
> * try_get_or_populate_once() only invokes f() once, under lock
> * try_get_or_populate_racy() may invoke f() multiple times
>
> Though the _once naming isn't amazing since it still may invoke f()
> multiple times if it fails. On failure, the next initializer gets to try
> again.
>
> Thoughts?

I cannot come up with a safer design than the current one, and adding a
variant that can deadlock sounds like adding a different footgun that
just happens to be aimed at the other foot. I'd rather settle for a
single operation where the caveats are well documented.

Making `SGTableMap` resilient to multiple instantiations can be done by
just adding an atomic counter to GEM shmem objects, so let me try and do
that. Then we can update the GEM shmem code to use `try_get_or_populate`
after this series lands.

Which means that as long as the contract on `f` is made clear in its
doc, the patch in this current form looks appropriate to me.