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

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

[Nathan Chancellor]

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

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

[Naresh Kamboju]

On Sun, 23 Jul 2023 at 03:36, Nathan Chancellor <nathan@kernel.org> wrote:
>
> 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);
>         |                 ^

LKFT also noticed these build failures on arm and arm64 with clang nightly.


>   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.

Thanks for looking into this problem.


>
> [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

- Naresh