On 29/07/2020 15:52, Chris Wilson wrote:
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
Sounds good. With that:
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
