On Thu, Dec 17, 2015 at 01:46:58PM +0000, Tvrtko Ursulin wrote:
>
> Hi,
>
> On 14/12/15 11:36, Chris Wilson wrote:
> >In order to prevent a leak of the vma on shared objects, we need to
> >hook into the object_close callback to destroy the vma on the object for
> >this file. However, if we destroyed that vma immediately we may cause
> >unexpected application stalls as we try to unbind a busy vma - hence we
> >defer the unbind to when we retire the vma.
> >
> >Testcase: igt/gem_ppggtt/flink-and-close-vma-leak
> >Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> >Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx
> >---
> > drivers/gpu/drm/i915/i915_drv.c | 1 +
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/i915_gem.c | 41 ++++++++++++++++++++++---------------
> > drivers/gpu/drm/i915/i915_gem_gtt.c | 2 ++
> > drivers/gpu/drm/i915/i915_gem_gtt.h | 1 +
> > 5 files changed, 30 insertions(+), 16 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >index e7eef5fd6918..70979339d58a 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.c
> >+++ b/drivers/gpu/drm/i915/i915_drv.c
> >@@ -1656,6 +1656,7 @@ static struct drm_driver driver = {
> > .debugfs_init = i915_debugfs_init,
> > .debugfs_cleanup = i915_debugfs_cleanup,
> > #endif
> >+ .gem_close_object = i915_gem_close_object,
> > .gem_free_object = i915_gem_free_object,
> > .gem_vm_ops = &i915_gem_vm_ops,
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index eb775eb1c693..696469a06715 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -2686,6 +2686,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
> > size_t size);
> > struct drm_i915_gem_object *i915_gem_object_create_from_data(
> > struct drm_device *dev, const void *data, size_t size);
> >+void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *file);
> > void i915_gem_free_object(struct drm_gem_object *obj);
> > void i915_gem_vma_destroy(struct i915_vma *vma);
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 1d21c5b79215..7c13c27a6470 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -2367,6 +2367,30 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
> > return 0;
> > }
> >
> >+static void i915_vma_close(struct i915_vma *vma)
> >+{
> >+ RQ_BUG_ON(vma->closed);
>
> Same complaint as in the previous patch, cannot use RQ_BUG_ON. Maybe
> need GEM_BUG_ON then, don't know.
Hopefully Joonas will jump in to the rescue. GEM_BUG_ON() works for me.
> >+ vma->closed = true;
>
> Hmmm, vma->detached? Because VMA is not really closed. And
> i915_vma_detach - it would symbolise then that VMA has been detached
> from the object and is lingering only on the VM lists.
Perhaps. I chose _close() simply because that it the user action that
initiated all the actitive (either GEM_CLOSE or GEM_CONTEXT_CLOSE, or
the implicit close from close(fd)/task_exit).
detach feels a little too undefined, close at least implies termination
to me.
Of course on the vfs side, close() is handled by fput/delayed_fput!
Maybe:
gem_object_close -> i915_vma_release
context_close -> i915_ppgtt_release -> i915_vma_release
though release is already used by kref tracking (i915_ppgtt_release).
I'm not keen on using i915_vma_get/i915_vma_put, precisely because we
have managed to avoid using kref vma so far (and so we are not doing
typical reference tracking).
gem_object_close -> i915_vma_detach
context_close -> i915_ppgtt_detach -> i915_vma_detach
Still liking the consistency of close.
gem_object_close -> i915_vma_close
context_close -> i915_ppgtt_close -> i915_vma_close
Could be worse, but also there may easily be a better naming pattern.
> >@@ -3792,20 +3813,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> > trace_i915_gem_object_destroy(obj);
> >
> > list_for_each_entry_safe(vma, next, &obj->vma_list, obj_link) {
> >- int ret;
> >-
> > vma->pin_count = 0;
> >- ret = i915_vma_unbind(vma);
> >- if (WARN_ON(ret == -ERESTARTSYS)) {
> >- bool was_interruptible;
> >-
> >- was_interruptible = dev_priv->mm.interruptible;
> >- dev_priv->mm.interruptible = false;
> >-
> >- WARN_ON(i915_vma_unbind(vma));
> >-
> >- dev_priv->mm.interruptible = was_interruptible;
> >- }
> >+ i915_vma_close(vma);
>
> In what circumstances can there be any VMAs still left unclosed at
> this point? I thought i915_gem_close_object would had closed them
> all.
vma belonging to GGTT are not owned by any one file but shared, so we
still expect them here as well.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
