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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
