Message-ID: <BYAPR02MB4501BA05FDCDAAC8842B3EBD942F0@BYAPR02MB4501.namprd02.prod.outlook.com>
Date:   Mon, 30 Jul 2018 19:08:55 +0000
From:   David Chen <david.chen@...anix.com>
To:     "paulmck@...ux.vnet.ibm.com" <paulmck@...ux.vnet.ibm.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: RCU nocb list not reclaiming causing OOM

Hi Paul,

Just want to know what's your plan on stable branches regarding this issue.
Do you intend to backport the ->nocb_lock? Or are you going with just the
memory barrier change?

Thanks,
David


From: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
Sent: Saturday, July 28, 2018 1:29 AM
To: David Chen
Cc: linux-kernel@...r.kernel.org
Subject: Re: RCU nocb list not reclaiming causing OOM
  

On Fri, Jul 27, 2018 at 06:47:00PM -0700, Paul E. McKenney wrote:
> 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.

Another approach would be to backport only the ->nocb_lock portions
of that patch.  This would still potentially leave failure-to-wake
(as opposed to misordering-on-wake) issues, but it should cover all
of the misordering-on-wake issues.

                                                        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 <paulmck@...ux.vnet.ibm.com>
> > Sent: Friday, July 27, 2018 4:47 PM
> > To: David Chen
> > Cc: linux-kernel@...r.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 <paulmck@...ux.vnet.ibm.com>
> > > Sent: Friday, July 27, 2018 3:31 PM
> > > To: David Chen
> > > Cc: linux-kernel@...r.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: paulmck@...ux.vnet.ibm.com
> > > > Cc: linux-kernel@...r.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 <paulmck@...ux.vnet.ibm.com>
> > > > Sent: Friday, July 20, 2018 4:32:12 PM
> > > > To: David Chen
> > > > Cc: linux-kernel@...r.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 <paulmck@...ux.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 <paulmck@...ux.vnet.ibm.com>
> > > >     Tested-by: Krister Johansen <kjlx@...pleofstupid.com>
> > > >     Cc: <stable@...r.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)
> > > > 
> > > >      
> > > 
> > >     
> > 
> >     

    

Powered by blists - more mailing lists