diff mbox

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

Message ID 1480601214-26583-5-git-send-email-nhaehnle@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nicolai Hähnle Dec. 1, 2016, 2:06 p.m. UTC 
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(-)

Comments

Peter Zijlstra Dec. 6, 2016, 3:14 p.m. UTC | #1 
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.
Peter Zijlstra Dec. 6, 2016, 3:25 p.m. UTC | #2 
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)
Nicolai Hähnle Dec. 16, 2016, 1:17 p.m. UTC | #3 
On 06.12.2016 16:25, Peter Zijlstra wrote:
> 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).

Sorry for taking so long to get back to you.

In my experience, the undefined behavior sanitizer in GCC for userspace 
programs complains about merely casting a pointer to the wrong type. I 
never went into the standards rabbit hole to figure out the details. It 
might be a C++ only thing (ubsan cannot tell the difference otherwise 
anyway), but that was the reason for doing the change in this more 
complicated way.

Are you sure that this is defined behavior in C? If so, I'd be happy to 
go with the version that has less churn.

I'll also get rid of those ww_mutex_lock* wrapper functions.

Thanks,
Nicolai

>
>> @@ -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)
>
>
Maarten Lankhorst Dec. 17, 2016, 7:53 a.m. UTC | #4 
Op 16-12-16 om 14:17 schreef Nicolai Hähnle:
> On 06.12.2016 16:25, Peter Zijlstra wrote:
>> 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).
>
> Sorry for taking so long to get back to you.
>
> In my experience, the undefined behavior sanitizer in GCC for userspace programs complains about merely casting a pointer to the wrong type. I never went into the standards rabbit hole to figure out the details. It might be a C++ only thing (ubsan cannot tell the difference otherwise anyway), but that was the reason for doing the change in this more complicated way.
>
> Are you sure that this is defined behavior in C? If so, I'd be happy to go with the version that has less churn.
>
> I'll also get rid of those ww_mutex_lock* wrapper functions. 

ww_ctx = use_ww_ctx ? container_of : NULL ?
Peter Zijlstra Dec. 17, 2016, 1:49 p.m. UTC | #5 
On Fri, Dec 16, 2016 at 02:17:25PM +0100, Nicolai Hähnle wrote:
> On 06.12.2016 16:25, Peter Zijlstra wrote:
> >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).
> 
> Sorry for taking so long to get back to you.
> 
> In my experience, the undefined behavior sanitizer in GCC for userspace
> programs complains about merely casting a pointer to the wrong type. I never
> went into the standards rabbit hole to figure out the details. It might be a
> C++ only thing (ubsan cannot tell the difference otherwise anyway), but that
> was the reason for doing the change in this more complicated way.

Note that C only has what C++ calls reinterpret_cast<>(). It cannot
complain about a 'wrong' cast, there is no such thing.

Also, container_of() works, irrespective of what C language says about
it -- note that the kernel in general hard relies on a lot of things C
calls undefined behaviour.

> Are you sure that this is defined behavior in C? If so, I'd be happy to go
> with the version that has less churn.

It should very much work with kernel C.
diff mbox

Patch

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;
 	}