[Intel-gfx] [PATCH 1/5] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-11-21 16:30:08)
> 
> On 21/11/2019 13:51, Chris Wilson wrote:
> > As we start peeking into requests for longer and longer, e.g.
> > incorporating use of spinlocks when only protected by an
> > rcu_read_lock(), we need to be careful in how we reset the request when
> > recycling and need to preserve any barriers that may still be in use as
> > the request is reset for reuse.
> 
> How do you tell which members should be left untouched on request_create?

Chicken entrails.

As I remember it, we basically have to ensure that anything stays intact
(where intact can just mean we continue to get expected results, not
necessarily for the same reason) until both parties have passed a strong
memory barrier. Typically that is a successful kref_get for the reader,
and in the request create we have an mb() in the middle of rcustate.

The situations that have been popping up most recently are using
rq->lock across the recycling. The reader would acquire the spinlock,
but i915_request_create would then call spinlock_init and from there it
goes downhill.

> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_request.c   | 37 ++++++++++++++++++---------
> >   drivers/gpu/drm/i915/i915_scheduler.c |  6 +++++
> >   drivers/gpu/drm/i915/i915_scheduler.h |  1 +
> >   drivers/gpu/drm/i915/i915_sw_fence.c  |  8 ++++++
> >   drivers/gpu/drm/i915/i915_sw_fence.h  |  2 ++
> >   5 files changed, 42 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 00011f9533b6..5e5bd7d57134 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -214,7 +214,7 @@ static void remove_from_engine(struct i915_request *rq)
> >               spin_lock(&engine->active.lock);
> >               locked = engine;
> >       }
> > -     list_del(&rq->sched.link);
> > +     list_del_init(&rq->sched.link);
> >       spin_unlock_irq(&locked->active.lock);
> >   }
> >   
> > @@ -586,6 +586,19 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t gfp)
> >       return kmem_cache_alloc(global.slab_requests, gfp);
> >   }
> >   
> > +static void __i915_request_ctor(void *arg)
> > +{
> > +     struct i915_request *rq = arg;
> > +
> > +     spin_lock_init(&rq->lock);
> > +     i915_sched_node_init(&rq->sched);
> > +     i915_sw_fence_init(&rq->submit, submit_notify);
> > +     i915_sw_fence_init(&rq->semaphore, semaphore_notify);
> > +
> > +     INIT_LIST_HEAD(&rq->execute_cb);
> > +     rq->file_priv = NULL;
> > +}
> > +
> >   struct i915_request *
> >   __i915_request_create(struct intel_context *ce, gfp_t gfp)
> >   {
> > @@ -655,24 +668,20 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp)
> >   
> >       rq->rcustate = get_state_synchronize_rcu(); /* acts as smp_mb() */
> >   
> > -     spin_lock_init(&rq->lock);
> >       dma_fence_init(&rq->fence, &i915_fence_ops, &rq->lock,
> >                      tl->fence_context, seqno);
> >   
> >       /* We bump the ref for the fence chain */
> > -     i915_sw_fence_init(&i915_request_get(rq)->submit, submit_notify);
> > -     i915_sw_fence_init(&i915_request_get(rq)->semaphore, semaphore_notify);
> > +     i915_sw_fence_reinit(&i915_request_get(rq)->submit);
> > +     i915_sw_fence_reinit(&i915_request_get(rq)->semaphore);
> >   
> > -     i915_sched_node_init(&rq->sched);
> > +     i915_sched_node_reinit(&rq->sched);
> >   
> >       /* No zalloc, must clear what we need by hand */
> > -     rq->file_priv = NULL;
> >       rq->batch = NULL;
> >       rq->capture_list = NULL;
> >       rq->flags = 0;
> >   
> > -     INIT_LIST_HEAD(&rq->execute_cb);
> 
> For instance this member is now untouched. How do we assert it is in the 
> correct state before it gets used with the new request? What if it has 
> dangling state?

We always maintain it in the correct state. We can assert it that is
empty here.

> Same question for rq->file_priv? We have to be sure it is not read and 
> acted upon before it is set, right?

GEM_BUG_ON(rq->file_priv == NULL);

I see a pattern forming.
-Chris