| Message ID | 20190703171913.16585-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915/gem: Defer obj->base.resv fini until RCU callback | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > Since reservation_object_fini() does an immediate free, rather than > kfree_rcu as normal, we have to delay the release until after the RCU > grace period has elapsed (i.e. from the rcu cleanup callback) so that we > can rely on the RCU protected access to the fences while the object is a > zombie. > > i915_gem_busy_ioctl relies on having an RCU barrier to protect the > reservation in order to avoid having to take a reference and strong > memory barriers. Ok so for gem busy to be able to operate on a 'to be freed' object we need to keep the reservation object alive? > > Fixes: c03467ba40f7 ("drm/i915/gem: Free pages before rcu-freeing the object") > Testcase: igt/gem_busy/close-race > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Matthew Auld <matthew.auld@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_object.c | 3 ++- > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 7 +++++++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c > index d3e96f09c6b7..0dced4a20e20 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > @@ -152,6 +152,7 @@ static void __i915_gem_free_object_rcu(struct rcu_head *head) > container_of(head, typeof(*obj), rcu); > struct drm_i915_private *i915 = to_i915(obj->base.dev); > > + reservation_object_fini(&obj->base._resv); > i915_gem_object_free(obj); > > GEM_BUG_ON(!atomic_read(&i915->mm.free_count)); > @@ -198,7 +199,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915, > if (obj->base.import_attach) > drm_prime_gem_destroy(&obj->base, NULL); > > - drm_gem_object_release(&obj->base); > + drm_gem_free_mmap_offset(&obj->base); > > /* But keep the pointer alive for RCU-protected lookups */ > call_rcu(&obj->rcu, __i915_gem_free_object_rcu); > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > index 19d9ecdb2894..d2a1158868e7 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > @@ -414,6 +414,11 @@ shmem_pwrite(struct drm_i915_gem_object *obj, > return 0; > } > > +static void shmem_release(struct drm_i915_gem_object *obj) > +{ > + fput(obj->base.filp); We lose the check for filp existence. But as internal ops have their own mechanics, we should always have the filp. We lose a warn for dma_buf existence tho. -Mika > +} > + > const struct drm_i915_gem_object_ops i915_gem_shmem_ops = { > .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE | > I915_GEM_OBJECT_IS_SHRINKABLE, > @@ -424,6 +429,8 @@ const struct drm_i915_gem_object_ops i915_gem_shmem_ops = { > .writeback = shmem_writeback, > > .pwrite = shmem_pwrite, > + > + .release = shmem_release, > }; > > static int create_shmem(struct drm_i915_private *i915, > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Mika Kuoppala (2019-07-04 14:53:09) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Since reservation_object_fini() does an immediate free, rather than > > kfree_rcu as normal, we have to delay the release until after the RCU > > grace period has elapsed (i.e. from the rcu cleanup callback) so that we > > can rely on the RCU protected access to the fences while the object is a > > zombie. > > > > i915_gem_busy_ioctl relies on having an RCU barrier to protect the > > reservation in order to avoid having to take a reference and strong > > memory barriers. > > Ok so for gem busy to be able to operate on a 'to be freed' object > we need to keep the reservation object alive? Yup. It could equally be kept alive if resv_obj_fini used kfree_rcu() instead, but we already need an RCU barrier for our object lookup so might as well use one stone for both birds. > > Fixes: c03467ba40f7 ("drm/i915/gem: Free pages before rcu-freeing the object") > > Testcase: igt/gem_busy/close-race > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Matthew Auld <matthew.auld@intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_object.c | 3 ++- > > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 7 +++++++ > > 2 files changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c > > index d3e96f09c6b7..0dced4a20e20 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > > @@ -152,6 +152,7 @@ static void __i915_gem_free_object_rcu(struct rcu_head *head) > > container_of(head, typeof(*obj), rcu); > > struct drm_i915_private *i915 = to_i915(obj->base.dev); > > > > + reservation_object_fini(&obj->base._resv); > > i915_gem_object_free(obj); > > > > GEM_BUG_ON(!atomic_read(&i915->mm.free_count)); > > @@ -198,7 +199,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915, > > if (obj->base.import_attach) > > drm_prime_gem_destroy(&obj->base, NULL); > > > > - drm_gem_object_release(&obj->base); > > + drm_gem_free_mmap_offset(&obj->base); > > > > /* But keep the pointer alive for RCU-protected lookups */ > > call_rcu(&obj->rcu, __i915_gem_free_object_rcu); > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > > index 19d9ecdb2894..d2a1158868e7 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > > @@ -414,6 +414,11 @@ shmem_pwrite(struct drm_i915_gem_object *obj, > > return 0; > > } > > > > +static void shmem_release(struct drm_i915_gem_object *obj) > > +{ > > + fput(obj->base.filp); > > We lose the check for filp existence. But as internal > ops have their own mechanics, we should always have the filp. Exactly. drm_gem_object should not have filp anymore. > We lose a warn for dma_buf existence tho. Good. Let me hand you a tiny violin ;) -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > Quoting Mika Kuoppala (2019-07-04 14:53:09) >> Chris Wilson <chris@chris-wilson.co.uk> writes: >> >> > Since reservation_object_fini() does an immediate free, rather than >> > kfree_rcu as normal, we have to delay the release until after the RCU >> > grace period has elapsed (i.e. from the rcu cleanup callback) so that we >> > can rely on the RCU protected access to the fences while the object is a >> > zombie. >> > >> > i915_gem_busy_ioctl relies on having an RCU barrier to protect the >> > reservation in order to avoid having to take a reference and strong >> > memory barriers. >> >> Ok so for gem busy to be able to operate on a 'to be freed' object >> we need to keep the reservation object alive? > > Yup. It could equally be kept alive if resv_obj_fini used kfree_rcu() > instead, but we already need an RCU barrier for our object lookup so > might as well use one stone for both birds. > >> > Fixes: c03467ba40f7 ("drm/i915/gem: Free pages before rcu-freeing the object") >> > Testcase: igt/gem_busy/close-race >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> > Cc: Matthew Auld <matthew.auld@intel.com> >> > --- >> > drivers/gpu/drm/i915/gem/i915_gem_object.c | 3 ++- >> > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 7 +++++++ >> > 2 files changed, 9 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c >> > index d3e96f09c6b7..0dced4a20e20 100644 >> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >> > @@ -152,6 +152,7 @@ static void __i915_gem_free_object_rcu(struct rcu_head *head) >> > container_of(head, typeof(*obj), rcu); >> > struct drm_i915_private *i915 = to_i915(obj->base.dev); >> > >> > + reservation_object_fini(&obj->base._resv); >> > i915_gem_object_free(obj); >> > >> > GEM_BUG_ON(!atomic_read(&i915->mm.free_count)); >> > @@ -198,7 +199,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915, >> > if (obj->base.import_attach) >> > drm_prime_gem_destroy(&obj->base, NULL); >> > >> > - drm_gem_object_release(&obj->base); >> > + drm_gem_free_mmap_offset(&obj->base); >> > >> > /* But keep the pointer alive for RCU-protected lookups */ >> > call_rcu(&obj->rcu, __i915_gem_free_object_rcu); >> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >> > index 19d9ecdb2894..d2a1158868e7 100644 >> > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >> > @@ -414,6 +414,11 @@ shmem_pwrite(struct drm_i915_gem_object *obj, >> > return 0; >> > } >> > >> > +static void shmem_release(struct drm_i915_gem_object *obj) >> > +{ >> > + fput(obj->base.filp); >> >> We lose the check for filp existence. But as internal >> ops have their own mechanics, we should always have the filp. > > Exactly. drm_gem_object should not have filp anymore. ..for internal objects. > >> We lose a warn for dma_buf existence tho. > > Good. Let me hand you a tiny violin ;) Let's see how it plays out. Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > -Chris
Quoting Mika Kuoppala (2019-07-04 15:18:40) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Quoting Mika Kuoppala (2019-07-04 14:53:09) > >> Chris Wilson <chris@chris-wilson.co.uk> writes: > >> > +static void shmem_release(struct drm_i915_gem_object *obj) > >> > +{ > >> > + fput(obj->base.filp); > >> > >> We lose the check for filp existence. But as internal > >> ops have their own mechanics, we should always have the filp. > > > > Exactly. drm_gem_object should not have filp anymore. > > ..for internal objects. I mean the struct drm_gem_object should not include a struct file *filp anymore as not all subclasses are shmem based. And where people want to use it, it raises more questions than answers. -Chris
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index d3e96f09c6b7..0dced4a20e20 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -152,6 +152,7 @@ static void __i915_gem_free_object_rcu(struct rcu_head *head) container_of(head, typeof(*obj), rcu); struct drm_i915_private *i915 = to_i915(obj->base.dev); + reservation_object_fini(&obj->base._resv); i915_gem_object_free(obj); GEM_BUG_ON(!atomic_read(&i915->mm.free_count)); @@ -198,7 +199,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915, if (obj->base.import_attach) drm_prime_gem_destroy(&obj->base, NULL); - drm_gem_object_release(&obj->base); + drm_gem_free_mmap_offset(&obj->base); /* But keep the pointer alive for RCU-protected lookups */ call_rcu(&obj->rcu, __i915_gem_free_object_rcu); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c index 19d9ecdb2894..d2a1158868e7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c @@ -414,6 +414,11 @@ shmem_pwrite(struct drm_i915_gem_object *obj, return 0; } +static void shmem_release(struct drm_i915_gem_object *obj) +{ + fput(obj->base.filp); +} + const struct drm_i915_gem_object_ops i915_gem_shmem_ops = { .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE | I915_GEM_OBJECT_IS_SHRINKABLE, @@ -424,6 +429,8 @@ const struct drm_i915_gem_object_ops i915_gem_shmem_ops = { .writeback = shmem_writeback, .pwrite = shmem_pwrite, + + .release = shmem_release, }; static int create_shmem(struct drm_i915_private *i915,
Since reservation_object_fini() does an immediate free, rather than kfree_rcu as normal, we have to delay the release until after the RCU grace period has elapsed (i.e. from the rcu cleanup callback) so that we can rely on the RCU protected access to the fences while the object is a zombie. i915_gem_busy_ioctl relies on having an RCU barrier to protect the reservation in order to avoid having to take a reference and strong memory barriers. Fixes: c03467ba40f7 ("drm/i915/gem: Free pages before rcu-freeing the object") Testcase: igt/gem_busy/close-race Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_object.c | 3 ++- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-)