[Intel-gfx] [PATCH 3/6] drm/i915: Tighten atomicity of i915_active_acquire vs i915_active_release

[Tvrtko Ursulin]

On 26/01/2020 10:23, Chris Wilson wrote:
> As we use a mutex to serialise the first acquire (as it may be a lengthy
> operation), but only an atomic decrement for the release, we have to
> be careful in case a second thread races and completes both
> acquire/release as the first finishes its acquire.
> 
> Thread A			Thread B
> i915_active_acquire		i915_active_acquire
>    atomic_read() == 0		  atomic_read() == 0
>    mutex_lock()			  mutex_lock()
> 				  atomic_read() == 0
> 				    ref->active();
> 				  atomic_inc()
> 				  mutex_unlock()
>    atomic_read() == 1
> 				i915_active_release
> 				  atomic_dec_and_test() -> 0
> 				    ref->retire()
>    atomic_inc() -> 1
>    mutex_unlock()
> 
> So thread A has acquired the ref->active_count but since the ref was
> still active at the time, it did not initialise it. By switching the
> check inside the mutex to an atomic increment only if already active, we
> close the race.
> 
> Fixes: c9ad602feabe ("drm/i915: Split i915_active.mutex into an irq-safe spinlock for the rbtree")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_active.c | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index ace55d5d4ca7..9d6830885d2e 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -416,13 +416,15 @@ int i915_active_acquire(struct i915_active *ref)
>   	if (err)
>   		return err;
>   
> -	if (!atomic_read(&ref->count) && ref->active)
> -		err = ref->active(ref);
> -	if (!err) {
> -		spin_lock_irq(&ref->tree_lock); /* vs __active_retire() */
> -		debug_active_activate(ref);
> -		atomic_inc(&ref->count);
> -		spin_unlock_irq(&ref->tree_lock);
> +	if (likely(!i915_active_acquire_if_busy(ref))) {
> +		if (ref->active)
> +			err = ref->active(ref);
> +		if (!err) {
> +			spin_lock_irq(&ref->tree_lock); /* __active_retire() */
> +			debug_active_activate(ref);
> +			atomic_inc(&ref->count);
> +			spin_unlock_irq(&ref->tree_lock);
> +		}
>   	}
>   
>   	mutex_unlock(&ref->mutex);
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko