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

["Benno Lossin" <lossin@kernel.org>]

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

---
Cheers,
Benno