Re: [PATCH v12 5/6] rust: ww_mutex: implement LockSet

[Onur Özkan]

On Thu, 15 Oct 2026 12:53:10 +0100
Gary Guo <gary@xxxxxxxxxxx> wrote:

> On Thu Oct 15, 2026 at 12:26 PM BST, Onur Özkan wrote:
> > On Mon, 12 Oct 2026 13:02:15 +0100
> > Gary Guo <gary@xxxxxxxxxxx> wrote:
> >
> >> On Mon Oct 12, 2026 at 11:26 AM BST, Onur Özkan wrote:
> >> > LockSet is a high-level and safe API built on top of
> >> > ww_mutex which provides a safe and easy to use API
> >> > while keeping the ww_mutex semantics.
> >> >
> >> > When EDEADLK is hit it drops all held locks and waits for the
> >> > contended mutex. It then retries the given (by the user)
> >> > locking algorithm with that mutex held and the same acquire
> >> > context until it succeeds.
> >> 
> >> Do we really need this, or this can be achieved by simple retry loops?
> >> 
> >>     fn retry_if_deadlock<R>(f: impl FnOnce() -> Result<R>) -> Result<R> {
> >>         loop {
> >>             match f() {
> >>                 Err(EDEADLK) => (),
> >>                 e => return e,
> >>             }
> >>         }
> >>     }
> >> 
> >>     stack_pin_init!(let acq_ctx = AcquireCtx::new(&class));
> >>     retry_if_deadlock(|| {
> >>         let mut gaurd1 = acq_ctx.lock(&mutex1)?;
> >>         let mut guard2 = acq_ctx.lock(&mutex2)?;
> >> 
> >>         acq_ctx.done();
> >> 
> >>         *guard1 += 1;
> >>         *guard2 += 1;
> >>     });
> >> 
> >> I think the only difference is that the `LockSet` acquires the contending mutex
> >> first. However, I imagine we can solve this by having a wrapped `AcquireCtx`
> >> that provides that.
> >
> > That's what LockSet already is. How would you design that wrapped AcquireCtx and
> > what would we gain by replacing LockSet with another wrapper?
> 
> The LockSet API have multiple issues that I consider problematic:
> 
> * It always allocate, even if you don't need a dynamic set.
> 
> * Accessing data searches through the vec. To access all data, you will have
>   O(n^2) complexity spent just on vec traversal.
>  
> * There's no way of getting multiple mutable borrow of the distinct locks.

I can work on these.

> 
> My example above has none of the issue.
> 
> If you do not need the "acquire the contended lock first" behaviour, then
> there's no need for wrapper, even. Also, if this behaviour is truly universal,
> perhaps it should become part of `AcquireCtx`'s default behaviour instead.

We need that functionality for the DRM use-case, it's not something that should
be applied to the ww mutex primitives but I would like to hear @Daniel's opinion
on this.

> 
> >
> > You need to remember the contended mutex, release all acquired locks before
> > waiting for it and retry with that mutex held and the same acquire context. 
> 
> My example above doesn't have these issues. Releasing acquired locks if free
> with `?`. Retrying is done by the helper function and not handled by user.
> Acquire context if really easy to be misued, can be done by the helper function
> too:
>  
>     fn ww_transaction<R>(
>         class: &Class, f: impl FnOnce(&AcquireCtx) -> Result<R>
>     ) -> Result<R> {
>         stack_pin_init!(let acq_ctx = AcquireCtx::new(&class));
>         loop {
>             match f(&acq_Ctx) {
>                 Err(EDEADLK) => (),
>                 e => return e,
>             }
>         }
>     }
> 
>     ww_transaction(&class, |acq_ctx| {
>         let mut guard1 = acq_ctx.lock(&mutex1)?;
>         let mut guard2 = acq_ctx.lock(&mutex2)?;
> 
>         acq_ctx.done();
> 
>         *guard1 += 1;
>         *guard2 += 1;
>     })
> 

This still needs the contended-lock backoff when retrying the full transaction.
Simply restarting the callback can repeatedly hit EDEADLK, so the proposed code
is not going to work.

Cc @Daniel

Thanks,
Onur

> 
> > Also for a dynamic set, you also need to keep the acquired locks held across loop
> > iterations and helper calls and handle repeated objects. With ordinary guards,
> > that bookkeeping falls on the caller unless the wrapper handles it. LockSet
> > already does all this.
> 
> You can make dynamic lock set just a data structure and not do everything at
> once, something like:
> 
>     ww_transaction(&class, |acq_ctx| {
>         let dynset = DynLockSet::new()?;
> 
>         // Check if already inside the set, if not, lock it.
>         dynset.lock(acq_ctx, &mutex1)?;
>         dynset.lock(acq_ctx, &mutex2)?;
> 
>         acq_ctx.done();
> 
>         dynset.with_locked(&mutex1, |guard1| *guard1 += 1)?;
>         dynset.with_locked(&mutex2, |guard2| *guard2 += 1)?;
>     })
> 
> Best,
> Gary