On Sat Aug 2, 2025 at 4:15 PM CEST, Daniel Almeida wrote:
> On 2 Aug 2025, at 07:42, Benno Lossin <los...@kernel.org> wrote:
>> On Fri Aug 1, 2025 at 11:22 PM CEST, Daniel Almeida wrote:
>>> 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.2cu5xp7uy7...@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.
That's an important detail. In that case, a type state won't we a good
idea. Unless it's also common to have a finite number of them, in which
case we should have two API.
>>> /**
>>> * 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:
Ahh, I missed the `:` at the end of the line. Thanks for explaining!
(also Miguel in the other reply!) If you don't mind I'll ask some more
basic C questions :)
And yeah it's pretty cursed...
> /**
> * 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 `do { ... } while(0)` is used because C doesn't have `{ ... }`
scopes? (& because you want to be able to have this be called from an if
without braces?)
> 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.
Which function call locks the mutexes?
> return 0;
>
>
> So, something like:
>
> some_unique_label:
> for(void *retry_ptr;
> ({ retry_ptr = &some_unique_label; drm_exec_cleanup(); });
Normally this should be a condition, or rather an expression evaluating
to bool, why is this okay? Or does C just take the value of the last
function call due to the `({})`?
Why isn't `({})` used instead of `do { ... } while(0)` above?
> /* empty *) {
> /* user code here, which potentially jumps back to some_unique_label */
> }
Thanks for the example & the macro expansion. What I gather from this is
that we'd probably want a closure that executes the code & reruns it
when contention is detected.
>>> 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?
I saw "array of the locked objects" and thus thought so this must only
work for an array of locks. Looking at the type a bit closer, it
actually is an array of pointers, so it does work for arbitrary data
structures storing the locks.
So essentially it would amount to storing `Vec<WwMutexGuard<'_, T>>` in
Rust IIUC. I was under the impression that we wanted to avoid that,
because it's an extra allocation.
But maybe that's actually what's done on the C side.
> Btw, I can also try to implement a proof of concept, so long as people agree
> that
> this approach makes sense.
I'm not sure I understand what you are proposing, so I can't give a
recommendation yet.
---
Cheers,
Benno
