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

[Tvrtko Ursulin]

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 at intel.com>

Regards,

Tvrtko