* [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 related [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.