Message-Id: <182E916F-3B59-4721-B415-81C3CF175DA7@collabora.com>
Date: Thu, 14 Aug 2025 15:22:57 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Onur <work@...rozkan.dev>
Cc: Benno Lossin <lossin@...nel.org>,
 Lyude Paul <lyude@...hat.com>,
 linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org,
 ojeda@...nel.org,
 alex.gaynor@...il.com,
 boqun.feng@...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
Subject: Re: [PATCH v5 0/3] rust: add `ww_mutex` support


Hi Onur,

> On 14 Aug 2025, at 12:56, Onur <work@...rozkan.dev> wrote:
> 
> On Thu, 14 Aug 2025 09:38:38 -0300
> Daniel Almeida <daniel.almeida@...labora.com> wrote:
> 
>> Hi Onur,
>> 
>>> On 14 Aug 2025, at 08:13, Onur Özkan <work@...rozkan.dev> wrote:
>>> 
>>> Hi all,
>>> 
>>> I have been brainstorming on the auto-unlocking (on dynamic number
>>> of mutexes) idea we have been discussing for some time.
>>> 
>>> There is a challange with how we handle lock guards and my current
>>> thought is to remove direct data dereferencing from guards. Instead,
>>> data access would only be possible through a fallible method (e.g.,
>>> `try_get`). If the guard is no longer valid, this method would fail
>>> to not allow data-accessing after auto-unlock.
>>> 
>>> In practice, it would work like this:
>>> 
>>> let a_guard = ctx.lock(mutex_a)?;
>>> let b_guard = ctx.lock(mutex_b)?;
>>> 
>>> // Suppose user tries to lock `mutex_c` without aborting the
>>> // entire function (for some reason). This means that even on
>>> // failure, `a_guard` and `b_guard` will still be accessible.
>>> if let Ok(c_guard) = ctx.lock(mutex_c) {
>>>    // ...some logic
>>> }
>>> 
>>> let a_data = a_guard.try_get()?;
>>> let b_data = b_guard.try_get()?;
>> 
>> Can you add more code here? How is this going to look like with the
>> two closures we’ve been discussing?
> 
> Didn't we said that tuple-based closures are not sufficient when
> dealing with a dynamic number of locks (ref [1]) and ww_mutex is mostly
> used with dynamic locks? I thought implementing that approach is not
> worth it (at least for now) because of that.
> 
> [1]: https://lore.kernel.org/all/DBS8REY5E82S.3937FAHS25ANA@kernel.org
> 
> Regards,
> Onur



I am referring to this [0]. See the discussion and itemized list at the end.

To recap, I am proposing a separate type that is similar to drm_exec, and that
implements this:

```
a) run a user closure where the user can indicate which ww_mutexes they want to lock
b) keep track of the objects above
c) keep track of whether a contention happened
d) rollback if a contention happened, releasing all locks
e) rerun the user closure from a clean slate after rolling back
f) run a separate user closure whenever we know that all objects have been locked.
```

In other words, we need to run a closure to let the user implement a given
locking strategy, and then one closure that runs when the user signals that
there are no more locks to take.

What I said is different from what Benno suggested here:

>>>>>>    let (a, c, d) = ctx.begin()
>>>>>>        .lock(a)
>>>>>>        .lock(b)
>>>>>>        .lock(c)
>>>>>>        .custom(|(a, _, c)| (a, c))
>>>>>>        .lock(d)
>>>>>>        .finish();

i.e.: here is a brief example of how the API should be used by clients:

```
// The Context keeps track of which locks were successfully taken.
let locking_algorithm = |ctx: &Context| {
  // client-specific code, likely some loop trying to acquire multiple locks:
  //
  // note that it does not _have_ to be a loop, though. It up to the clients to
  // provide a suitable implementation here.
  for (..) {
    ctx.lock(foo); // If this succeeds, the context will add  "foo" to the list of taken locks.
  }

  // if this closure returns EDEADLK, then our abstraction must rollback, and
  // run it again.
};

// This runs when the closure above has indicated that there are no more locks
// to take.
let on_all_locks_taken = |ctx: &Context| {
  // everything is locked here, give access to the data in the guards.
};

ctx.lock_all(locking_algorithm, on_all_locks_taken)?;
```

Yes, this will allocate but that is fine because drm_exec allocates as well.

We might be able to give more control of when the allocation happens if the
number of locks is known in advance, e.g.:

```
struct Context<T> {
  taken_locks: KVec<Guard<T>>,
}

impl<T> Context<T> {
  fn prealloc_slots(num_slots: usize, flags: ...) -> Result<Self> {
    let taken_locks = ... // pre-alloc a KVec here. 
    Self {
      taken_slots,
    } 
  }
}
```

The main point is that this API is optional. It builds a lot of convenience of
top of the Rust WWMutex abstraction, but no one is forced to use it.

IOW: What I said should be implementable with a dynamic number of locks. Please
let me know if I did not explain this very well. 

[0]: https://lore.kernel.org/rust-for-linux/8B1FB608-7D43-4DD9-8737-DCE59ED74CCA@collabora.com/

Powered by blists - more mailing lists