Re: [PATCH 2/4] dma-buf: lock the reservation object during (un)map_dma_buf v2

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 03, 2018 at 03:02:11PM +0200, Christian König wrote:
> Am 03.07.2018 um 14:52 schrieb Daniel Vetter:
> > On Tue, Jul 03, 2018 at 01:46:44PM +0200, Christian König wrote:
> > > Am 25.06.2018 um 11:12 schrieb Daniel Vetter:
> > > > On Mon, Jun 25, 2018 at 10:22:31AM +0200, Daniel Vetter wrote:
> > > > > On Fri, Jun 22, 2018 at 04:11:01PM +0200, Christian König wrote:
> > > > > > First step towards unpinned DMA buf operation.
> > > > > > 
> > > > > > I've checked the DRM drivers to potential locking of the reservation
> > > > > > object, but essentially we need to audit all implementations of the
> > > > > > dma_buf _ops for this to work.
> > > > > > 
> > > > > > v2: reordered
> > > > > > 
> > > > > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Ok I did review drivers a bit, but apparently not well enough by far. i915
> > > > CI is unhappy:
> > > > 
> > > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9400/fi-whl-u/igt@gem_mmap_gtt@basic-small-bo-tiledx.html
> > > > 
> > > > So yeah inserting that lock in there isn't the most trivial thing :-/
> > > > 
> > > > I kinda assume that other drivers will have similar issues, e.g. omapdrm's
> > > > use of dev->struct_mutex also very much looks like it'll result in a new
> > > > locking inversion.
> > > Ah, crap. Already feared that this wouldn't be easy, but yeah that it is as
> > > bad as this is rather disappointing.
> > > 
> > > Thanks for the info, going to keep thinking about how to solve those issues.
> > Side note: We want to make sure that drivers don't get the reservation_obj
> > locking hierarchy wrong in other places (using dev->struct_mutex is kinda
> > a pre-existing mis-use that we can't wish away retroactively
> > unfortunately). One really important thing is that shrinker vs resv_obj
> > must work with trylocks in the shrinker, so that you can allocate memory
> > while holding reservation objects.
> > 
> > One neat trick to teach lockdep about this would be to have a dummy
> > 
> > if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {
> > 	ww_mutex_lock(dma_buf->resv_obj);
> > 	fs_reclaim_acquire(GFP_KERNEL);
> > 	fs_reclaim_release(GFP_KERNEL);
> > 	ww_mutex_unlock(dma_buf->resv_obj);
> > }
> > 
> > in dma_buf_init(). We're using the fs_reclaim_acquire/release check very
> > successfully to improve our igt test coverage for i915.ko in other areas.
> > 
> > Totally unrelated to dev->struct_mutex, but thoughts? Well for
> > dev->struct_mutex we could at least decide on one true way to nest
> > resv_obj vs. dev->struct_mutex as maybe an interim step, but not sure how
> > much that would help.
> 
> I don't think that would help. As far as I can see we only have two choices:
> 
> 1. Either have a big patch which fixes all DMA-buf implementations to allow
> the reservation lock to be held during map/unmap (unrealistic).
> 
> 2. Add a flag to at least in the mid term tell the DMA-buf helper functions
> what to do. E.g. create the mapping without the reservation lock held.
> 
> 
> How about moving the SGL caching from the DRM layer into the DMA-buf layer
> and add a flag if the exporter wants/needs this caching?
> 
> Then only the implementations which can deal with dynamic invalidation
> disable SGL caching and with it enable creating the sgl with the reservation
> object locked.
> 
> This way we can kill two birds with one stone by both avoiding the SGL
> caching in the DRM layer as well as having a sane handling for the locking.
> 
> Thoughts?

I don't see how the SGL stuff factors into neither the dev->struct_mutex
nor into the need to do allocations while holding resv_obj. Neither
changes by moving that piece around. At least as far as I can see it SGL
caching is fully orthogonal to any kind of locking fun.

Why do you see a connection here?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch