[Intel-gfx] [PATCH] drm/i915/userptr: Wrap mmu_notifier inside its own rw_semaphore

[Daniel Vetter]

On Mon, Mar 26, 2018 at 05:28:58PM +0100, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-03-26 16:59:20)
> > 
> > On 26/03/2018 15:59, Chris Wilson wrote:
> > > We've always been blatantly ignoring mmu_notifier.h:
> > > 
> > >   * Invalidation of multiple concurrent ranges may be
> > >   * optionally permitted by the driver. Either way the
> > >   * establishment of sptes is forbidden in the range passed to
> > >   * invalidate_range_begin/end for the whole duration of the
> > >   * invalidate_range_begin/end critical section.
> > > 
> > > by not preventing concurrent calls to gup while an invalidate_range is
> > > being processed. Wrap gup and invalidate_range in a paired rw_semaphore
> > > to allow concurrent lookups, that are then interrupted and disabled
> > > across the invalidate_range. Further refinement can be applied by
> > > tracking the invalidate_range versus individual gup, which should be
> > > done using a common set of helpers for all mmu_notifier subsystems share
> > > the same need. I hear HMM is one such toolbox...
> > > 
> > > For the time being, assume concurrent invalidate and lookup are rare,
> > > but not rare enough to completely ignore.
> > 
> > I think I suggested a few times we should just "ban" the object on first 
> > invalidate and never ever for its lifetime allow it to obtain backing 
> > store again. I just don't remember why we decided not to go with that 
> > approach. :( Thinking about it now I still don't see that this 
> > restriction would be a problem and would simplify things.
> 
> You still have the problem where it is being banned as we are trying to
> instantiate it the first time. Imo, we are re-implementing mmap_sem
> crudely. (Even more so when every mmu notifier must implement the same
> code, and more than one will be called everytime the mm is touched.)

Jerome Glisse is promising to get that fixed and provide neater
primitives. Apparently we can reuse parts of the HMM stuff since that's
built as a helper library (even though the docs don't make it clear).

> And we can get perfectly innocent invalidates, e.g. mprotect.

Also hugepage consolidation, memory migration, swapout and everything else
which really should work.

> > With more locks I am quite fearful what lockdep will say, but lets see...
> 
> Same here.

Cross-release enabled lockdep would be even better, because of our various
worker tricks which do hide lock deps from current lockdep. We should
still have the required fixups hanging around in topic/core-for-CI.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch