[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <E997DCAF-552F-4EF2-BF94-1385ECADF543@collabora.com>
Date: Sat, 2 Aug 2025 11:15:07 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Benno Lossin <lossin@...nel.org>
Cc: Onur <work@...rozkan.dev>,
Boqun Feng <boqun.feng@...il.com>,
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,
dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v5 2/3] implement ww_mutex abstraction for the Rust tree
> On 2 Aug 2025, at 07:42, Benno Lossin <lossin@...nel.org> wrote:
>
> On Fri Aug 1, 2025 at 11:22 PM CEST, Daniel Almeida wrote:
>> Hi Benno,
>>
>>> On 7 Jul 2025, at 16:48, Benno Lossin <lossin@...nel.org> wrote:
>>>
>>> On Mon Jul 7, 2025 at 8:06 PM CEST, Onur wrote:
>>>> On Mon, 07 Jul 2025 17:31:10 +0200
>>>> "Benno Lossin" <lossin@...nel.org> wrote:
>>>>
>>>>> On Mon Jul 7, 2025 at 3:39 PM CEST, Onur wrote:
>>>>>> On Mon, 23 Jun 2025 17:14:37 +0200
>>>>>> "Benno Lossin" <lossin@...nel.org> wrote:
>>>>>>
>>>>>>>> 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();
>>>>>>
>>>>>>
>>>>>> Instead of `begin` and `custom`, why not something like this:
>>>>>>
>>>>>> let (a, c, d) = ctx.init()
>>>>>> .lock(a)
>>>>>> .lock(b)
>>>>>> .lock(c)
>>>>>> .unlock(b)
>>>>>> .lock(d)
>>>>>> .finish();
>>>>>>
>>>>>> Instead of `begin`, `init` would be better naming to imply `fini`
>>>>>> on the C side, and `unlock` instead of `custom` would make the
>>>>>> intent clearer when dropping locks mid chain.
>>>
>>> Also, I'm not really fond of the name `init`, how about `enter`?
>>>
>>>>>
>>>>> I don't think that this `unlock` operation will work. How would you
>>>>> implement it?
>>>>
>>>>
>>>> We could link mutexes to locks using some unique value, so that we can
>>>> access locks by passing mutexes (though that sounds a bit odd).
>>>>
>>>> Another option would be to unlock by the index, e.g.,:
>>>>
>>>> let (a, c) = ctx.init()
>>>> .lock(a)
>>>> .lock(b)
>>>> .unlock::<1>()
>>
>> Why do we need this random unlock() here? We usually want to lock everything
>> and proceed, or otherwise backoff completely so that someone else can proceed.
>
> No the `unlock` was just to show that we could interleave locking and
> unlocking.
>
>> One thing I didn’t understand with your approach: is it amenable to loops?
>> i.e.: are things like drm_exec() implementable?
>
> I don't think so, see also my reply here:
>
> https://lore.kernel.org/all/DBOPIJHY9NZ7.2CU5XP7UY7ES3@kernel.org
>
> The type-based approach with tuples doesn't handle dynamic number of
> locks.
>
This is probably the default use-case by the way.
>> /**
>> * drm_exec_until_all_locked - loop until all GEM objects are locked
>> * @exec: drm_exec object
>> *
>> * Core functionality of the drm_exec object. Loops until all GEM objects are
>> * locked and no more contention exists. At the beginning of the loop it is
>> * guaranteed that no GEM object is locked.
>> *
>> * Since labels can't be defined local to the loops body we use a jump pointer
>> * to make sure that the retry is only used from within the loops body.
>> */
>> #define drm_exec_until_all_locked(exec) \
>> __PASTE(__drm_exec_, __LINE__): \
>> for (void *__drm_exec_retry_ptr; ({ \
>> __drm_exec_retry_ptr = &&__PASTE(__drm_exec_, __LINE__);\
>> (void)__drm_exec_retry_ptr; \
>> drm_exec_cleanup(exec); \
>> });)
>
> My understanding of C preprocessor macros is not good enough to parse or
> understand this :( What is that `__PASTE` thing?
This macro is very useful, but also cursed :)
This declares a unique label before the loop, so you can jump back to it on
contention. It is usually used in conjunction with:
/**
* drm_exec_retry_on_contention - restart the loop to grap all locks
* @exec: drm_exec object
*
* Control flow helper to continue when a contention was detected and we need to
* clean up and re-start the loop to prepare all GEM objects.
*/
#define drm_exec_retry_on_contention(exec) \
do { \
if (unlikely(drm_exec_is_contended(exec))) \
goto *__drm_exec_retry_ptr; \
} while (0)
The termination is handled by:
/**
* drm_exec_cleanup - cleanup when contention is detected
* @exec: the drm_exec object to cleanup
*
* Cleanup the current state and return true if we should stay inside the retry
* loop, false if there wasn't any contention detected and we can keep the
* objects locked.
*/
bool drm_exec_cleanup(struct drm_exec *exec)
{
if (likely(!exec->contended)) {
ww_acquire_done(&exec->ticket);
return false;
}
if (likely(exec->contended == DRM_EXEC_DUMMY)) {
exec->contended = NULL;
ww_acquire_init(&exec->ticket, &reservation_ww_class);
return true;
}
drm_exec_unlock_all(exec);
exec->num_objects = 0;
return true;
}
EXPORT_SYMBOL(drm_exec_cleanup);
The third clause in the loop is empty.
For example, in amdgpu:
/**
* reserve_bo_and_vm - reserve a BO and a VM unconditionally.
* @mem: KFD BO structure.
* @vm: the VM to reserve.
* @ctx: the struct that will be used in unreserve_bo_and_vms().
*/
static int reserve_bo_and_vm(struct kgd_mem *mem,
struct amdgpu_vm *vm,
struct bo_vm_reservation_context *ctx)
{
struct amdgpu_bo *bo = mem->bo;
int ret;
WARN_ON(!vm);
ctx->n_vms = 1;
ctx->sync = &mem->sync;
drm_exec_init(&ctx->exec, DRM_EXEC_INTERRUPTIBLE_WAIT, 0);
drm_exec_until_all_locked(&ctx->exec) {
ret = amdgpu_vm_lock_pd(vm, &ctx->exec, 2);
drm_exec_retry_on_contention(&ctx->exec);
if (unlikely(ret))
goto error;
ret = drm_exec_prepare_obj(&ctx->exec, &bo->tbo.base, 1);
drm_exec_retry_on_contention(&ctx->exec);
if (unlikely(ret))
goto error;
}
// <—— everything is locked at this point.
return 0;
So, something like:
some_unique_label:
for(void *retry_ptr;
({ retry_ptr = &some_unique_label; drm_exec_cleanup(); });
/* empty *) {
/* user code here, which potentially jumps back to some_unique_label */
}
>
>> In fact, perhaps we can copy drm_exec, basically? i.e.:
>>
>> /**
>> * struct drm_exec - Execution context
>> */
>> struct drm_exec {
>> /**
>> * @flags: Flags to control locking behavior
>> */
>> u32 flags;
>>
>> /**
>> * @ticket: WW ticket used for acquiring locks
>> */
>> struct ww_acquire_ctx ticket;
>>
>> /**
>> * @num_objects: number of objects locked
>> */
>> unsigned int num_objects;
>>
>> /**
>> * @max_objects: maximum objects in array
>> */
>> unsigned int max_objects;
>>
>> /**
>> * @objects: array of the locked objects
>> */
>> struct drm_gem_object **objects;
>>
>> /**
>> * @contended: contended GEM object we backed off for
>> */
>> struct drm_gem_object *contended;
>>
>> /**
>> * @prelocked: already locked GEM object due to contention
>> */
>> struct drm_gem_object *prelocked;
>> };
>>
>> This is GEM-specific, but we could perhaps implement the same idea by
>> tracking ww_mutexes instead of GEM objects.
>
> But this would only work for `Vec<WwMutex<T>>`, right?
I’m not sure if I understand your point here.
The list of ww_mutexes that we've managed to currently lock would be something
that we'd keep track internally in our context. In what way is a KVec an issue?
>
>> Also, I’d appreciate if the rollback logic could be automated, which is
>> what you’re trying to do, so +1 to your idea.
>
> Good to see that it seems useful to you :)
>
> ---
> Cheers,
> Benno
Btw, I can also try to implement a proof of concept, so long as people agree that
this approach makes sense.
By the way, dri-devel seems to not be on cc? Added them now.
Full context at [0].
— Daniel
[0]: https://lore.kernel.org/rust-for-linux/DBPC27REX4N1.3JA4SSHDYXAHJ@kernel.org/T/#m17a1e3a913ead2648abdff0fc2d927c8323cb8c3
Powered by blists - more mailing lists