Message-ID: <aFmKsE_nJkaVMv0T@tardis.local>
Date: Mon, 23 Jun 2025 10:11:12 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Benno Lossin <lossin@...nel.org>
Cc: Onur Özkan <work@...rozkan.dev>,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	ojeda@...nel.org, alex.gaynor@...il.com, gary@...yguo.net,
	a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu,
	dakr@...nel.org, peterz@...radead.org, mingo@...hat.com,
	will@...nel.org, longman@...hat.com, felipe_life@...e.com,
	daniel@...lak.dev, bjorn3_gh@...tonmail.com, simona@...ll.ch,
	airlied@...il.com, dri-devel@...ts.freedesktop.org,
	lyude@...hat.com
Subject: Re: [PATCH v5 2/3] implement ww_mutex abstraction for the Rust tree

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*

>         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();

>         _ctx: PhantomData<&'a WwAcquireCtx>,

We can still take a reference to WwAcquireCtx here I think.

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

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

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

Regards,
Boqun

> >>     let _: &mut T = t;
> >>     let _: &mut Foo = foo;
> >>     let _: &mut Foo = foo2;
> 
> Ah these will actually be `WwMutexGuard<'_, ...>`, but that should be
> expected.
> 
> ---
> Cheers,
> Benno

Powered by blists - more mailing lists