Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1534118 > unrolled thread

[PATCH v2 04/11] locking/ww_mutex: Set use_ww_ctx even when locking without a context

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 04/11] locking/ww_mutex: Set use_ww_ctx even when locking without a context Nicolai Hähnle <nhaehnle@gmail.com> - 2016-12-01 15:10 +0100
    Re: [PATCH v2 04/11] locking/ww_mutex: Set use_ww_ctx even when  locking without a context Peter Zijlstra <peterz@infradead.org> - 2016-12-06 16:20 +0100
    Re: [PATCH v2 04/11] locking/ww_mutex: Set use_ww_ctx even when  locking without a context Peter Zijlstra <peterz@infradead.org> - 2016-12-06 16:30 +0100

#1534118 — [PATCH v2 04/11] locking/ww_mutex: Set use_ww_ctx even when locking without a context

From: Nicolai Hähnle <Nicolai.Haehnle@amd.com>

We will add a new field to struct mutex_waiter.  This field must be
initialized for all waiters if any waiter uses the ww_use_ctx path.

So there is a trade-off: Keep ww_mutex locking without a context on the
faster non-use_ww_ctx path, at the cost of adding the initialization to all
mutex locks (including non-ww_mutexes), or avoid the additional cost for
non-ww_mutex locks, at the cost of adding additional checks to the
use_ww_ctx path.

We take the latter choice.  It may be worth eliminating the users of
ww_mutex_lock(lock, NULL), but there are a lot of them.

Move the declaration of ww in __mutex_lock_common closer to its uses
because gcc otherwise (incorrectly) believes ww may be used without
initialization.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Maarten Lankhorst <dev@mblankhorst.nl>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Nicolai Hähnle <Nicolai.Haehnle@amd.com>
---
 include/linux/ww_mutex.h | 11 ++---------
 kernel/locking/mutex.c   | 37 +++++++++++++++++++++++++------------
 2 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
index 2bb5deb..a5960e5 100644
--- a/include/linux/ww_mutex.h
+++ b/include/linux/ww_mutex.h
@@ -222,11 +222,7 @@ extern int __must_check __ww_mutex_lock_interruptible(struct ww_mutex *lock,
  */
 static inline int ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
 {
-	if (ctx)
-		return __ww_mutex_lock(lock, ctx);
-
-	mutex_lock(&lock->base);
-	return 0;
+	return __ww_mutex_lock(lock, ctx);
 }
 
 /**
@@ -262,10 +258,7 @@ static inline int ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ct
 static inline int __must_check ww_mutex_lock_interruptible(struct ww_mutex *lock,
 							   struct ww_acquire_ctx *ctx)
 {
-	if (ctx)
-		return __ww_mutex_lock_interruptible(lock, ctx);
-	else
-		return mutex_lock_interruptible(&lock->base);
+	return __ww_mutex_lock_interruptible(lock, ctx);
 }
 
 /**
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 200629a..585627f 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -381,7 +381,7 @@ bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner,
 			break;
 		}
 
-		if (use_ww_ctx && ww_ctx->acquired > 0) {
+		if (use_ww_ctx && ww_ctx && ww_ctx->acquired > 0) {
 			struct ww_mutex *ww;
 
 			ww = container_of(lock, struct ww_mutex, base);
@@ -640,10 +640,11 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 	struct mutex_waiter waiter;
 	unsigned long flags;
 	bool first = false;
-	struct ww_mutex *ww;
 	int ret;
 
-	if (use_ww_ctx) {
+	if (use_ww_ctx && ww_ctx) {
+		struct ww_mutex *ww;
+
 		ww = container_of(lock, struct ww_mutex, base);
 		if (unlikely(ww_ctx == READ_ONCE(ww->ctx)))
 			return -EALREADY;
@@ -656,8 +657,12 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 	    mutex_optimistic_spin(lock, ww_ctx, use_ww_ctx, false)) {
 		/* got the lock, yay! */
 		lock_acquired(&lock->dep_map, ip);
-		if (use_ww_ctx)
+		if (use_ww_ctx && ww_ctx) {
+			struct ww_mutex *ww;
+
+			ww = container_of(lock, struct ww_mutex, base);
 			ww_mutex_set_context_fastpath(ww, ww_ctx);
+		}
 		preempt_enable();
 		return 0;
 	}
@@ -702,7 +707,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 			goto err;
 		}
 
