Message-ID: <CAKwvOd=Y2qM+Urh39JDei_STpijUB4wOysTVMuHkShjOsgXZqg@mail.gmail.com>
Date:   Wed, 2 Aug 2023 14:24:02 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Boris Brezillon <boris.brezillon@...labora.com>
Cc:     Christian König <ckoenig.leichtzumerken@...il.com>,
        maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
        tzimmermann@...e.de, airlied@...il.com, daniel@...ll.ch,
        trix@...hat.com, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
        naresh.kamboju@...aro.org, dakr@...hat.com
Subject: Re: [PATCH 1/2] drm/exec: use unique instead of local label

On Wed, Aug 2, 2023 at 1:44 AM Boris Brezillon
<boris.brezillon@...labora.com> wrote:
>
> On Tue, 1 Aug 2023 13:35:13 -0700
> Nick Desaulniers <ndesaulniers@...gle.com> wrote:
>
> > On Mon, Jul 31, 2023 at 5:36 AM Christian König
> > <ckoenig.leichtzumerken@...il.com> wrote:
> > >
> > > GCC forbids to jump to labels in loop conditions and a new clang
> > > check stumbled over this.
> > >
> > > So instead using a local label inside the loop condition use an
> > > unique label outside of it.
> > >
> > > Fixes: commit 09593216bff1 ("drm: execution context for GEM buffers v7")
> > > Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/1890
> > > Link: https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067
> > > Reported-by: Nathan Chancellor <nathan@...nel.org>
> > > Reported-by: Naresh Kamboju <naresh.kamboju@...aro.org>
> > > CC: Boris Brezillon <boris.brezillon@...labora.com>
> > > Signed-off-by: Christian König <christian.koenig@....com>
> >
> > Works for me; thanks for the patch!
> > Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.com>
> >
> > I suspect it's possible to change the indirect goto into a direct goto
> > with some further refactoring (macros can take block statements; if
> > drm_exec_until_all_locked accepted a block statement arg then you
> > could introduce a new scope, and a new local label to that scope, then
> > just use direct goto),
>
> Maybe I'm wrong, but this sounds like the version I proposed here [1].

Nearly; here's what I was imagining:
```
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 977e1804718d..3ea8beb159f0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -904,7 +904,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
                e->user_invalidated = userpage_invalidated;
        }

-       drm_exec_until_all_locked(&p->exec) {
+       drm_exec_until_all_locked(&p->exec, {
                r = amdgpu_vm_lock_pd(&fpriv->vm, &p->exec, 1 + p->gang_size);
                drm_exec_retry_on_contention(&p->exec);
                if (unlikely(r))
@@ -928,7 +928,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
                        if (unlikely(r))
                                goto out_free_user_pages;
                }
-       }
+       })

        amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
                struct mm_struct *usermm;
diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
index 73205afec162..8e32a9b704e7 100644
--- a/include/drm/drm_exec.h
+++ b/include/drm/drm_exec.h
@@ -74,14 +74,13 @@ struct drm_exec {
  * 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);                         \
-       });)
+#define drm_exec_until_all_locked(exec, block)                         \
+       {       \
+               __label__ __drm_exec_retry;     \
+__drm_exec_retry:      \
+               while (drm_exec_cleanup(exec))  \
+                       block   \
+}

 /**
  * drm_exec_retry_on_contention - restart the loop to grap all locks
@@ -93,7 +92,7 @@ __drm_exec_retry:
         \
 #define drm_exec_retry_on_contention(exec)                     \
        do {                                                    \
                if (unlikely(drm_exec_is_contended(exec)))      \
-                       goto *__drm_exec_retry_ptr;             \
+                       goto __drm_exec_retry;          \
        } while (0)

 /**
```
(only updated one macro expansion site in
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c, didn't add proper trailing
tabs to macro but you get the gist).

But I think both compilers can optimize out the unnecessary
indirection when it's obvious, so I don't think it matters much, other
than the tastes of whoever has to maintain this.


>
> > but this will probably apply cleaner. (oh, is
> > 09593216bff1 only in next at the moment? The AuthorDate threw me.)
> >
> > There are some curious cases where __attribute__((cleanup())) doesn't
> > mesh well with indirect gotos.
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=37722
> >
> > May not ever be a problem here...
>
> [1]https://patchwork.freedesktop.org/patch/543077/



-- 
Thanks,
~Nick Desaulniers

Powered by blists - more mailing lists