mutex: Fix mutex_can_spin_on_owner
PATCH 1/1 mutex: Fix mutex_can_spin_on_owner
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.
--- kernel/mutex.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
kernel/mutex.c +4 -2
@@ -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
Quoted 5 lines · Peter Zijlstra
mutex_can_spin_on_owner()is broken in that it would allow the compiler to loadlock->ownertwice, seeing a pointer first time and a MULL pointer the second time.Signed-off-by Peter Zijlstra <[email protected]>
Yep, I remember this from the rwsem optimistic spinning thread.
Quoted 24 lines
--- kernel/mutex.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)kernel/mutex.c +4
-2@@ -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
Quoted 19 lines · Peter Zijlstra
mutex_can_spin_on_owner()is broken in that it would allow the compiler to loadlock->ownertwice, seeing a pointer first time and a MULL pointer the second time.Signed-off-by Peter Zijlstra<[email protected]>--- kernel/mutex.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)kernel/mutex.c +1
-0@@ -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 ;
kernel/mutex.c int mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner) · line 221
Quoted 10 lines
kernel/mutex.c +3
-2rcu_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.
Quoted 17 lines · Waiman Long
Quoted 10 lines · Peter Zijlstra
rcu_read_lock();kernel/mutex.c +3
-2-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
Quoted 4 lines · Thomas Gleixner
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
Quoted 16 lines · Thomas Gleixner
Quoted 15 lines · Waiman Long
Quoted 10 lines · Peter Zijlstra
rcu_read_lock();kernel/mutex.c +3
-2-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:
Quoted 7 lines
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
Quoted 3 lines · 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
Quoted 21 lines · Waiman Long
Quoted 20 lines · Peter Zijlstra
mutex_can_spin_on_owner()is broken in that it would allow the compiler to loadlock->ownertwice, seeing a pointer first time and a MULL pointer the second time.Signed-off-by Peter Zijlstra<[email protected]>--- kernel/mutex.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)kernel/mutex.c +1
-0@@ -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 ;
Quoted 13 lines
kernel/mutex.c int mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner) · line 221
Quoted 9 lines
kernel/mutex.c +3
-2rcu_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.
Quoted 6 lines · Peter Zijlstra
mutex_can_spin_on_owner()is broken in that it would allow the compiler to loadlock->ownertwice, seeing a pointer first time and a MULL pointer the second time.Signed-off-by Peter Zijlstra <[email protected]>
Signature · 1 line
All rights reversed
tip-bot for Peter Zijlstra
merge bot[tip:core/locking] mutex: Fix/ document access-once assumption in mutex_can_spin_on_owner()
In reply to Peter Zijlstramutex: Fix/document access-once assumption in mutex_can_spin_on_owner()
Commit-ID: 1e40c2edef2537f87f94d0baf80aeaeb7d51cc23 Gitweb: http://git.kernel.org/tip/1e40c2edef2537f87f94d0baf80aeaeb7d51cc23 Author: Peter Zijlstra <[email protected]> AuthorDate: Fri, 19 Jul 2013 20:31:01 +0200 Committer: Ingo Molnar <[email protected]> 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.
--- kernel/mutex.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
kernel/mutex.c +4 -2
@@ -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