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);
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
