Message-ID: <YnI3lE0TxLfZaQjE@phenom.ffwll.local>
Date:   Wed, 4 May 2022 10:21:40 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Dmitry Osipenko <dmitry.osipenko@...labora.com>
Cc:     Daniel Stone <daniel@...ishbar.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...ux.ie>,
        Gerd Hoffmann <kraxel@...hat.com>,
        Gurchetan Singh <gurchetansingh@...omium.org>,
        Chia-I Wu <olvaffe@...il.com>,
        Daniel Almeida <daniel.almeida@...labora.com>,
        Gert Wollny <gert.wollny@...labora.com>,
        Gustavo Padovan <gustavo.padovan@...labora.com>,
        Tomeu Vizoso <tomeu.vizoso@...labora.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Rob Herring <robh@...nel.org>,
        Steven Price <steven.price@....com>,
        Alyssa Rosenzweig <alyssa.rosenzweig@...labora.com>,
        Rob Clark <robdclark@...il.com>,
        Emil Velikov <emil.l.velikov@...il.com>,
        Robin Murphy <robin.murphy@....com>,
        Dmitry Osipenko <digetx@...il.com>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v4 10/15] drm/shmem-helper: Take reservation lock instead
 of drm_gem_shmem locks

On Thu, Apr 28, 2022 at 09:31:00PM +0300, Dmitry Osipenko wrote:
> Hello Daniel,
> 
> 27.04.2022 17:50, Daniel Vetter пишет:
> > On Mon, Apr 18, 2022 at 10:18:54PM +0300, Dmitry Osipenko wrote:
> >> Hello,
> >>
> >> On 4/18/22 21:38, Thomas Zimmermann wrote:
> >>> Hi
> >>>
> >>> Am 18.04.22 um 00:37 schrieb Dmitry Osipenko:
> >>>> Replace drm_gem_shmem locks with the reservation lock to make GEM
> >>>> lockings more consistent.
> >>>>
> >>>> Previously drm_gem_shmem_vmap() and drm_gem_shmem_get_pages() were
> >>>> protected by separate locks, now it's the same lock, but it doesn't
> >>>> make any difference for the current GEM SHMEM users. Only Panfrost
> >>>> and Lima drivers use vmap() and they do it in the slow code paths,
> >>>> hence there was no practical justification for the usage of separate
> >>>> lock in the vmap().
> >>>>
> >>>> Suggested-by: Daniel Vetter <daniel@...ll.ch>
> >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@...labora.com>
> >>>> ---
> >> ...
> >>>>   @@ -310,7 +306,7 @@ static int drm_gem_shmem_vmap_locked(struct
> >>>> drm_gem_shmem_object *shmem,
> >>>>       } else {
> >>>>           pgprot_t prot = PAGE_KERNEL;
> >>>>   -        ret = drm_gem_shmem_get_pages(shmem);
> >>>> +        ret = drm_gem_shmem_get_pages_locked(shmem);
> >>>>           if (ret)
> >>>>               goto err_zero_use;
> >>>>   @@ -360,11 +356,11 @@ int drm_gem_shmem_vmap(struct
> >>>> drm_gem_shmem_object *shmem,
> >>>>   {
> >>>>       int ret;
> >>>>   -    ret = mutex_lock_interruptible(&shmem->vmap_lock);
> >>>> +    ret = dma_resv_lock_interruptible(shmem->base.resv, NULL);
> >>>>       if (ret)
> >>>>           return ret;
> >>>>       ret = drm_gem_shmem_vmap_locked(shmem, map);
> >>>
> >>> Within drm_gem_shmem_vmap_locked(), there's a call to dma_buf_vmap() for
> >>> imported pages. If the exporter side also holds/acquires the same
> >>> reservation lock as our object, the whole thing can deadlock. We cannot
> >>> move dma_buf_vmap() out of the CS, because we still need to increment
> >>> the reference counter. I honestly don't know how to easily fix this
> >>> problem. There's a TODO item about replacing these locks at [1]. As
> >>> Daniel suggested this patch, we should talk to him about the issue.
> >>>
> >>> Best regards
> >>> Thomas
> >>>
> >>> [1]
> >>> https://www.kernel.org/doc/html/latest/gpu/todo.html#move-buffer-object-locking-to-dma-resv-lock
> >>
> >> Indeed, good catch! Perhaps we could simply use a separate lock for the
> >> vmapping of the *imported* GEMs? The vmap_use_count is used only by
> >> vmap/vunmap, so it doesn't matter which lock is used by these functions
> >> in the case of imported GEMs since we only need to protect the
> >> vmap_use_count.
> > 
> > Apologies for the late reply, I'm flooded.
> > 
> > I discussed this with Daniel Stone last week in a chat, roughly what we
> > need to do is:
> > 
> > 1. Pick a function from shmem helpers.
> > 
> > 2. Go through all drivers that call this, and make sure that we acquire
> > dma_resv_lock in the top level driver entry point for this.
> > 
> > 3. Once all driver code paths are converted, add a dma_resv_assert_held()
> > call to that function to make sure you have it all correctly.
> > 4. Repeate 1-3 until all shmem helper functions are converted over.
> Somehow I didn't notice the existence of dma_resv_assert_held(), thank
> you for the suggestion :)
> 
> > 
> > 5. Ditch the 3 different shmem helper locks.
> > 
> > The trouble is that I forgot that vmap is a thing, so that needs more
> > work. I think there's two approaches here:
> > - Do the vmap at import time. This is the trick we used to untangle the
> >   dma_resv_lock issues around dma_buf_attachment_map()
> 
> > - Change the dma_buf_vmap rules that callers must hold the dma_resv_lock.
> 
> I'll consider this option for v6, thank you.
> 
> I see now that you actually want to define the new rules for the
> dma-bufs in general and not only in the context of the DRM code, this
> now makes much more sense to me.

Yeah dma-buf is a cross driver interface, so we should try to be
consistent here. We didn't do this in the past, where the only reason you
didn't get lockdep splats was because you normally didn't run all possible
combinations of drivers and importer/exporter relationships in one system.
But that means it becomes very tricky to reason about how dma-buf really
works.

> > - Maybe also do what you suggest and keep a separate lock for this, but
> >   the fundamental issue is that this doesn't really work - if you share
> >   buffers both ways with two drivers using shmem helpers, then the
> >   ordering of this vmap_count_mutex vs dma_resv_lock is inconsistent and
> >   you can get some nice deadlocks. So not a great approach (and also the
> >   reason why we really need to get everyone to move towards dma_resv_lock
> >   as _the_ buffer object lock, since otherwise we'll never get a
> >   consistent lock nesting hierarchy).
> 
> The separate locks should work okay because it will be always the
> exporter that takes the dma_resv_lock. But I agree that it's less ideal
> than defining the new rules for dma-bufs since sometime you will take
> the resv lock and sometime not, potentially hiding bugs related to lockings.

That's the issue, some importers need to take the dma_resv_lock for
dma_buf_vmap too (e.g. to first nail the buffer in place when it's a
dynamic memory manager). In practice it'll work as well as what we have
currently, which is similarly inconsistent, except with per-driver locks
instead of shared locks from shmem helpers or dma-buf, so less obvious
that things are inconsistent.

So yeah if it's too messy maybe the approach is to have a separate lock
for vmap for now, land things, and then fix up dma_buf_vmap in a follow up
series.
-Daniel

> > The trouble here is that trying to be clever and doing the conversion just
> > in shmem helpers wont work, because there's a lot of cases where the
> > drivers are all kinds of inconsistent with their locking.
> > 
> > Adding Daniel S, also maybe for questions it'd be fastest to chat on irc?
> 
> My nickname is digetx on the #dri-devel channel, feel free to ping me if
> needed. Right now yours suggestions are clear to me, hence no extra
> questions.
> 
> Thank you for the review.

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists