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 <[email protected]>
---
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
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 <[email protected]>
Yep, I remember this from the rwsem optimistic spinning thread.
Acked-by: Davidlohr Bueso <[email protected]>
> ---
> 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
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<[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
Reviewed-by: Rik van Riel <[email protected]>
--
All rights reversed
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
On Fri, Jul 19, 2013 at 12:41 PM, Thomas Gleixner <[email protected]> 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
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
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<[email protected]>
> >---
> > 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.
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.
Signed-off-by: Peter Zijlstra <[email protected]>
Acked-by: Davidlohr Bueso <[email protected]>
Acked-by: Waiman Long <[email protected]>
Acked-by: Linus Torvalds <[email protected]>
Acked-by: Thomas Gleixner <[email protected]>
Acked-by: Rik van Riel <[email protected]>
Cc: Paul E. McKenney <[email protected]>
Cc: David Howells <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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
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