Re: [PATCH v5 2/3] implement ww_mutex abstraction for the Rust tree

[Onur <work@onurozkan.dev>]

On Tue, 24 Jun 2025 01:22:05 +0200
"Benno Lossin" <lossin@kernel.org> wrote:

> On Mon Jun 23, 2025 at 7:11 PM CEST, Boqun Feng wrote:
> > On Mon, Jun 23, 2025 at 05:14:37PM +0200, Benno Lossin wrote:
> >> On Mon Jun 23, 2025 at 4:47 PM CEST, Boqun Feng wrote:
> >> > On Mon, Jun 23, 2025 at 03:44:58PM +0200, Benno Lossin wrote:
> >> >> I didn't have a concrete API in mind, but after having read the
> >> >> abstractions more, would this make sense?
> >> >> 
> >> >>     let ctx: &WwAcquireCtx = ...;
> >> >>     let m1: &WwMutex<T> = ...;
> >> >>     let m2: &WwMutex<Foo> = ...;
> >> >> 
> >> >>     let (t, foo, foo2) = ctx
> >> >>         .begin()
> >> >>         .lock(m1)
> >> >>         .lock(m2)
> >> >>         .lock_with(|(t, foo)| &*foo.other)
> >> >>         .finish();
> >> >> 
> >> >
> >> > Cute!
> >> >
> >> > However, each `.lock()` will need to be polymorphic over a tuple
> >> > of locks that are already held, right? Otherwise I don't see how
> >> > `.lock_with()` knows it's already held two locks. That sounds
> >> > like a challenge for implementation.
> >> 
> >> I think it's doable if we have 
> >> 
> >>     impl WwActiveCtx {
> >
> > I think you mean *WwAcquireCtx*
> 
> Oh yeah.
> 
> >>         fn begin(&self) -> WwActiveCtx<'_, ()>;
> >>     }
> >> 
> >>     struct WwActiveCtx<'a, Locks> {
> >>         locks: Locks,
> >
> > This probably need to to be Result<Locks>, because we may detect
> > -DEADLOCK in the middle.
> >
> >     let (a, c, d) = ctx.begin()
> >         .lock(a)
> >         .lock(b) // <- `b` may be locked by someone else. So we
> > should // drop `a` and switch `locks` to an `Err(_)`.
> >         .lock(c) // <- this should be a no-op if `locks` is an
> > `Err(_)`. .finish();
> 
> Hmm, I thought that we would go for the `lock_slow_path` thing, but
> maybe that's the wrong thing to do? Maybe `lock` should return a
> result? I'd have to see the use-cases...
> 
> >>         _ctx: PhantomData<&'a WwAcquireCtx>,
> >
> > We can still take a reference to WwAcquireCtx here I think.
> 
> Yeah we have to do that in order to call lock on the mutexes.
> 
> >>     }
> >> 
> >>     impl<'a, Locks> WwActiveCtx<'a, Locks>
> >>     where
> >>         Locks: Tuple
> >>     {
> >>         fn lock<'b, T>(
> >>             self,
> >>             lock: &'b WwMutex<T>,
> >>         ) -> WwActiveCtx<'a, Locks::Append<WwMutexGuard<'b, T>>>;
> >> 
> >>         fn lock_with<'b, T>(
> >>             self,
> >>             get_lock: impl FnOnce(&Locks) -> &'b WwMutex<T>,
> >>         ) -> WwActiveCtx<'a, Locks::Append<WwMutexGuard<'b, T>>>;
> >>         // I'm not 100% sure that the lifetimes will work out...
> >
> > I think we can make the following work?
> >
> >     impl<'a, Locks> WwActiveCtx<'a, Locks>
> >     where
> >         Locks: Tuple
> >     {
> >         fn lock_with<T>(
> > 	    self,
> > 	    get_lock: impl FnOnce(&Locks) -> &WmMutex<T>,
> > 	) -> WwActiveCtx<'a, Locks::Append<WmMutexGuard<'a, T>>
> >     }
> >
> > because with a `WwActiveCtx<'a, Locks>`, we can get a `&'a Locks`,
> > which will give us a `&'a WmMutex<T>`, and should be able to give
> > us a `WmMutexGuard<'a, T>`.
> 
> I think this is more restrictive, since this will require that the
> mutex is (potentially) locked for `'a` (you can drop the guard
> before, but you can't drop the mutex itself). So again concrete
> use-cases should inform our choice here.
> 
> >>         fn finish(self) -> Locks;
> >>     }
> >> 
> >>     trait Tuple {
> >>         type Append<T>;
> >> 
> >>         fn append<T>(self, value: T) -> Self::Append<T>;
> >>     }
> >> 
> >
> > `Tuple` is good enough for its own, if you could remember, we have
> > some ideas about using things like this to consolidate multiple
> > `RcuOld` so that we can do one `synchronize_rcu()` for `RcuOld`s.
> 
> Yeah that's true, feel free to make a patch or good-first-issue, I
> won't have time to create a series.
> 
> >>     impl Tuple for () {
> >>         type Append<T> = (T,);
> >> 
> >>         fn append<T>(self, value: T) -> Self::Append<T> {
> >>             (value,)
> >>         }
> >>     }
> >>     
> >>     impl<T1> Tuple for (T1,) {
> >>         type Append<T> = (T1, T);
> >> 
> >>         fn append<T>(self, value: T) -> Self::Append<T> {
> >>             (self.0, value,)
> >>         }
> >>     }
> >> 
> >>     impl<T1, T2> Tuple for (T1, T2) {
> >>         type Append<T> = (T1, T2, T);
> >> 
> >>         fn append<T>(self, value: T) -> Self::Append<T> {
> >>             (self.0, self.1, value,)
> >>         }
> >>     }
> >> 
> >>     /* these can easily be generated by a macro */
> >> 
> >> > We also need to take into consideration that the user want to
> >> > drop any lock in the sequence? E.g. the user acquires a, b and
> >> > c, and then drop b, and then acquires d. Which I think is
> >> > possible for ww_mutex.
> >> 
> >> Hmm what about adding this to the above idea?:
> >> 
> >>     impl<'a, Locks> WwActiveCtx<'a, Locks>
> >>     where
> >>         Locks: Tuple
> >>     {
> >>         fn custom<L2>(self, action: impl FnOnce(Locks) -> L2) ->
> >> WwActiveCtx<'a, L2>; }
> >> 
> >> Then you can do:
> >> 
> >>     let (a, c, d) = ctx.begin()
> >>         .lock(a)
> >>         .lock(b)
> >>         .lock(c)
> >>         .custom(|(a, _, c)| (a, c))
> >>         .lock(d)
> >>         .finish();
> >> 
> >
> > Seems reasonable. But we still need to present this to the end user
> > to see how much they like it. For ww_mutex I think the major user
> > is DRM, so add them into Cc list.
> 
> Yeah let's see some use-cases :)


Should we handle this in the initial implementation or leave it for
follow-up patches after the core abstraction of ww_mutex has landed?

---

Regards,
Onur