Re: [PATCH 1/6] drm: execution context for GEM buffers v7

[Nathan Chancellor <nathan@kernel.org>]

Hi Christian,

On Tue, Jul 11, 2023 at 03:31:17PM +0200, Christian König wrote:
> This adds the infrastructure for an execution context for GEM buffers
> which is similar to the existing TTMs execbuf util and intended to replace
> it in the long term.
> 
> The basic functionality is that we abstracts the necessary loop to lock
> many different GEM buffers with automated deadlock and duplicate handling.
> 
> v2: drop xarray and use dynamic resized array instead, the locking
>     overhead is unnecessary and measurable.
> v3: drop duplicate tracking, radeon is really the only one needing that.
> v4: fixes issues pointed out by Danilo, some typos in comments and a
>     helper for lock arrays of GEM objects.
> v5: some suggestions by Boris Brezillon, especially just use one retry
>     macro, drop loop in prepare_array, use flags instead of bool
> v6: minor changes suggested by Thomas, Boris and Danilo
> v7: minor typos pointed out by checkpatch.pl fixed
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> Reviewed-by: Danilo Krummrich <dakr@redhat.com>
> Tested-by: Danilo Krummrich <dakr@redhat.com>

<snip>

> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> new file mode 100644
> index 000000000000..73205afec162
> --- /dev/null
> +++ b/include/drm/drm_exec.h

<snip>

> + * 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)				\
> +	for (void *__drm_exec_retry_ptr; ({			\
> +		__label__ __drm_exec_retry;			\
> +__drm_exec_retry:						\
> +		__drm_exec_retry_ptr = &&__drm_exec_retry;	\
> +		(void)__drm_exec_retry_ptr;			\
> +		drm_exec_cleanup(exec);				\
> +	});)
> +
> +/**
> + * 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)

This construct of using a label as a value and goto to jump into a GNU
C statement expression is explicitly documented in GCC's manual [1] as
undefined behavior:

"Jumping into a statement expression with a computed goto (see Labels as
Values [2]) has undefined behavior. "

A recent change in clang [3] to prohibit this altogether points this out, so
builds using clang-17 and newer will break with this change applied:

  drivers/gpu/drm/tests/drm_exec_test.c:41:3: error: cannot jump from this indirect goto statement to one of its possible targets
     41 |                 drm_exec_retry_on_contention(&exec);
        |                 ^
  include/drm/drm_exec.h:96:4: note: expanded from macro 'drm_exec_retry_on_contention'
     96 |                         goto *__drm_exec_retry_ptr;             \
        |                         ^
  drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: possible target of indirect goto statement
     39 |         drm_exec_until_all_locked(&exec) {
        |         ^
  include/drm/drm_exec.h:79:33: note: expanded from macro 'drm_exec_until_all_locked'
     79 |                 __label__ __drm_exec_retry;                     \
        |                                                                 ^
  drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: jump enters a statement expression

It seems like if this construct works, it is by chance, although I am
not sure if there is another solution.

[1]: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
[2]: https://gcc.gnu.org/onlinedocs/gcc/Labels-as-Values.html
[3]: https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067

Cheers,
Nathan