[Intel-gfx] [PATCH] drm/i915/breadcrumbs: Drop request reference for the signaler thread

[Chris Wilson]

Quoting Chris Wilson (2018-01-31 20:40:03)
> Quoting Chris Wilson (2018-01-31 17:30:46)
> > Quoting Tvrtko Ursulin (2018-01-31 17:18:55)
> > > 
> > > On 22/01/2018 15:41, Chris Wilson wrote:
> > > > +static struct drm_i915_gem_request *first_signal(struct intel_breadcrumbs *b)
> > > > +{
> > > > +     /*
> > > > +      * See the big warnings for i915_gem_active_get_rcu() and similarly
> > > > +      * for dma_fence_get_rcu_safe() that explain the intricacies involved
> > > > +      * here with defeating CPU/compiler speculation and enforcing
> > > > +      * the required memory barriers.
> > > > +      */
> > > > +     do {
> > > > +             struct drm_i915_gem_request *request;
> > > > +
> > > > +             request = rcu_dereference(b->first_signal);
> > > > +             if (request)
> > > > +                     request = i915_gem_request_get_rcu(request);
> > > > +
> > > > +             barrier();
> > > > +
> > > > +             if (!request || request == rcu_access_pointer(b->first_signal))
> > > > +                     return rcu_pointer_handoff(request);
> > > > +
> > > > +             i915_gem_request_put(request);
> > > > +     } while (1);
> > > > +}
> > > > +
> > > >   static int intel_breadcrumbs_signaler(void *arg)
> > > >   {
> > > >       struct intel_engine_cs *engine = arg;
> > > > @@ -667,41 +715,21 @@ static int intel_breadcrumbs_signaler(void *arg)
> > > >                * a new client.
> > > >                */
> > > >               rcu_read_lock();
> > > > -             request = rcu_dereference(b->first_signal);
> > > > -             if (request)
> > > > -                     request = i915_gem_request_get_rcu(request);
> > > > +             request = first_signal(b);
> > > 
> > > get_ prefix would be good to signify a get vs peek. Maybe even _rcu suffix.
> > 
> > Sold.
> 
> Whilst you are looking at this, I should just say that first_signal() is
> what we should have been doing all this time; it's really a very obscure
> bug fix. And fwiw, the s/rbtree/list/ patch eliminates it.

Hmm, actually no, no underlying bug here as the reference was previously
carried by the signaler, and with its removal do we need to do the
double dance. (It's a bad sign when I can remember having that exact
conversation with myself; and forgot until too late.)
-Chris