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

[Daniel Almeida <daniel.almeida@collabora.com>]

> On 2 Aug 2025, at 07:42, Benno Lossin <lossin@kernel.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@kernel.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@kernel.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@kernel.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