Quoting Mika Kuoppala (2019-07-04 14:53:09)
> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
