* [PATCH] mutex: Introduce arch_mutex_cpu_relax()
@ 2010-11-22 14:47 Gerald Schaefer
2010-11-22 20:10 ` Andrew Morton
2010-11-26 15:02 ` [tip:sched/core] mutexes, sched: " tip-bot for Gerald Schaefer
0 siblings, 2 replies; 6+ messages in thread
From: Gerald Schaefer @ 2010-11-22 14:47 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Andrew Morton
Cc: Martin Schwidefsky, LKML, linux-s390, Heiko Carstens
From: Gerald Schaefer <gerald.schaefer@de.ibm.com>
The spinning mutex implementation uses cpu_relax() in busy loops as a
compiler barrier. Depending on the architecture, cpu_relax() may do more
than needed in this specific mutex spin loops. On System z we also give
up the time slice of the virtual cpu in cpu_relax(), which prevents
effective spinning on the mutex.
This patch replaces cpu_relax() in the spinning mutex code with
arch_mutex_cpu_relax(), which can be defined by each architecture that
selects HAVE_ARCH_MUTEX_CPU_RELAX. The default is still cpu_relax(), so
this patch should not affect other architectures than System z for now.
Signed-off-by: Gerald Schaefer <gerald.schaefer@de.ibm.com>
---
arch/Kconfig | 3 +++
arch/s390/Kconfig | 1 +
arch/s390/include/asm/mutex.h | 2 ++
include/linux/mutex.h | 4 ++++
kernel/mutex.c | 2 +-
kernel/sched.c | 3 ++-
6 files changed, 13 insertions(+), 2 deletions(-)
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -175,4 +175,7 @@ config HAVE_PERF_EVENTS_NMI
config HAVE_ARCH_JUMP_LABEL
bool
+config HAVE_ARCH_MUTEX_CPU_RELAX
+ bool
+
source "kernel/gcov/Kconfig"
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -99,6 +99,7 @@ config S390
select HAVE_KERNEL_LZMA
select HAVE_KERNEL_LZO
select HAVE_GET_USER_PAGES_FAST
+ select HAVE_ARCH_MUTEX_CPU_RELAX
select ARCH_INLINE_SPIN_TRYLOCK
select ARCH_INLINE_SPIN_TRYLOCK_BH
select ARCH_INLINE_SPIN_LOCK
--- a/arch/s390/include/asm/mutex.h
+++ b/arch/s390/include/asm/mutex.h
@@ -7,3 +7,5 @@
*/
#include <asm-generic/mutex-dec.h>
+
+#define arch_mutex_cpu_relax() barrier()
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -160,4 +160,8 @@ extern int mutex_trylock(struct mutex *l
extern void mutex_unlock(struct mutex *lock);
extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
+#ifndef CONFIG_HAVE_ARCH_MUTEX_CPU_RELAX
+#define arch_mutex_cpu_relax() cpu_relax()
+#endif
+
#endif
--- a/kernel/mutex.c
+++ b/kernel/mutex.c
@@ -199,7 +199,7 @@ __mutex_lock_common(struct mutex *lock,
* memory barriers as we'll eventually observe the right
* values at the cost of a few extra spins.
*/
- cpu_relax();
+ arch_mutex_cpu_relax();
}
#endif
spin_lock_mutex(&lock->wait_lock, flags);
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -75,6 +75,7 @@
#include <asm/tlb.h>
#include <asm/irq_regs.h>
+#include <asm/mutex.h>
#include "sched_cpupri.h"
#include "workqueue_sched.h"
@@ -4029,7 +4030,7 @@ int mutex_spin_on_owner(struct mutex *lo
if (task_thread_info(rq->curr) != owner || need_resched())
return 0;
- cpu_relax();
+ arch_mutex_cpu_relax();
}
return 1;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mutex: Introduce arch_mutex_cpu_relax()
2010-11-22 14:47 [PATCH] mutex: Introduce arch_mutex_cpu_relax() Gerald Schaefer
@ 2010-11-22 20:10 ` Andrew Morton
2010-11-23 14:12 ` Gerald Schaefer
2010-11-26 15:02 ` [tip:sched/core] mutexes, sched: " tip-bot for Gerald Schaefer
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2010-11-22 20:10 UTC (permalink / raw)
To: gerald.schaefer
Cc: Peter Zijlstra, Ingo Molnar, Martin Schwidefsky, LKML,
linux-s390, Heiko Carstens
On Mon, 22 Nov 2010 15:47:36 +0100
Gerald Schaefer <gerald.schaefer@de.ibm.com> wrote:
> From: Gerald Schaefer <gerald.schaefer@de.ibm.com>
>
> The spinning mutex implementation uses cpu_relax() in busy loops as a
> compiler barrier. Depending on the architecture, cpu_relax() may do more
> than needed in this specific mutex spin loops. On System z we also give
> up the time slice of the virtual cpu in cpu_relax(), which prevents
> effective spinning on the mutex.
>
> This patch replaces cpu_relax() in the spinning mutex code with
> arch_mutex_cpu_relax(), which can be defined by each architecture that
> selects HAVE_ARCH_MUTEX_CPU_RELAX. The default is still cpu_relax(), so
> this patch should not affect other architectures than System z for now.
>
> ...
>
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -160,4 +160,8 @@ extern int mutex_trylock(struct mutex *l
> extern void mutex_unlock(struct mutex *lock);
> extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
>
> +#ifndef CONFIG_HAVE_ARCH_MUTEX_CPU_RELAX
> +#define arch_mutex_cpu_relax() cpu_relax()
> +#endif
A simpler way of doing this is to remove the CONFIG_ variable
altogether and do
#ifndef arch_mutex_cpu_relax
#define arch_mutex_cpu_relax() cpu_relax()
#endif
When doing this, one should be clear about _which_ arch file has the
responsibility of defining arch_mutex_cpu_relax, and make sure that
this arch file is reliably included in the .c file.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mutex: Introduce arch_mutex_cpu_relax()
2010-11-22 20:10 ` Andrew Morton
@ 2010-11-23 14:12 ` Gerald Schaefer
2010-11-23 14:20 ` Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Gerald Schaefer @ 2010-11-23 14:12 UTC (permalink / raw)
To: Andrew Morton
Cc: Peter Zijlstra, Ingo Molnar, Martin Schwidefsky, LKML,
linux-s390, Heiko Carstens
On Mo, 2010-11-22 at 12:10 -0800, Andrew Morton wrote:
> On Mon, 22 Nov 2010 15:47:36 +0100
> Gerald Schaefer <gerald.schaefer@de.ibm.com> wrote:
>
> > From: Gerald Schaefer <gerald.schaefer@de.ibm.com>
> >
> > The spinning mutex implementation uses cpu_relax() in busy loops as a
> > compiler barrier. Depending on the architecture, cpu_relax() may do more
> > than needed in this specific mutex spin loops. On System z we also give
> > up the time slice of the virtual cpu in cpu_relax(), which prevents
> > effective spinning on the mutex.
> >
> > This patch replaces cpu_relax() in the spinning mutex code with
> > arch_mutex_cpu_relax(), which can be defined by each architecture that
> > selects HAVE_ARCH_MUTEX_CPU_RELAX. The default is still cpu_relax(), so
> > this patch should not affect other architectures than System z for now.
> >
> > ...
> >
> > --- a/include/linux/mutex.h
> > +++ b/include/linux/mutex.h
> > @@ -160,4 +160,8 @@ extern int mutex_trylock(struct mutex *l
> > extern void mutex_unlock(struct mutex *lock);
> > extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
> >
> > +#ifndef CONFIG_HAVE_ARCH_MUTEX_CPU_RELAX
> > +#define arch_mutex_cpu_relax() cpu_relax()
> > +#endif
>
> A simpler way of doing this is to remove the CONFIG_ variable
> altogether and do
>
> #ifndef arch_mutex_cpu_relax
> #define arch_mutex_cpu_relax() cpu_relax()
> #endif
>
> When doing this, one should be clear about _which_ arch file has the
> responsibility of defining arch_mutex_cpu_relax, and make sure that
> this arch file is reliably included in the .c file.
Well, I've tried that with my last approach, defining arch_mutex_cpu_relax()
in <asm/mutex.h> and including that from <linux/mutex.h>. This didn't work
well because of ugly header file dependencies, and Peter also commented
that "including "asm/mutex.h" isn't advised". The problem is the following
code in kernel/mutex.c (after including <linux/mutex.h>) when
CONFIG_DEBUG_MUTEXES is set:
#ifdef CONFIG_DEBUG_MUTEXES
# include "mutex-debug.h"
# include <asm-generic/mutex-null.h>
#else
# include "mutex.h"
# include <asm/mutex.h>
#endif
So I can only include <asm/mutex.h> from <linux/mutex.h> with an ugly
"#ifndef CONFIG_DEBUG_MUTEXES" around it, or use a completely different
or new arch header file (but <asm/mutex.h> seems like the right place
for this). The CONFIG_ approach avoids all this header file dependency
mess, or did I miss something (or maybe it's just me and it is not ugly
at all)?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mutex: Introduce arch_mutex_cpu_relax()
2010-11-23 14:12 ` Gerald Schaefer
@ 2010-11-23 14:20 ` Peter Zijlstra
2010-11-23 15:03 ` Gerald Schaefer
0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2010-11-23 14:20 UTC (permalink / raw)
To: gerald.schaefer
Cc: Andrew Morton, Ingo Molnar, Martin Schwidefsky, LKML, linux-s390,
Heiko Carstens
On Tue, 2010-11-23 at 15:12 +0100, Gerald Schaefer wrote:
> On Mo, 2010-11-22 at 12:10 -0800, Andrew Morton wrote:
> > On Mon, 22 Nov 2010 15:47:36 +0100
> > Gerald Schaefer <gerald.schaefer@de.ibm.com> wrote:
> >
> > > From: Gerald Schaefer <gerald.schaefer@de.ibm.com>
> > >
> > > The spinning mutex implementation uses cpu_relax() in busy loops as a
> > > compiler barrier. Depending on the architecture, cpu_relax() may do more
> > > than needed in this specific mutex spin loops. On System z we also give
> > > up the time slice of the virtual cpu in cpu_relax(), which prevents
> > > effective spinning on the mutex.
> > >
> > > This patch replaces cpu_relax() in the spinning mutex code with
> > > arch_mutex_cpu_relax(), which can be defined by each architecture that
> > > selects HAVE_ARCH_MUTEX_CPU_RELAX. The default is still cpu_relax(), so
> > > this patch should not affect other architectures than System z for now.
> > >
> > > ...
> > >
> > > --- a/include/linux/mutex.h
> > > +++ b/include/linux/mutex.h
> > > @@ -160,4 +160,8 @@ extern int mutex_trylock(struct mutex *l
> > > extern void mutex_unlock(struct mutex *lock);
> > > extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
> > >
> > > +#ifndef CONFIG_HAVE_ARCH_MUTEX_CPU_RELAX
> > > +#define arch_mutex_cpu_relax() cpu_relax()
> > > +#endif
> >
> > A simpler way of doing this is to remove the CONFIG_ variable
> > altogether and do
> >
> > #ifndef arch_mutex_cpu_relax
> > #define arch_mutex_cpu_relax() cpu_relax()
> > #endif
> >
> > When doing this, one should be clear about _which_ arch file has the
> > responsibility of defining arch_mutex_cpu_relax, and make sure that
> > this arch file is reliably included in the .c file.
>
> Well, I've tried that with my last approach, defining arch_mutex_cpu_relax()
> in <asm/mutex.h> and including that from <linux/mutex.h>. This didn't work
> well because of ugly header file dependencies, and Peter also commented
> that "including "asm/mutex.h" isn't advised". The problem is the following
> code in kernel/mutex.c (after including <linux/mutex.h>) when
> CONFIG_DEBUG_MUTEXES is set:
>
> #ifdef CONFIG_DEBUG_MUTEXES
> # include "mutex-debug.h"
> # include <asm-generic/mutex-null.h>
> #else
> # include "mutex.h"
> # include <asm/mutex.h>
> #endif
>
> So I can only include <asm/mutex.h> from <linux/mutex.h> with an ugly
> "#ifndef CONFIG_DEBUG_MUTEXES" around it, or use a completely different
> or new arch header file (but <asm/mutex.h> seems like the right place
> for this). The CONFIG_ approach avoids all this header file dependency
> mess, or did I miss something (or maybe it's just me and it is not ugly
> at all)?
Yeah, that all cause massive grief.. I've applied your patch as is,
assuming s390 already has the needed arch_mutex_cpu_relax()
implementation (otherwise I've just broken my s390 build).
Thanks
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mutex: Introduce arch_mutex_cpu_relax()
2010-11-23 14:20 ` Peter Zijlstra
@ 2010-11-23 15:03 ` Gerald Schaefer
0 siblings, 0 replies; 6+ messages in thread
From: Gerald Schaefer @ 2010-11-23 15:03 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Andrew Morton, Ingo Molnar, Martin Schwidefsky, LKML, linux-s390,
Heiko Carstens
On Di, 2010-11-23 at 15:20 +0100, Peter Zijlstra wrote:
> On Tue, 2010-11-23 at 15:12 +0100, Gerald Schaefer wrote:
> > On Mo, 2010-11-22 at 12:10 -0800, Andrew Morton wrote:
> > > On Mon, 22 Nov 2010 15:47:36 +0100
> > > Gerald Schaefer <gerald.schaefer@de.ibm.com> wrote:
> > >
> > > > From: Gerald Schaefer <gerald.schaefer@de.ibm.com>
> > > >
> > > > The spinning mutex implementation uses cpu_relax() in busy loops as a
> > > > compiler barrier. Depending on the architecture, cpu_relax() may do more
> > > > than needed in this specific mutex spin loops. On System z we also give
> > > > up the time slice of the virtual cpu in cpu_relax(), which prevents
> > > > effective spinning on the mutex.
> > > >
> > > > This patch replaces cpu_relax() in the spinning mutex code with
> > > > arch_mutex_cpu_relax(), which can be defined by each architecture that
> > > > selects HAVE_ARCH_MUTEX_CPU_RELAX. The default is still cpu_relax(), so
> > > > this patch should not affect other architectures than System z for now.
> > > >
> > > > ...
> > > >
> > > > --- a/include/linux/mutex.h
> > > > +++ b/include/linux/mutex.h
> > > > @@ -160,4 +160,8 @@ extern int mutex_trylock(struct mutex *l
> > > > extern void mutex_unlock(struct mutex *lock);
> > > > extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
> > > >
> > > > +#ifndef CONFIG_HAVE_ARCH_MUTEX_CPU_RELAX
> > > > +#define arch_mutex_cpu_relax() cpu_relax()
> > > > +#endif
> > >
> > > A simpler way of doing this is to remove the CONFIG_ variable
> > > altogether and do
> > >
> > > #ifndef arch_mutex_cpu_relax
> > > #define arch_mutex_cpu_relax() cpu_relax()
> > > #endif
> > >
> > > When doing this, one should be clear about _which_ arch file has the
> > > responsibility of defining arch_mutex_cpu_relax, and make sure that
> > > this arch file is reliably included in the .c file.
> >
> > Well, I've tried that with my last approach, defining arch_mutex_cpu_relax()
> > in <asm/mutex.h> and including that from <linux/mutex.h>. This didn't work
> > well because of ugly header file dependencies, and Peter also commented
> > that "including "asm/mutex.h" isn't advised". The problem is the following
> > code in kernel/mutex.c (after including <linux/mutex.h>) when
> > CONFIG_DEBUG_MUTEXES is set:
> >
> > #ifdef CONFIG_DEBUG_MUTEXES
> > # include "mutex-debug.h"
> > # include <asm-generic/mutex-null.h>
> > #else
> > # include "mutex.h"
> > # include <asm/mutex.h>
> > #endif
> >
> > So I can only include <asm/mutex.h> from <linux/mutex.h> with an ugly
> > "#ifndef CONFIG_DEBUG_MUTEXES" around it, or use a completely different
> > or new arch header file (but <asm/mutex.h> seems like the right place
> > for this). The CONFIG_ approach avoids all this header file dependency
> > mess, or did I miss something (or maybe it's just me and it is not ugly
> > at all)?
>
> Yeah, that all cause massive grief.. I've applied your patch as is,
> assuming s390 already has the needed arch_mutex_cpu_relax()
> implementation (otherwise I've just broken my s390 build).
Thanks, my patch already includes the s390 implementation for
arch_mutex_cpu_relax() in arch/s390/include/asm/mutex.h (we use a
simple barrier() in this case). We still have HAVE_DEFAULT_NO_SPIN_MUTEXES
selected though, this will be removed with one of Martins next patches,
once this preparation patch is upstream.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:sched/core] mutexes, sched: Introduce arch_mutex_cpu_relax()
2010-11-22 14:47 [PATCH] mutex: Introduce arch_mutex_cpu_relax() Gerald Schaefer
2010-11-22 20:10 ` Andrew Morton
@ 2010-11-26 15:02 ` tip-bot for Gerald Schaefer
1 sibling, 0 replies; 6+ messages in thread
From: tip-bot for Gerald Schaefer @ 2010-11-26 15:02 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, a.p.zijlstra, gerald.schaefer, tglx, mingo
Commit-ID: 335d7afbfb71faac833734a94240c1e07cf0ead8
Gitweb: http://git.kernel.org/tip/335d7afbfb71faac833734a94240c1e07cf0ead8
Author: Gerald Schaefer <gerald.schaefer@de.ibm.com>
AuthorDate: Mon, 22 Nov 2010 15:47:36 +0100
Committer: Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 26 Nov 2010 15:05:34 +0100
mutexes, sched: Introduce arch_mutex_cpu_relax()
The spinning mutex implementation uses cpu_relax() in busy loops as a
compiler barrier. Depending on the architecture, cpu_relax() may do more
than needed in this specific mutex spin loops. On System z we also give
up the time slice of the virtual cpu in cpu_relax(), which prevents
effective spinning on the mutex.
This patch replaces cpu_relax() in the spinning mutex code with
arch_mutex_cpu_relax(), which can be defined by each architecture that
selects HAVE_ARCH_MUTEX_CPU_RELAX. The default is still cpu_relax(), so
this patch should not affect other architectures than System z for now.
Signed-off-by: Gerald Schaefer <gerald.schaefer@de.ibm.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <1290437256.7455.4.camel@thinkpad>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
arch/Kconfig | 3 +++
arch/s390/Kconfig | 1 +
arch/s390/include/asm/mutex.h | 2 ++
include/linux/mutex.h | 4 ++++
kernel/mutex.c | 2 +-
kernel/sched.c | 3 ++-
6 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index 8bf0fa65..f78c2be 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -175,4 +175,7 @@ config HAVE_PERF_EVENTS_NMI
config HAVE_ARCH_JUMP_LABEL
bool
+config HAVE_ARCH_MUTEX_CPU_RELAX
+ bool
+
source "kernel/gcov/Kconfig"
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index e0b98e7..6c6d7b3 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -99,6 +99,7 @@ config S390
select HAVE_KERNEL_LZMA
select HAVE_KERNEL_LZO
select HAVE_GET_USER_PAGES_FAST
+ select HAVE_ARCH_MUTEX_CPU_RELAX
select ARCH_INLINE_SPIN_TRYLOCK
select ARCH_INLINE_SPIN_TRYLOCK_BH
select ARCH_INLINE_SPIN_LOCK
diff --git a/arch/s390/include/asm/mutex.h b/arch/s390/include/asm/mutex.h
index 458c1f7..688271f 100644
--- a/arch/s390/include/asm/mutex.h
+++ b/arch/s390/include/asm/mutex.h
@@ -7,3 +7,5 @@
*/
#include <asm-generic/mutex-dec.h>
+
+#define arch_mutex_cpu_relax() barrier()
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index f363bc8..94b48bd 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -160,4 +160,8 @@ extern int mutex_trylock(struct mutex *lock);
extern void mutex_unlock(struct mutex *lock);
extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
+#ifndef CONFIG_HAVE_ARCH_MUTEX_CPU_RELAX
+#define arch_mutex_cpu_relax() cpu_relax()
+#endif
+
#endif
diff --git a/kernel/mutex.c b/kernel/mutex.c
index 200407c..a5889fb 100644
--- a/kernel/mutex.c
+++ b/kernel/mutex.c
@@ -199,7 +199,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
* memory barriers as we'll eventually observe the right
* values at the cost of a few extra spins.
*/
- cpu_relax();
+ arch_mutex_cpu_relax();
}
#endif
spin_lock_mutex(&lock->wait_lock, flags);
diff --git a/kernel/sched.c b/kernel/sched.c
index 3e8a7db..abe7aec 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -75,6 +75,7 @@
#include <asm/tlb.h>
#include <asm/irq_regs.h>
+#include <asm/mutex.h>
#include "sched_cpupri.h"
#include "workqueue_sched.h"
@@ -3888,7 +3889,7 @@ int mutex_spin_on_owner(struct mutex *lock, struct thread_info *owner)
if (task_thread_info(rq->curr) != owner || need_resched())
return 0;
- cpu_relax();
+ arch_mutex_cpu_relax();
}
return 1;
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-11-26 15:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-22 14:47 [PATCH] mutex: Introduce arch_mutex_cpu_relax() Gerald Schaefer
2010-11-22 20:10 ` Andrew Morton
2010-11-23 14:12 ` Gerald Schaefer
2010-11-23 14:20 ` Peter Zijlstra
2010-11-23 15:03 ` Gerald Schaefer
2010-11-26 15:02 ` [tip:sched/core] mutexes, sched: " tip-bot for Gerald Schaefer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome