* [PATCH] seqlock: don't smp_rmb in seqlock reader spin loop, [PATCH] seqlock: don't smp_rmb in seqlock reader spin loop
[not found] ` <1304724519.20980.139.camel@work-vm>
@ 2011-05-12 9:13 ` Milton Miller
2011-05-12 9:35 ` Eric Dumazet
2011-05-12 14:08 ` Andi Kleen
0 siblings, 2 replies; 3+ messages in thread
From: Milton Miller @ 2011-05-12 9:13 UTC (permalink / raw)
To: Andrew Morton, Nick Piggin, Benjamin Herrenschmidt,
Anton Blanchard, Thomas Gleixner, Eric Dumazet
Cc: Linus Torvalds, Ingo Molnar, Andi Kleen, linuxppc-dev, linux-kernel
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: <stable@vger.kernel.org>
Signed-off-by: Milton Miller <miltonm@bga.com>
---
To the readers of [RFC] time: xtime_lock is held too long:
I initially thought x86 would not see this because rmb would
be a nop, but upon closer inspection X86_PPRO_FENCE will add
a lfence for rmb.
milton
Index: common/include/linux/seqlock.h
===================================================================
--- common.orig/include/linux/seqlock.h 2011-04-06 03:27:02.000000000 -0500
+++ common/include/linux/seqlock.h 2011-04-06 03:35:02.000000000 -0500
@@ -88,12 +88,12 @@ static __always_inline unsigned read_seq
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;
}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] seqlock: don't smp_rmb in seqlock reader spin loop
2011-05-12 9:13 ` [PATCH] seqlock: don't smp_rmb in seqlock reader spin loop, [PATCH] seqlock: don't smp_rmb in seqlock reader spin loop Milton Miller
@ 2011-05-12 9:35 ` Eric Dumazet
2011-05-12 14:08 ` Andi Kleen
1 sibling, 0 replies; 3+ messages in thread
From: Eric Dumazet @ 2011-05-12 9:35 UTC (permalink / raw)
To: Milton Miller
Cc: Nick Piggin, Ingo Molnar, linux-kernel, Linus Torvalds,
Andi Kleen, Anton Blanchard, Andrew Morton, linuxppc-dev,
Thomas Gleixner
Le jeudi 12 mai 2011 à 04:13 -0500, Milton Miller a écrit :
> 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: <stable@vger.kernel.org>
> Signed-off-by: Milton Miller <miltonm@bga.com>
> ---
>
> To the readers of [RFC] time: xtime_lock is held too long:
>
> I initially thought x86 would not see this because rmb would
> be a nop, but upon closer inspection X86_PPRO_FENCE will add
> a lfence for rmb.
>
> milton
>
> Index: common/include/linux/seqlock.h
> ===================================================================
> --- common.orig/include/linux/seqlock.h 2011-04-06 03:27:02.000000000 -0500
> +++ common/include/linux/seqlock.h 2011-04-06 03:35:02.000000000 -0500
> @@ -88,12 +88,12 @@ static __always_inline unsigned read_seq
> 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;
> }
I fully agree with your analysis. This is a call to make the change I
suggested earlier [1]. (Use a seqcount object in seqlock_t)
typedef struct {
seqcount_t seq
spinlock_t lock;
} seqlock_t;
I'll submit a patch for 2.6.40
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
Thanks
[1] Ref: https://lkml.org/lkml/2011/5/6/351
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] seqlock: don't smp_rmb in seqlock reader spin loop
2011-05-12 9:13 ` [PATCH] seqlock: don't smp_rmb in seqlock reader spin loop, [PATCH] seqlock: don't smp_rmb in seqlock reader spin loop Milton Miller
2011-05-12 9:35 ` Eric Dumazet
@ 2011-05-12 14:08 ` Andi Kleen
1 sibling, 0 replies; 3+ messages in thread
From: Andi Kleen @ 2011-05-12 14:08 UTC (permalink / raw)
To: Milton Miller
Cc: Nick Piggin, Eric Dumazet, Ingo Molnar, linux-kernel,
Linus Torvalds, Andi Kleen, Anton Blanchard, Andrew Morton,
linuxppc-dev, Thomas Gleixner
On Thu, May 12, 2011 at 04:13:54AM -0500, Milton Miller wrote:
>
> 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
Which core was that?
-Andi
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-05-12 14:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <alpine.LFD.2.02.1105091036000.2895@ionos>
[not found] ` <1304574244.32152.666.camel@edumazet-laptop>
[not found] ` <1304576495.2943.40.camel@work-vm>
[not found] ` <1304604284.3032.78.camel@edumazet-laptop>
[not found] ` <alpine.LFD.2.02.1105051630390.3005@ionos>
[not found] ` <1304608095.3032.95.camel@edumazet-laptop>
[not found] ` <alpine.LFD.2.02.1105051750540.3005@ionos>
[not found] ` <20110505210118.GI2925@one.firstfloor.org>
[not found] ` <alpine.LFD.2.02.1105061117220.3005@ionos>
[not found] ` <20110506165913.GF11636@one.firstfloor.org>
[not found] ` <1304703767.3066.211.camel@edumazet-laptop>
[not found] ` <20110506175051.GL11636@one.firstfloor.org>
[not found] ` <1304710003.2821.0.camel@edumazet-laptop>
[not found] ` <1304712267.2821.29.camel@edumazet-laptop>
[not found] ` <1304713443.20980.124.camel@work-vm>
[not found] ` <1304721004.2821.148.camel@edumazet-laptop>
[not found] ` <1304722000.20980.130.camel@work-vm>
[not found] ` <1304722835.2821.192.camel@edumazet-laptop>
[not found] ` <1304724519.20980.139.camel@work-vm>
2011-05-12 9:13 ` [PATCH] seqlock: don't smp_rmb in seqlock reader spin loop, [PATCH] seqlock: don't smp_rmb in seqlock reader spin loop Milton Miller
2011-05-12 9:35 ` Eric Dumazet
2011-05-12 14:08 ` Andi Kleen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).