From 5db1256a5131d3b133946fa02ac9770a784e6eb2 Mon Sep 17 00:00:00 2001 From: Milton Miller Date: Thu, 12 May 2011 04:13:54 -0500 Subject: seqlock: Don't smp_rmb in seqlock reader spin loop Move the smp_rmb after cpu_relax loop in read_seqlock and add ACCESS_ONCE to make sure the test and return are consistent. A multi-threaded core in the lab didn't like the update from 2.6.35 to 2.6.36, to the point it would hang during boot when multiple threads were active. Bisection showed af5ab277ded04bd9bc6b048c5a2f0e7d70ef0867 (clockevents: Remove the per cpu tick skew) as the culprit and it is supported with stack traces showing xtime_lock waits including tick_do_update_jiffies64 and/or update_vsyscall. Experimentation showed the combination of cpu_relax and smp_rmb was significantly slowing the progress of other threads sharing the core, and this patch is effective in avoiding the hang. A theory is the rmb is affecting the whole core while the cpu_relax is causing a resource rebalance flush, together they cause an interfernce cadance that is unbroken when the seqlock reader has interrupts disabled. At first I was confused why the refactor in 3c22cd5709e8143444a6d08682a87f4c57902df3 (kernel: optimise seqlock) didn't affect this patch application, but after some study that affected seqcount not seqlock. The new seqcount was not factored back into the seqlock. I defer that the future. While the removal of the timer interrupt offset created contention for the xtime lock while a cpu does the additonal work to update the system clock, the seqlock implementation with the tight rmb spin loop goes back much further, and is just waiting for the right trigger. Cc: Signed-off-by: Milton Miller Cc: Cc: Linus Torvalds Cc: Andi Kleen Cc: Nick Piggin Cc: Benjamin Herrenschmidt Cc: Anton Blanchard Cc: Paul McKenney Acked-by: Eric Dumazet Link: http://lkml.kernel.org/r/%3Cseqlock-rmb%40mdm.bga.com%3E Signed-off-by: Thomas Gleixner --- include/linux/seqlock.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'include/linux/seqlock.h') diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h index e98cd2e57194..06d69648fc86 100644 --- a/include/linux/seqlock.h +++ b/include/linux/seqlock.h @@ -88,12 +88,12 @@ static __always_inline unsigned read_seqbegin(const seqlock_t *sl) unsigned ret; repeat: - ret = sl->sequence; - smp_rmb(); + ret = ACCESS_ONCE(sl->sequence); if (unlikely(ret & 1)) { cpu_relax(); goto repeat; } + smp_rmb(); return ret; } -- cgit v1.2.3 From c4dbe54ed7296ac3249c415d512dd6d649f66f4b Mon Sep 17 00:00:00 2001 From: Eric Dumazet Date: Tue, 24 May 2011 14:08:08 +0200 Subject: seqlock: Get rid of SEQLOCK_UNLOCKED All static seqlock should be initialized with the lockdep friendly __SEQLOCK_UNLOCKED() macro. Remove legacy SEQLOCK_UNLOCKED() macro. Signed-off-by: Eric Dumazet Cc: David Miller Link: http://lkml.kernel.org/r/%3C1306238888.3026.31.camel%40edumazet-laptop%3E Signed-off-by: Thomas Gleixner --- arch/ia64/kernel/time.c | 2 +- arch/x86/kernel/vsyscall_64.c | 2 +- include/linux/seqlock.h | 3 --- net/ipv4/inet_connection_sock.c | 2 +- 4 files changed, 3 insertions(+), 6 deletions(-) (limited to 'include/linux/seqlock.h') diff --git a/arch/ia64/kernel/time.c b/arch/ia64/kernel/time.c index 04440cc09b40..85118dfe9bb5 100644 --- a/arch/ia64/kernel/time.c +++ b/arch/ia64/kernel/time.c @@ -36,7 +36,7 @@ static cycle_t itc_get_cycles(struct clocksource *cs); struct fsyscall_gtod_data_t fsyscall_gtod_data = { - .lock = SEQLOCK_UNLOCKED, + .lock = __SEQLOCK_UNLOCKED(fsyscall_gtod_data.lock), }; struct itc_jitter_data_t itc_jitter_data; diff --git a/arch/x86/kernel/vsyscall_64.c b/arch/x86/kernel/vsyscall_64.c index dcbb28c4b694..59be48d0d75c 100644 --- a/arch/x86/kernel/vsyscall_64.c +++ b/arch/x86/kernel/vsyscall_64.c @@ -59,7 +59,7 @@ int __vgetcpu_mode __section_vgetcpu_mode; struct vsyscall_gtod_data __vsyscall_gtod_data __section_vsyscall_gtod_data = { - .lock = SEQLOCK_UNLOCKED, + .lock = __SEQLOCK_UNLOCKED(__vsyscall_gtod_data.lock), .sysctl_enabled = 1, }; diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h index 06d69648fc86..e9811892844f 100644 --- a/include/linux/seqlock.h +++ b/include/linux/seqlock.h @@ -41,9 +41,6 @@ typedef struct { #define __SEQLOCK_UNLOCKED(lockname) \ { 0, __SPIN_LOCK_UNLOCKED(lockname) } -#define SEQLOCK_UNLOCKED \ - __SEQLOCK_UNLOCKED(old_style_seqlock_init) - #define seqlock_init(x) \ do { \ (x)->sequence = 0; \ diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c index 61fac4cabc78..c14d88ad348d 100644 --- a/net/ipv4/inet_connection_sock.c +++ b/net/ipv4/inet_connection_sock.c @@ -33,7 +33,7 @@ EXPORT_SYMBOL(inet_csk_timer_bug_msg); * This struct holds the first and last local port number. */ struct local_ports sysctl_local_ports __read_mostly = { - .lock = SEQLOCK_UNLOCKED, + .lock = __SEQLOCK_UNLOCKED(sysctl_local_ports.lock), .range = { 32768, 61000 }, }; -- cgit v1.2.3 From 56a210526adb2854d5b7c398a40260720390ee05 Mon Sep 17 00:00:00 2001 From: David Howells Date: Sat, 11 Jun 2011 12:29:58 +0100 Subject: linux/seqlock.h should #include asm/processor.h for cpu_relax() It uses cpu_relax(), and so needs Without this patch, I see: CC arch/mn10300/kernel/asm-offsets.s In file included from include/linux/time.h:8, from include/linux/timex.h:56, from include/linux/sched.h:57, from arch/mn10300/kernel/asm-offsets.c:7: include/linux/seqlock.h: In function 'read_seqbegin': include/linux/seqlock.h:91: error: implicit declaration of function 'cpu_relax' whilst building asb2364_defconfig on MN10300. Signed-off-by: David Howells Signed-off-by: Linus Torvalds --- include/linux/seqlock.h | 1 + 1 file changed, 1 insertion(+) (limited to 'include/linux/seqlock.h') diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h index e9811892844f..c6db9fb33c44 100644 --- a/include/linux/seqlock.h +++ b/include/linux/seqlock.h @@ -28,6 +28,7 @@ #include #include +#include typedef struct { unsigned sequence; -- cgit v1.2.3