-		if (use_ww_ctx && ww_ctx->acquired > 0) {
+		if (use_ww_ctx && ww_ctx && ww_ctx->acquired > 0) {
 			ret = __ww_mutex_lock_check_stamp(lock, ww_ctx);
 			if (ret)
 				goto err;
@@ -742,8 +747,12 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 	/* got the lock - cleanup and rejoice! */
 	lock_acquired(&lock->dep_map, ip);
 
-	if (use_ww_ctx)
+	if (use_ww_ctx && ww_ctx) {
+		struct ww_mutex *ww;
+
+		ww = container_of(lock, struct ww_mutex, base);
 		ww_mutex_set_context_slowpath(ww, ww_ctx);
+	}
 
 	spin_unlock_mutex(&lock->wait_lock, flags);
 	preempt_enable();
@@ -830,8 +839,9 @@ __ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
 
 	might_sleep();
 	ret =  __mutex_lock_common(&lock->base, TASK_UNINTERRUPTIBLE,
-				   0, &ctx->dep_map, _RET_IP_, ctx, 1);
-	if (!ret && ctx->acquired > 1)
+				   0, ctx ? &ctx->dep_map : NULL, _RET_IP_,
+				   ctx, 1);
+	if (!ret && ctx && ctx->acquired > 1)
 		return ww_mutex_deadlock_injection(lock, ctx);
 
 	return ret;
@@ -845,9 +855,10 @@ __ww_mutex_lock_interruptible(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
 
 	might_sleep();
 	ret = __mutex_lock_common(&lock->base, TASK_INTERRUPTIBLE,
-				  0, &ctx->dep_map, _RET_IP_, ctx, 1);
+				  0, ctx ? &ctx->dep_map : NULL, _RET_IP_,
+				  ctx, 1);
 
-	if (!ret && ctx->acquired > 1)
+	if (!ret && ctx && ctx->acquired > 1)
 		return ww_mutex_deadlock_injection(lock, ctx);
 
 	return ret;
@@ -1034,7 +1045,8 @@ __ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
 	might_sleep();
 
 	if (__mutex_trylock_fast(&lock->base)) {
-		ww_mutex_set_context_fastpath(lock, ctx);
+		if (ctx)
+			ww_mutex_set_context_fastpath(lock, ctx);
 		return 0;
 	}
 
@@ -1048,7 +1060,8 @@ __ww_mutex_lock_interruptible(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
 	might_sleep();
 
 	if (__mutex_trylock_fast(&lock->base)) {
-		ww_mutex_set_context_fastpath(lock, ctx);
+		if (ctx)
+			ww_mutex_set_context_fastpath(lock, ctx);
 		return 0;
 	}
 
-- 
2.7.4

[toc] | [next] | [standalone]

#1537034 — Re: [PATCH v2 04/11] locking/ww_mutex: Set use_ww_ctx even when locking without a context

On Thu, Dec 01, 2016 at 03:06:47PM +0100, Nicolai Hähnle wrote:
> +++ b/include/linux/ww_mutex.h
> @@ -222,11 +222,7 @@ extern int __must_check __ww_mutex_lock_interruptible(struct ww_mutex *lock,
>   */
>  static inline int ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
>  {
> -	if (ctx)
> -		return __ww_mutex_lock(lock, ctx);
> -
> -	mutex_lock(&lock->base);
> -	return 0;
> +	return __ww_mutex_lock(lock, ctx);
>  }
>  
>  /**
> @@ -262,10 +258,7 @@ static inline int ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ct
>  static inline int __must_check ww_mutex_lock_interruptible(struct ww_mutex *lock,
>  							   struct ww_acquire_ctx *ctx)
>  {
> -	if (ctx)
> -		return __ww_mutex_lock_interruptible(lock, ctx);
> -	else
> -		return mutex_lock_interruptible(&lock->base);
> +	return __ww_mutex_lock_interruptible(lock, ctx);
>  }
>  

After this the entire point of __ww_mutex_lock*() is gone, right? Might
as well rename them to ww_mutex_lock() and remove this pointless
wrapper.

[toc] | [prev] | [next] | [standalone]

#1537040 — Re: [PATCH v2 04/11] locking/ww_mutex: Set use_ww_ctx even when locking without a context

On Thu, Dec 01, 2016 at 03:06:47PM +0100, Nicolai Hähnle wrote:

> @@ -640,10 +640,11 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>  	struct mutex_waiter waiter;
>  	unsigned long flags;
>  	bool first = false;
> -	struct ww_mutex *ww;
>  	int ret;
>  
> -	if (use_ww_ctx) {
> +	if (use_ww_ctx && ww_ctx) {
> +		struct ww_mutex *ww;
> +
>  		ww = container_of(lock, struct ww_mutex, base);
>  		if (unlikely(ww_ctx == READ_ONCE(ww->ctx)))
>  			return -EALREADY;

So I don't see the point of removing *ww from the function scope, we can
still compute that container_of() even if !ww_ctx, right? That would
safe a ton of churn below, adding all those struct ww_mutex declarations
and container_of() casts.

(and note that the container_of() is a fancy NO-OP because base is the
first member).

> @@ -656,8 +657,12 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>  	    mutex_optimistic_spin(lock, ww_ctx, use_ww_ctx, false)) {
>  		/* got the lock, yay! */
>  		lock_acquired(&lock->dep_map, ip);
> -		if (use_ww_ctx)
> +		if (use_ww_ctx && ww_ctx) {
> +			struct ww_mutex *ww;
> +
> +			ww = container_of(lock, struct ww_mutex, base);
>  			ww_mutex_set_context_fastpath(ww, ww_ctx);
> +		}
>  		preempt_enable();
>  		return 0;
>  	}
> @@ -702,7 +707,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>  			goto err;
>  		}
>  
> -		if (use_ww_ctx && ww_ctx->acquired > 0) {
> +		if (use_ww_ctx && ww_ctx && ww_ctx->acquired > 0) {
>  			ret = __ww_mutex_lock_check_stamp(lock, ww_ctx);
>  			if (ret)
>  				goto err;
> @@ -742,8 +747,12 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>  	/* got the lock - cleanup and rejoice! */
>  	lock_acquired(&lock->dep_map, ip);
>  
> -	if (use_ww_ctx)
> +	if (use_ww_ctx && ww_ctx) {
> +		struct ww_mutex *ww;
> +
> +		ww = container_of(lock, struct ww_mutex, base);
>  		ww_mutex_set_context_slowpath(ww, ww_ctx);
> +	}
>  
>  	spin_unlock_mutex(&lock->wait_lock, flags);
>  	preempt_enable();

All that then reverts to:

-	if (use_ww_ctx)
+	if (use_ww_ctx && ww_ctx)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web