[PATCH] mutex: Fix mutex_can_spin_on_owner

[PATCH] mutex: Fix mutex_can_spin_on_owner

[Peter Zijlstra]

mutex_can_spin_on_owner() is broken in that it would allow the compiler
to load lock->owner twice, seeing a pointer first time and a MULL
pointer the second time.

Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 kernel/mutex.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/mutex.c b/kernel/mutex.c
index ff05f4b..7ff48c5 100644
--- a/kernel/mutex.c
+++ b/kernel/mutex.c
@@ -209,11 +209,13 @@ int mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner)
  */
 static inline int mutex_can_spin_on_owner(struct mutex *lock)
 {
+	struct task_struct *owner;
 	int retval = 1;
 
 	rcu_read_lock();
-	if (lock->owner)
-		retval = lock->owner->on_cpu;
+	owner = ACCESS_ONCE(lock->owner);
+	if (owner)
+		retval = owner->on_cpu;
 	rcu_read_unlock();
 	/*
 	 * if lock->owner is not set, the mutex owner may have just acquired

Re: [PATCH] mutex: Fix mutex_can_spin_on_owner

[Davidlohr Bueso]

On Fri, 2013-07-19 at 20:31 +0200, Peter Zijlstra wrote:
> mutex_can_spin_on_owner() is broken in that it would allow the compiler
> to load lock->owner twice, seeing a pointer first time and a MULL
> pointer the second time.
> 
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>

Yep, I remember this from the rwsem optimistic spinning thread.

Acked-by: Davidlohr Bueso <davidlohr.bueso@hp.com>

> ---
>  kernel/mutex.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/mutex.c b/kernel/mutex.c
> index ff05f4b..7ff48c5 100644
> --- a/kernel/mutex.c
> +++ b/kernel/mutex.c
> @@ -209,11 +209,13 @@ int mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner)
>   */
>  static inline int mutex_can_spin_on_owner(struct mutex *lock)
>  {
> +	struct task_struct *owner;
>  	int retval = 1;
>  
>  	rcu_read_lock();
> -	if (lock->owner)
> -		retval = lock->owner->on_cpu;
> +	owner = ACCESS_ONCE(lock->owner);
> +	if (owner)
> +		retval = owner->on_cpu;
>  	rcu_read_unlock();
>  	/*
>  	 * if lock->owner is not set, the mutex owner may have just acquired

Re: [PATCH] mutex: Fix mutex_can_spin_on_owner

[Waiman Long]

On 07/19/2013 02:31 PM, Peter Zijlstra wrote:
> mutex_can_spin_on_owner() is broken in that it would allow the compiler
> to load lock->owner twice, seeing a pointer first time and a MULL
> pointer the second time.
>
> Signed-off-by: Peter Zijlstra<peterz@infradead.org>
> ---
>   kernel/mutex.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/mutex.c b/kernel/mutex.c
> index ff05f4b..7ff48c5 100644
> --- a/kernel/mutex.c
> +++ b/kernel/mutex.c
> @@ -209,11 +209,13 @@ int mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner)
>    */
>   static inline int mutex_can_spin_on_owner(struct mutex *lock)
>   {
> +	struct task_struct *owner;
>   	int retval = 1;
>
>   	rcu_read_lock();
> -	if (lock->owner)
> -		retval = lock->owner->on_cpu;
> +	owner = ACCESS_ONCE(lock->owner);
> +	if (owner)
> +		retval = owner->on_cpu;
>   	rcu_read_unlock();
>   	/*
>   	 * if lock->owner is not set, the mutex owner may have just acquired

I am fine with this change. However, the compiler is smart enough to not 
do two memory accesses to the same memory location. So this will not 
change the generated code. Below is the relevant x86 code for that 
section of code:

    0x00000000000005d2 <+34>:    mov    0x18(%rdi),%rdx
    0x00000000000005d6 <+38>:    mov    $0x1,%eax
    0x00000000000005db <+43>:    test   %rdx,%rdx
    0x00000000000005de <+46>:    je     0x5e3 <__mutex_lock_slowpath+51>
    0x00000000000005e0 <+48>:    mov    0x28(%rdx),%eax
    0x00000000000005e3 <+51>:    test   %eax,%eax
    0x00000000000005e5 <+53>:    je     0x6d3 <__mutex_lock_slowpath+291>

Only one memory access is done.

Ack-by: Waiman Long <Waiman.Long@hp.com>

Re: [PATCH] mutex: Fix mutex_can_spin_on_owner

[Rik van Riel]

On 07/19/2013 02:31 PM, Peter Zijlstra wrote:
>
> mutex_can_spin_on_owner() is broken in that it would allow the compiler
> to load lock->owner twice, seeing a pointer first time and a MULL
> pointer the second time.
>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>

Reviewed-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

Re: [PATCH] mutex: Fix mutex_can_spin_on_owner

[Thomas Gleixner]

On Fri, 19 Jul 2013, Waiman Long wrote:
> On 07/19/2013 02:31 PM, Peter Zijlstra wrote:
> >   	rcu_read_lock();
> > -	if (lock->owner)
> > -		retval = lock->owner->on_cpu;
> > +	owner = ACCESS_ONCE(lock->owner);
> > +	if (owner)
> > +		retval = owner->on_cpu;
> >   	rcu_read_unlock();
> >   	/*
> >   	 * if lock->owner is not set, the mutex owner may have just
> > acquired
> 

> I am fine with this change. However, the compiler is smart enough to
> not do two memory accesses to the same memory location. So this will
> not change the generated code. Below is the relevant x86 code for
> that section of code:

That's true for your particular compiler, but it's not guaranteed at
all. So it matters even when your compiler generates the same
code. Others might not. There is a world outside of x8664.
 
Thanks,

	tglx

Re: [PATCH] mutex: Fix mutex_can_spin_on_owner

[Linus Torvalds]

On Fri, Jul 19, 2013 at 12:41 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> That's true for your particular compiler, but it's not guaranteed at
> all. So it matters even when your compiler generates the same
> code. Others might not. There is a world outside of x8664.

Well, in this case, I think we can pretty safely say that a compiler
has to be seriously buggered to not just doit as a single load. If
there are reloads in that sequence, the compiler is past bad.

That said, I'm absolutely not arguing against the fix, and it needs to
be done. Not so much because of "a compiler might screw it up" as
simply because it's (a) the right thing to do and (b) documentation
about the fact that we're doing special things that aren't really
locked.

In particular, I could imagine us some day checking that ACCESS_ONCE()
accesses are never cacheline crossers or multiword accesses, neither
of which may be atomic. A 64-bit ACCESS_ONCE on a 32-bit machine would
be a bug, for example. Not that I think we do that, but the point is
that ACCESS_ONCE() is about more than just compiler code generation.
It's documentation, and it's a possible venue for sanity-checking.

               Linus

Re: [PATCH] mutex: Fix mutex_can_spin_on_owner

[Waiman Long]

On 07/19/2013 03:41 PM, Thomas Gleixner wrote:
> On Fri, 19 Jul 2013, Waiman Long wrote:
>> On 07/19/2013 02:31 PM, Peter Zijlstra wrote:
>>>    	rcu_read_lock();
>>> -	if (lock->owner)
>>> -		retval = lock->owner->on_cpu;
>>> +	owner = ACCESS_ONCE(lock->owner);
>>> +	if (owner)
>>> +		retval = owner->on_cpu;
>>>    	rcu_read_unlock();
>>>    	/*
>>>    	 * if lock->owner is not set, the mutex owner may have just
>>> acquired
>> I am fine with this change. However, the compiler is smart enough to
>> not do two memory accesses to the same memory location. So this will
>> not change the generated code. Below is the relevant x86 code for
>> that section of code:
> That's true for your particular compiler, but it's not guaranteed at
> all. So it matters even when your compiler generates the same
> code. Others might not. There is a world outside of x8664.
>
> Thanks,
>
> 	tglx

I supposed that only the gcc compiler can be used to build Linux kernel 
as the kernel source uses a lot of features specific to gcc. 
Optimizations like these are done by the front end of the compiler which 
should be universal across all the architecture. So what I want to say 
is that there is nothing specific to x86-64 or any architecture here. 
This is what a good compiler should do.

I am not against the fix as it makes the intention more clear. I am just 
saying that there won't be any performance change because of this.

Regards,
Longman

Re: [PATCH] mutex: Fix mutex_can_spin_on_owner

[Peter Zijlstra]

On Fri, Jul 19, 2013 at 03:08:36PM -0400, Waiman Long wrote:
> On 07/19/2013 02:31 PM, Peter Zijlstra wrote:
> >mutex_can_spin_on_owner() is broken in that it would allow the compiler
> >to load lock->owner twice, seeing a pointer first time and a MULL
> >pointer the second time.
> >
> >Signed-off-by: Peter Zijlstra<peterz@infradead.org>
> >---
> >  kernel/mutex.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> >diff --git a/kernel/mutex.c b/kernel/mutex.c
> >index ff05f4b..7ff48c5 100644
> >--- a/kernel/mutex.c
> >+++ b/kernel/mutex.c
> >@@ -209,11 +209,13 @@ int mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner)
> >   */
> >  static inline int mutex_can_spin_on_owner(struct mutex *lock)
> >  {
> >+	struct task_struct *owner;
> >  	int retval = 1;
> >
> >  	rcu_read_lock();
> >-	if (lock->owner)
> >-		retval = lock->owner->on_cpu;
> >+	owner = ACCESS_ONCE(lock->owner);
> >+	if (owner)
> >+		retval = owner->on_cpu;
> >  	rcu_read_unlock();
> >  	/*
> >  	 * if lock->owner is not set, the mutex owner may have just acquired
> 
> I am fine with this change. However, the compiler is smart enough to not do
> two memory accesses to the same memory location. So this will not change the
> generated code. Below is the relevant x86 code for that section of code:

Yes I know, but the compiler would be allowed to do so; not so after the
change.

Also, GCC can be surprisingly stupid at times, depending on the options
given, never rely/trust on anything you don't have to.

[tip:core/locking] mutex: Fix/ document access-once assumption in mutex_can_spin_on_owner()

[tip-bot for Peter Zijlstra]

Commit-ID:  1e40c2edef2537f87f94d0baf80aeaeb7d51cc23
Gitweb:     http://git.kernel.org/tip/1e40c2edef2537f87f94d0baf80aeaeb7d51cc23
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Fri, 19 Jul 2013 20:31:01 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 22 Jul 2013 10:33:39 +0200

mutex: Fix/document access-once assumption in mutex_can_spin_on_owner()

mutex_can_spin_on_owner() is technically broken in that it would
in theory allow the compiler to load lock->owner twice, seeing a
pointer first time and a NULL pointer the second time.

Linus pointed out that a compiler has to be seriously broken to
not compile this correctly - but nevertheless this change
is correct as it will better document the implementation.

Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Acked-by: Davidlohr Bueso <davidlohr.bueso@hp.com>
Acked-by: Waiman Long <Waiman.Long@hp.com>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Rik van Riel <riel@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Link: http://lkml.kernel.org/r/20130719183101.GA20909@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/mutex.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/mutex.c b/kernel/mutex.c
index ff05f4b..7ff48c5 100644
--- a/kernel/mutex.c
+++ b/kernel/mutex.c
@@ -209,11 +209,13 @@ int mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner)
  */
 static inline int mutex_can_spin_on_owner(struct mutex *lock)
 {
+	struct task_struct *owner;
 	int retval = 1;
 
 	rcu_read_lock();
-	if (lock->owner)
-		retval = lock->owner->on_cpu;
+	owner = ACCESS_ONCE(lock->owner);
+	if (owner)
+		retval = owner->on_cpu;
 	rcu_read_unlock();
 	/*
 	 * if lock->owner is not set, the mutex owner may have just acquired

Re: [PATCH] mutex: Fix mutex_can_spin_on_owner

[Jan-Simon Möller]

On Friday 19 July 2013 16:58:09 Waiman Long wrote:
> 
> I supposed that only the gcc compiler can be used to build Linux kernel
> as the kernel source uses a lot of features specific to gcc.

Look at  http://llvm.linuxfoundation.org  -  they're using clang with more and 
more success.

--
JS