[Intel-gfx] [PATCH v4] drm/i915: Clean up associated VMAs on context destruction

[Daniel Vetter]

On Tue, Oct 06, 2015 at 10:04:31AM +0100, Chris Wilson wrote:
> On Tue, Oct 06, 2015 at 10:58:01AM +0200, Daniel Vetter wrote:
> > On Mon, Oct 05, 2015 at 01:26:36PM +0100, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > 
> > > Prevent leaking VMAs and PPGTT VMs when objects are imported
> > > via flink.
> > > 
> > > Scenario is that any VMAs created by the importer will be left
> > > dangling after the importer exits, or destroys the PPGTT context
> > > with which they are associated.
> > > 
> > > This is caused by object destruction not running when the
> > > importer closes the buffer object handle due the reference held
> > > by the exporter. This also leaks the VM since the VMA has a
> > > reference on it.
> > > 
> > > In practice these leaks can be observed by stopping and starting
> > > the X server on a kernel with fbcon compiled in. Every time
> > > X server exits another VMA will be leaked against the fbcon's
> > > frame buffer object.
> > > 
> > > Also on systems where flink buffer sharing is used extensively,
> > > like Android, this leak has even more serious consequences.
> > > 
> > > This version is takes a general approach from the  earlier work
> > > by Rafael Barbalho (drm/i915: Clean-up PPGTT on context
> > > destruction) and tries to incorporate the subsequent discussion
> > > between Chris Wilson and Daniel Vetter.
> > > 
> > > v2:
> > > 
> > > Removed immediate cleanup on object retire - it was causing a
> > > recursive VMA unbind via i915_gem_object_wait_rendering. And
> > > it is in fact not even needed since by definition context
> > > cleanup worker runs only after the last context reference has
> > > been dropped, hence all VMAs against the VM belonging to the
> > > context are already on the inactive list.
> > > 
> > > v3:
> > > 
> > > Previous version could deadlock since VMA unbind waits on any
> > > rendering on an object to complete. Objects can be busy in a
> > > different VM which would mean that the cleanup loop would do
> > > the wait with the struct mutex held.
> > > 
> > > This is an even simpler approach where we just unbind VMAs
> > > without waiting since we know all VMAs belonging to this VM
> > > are idle, and there is nothing in flight, at the point
> > > context destructor runs.
> > > 
> > > v4:
> > > 
> > > Double underscore prefix for __915_vma_unbind_no_wait and a
> > > commit message typo fix. (Michel Thierry)
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > Testcase: igt/gem_ppgtt.c/flink-and-exit-vma-leak
> > > Reviewed-by: Michel Thierry <michel.thierry at intel.com>
> > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > Cc: Rafael Barbalho <rafael.barbalho at intel.com>
> > > Cc: Michel Thierry <michel.thierry at intel.com>
> > 
> > Queued for -next, thanks for the patch.
> 
> Please no, it's an awful patch and does not even fix the root cause of
> the leak (that the vma are not closed when the handle is).

It's a lose-lose situation for me as maintainer (and this holds in general
really, not just for this patch):
- Either I wield my considerable maintainer powers and force proper
  solution, which will piss of a lot of people.
- Or I just merge intermediate stuff and piss of another set of people
  (including likely our all future selves because we're slowly digging a
  tech debt grave).

What I can't do with maintainer fu is force collaboration, I can only try
to piss off everyone equally. And today the die rolled a "merge".

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