| Message ID | 20180816073448.19396-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Stop holding a ref to the ppgtt from each vma | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > The context owns both the ppgtt and the vma within it, and our activity > tracking on the context ensures that we do not release active ppgtt. As > the context fulfils our obligations for active memory tracking, we can > relinquish the reference from the vma. > The part of owning the vma within it escapes me. The vma is tied to the object. Is it about that active objects, with their vmas hold the refs to the context they are executing on? > This fixes a silly transient refleak from closed vma being kept alive > until the entire system was idle, keeping all vm alive as well. > > Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > Testcase: igt/gem_ctx_create/files > Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_vma.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c > index 274fd2a7bcb6..31efc971a3a8 100644 > --- a/drivers/gpu/drm/i915/i915_vma.c > +++ b/drivers/gpu/drm/i915/i915_vma.c > @@ -199,7 +199,6 @@ vma_create(struct drm_i915_gem_object *obj, > vma->flags |= I915_VMA_GGTT; > list_add(&vma->obj_link, &obj->vma_list); > } else { > - i915_ppgtt_get(i915_vm_to_ppgtt(vm)); It seems this is the sole user of i915_ppgtt_get so you can remove that too. -Mika > list_add_tail(&vma->obj_link, &obj->vma_list); > } > > @@ -810,9 +809,6 @@ static void __i915_vma_destroy(struct i915_vma *vma) > if (vma->obj) > rb_erase(&vma->obj_node, &vma->obj->vma_tree); > > - if (!i915_vma_is_ggtt(vma)) > - i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm)); > - > rbtree_postorder_for_each_entry_safe(iter, n, &vma->active, node) { > GEM_BUG_ON(i915_gem_active_isset(&iter->base)); > kfree(iter); > -- > 2.18.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Mika Kuoppala (2018-08-16 12:42:08) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > The context owns both the ppgtt and the vma within it, and our activity > > tracking on the context ensures that we do not release active ppgtt. As > > the context fulfils our obligations for active memory tracking, we can > > relinquish the reference from the vma. > > > > The part of owning the vma within it escapes me. The vma is tied > to the object. Is it about that active objects, with their vmas > hold the refs to the context they are executing on? The vma belongs to the (object, context). If the context is closed (or on close(/dev/dri/card0)), so is the ppgtt. Closing the object (gem_close(fd, obj)) closes the vma that match the contexts for the fd, and if it was the last handle, then frees the object. > > This fixes a silly transient refleak from closed vma being kept alive > > until the entire system was idle, keeping all vm alive as well. > > > > Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Testcase: igt/gem_ctx_create/files > > Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close") > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/i915_vma.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c > > index 274fd2a7bcb6..31efc971a3a8 100644 > > --- a/drivers/gpu/drm/i915/i915_vma.c > > +++ b/drivers/gpu/drm/i915/i915_vma.c > > @@ -199,7 +199,6 @@ vma_create(struct drm_i915_gem_object *obj, > > vma->flags |= I915_VMA_GGTT; > > list_add(&vma->obj_link, &obj->vma_list); > > } else { > > - i915_ppgtt_get(i915_vm_to_ppgtt(vm)); > > It seems this is the sole user of i915_ppgtt_get so you can > remove that too. Such shortsightedness; it will reappear shortly. See the patches to share vm between contexts. -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > Quoting Mika Kuoppala (2018-08-16 12:42:08) >> Chris Wilson <chris@chris-wilson.co.uk> writes: >> >> > The context owns both the ppgtt and the vma within it, and our activity >> > tracking on the context ensures that we do not release active ppgtt. As >> > the context fulfils our obligations for active memory tracking, we can >> > relinquish the reference from the vma. >> > >> >> The part of owning the vma within it escapes me. The vma is tied >> to the object. Is it about that active objects, with their vmas >> hold the refs to the context they are executing on? > > The vma belongs to the (object, context). If the context is closed (or > on close(/dev/dri/card0)), so is the ppgtt. Closing the object > (gem_close(fd, obj)) closes the vma that match the contexts for the fd, > and if it was the last handle, then frees the object. > >> > This fixes a silly transient refleak from closed vma being kept alive >> > until the entire system was idle, keeping all vm alive as well. >> > >> > Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > Testcase: igt/gem_ctx_create/files >> > Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close") >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > --- >> > drivers/gpu/drm/i915/i915_vma.c | 4 ---- >> > 1 file changed, 4 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c >> > index 274fd2a7bcb6..31efc971a3a8 100644 >> > --- a/drivers/gpu/drm/i915/i915_vma.c >> > +++ b/drivers/gpu/drm/i915/i915_vma.c >> > @@ -199,7 +199,6 @@ vma_create(struct drm_i915_gem_object *obj, >> > vma->flags |= I915_VMA_GGTT; >> > list_add(&vma->obj_link, &obj->vma_list); >> > } else { >> > - i915_ppgtt_get(i915_vm_to_ppgtt(vm)); >> >> It seems this is the sole user of i915_ppgtt_get so you can >> remove that too. > > Such shortsightedness; it will reappear shortly. See the patches to > share vm between contexts. Ok let it float in there for a while. Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> Also Tested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> I also been after this one previously and managed to bring up a testcase where we create a mapping to an object and then create context, do execbuf with mapped object as relocatable, and destroy context. That would also end up keeping ppgtts alive until client closes. Would it be worthwhile to include such test or do you think gem_ctx_create@files covers enough? -Mika
Quoting Mika Kuoppala (2018-08-16 13:14:37) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Quoting Mika Kuoppala (2018-08-16 12:42:08) > >> Chris Wilson <chris@chris-wilson.co.uk> writes: > >> > >> > The context owns both the ppgtt and the vma within it, and our activity > >> > tracking on the context ensures that we do not release active ppgtt. As > >> > the context fulfils our obligations for active memory tracking, we can > >> > relinquish the reference from the vma. > >> > > >> > >> The part of owning the vma within it escapes me. The vma is tied > >> to the object. Is it about that active objects, with their vmas > >> hold the refs to the context they are executing on? > > > > The vma belongs to the (object, context). If the context is closed (or > > on close(/dev/dri/card0)), so is the ppgtt. Closing the object > > (gem_close(fd, obj)) closes the vma that match the contexts for the fd, > > and if it was the last handle, then frees the object. > > > >> > This fixes a silly transient refleak from closed vma being kept alive > >> > until the entire system was idle, keeping all vm alive as well. > >> > > >> > Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > Testcase: igt/gem_ctx_create/files > >> > Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close") > >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/i915_vma.c | 4 ---- > >> > 1 file changed, 4 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c > >> > index 274fd2a7bcb6..31efc971a3a8 100644 > >> > --- a/drivers/gpu/drm/i915/i915_vma.c > >> > +++ b/drivers/gpu/drm/i915/i915_vma.c > >> > @@ -199,7 +199,6 @@ vma_create(struct drm_i915_gem_object *obj, > >> > vma->flags |= I915_VMA_GGTT; > >> > list_add(&vma->obj_link, &obj->vma_list); > >> > } else { > >> > - i915_ppgtt_get(i915_vm_to_ppgtt(vm)); > >> > >> It seems this is the sole user of i915_ppgtt_get so you can > >> remove that too. > > > > Such shortsightedness; it will reappear shortly. See the patches to > > share vm between contexts. > > Ok let it float in there for a while. > > Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Also > > Tested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > I also been after this one previously and managed to bring > up a testcase where we create a mapping to an object and > then create context, do execbuf with mapped object as relocatable, > and destroy context. That would also end up keeping ppgtts > alive until client closes. > > Would it be worthwhile to include such test or do you think > gem_ctx_create@files covers enough? There's no linkage between the mmap and context, so it doesn't strike me as novel. gem_exec_reloc (and friends) include passing mmap(obj) as the execbuf, objects and/or relocations, so that should be covered; and gem_ctx_create covers using lots of temporary and long lived contexts. We put previous bugs in gem_ppgtt, e.g. the vma-leak, but part of the challenge there is that to stabilise the counters you need an idle system. However, bug discovery is serendipitous and minor differences between similar tests (even doing the same setup in a different order) can be all the difference between finding a bug and not. So don't let me stop you... You can use the igt_kcov tools to tell if a test case is similar to any existing one. -Chris
Quoting Mika Kuoppala (2018-08-16 13:14:37) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Quoting Mika Kuoppala (2018-08-16 12:42:08) > >> Chris Wilson <chris@chris-wilson.co.uk> writes: > >> > >> > The context owns both the ppgtt and the vma within it, and our activity > >> > tracking on the context ensures that we do not release active ppgtt. As > >> > the context fulfils our obligations for active memory tracking, we can > >> > relinquish the reference from the vma. > >> > > >> > >> The part of owning the vma within it escapes me. The vma is tied > >> to the object. Is it about that active objects, with their vmas > >> hold the refs to the context they are executing on? > > > > The vma belongs to the (object, context). If the context is closed (or > > on close(/dev/dri/card0)), so is the ppgtt. Closing the object > > (gem_close(fd, obj)) closes the vma that match the contexts for the fd, > > and if it was the last handle, then frees the object. > > > >> > This fixes a silly transient refleak from closed vma being kept alive > >> > until the entire system was idle, keeping all vm alive as well. > >> > > >> > Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > Testcase: igt/gem_ctx_create/files > >> > Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close") > >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/i915_vma.c | 4 ---- > >> > 1 file changed, 4 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c > >> > index 274fd2a7bcb6..31efc971a3a8 100644 > >> > --- a/drivers/gpu/drm/i915/i915_vma.c > >> > +++ b/drivers/gpu/drm/i915/i915_vma.c > >> > @@ -199,7 +199,6 @@ vma_create(struct drm_i915_gem_object *obj, > >> > vma->flags |= I915_VMA_GGTT; > >> > list_add(&vma->obj_link, &obj->vma_list); > >> > } else { > >> > - i915_ppgtt_get(i915_vm_to_ppgtt(vm)); > >> > >> It seems this is the sole user of i915_ppgtt_get so you can > >> remove that too. > > > > Such shortsightedness; it will reappear shortly. See the patches to > > share vm between contexts. > > Ok let it float in there for a while. > > Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Also > > Tested-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> Applied the bug fix, thank you for the review. -Chris
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 274fd2a7bcb6..31efc971a3a8 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -199,7 +199,6 @@ vma_create(struct drm_i915_gem_object *obj, vma->flags |= I915_VMA_GGTT; list_add(&vma->obj_link, &obj->vma_list); } else { - i915_ppgtt_get(i915_vm_to_ppgtt(vm)); list_add_tail(&vma->obj_link, &obj->vma_list); } @@ -810,9 +809,6 @@ static void __i915_vma_destroy(struct i915_vma *vma) if (vma->obj) rb_erase(&vma->obj_node, &vma->obj->vma_tree); - if (!i915_vma_is_ggtt(vma)) - i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm)); - rbtree_postorder_for_each_entry_safe(iter, n, &vma->active, node) { GEM_BUG_ON(i915_gem_active_isset(&iter->base)); kfree(iter);
The context owns both the ppgtt and the vma within it, and our activity tracking on the context ensures that we do not release active ppgtt. As the context fulfils our obligations for active memory tracking, we can relinquish the reference from the vma. This fixes a silly transient refleak from closed vma being kept alive until the entire system was idle, keeping all vm alive as well. Reported-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Testcase: igt/gem_ctx_create/files Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/i915_vma.c | 4 ---- 1 file changed, 4 deletions(-)