linux-kernel - Re: [PATCH] mutex: Do not spin/queue before performing ww

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f5144094-fceb-5d0d-a852-a326a59559fc@linux.intel.com>
Date:	Thu, 26 May 2016 12:37:30 +0200
From:	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
To:	Chris Wilson <chris@...is-wilson.co.uk>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>
Cc:	intel-gfx@...ts.freedesktop.org,
	Christian König <christian.koenig@....com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mutex: Do not spin/queue before performing ww_mutex
 deadlock avoidance

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@...is-wilson.co.uk>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Christian König <christian.koenig@....com>
> Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
> Cc: linux-kernel@...r.kernel.org
> ---
>  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