On Fri, Jul 29, 2016 at 10:43:17AM +0100, Chris Wilson wrote:
> On Fri, Jul 29, 2016 at 09:49:54AM +0100, Chris Wilson wrote:
> > On Fri, Jul 29, 2016 at 10:41:14AM +0200, Daniel Vetter wrote:
> > > I guess it doesn't hurt to make this really, really clear. Perfect! Well
> > > almost, one nit:
> > >
> > > >
> > > > /* What stops the following rcu_dereference() from occuring
> > > > * before the above i915_gem_request_get_rcu()? If we were
> > > > * to read the value before pausing to get the reference to
> > > > * the request, we may not notice a change in the active
> > > > * tracker.
> > > > *
> > > > * The rcu_dereference() is a mere read barrier, which means
> > >
> > > s/read barrier/barrier of depending reads/, rcu_dereference is not even a
> > > full rmb!
> > >
> > > > * that operations after it will appear after, neither the
> > >
> > > hence also: s/operations/any operations through the read pointer/
> >
> > Ah right, that needs to be dependent reads. Changes look good.
>
>
> do {
> struct drm_i915_gem_request *request;
>
> request = rcu_dereference(active->request);
> if (!request || i915_gem_request_completed(request))
> return NULL;
>
> request = i915_gem_request_get_rcu(request);
>
> /* What stops the following rcu_access_pointer() from occurring
> * before the above i915_gem_request_get_rcu()? If we were
> * to read the value before pausing to get the reference to
> * the request, we may not notice a change in the active
> * tracker.
> *
> * The rcu_access_pointer() is a mere compiler barrier, which
> * means both the CPU and compiler are free to perform the
> * memory read without constraint. The compiler only has to
> * ensure that any operations after the rcu_access_pointer()
> * occur afterwards in program order. This means the read may
> * be performed earlier by an out-of-order CPU, or adventurous
> * compiler.
> *
> * The atomic operation at the heart of
> * i915_gem_request_get_rcu(), see fence_get_rcu(), is
> * atomic_inc_not_zero() which is only a full memory barrier
> * when successful. That is, if i915_gem_request_get_rcu()
> * returns the request (and so with the reference counted
> * incremented) then the following read for rcu_access_pointer()
> * must occur after the atomic operation and so confirm
> * that this request is the one currently being tracked.
> */
> if (!request || request == rcu_access_pointer(active->request))
> return rcu_pointer_handoff(request);
>
> i915_gem_request_put(request);
> } while (1);
lgtm now, Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
