Re: [PATCH] mutex: Do not spin/queue before performing ww_mutex deadlock avoidance

[Maarten Lankhorst]

Op 26-05-16 om 10:31 schreef Chris Wilson:
> The ww_mutex has the property of allowing the lock to detect and report
> when it may be used in deadlocking scenarios (to allow the caller to
> unwind its locks and avoid the deadlock). This detection needs to be
> performed before we queue up for the spin, otherwise we wait on the
> osq_lock() for our turn to detect the deadlock that another thread is
> spinning on, waiting for us. Otherwise as we are stuck behind our waiter,
> throughput plummets.
>
> This can be demonstrated by trying concurrent atomic modesets.
>
> Testcase: igt/kms_cursor_legacy
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Christian KÃnig <christian.koenig@xxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
>  kernel/locking/mutex.c | 56 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index e364b424b019..d60f1ba3e64f 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -217,12 +217,35 @@ ww_mutex_set_context_slowpath(struct ww_mutex *lock,
>  }
>  
>  #ifdef CONFIG_MUTEX_SPIN_ON_OWNER
> +static bool ww_mutex_may_deadlock(struct mutex *lock,
> +				  struct ww_acquire_ctx *ww_ctx)
> +{
> +	if (ww_ctx && ww_ctx->acquired > 0) {
> +		struct ww_mutex *ww;
> +
> +		ww = container_of(lock, struct ww_mutex, base);
> +		/*
> +		 * If ww->ctx is set the contents are undefined, only
> +		 * by acquiring wait_lock there is a guarantee that
> +		 * they are not invalid when reading.
> +		 *
> +		 * As such, when deadlock detection needs to be
> +		 * performed the optimistic spinning cannot be done.
> +		 */
> +		if (READ_ONCE(ww->ctx))
> +			return true;
> +	}
> +
> +	return false;
> +}
The check should be at the beginning of __mutex_lock_common,
regardless of spin_on_owner.

This is because -EALREADY was originally designed to be exceptional,
but is used a lot by design in drm/atomic now.

The other check for -EALREADY can be killed, or changed to a
DEBUG_LOCKS_WARN_ON.

The check should also not be for NULL, but for use_ww_ctx.
This way the if check is optimized out for the ww_ctx path, where
ww_ctx is always non-null.

This would also be something for Cc: stable. :)

~Maarten