On Sat, Jul 28, 2018 at 12:07:19AM +0000, David Chen wrote: > Hi Paul, > > I wasn't talking about the xchg() though. > > The smp_mb__after_atomic() is not for xchg(), it's for `*tail = > rdp->nocb_gp_head;` > it's stated so in the comment. And I do think we need ordering between > `*tail = rdp->nocb_gp_head;` and wake up, because the waiter is checking on > head not tail. > > swait_event_interruptible(rdp->nocb_wq, > > READ_ONCE(rdp->nocb_follower_head)); > > So what I'm saying is that since we need to maintain ordering between `*tail > = rdp->nocb_gp_head;` > and wake up, we need to change the smp_mb__after_atomic() to smp_mb(). Because > smp_mb__after_atomic() wouldn't guarantee > > So this is what I'm proposing.
Good eyes! Hmmm... What do I do about this in mainline? Ah, I introduced a ->nocb_lock in mainline to prevent this from happening. In that same commit that you didn't want to use because it is hard to backport. ;-) So yes, but there might well be other misorderings fixed by the hard-to-backport commit that your change below does not cover. Still I do agree that you need full ordering at that point. Thanx, Paul > diff -ru linux-4.9.37.orig/kernel/rcu/tree_plugin.h > linux-4.9.37/kernel/rcu/tree_plugin.h > --- linux-4.9.37.orig/kernel/rcu/tree_plugin.h 2017-07-12 > 06:42:41.000000000 -0700 > +++ linux-4.9.37/kernel/rcu/tree_plugin.h 2018-07-27 16:23:41.349044259 > -0700 > @@ -2076,7 +2076,7 @@ > /* Append callbacks to follower's "done" list. */ > tail = xchg(&rdp->nocb_follower_tail, rdp->nocb_gp_tail); > *tail = rdp->nocb_gp_head; > - smp_mb__after_atomic(); /* Store *tail before wakeup. */ > + smp_mb(); /* Store *tail before wakeup. */ > if (rdp != my_rdp && tail == &rdp->nocb_follower_head) { > /* > * List was empty, wake up the follower. > > Thanks, > David > > From: Paul E. McKenney <paul...@linux.vnet.ibm.com> > Sent: Friday, July 27, 2018 4:47 PM > To: David Chen > Cc: linux-kernel@vger.kernel.org > Subject: Re: RCU nocb list not reclaiming causing OOM > > > On Fri, Jul 27, 2018 at 11:16:39PM +0000, David Chen wrote: > > Hi Paul, > > > > Thanks for the advice. > > The bug is kind of hard to hit, so I can't say for certain yet. > > Well, you can always remove the "tail == &rdp->nocb_follower_head" as an > extra belt-and-suspenders safety net. I am not putting that in mainline, > but in the privacy of your own copy of the kernel, I don't see any really > serious problem with it. (As long as you aren't going for absolute maximum > performance, but even then there are other more important tuning actions > and code changes you could make.) > > > Though after another look at the code, I found out the > > `smp_mb__after_atomic();` > > seems to be only a compiler barrier on x86. > > Yes, and that is because the locked xchg instruction used on x86 to > implement xchg() already provides full ordering. ;-) > > > tail = xchg(&rdp->nocb_follower_tail, rdp->nocb_gp_tail); > > *tail = rdp->nocb_gp_head; > > smp_mb__after_atomic(); /* Store *tail before wakeup. */ > > if (rdp != my_rdp && tail == &rdp->nocb_follower_head) { > > swake_up(&rdp->nocb_wq); > > > > But that wouldn't be enough right? Because from 6b5fc3a13318, it stated that > > wakeup operation don't guarantee ordering. And when the follower wakes up, > > it checks > > for nocb_follower_head, which is assigned by `*tail = rdp->nocb_gp_head;` > > which doesn't > > have LOCK prefix. So it's possible for follower to wake up and see the list > > is empty and go > > back to sleep. > > Again, xchg() is defined to provide full ordering against all operations > before and after it. Each architecture is required to do whatever is > necessary to implement that full ordering, and x86 need only provide > its "lock xchg" instruction. > > The smp_mb__after_atomic() has effect only after atomic read-modify-write > operations that do not return a value, for example, atomic_inc(). > If you use it after a value-returning atomic read-modify-write operation > like xchg(), all you do is needlessly slow things down on platforms > that provide non-empty smp_mb__after_atomic() definitions. So again, > smp_mb__after_atomic() after xchg() is pointless. > > Please take a look at Documentation/core-api/atomic_ops.rst in the > Linux-kernel source tree for more information. Or get a v4.17 kernel > source tree and check this using the memory model (tools/memory-model > in that version). > > Thanx, Paul > > > Thanks, > > David > > > > From: Paul E. McKenney <paul...@linux.vnet.ibm.com> > > Sent: Friday, July 27, 2018 3:31 PM > > To: David Chen > > Cc: linux-kernel@vger.kernel.org > > Subject: Re: RCU nocb list not reclaiming causing OOM > > > > > > On Fri, Jul 27, 2018 at 07:07:46PM +0000, David Chen wrote: > > > Hi Paul, > > > > > > I'd like to opinion again on this subject. > > > > > > So we are going to backport this patch: > > > 6b5fc3a13318 ("rcu: Add memory barriers for NOCB leader wakeup") > > > > Does this one solve the problem, or are you still seeing hangs? > > If you are no longer seeing hangs, my advice is "hands off keyboard", > > though some would no doubt point out that I should follow that advice > > more myself. ;-) > > > > > But the other one: > > > 8be6e1b15c54 ("rcu: Use timer as backstop for NOCB deferred wakeups") > > > It doesn't apply cleanly, and I'm not too comfortable porting it myself. > > > > Yeah, that one is a bit on the non-trivial side, no two ways about it. > > > > > So I'm wondering if I use the following change to always wake up follower > > > thread > > > regardless if the list was empty or not, just to be on the safe side. Do > > > you think > > > this change is reasonable? Do you see any problem it might cause? > > > > > > Thanks, > > > David > > > > > > diff -ru linux-4.9.37.orig/kernel/rcu/tree_plugin.h > > > linux-4.9.37/kernel/rcu/tree_plugin.h > > > --- linux-4.9.37.orig/kernel/rcu/tree_plugin.h 2017-07-12 > > > 06:42:41.000000000 -0700 > > > +++ linux-4.9.37/kernel/rcu/tree_plugin.h 2018-07-27 > > > 11:57:03.582134519 -0700 > > > @@ -2077,7 +2077,7 @@ > > > tail = xchg(&rdp->nocb_follower_tail, rdp->nocb_gp_tail); > > > *tail = rdp->nocb_gp_head; > > > smp_mb__after_atomic(); /* Store *tail before wakeup. */ > > > - if (rdp != my_rdp && tail == &rdp->nocb_follower_head) { > > > + if (rdp != my_rdp) { > > > > This will burn a bit of extra CPU time, but it should be fine other than > > that. > > > > Thanx, Paul > > > > > /* > > > * List was empty, wake up the follower. > > > * Memory barriers supplied by atomic_long_add(). > > > > > > > > > From: David Chen > > > Sent: Friday, July 20, 2018 5:12 PM > > > To: paul...@linux.vnet.ibm.com > > > Cc: linux-kernel@vger.kernel.org > > > Subject: Re: RCU nocb list not reclaiming causing OOM > > > > > > > > > Hi Paul, > > > > > > Ok, I'll try those patches. > > > > > > Thanks, > > > David > > > > > > From: Paul E. McKenney <paul...@linux.vnet.ibm.com> > > > Sent: Friday, July 20, 2018 4:32:12 PM > > > To: David Chen > > > Cc: linux-kernel@vger.kernel.org > > > Subject: Re: RCU nocb list not reclaiming causing OOM > > > > > > > > > On Fri, Jul 20, 2018 at 11:05:52PM +0000, David Chen wrote: > > > > Hi Paul, > > > > > > > > We hit an RCU issue on 4.9.37 kernel. One of the nocb_follower list > > > > grows too > > > > large, and not getting reclaimed, causing the system to OOM. > > > > > > > > Printing the culprit rcu_sched_data: > > > > > > > > nocb_q_count = { > > > > counter = 32369635 > > > > }, > > > > nocb_follower_head = 0xffff88ae901c0a00, > > > > nocb_follower_tail = 0xffff88af1538b8d8, > > > > nocb_kthread = 0xffff88b06d290000, > > > > > > > > As you can see here, the nocb_follower_head is not empty, so in theory, > > > > the > > > > nocb_kthread shouldn't go to sleep. However, if dump the stack of the > > > > kthread: > > > > > > > > crash> bt 0xffff88b06d290000 > > > > PID: 21 TASK: ffff88b06d290000 CPU: 3 COMMAND: "rcuos/1" > > > > #0 [ffffafc9020b7dc0] __schedule at ffffffff8d8789dc > > > > #1 [ffffafc9020b7e38] schedule at ffffffff8d878e76 > > > > #2 [ffffafc9020b7e50] rcu_nocb_kthread at ffffffff8d112337 > > > > #3 [ffffafc9020b7ec8] kthread at ffffffff8d0c6ce7 > > > > #4 [ffffafc9020b7f50] ret_from_fork at ffffffff8d87d755 > > > > > > > > And if we dis the address at ffffffff8d112337: > > > > > > > > /usr/src/debug/kernel-4.9.37/linux-4.9.37-29.nutanix.07142017.el7.centos.x86_64/kernel/rcu/tree_plugin.h: > > > > 2106 > > > > 0xffffffff8d11232d <rcu_nocb_kthread+381>: test %rax,%rax > > > > 0xffffffff8d112330 <rcu_nocb_kthread+384>: jne > > > > 0xffffffff8d112355 <rcu_nocb_kthread+421> > > > > 0xffffffff8d112332 <rcu_nocb_kthread+386>: callq > > > > 0xffffffff8d878e40 <schedule> > > > > 0xffffffff8d112337 <rcu_nocb_kthread+391>: lea -0x40(%rbp),%rsi > > > > > > > > So the kthread is blocked at swait_event_interruptible in the > > > > nocb_follower_wait. > > > > This contradict with the fact that nocb_follower_head was not empty. So > > > > I > > > > wonder if this is caused by the lack of memory barrier in the place > > > > shown below. > > > > If the head is set to NULL after doing xchg, it will overwrite the head > > > > set > > > > by leader. This caused the kthread to sleep the next iteration, and the > > > > leader > > > > won't wake him up as the tail doesn't point to head. > > > > > > > > Please tell me what do you think. > > > > > > > > Thanks, > > > > David > > > > > > > > diff -ru linux-4.9.37.orig/kernel/rcu/tree_plugin.h > > > > linux-4.9.37/kernel/rcu/tree_plugin.h > > > > --- linux-4.9.37.orig/kernel/rcu/tree_plugin.h 2017-07-12 > > > > 06:42:41.000000000 -0700 > > > > +++ linux-4.9.37/kernel/rcu/tree_plugin.h 2018-07-20 > > > > 15:25:57.311206343 -0700 > > > > @@ -2149,6 +2149,7 @@ > > > > BUG_ON(!list); > > > > trace_rcu_nocb_wake(rdp->rsp->name, rdp->cpu, > > > >"WokeNonEmpty"); > > > > WRITE_ONCE(rdp->nocb_follower_head, NULL); > > > > + smp_mb(); > > > > tail = xchg(&rdp->nocb_follower_tail, > > > >&rdp->nocb_follower_head); > > > > > > The xchg() operation implies full memory barriers both before and after, > > > so adding the smp_mb() before would have no effect. > > > > > > But let me take a look at post-4.9 changes to this code... > > > > > > I suggest trying out the following commit: > > > > > > 6b5fc3a13318 ("rcu: Add memory barriers for NOCB leader wakeup") > > > > > > If that one doesn't help, the following might be worth trying, but > > > probably > > > a lot harder to backport: > > > > > > 8be6e1b15c54 ("rcu: Use timer as backstop for NOCB deferred wakeups") > > > > > > Please let me know how it goes! > > > > > > Thanx, Paul > > > > > > ------------------------------------------------------------------------ > > > > > > commit 6b5fc3a1331810db407c9e0e673dc1837afdc9d0 > > > Author: Paul E. McKenney <paul...@linux.vnet.ibm.com> > > > Date: Fri Apr 28 20:11:09 2017 -0700 > > > > > > rcu: Add memory barriers for NOCB leader wakeup > > > > > > Wait/wakeup operations do not guarantee ordering on their own. > > > Instead, > > > either locking or memory barriers are required. This commit therefore > > > adds memory barriers to wake_nocb_leader() and nocb_leader_wait(). > > > > > > Signed-off-by: Paul E. McKenney <paul...@linux.vnet.ibm.com> > > > Tested-by: Krister Johansen <k...@templeofstupid.com> > > > Cc: <sta...@vger.kernel.org> # 4.6.x > > > > > > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h > > > index 0b1042545116..573fbe9640a0 100644 > > > --- a/kernel/rcu/tree_plugin.h > > > +++ b/kernel/rcu/tree_plugin.h > > > @@ -1810,6 +1810,7 @@ static void wake_nocb_leader(struct rcu_data *rdp, > > > bool force) > > > if (READ_ONCE(rdp_leader->nocb_leader_sleep) || force) { > > > /* Prior smp_mb__after_atomic() orders against prior > > > enqueue. */ > > > WRITE_ONCE(rdp_leader->nocb_leader_sleep, false); > > > + smp_mb(); /* ->nocb_leader_sleep before swake_up(). */ > > > swake_up(&rdp_leader->nocb_wq); > > > } > > > } > > > @@ -2064,6 +2065,7 @@ static void nocb_leader_wait(struct rcu_data > > > *my_rdp) > > > * nocb_gp_head, where they await a grace period. > > > */ > > > gotcbs = false; > > > + smp_mb(); /* wakeup before ->nocb_head reads. */ > > > for (rdp = my_rdp; rdp; rdp = rdp->nocb_next_follower) { > > > rdp->nocb_gp_head = READ_ONCE(rdp->nocb_head); > > > if (!rdp->nocb_gp_head) > > > > > > > > > > > >