[Intel-gfx] [PATCH 08/66] drm/i915: Make the stale cached active node available for any timeline

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-07-29 15:22:26)
> 
> On 29/07/2020 14:42, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-07-29 13:40:38)
> >>
> >> On 28/07/2020 15:28, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2020-07-17 14:04:58)
> >>>>
> >>>> On 15/07/2020 12:50, Chris Wilson wrote:
> >>>>> @@ -235,9 +239,22 @@ static struct active_node *__active_lookup(struct i915_active *ref, u64 idx)
> >>>>>     {
> >>>>>         struct active_node *it;
> >>>>>     
> >>>>> +     GEM_BUG_ON(idx == 0); /* 0 is the unordered timeline, rsvd for cache */
> >>>>> +
> >>>>>         it = READ_ONCE(ref->cache);
> >>>>> -     if (it && it->timeline == idx)
> >>>>> -             return it;
> >>>>> +     if (it) {
> >>>>> +             u64 cached = READ_ONCE(it->timeline);
> >>>>> +
> >>>>> +             if (cached == idx)
> >>>>> +                     return it;
> >>>>> +
> >>>>> +#ifdef CONFIG_64BIT /* for cmpxchg(u64) */
> >>>>> +             if (!cached && !cmpxchg(&it->timeline, 0, idx)) {
> >>>>> +                     GEM_BUG_ON(i915_active_fence_isset(&it->base));
> >>>>> +                     return it;
> >>>>
> >>>> cpmxchg suggests this needs to be atomic, however above the check for
> >>>> equality comes from a separate read.
> >>>
> >>> That's fine, and quite common to avoid cmpxchg if the current value
> >>> already does not match the expected condition.
> >>
> >> How? What is another thread is about to install its idx into
> >> it->timeline with cmpxchg and this thread does not see it because it
> >> just returned on the "cached == idx" condition.
> > 
> > Because it's nonzero.
> > 
> > If the idx is already non-zero, it will always remain non-zero until
> > everybody idles (and there are no more threads).
> > 
> > If the idx is zero, it can only transition to non-zero once, atomically
> > via cmpxchg. The first and only first cmpxchg will return that the
> > previous value was 0, and so return with it->idx == idx.
> 
> I think this is worthy of a comment to avoid future reader having to 
> re-figure it all out.

        if (it) {
                u64 cached = READ_ONCE(it->timeline);

+               /* Once claimed, this slot will only belong to this idx */
                if (cached == idx)
                        return it;

 #ifdef CONFIG_64BIT /* for cmpxchg(u64) */
+               /*
+                * An unclaimed cache [.timeline=0] can only be claimed once.
+                *
+                * If the value is already non-zero, some other thread has
+                * claimed the cache and we know that is does not match our
+                * idx. If, and only if, the timeline is currently zero is it
+                * worth competing to claim it atomically for ourselves (for
+		 * only the winner of that race will cmpxchg return the old
+		 * value of 0).
+                */
                if (!cached && !cmpxchg(&it->timeline, 0, idx))
                        return it;
 #endif