Re: [Intel-gfx] [PATCH 09/11] drm/i915: Release vma when the handle is closed

[Chris Wilson]

On Thu, Dec 17, 2015 at 01:46:58PM +, Tvrtko Ursulin wrote:
> > list_for_each_entry_safe(vma, next, >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.


diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 688162703070..edfa5ebc4e77 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3857,7 +3857,14 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 
trace_i915_gem_object_destroy(obj);
 
+   /* All file-owned VMA should have been released by this point (through
+* i915_gem_close_object). However, the object may also be bound into
+* the global GTT (e.g. older GPUs without per-process support, or
+* for direct access through the GTT either for the user or for
+* scanout). Those VMA still need to unbound now.
+*/
list_for_each_entry_safe(vma, next, >vma_list, obj_link) {
+   RQ_BUG_ON(!i915_is_ggtt(vma->vm));
RQ_BUG_ON(vma->active);
vma->pin_count = 0;
i915_vma_close(vma);

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 09/11] drm/i915: Release vma when the handle is closed

[Chris Wilson]

On Thu, Dec 17, 2015 at 01:46:58PM +, 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 
> >Cc: Tvrtko Ursulin 
> >Cc: Daniele Ceraolo Spurio  >---
> >  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 = _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, >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 

Re: [Intel-gfx] [PATCH 09/11] drm/i915: Release vma when the handle is closed

[Tvrtko Ursulin]

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 
Cc: Tvrtko Ursulin 
Cc: Daniele Ceraolo Spurio closed);


Same complaint as in the previous patch, cannot use RQ_BUG_ON. Maybe 
need GEM_BUG_ON then, don't know.



+   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.



+
+   list_del_init(>obj_link);
+   if (!vma->active)
+   WARN_ON(i915_vma_unbind(vma));
+}
+
+void i915_gem_close_object(struct drm_gem_object *gem,
+  struct drm_file *file)
+{
+   struct drm_i915_gem_object *obj = to_intel_bo(gem);
+   struct drm_i915_file_private *fpriv = file->driver_priv;
+   struct i915_vma *vma, *vn;
+
+   mutex_lock(>base.dev->struct_mutex);
+   list_for_each_entry_safe(vma, vn, >vma_list, obj_link)
+   if (vma->vm->file == fpriv)
+   i915_vma_close(vma);
+   mutex_unlock(>base.dev->struct_mutex);
+}
+
  /**
   * i915_gem_wait_ioctl - implements DRM_IOCTL_I915_GEM_WAIT
   * @DRM_IOCTL_ARGS: standard ioctl arguments
@@ -2577,9 +2601,6 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool 
wait)
RQ_BUG_ON(vma->active);
}

-   if (list_empty(>obj_link))
-   return 0;
-
if (!drm_mm_node_allocated(>node)) {
i915_gem_vma_destroy(vma);
return 0;
@@ -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, >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.



}

/* Stolen objects don't hold a ref, but do hold pin count. Fix that up
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 5505603f52af..9f594c33bd0a 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ 

Re: [Intel-gfx] [PATCH 09/11] drm/i915: Release vma when the handle is closed

[Tvrtko Ursulin]

On 17/12/15 14:21, Chris Wilson wrote:

On Thu, Dec 17, 2015 at 01:46:58PM +, Tvrtko Ursulin wrote:

list_for_each_entry_safe(vma, next, >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.



diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 688162703070..edfa5ebc4e77 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3857,7 +3857,14 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)

 trace_i915_gem_object_destroy(obj);

+   /* All file-owned VMA should have been released by this point (through
+* i915_gem_close_object). However, the object may also be bound into
+* the global GTT (e.g. older GPUs without per-process support, or
+* for direct access through the GTT either for the user or for
+* scanout). Those VMA still need to unbound now.
+*/
 list_for_each_entry_safe(vma, next, >vma_list, obj_link) {
+   RQ_BUG_ON(!i915_is_ggtt(vma->vm));
 RQ_BUG_ON(vma->active);
 vma->pin_count = 0;
 i915_vma_close(vma);



Ah yes, I've missed that detail. Very good to have it in a comment (and 
assert).


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 09/11] drm/i915: Release vma when the handle is closed

[Chris Wilson]

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 
Cc: Tvrtko Ursulin 
Cc: Daniele Ceraolo Spurio closed);
+   vma->closed = true;
+
+   list_del_init(>obj_link);
+   if (!vma->active)
+   WARN_ON(i915_vma_unbind(vma));
+}
+
+void i915_gem_close_object(struct drm_gem_object *gem,
+  struct drm_file *file)
+{
+   struct drm_i915_gem_object *obj = to_intel_bo(gem);
+   struct drm_i915_file_private *fpriv = file->driver_priv;
+   struct i915_vma *vma, *vn;
+
+   mutex_lock(>base.dev->struct_mutex);
+   list_for_each_entry_safe(vma, vn, >vma_list, obj_link)
+   if (vma->vm->file == fpriv)
+   i915_vma_close(vma);
+   mutex_unlock(>base.dev->struct_mutex);
+}
+
 /**
  * i915_gem_wait_ioctl - implements DRM_IOCTL_I915_GEM_WAIT
  * @DRM_IOCTL_ARGS: standard ioctl arguments
@@ -2577,9 +2601,6 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool 
wait)
RQ_BUG_ON(vma->active);
}
 
-   if (list_empty(>obj_link))
-   return 0;
-
if (!drm_mm_node_allocated(>node)) {
i915_gem_vma_destroy(vma);
return 0;
@@ -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, >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);
}
 
/* Stolen objects don't hold a ref, but do hold pin count. Fix that up
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 5505603f52af..9f594c33bd0a 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3240,6 +3240,8 @@ i915_vma_retire(struct drm_i915_gem_request_active 
*active,
return;
 
list_move_tail(>vm_link, >vm->inactive_list);
+   if (unlikely(vma->closed))
+   WARN_ON(i915_vma_unbind(vma));
 }
 
 static struct i915_vma *
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h 
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index c2f2c62ac88d..be7e8526b219 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++