[Intel-gfx] [PATCH 1/3] drm/i915/gem: Defer obj->base.resv fini until RCU callback

[Chris Wilson]

Quoting Mika Kuoppala (2019-07-04 14:53:09)
> Chris Wilson <chris at 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 at chris-wilson.co.uk>
> > Cc: Matthew Auld <matthew.auld at 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