Re: rcu_preempt caused oom

[Paul E. McKenney]

On Thu, Dec 13, 2018 at 03:28:46AM +0000, Zhang, Jun wrote:
> Ok, we will test it, thanks!

But please also try the sysrq-y with the earlier patch after a hang!

							Thanx, Paul

> -----Original Message-----
> From: Paul E. McKenney [mailto:paulmck@xxxxxxxxxxxxx] 
> Sent: Thursday, December 13, 2018 10:43
> To: Zhang, Jun <jun.zhang@xxxxxxxxx>
> Cc: He, Bo <bo.he@xxxxxxxxx>; Steven Rostedt <rostedt@xxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; josh@xxxxxxxxxxxxxxxx; mathieu.desnoyers@xxxxxxxxxxxx; jiangshanlai@xxxxxxxxx; Xiao, Jin <jin.xiao@xxxxxxxxx>; Zhang, Yanmin <yanmin.zhang@xxxxxxxxx>; Bai, Jie A <jie.a.bai@xxxxxxxxx>; Sun, Yi J <yi.j.sun@xxxxxxxxx>
> Subject: Re: rcu_preempt caused oom
> 
> On Thu, Dec 13, 2018 at 02:11:35AM +0000, Zhang, Jun wrote:
> > Hello, Paul
> > 
> > I think the next patch is better.
> > Because ULONG_CMP_GE could cause double write, which has risk that write back old value.
> > Please help review.
> > I don't test it. If you agree, we will test it.
> 
> Just to make sure that I understand, you are worried about something like the following, correct?
> 
> o	__note_gp_changes() compares rnp->gp_seq_needed and rdp->gp_seq_needed
> 	and finds them equal.
> 
> o	At just this time something like rcu_start_this_gp() assigns a new
> 	(larger) value to rdp->gp_seq_needed.
> 
> o	Then __note_gp_changes() overwrites rdp->gp_seq_needed with the
> 	old value.
> 
> This cannot happen because __note_gp_changes() runs with interrupts disabled on the CPU corresponding to the rcu_data structure referenced by the rdp pointer.  So there is no way for rcu_start_this_gp() to be invoked on the same CPU during this "if" statement.
> 
> Of course, there could be bugs.  For example:
> 
> o	__note_gp_changes() might be called on a different CPU than that
> 	corresponding to rdp.  You can check this with something like:
> 
> 	WARN_ON_ONCE(rdp->cpu != smp_processor_id());
> 
> o	The same things could happen with rcu_start_this_gp(), and the
> 	above WARN_ON_ONCE() would work there as well.
> 
> o	rcutree_prepare_cpu() is a special case, but is irrelevant unless
> 	you are doing CPU-hotplug operations.  (It can run on a CPU other
> 	than rdp->cpu, but only at times when rdp->cpu is offline.)
> 
> o	Interrupts might not really be disabled.
> 
> That said, your patch could reduce overhead slightly, given that the two values will be equal much of the time.  So it might be worth testing just for that reason.
> 
> So why not just test it anyway?  If it makes the bug go away, I will be surprised, but it would not be the first surprise for me.  ;-)
> 
> 							Thanx, Paul
> 
> > Thanks!
> > 
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 
> > 0b760c1..c00f34e 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -1849,7 +1849,7 @@ static bool __note_gp_changes(struct rcu_state *rsp, struct rcu_node *rnp,
> >                 zero_cpu_stall_ticks(rdp);
> >         }
> >         rdp->gp_seq = rnp->gp_seq;  /* Remember new grace-period state. */
> > -       if (ULONG_CMP_GE(rnp->gp_seq_needed, rdp->gp_seq_needed) || rdp->gpwrap)
> > +       if (ULONG_CMP_LT(rdp->gp_seq_needed, rnp->gp_seq_needed) || 
> > + rdp->gpwrap)
> >                 rdp->gp_seq_needed = rnp->gp_seq_needed;
> >         WRITE_ONCE(rdp->gpwrap, false);
> >         rcu_gpnum_ovf(rnp, rdp);
> > 
> > 
> > -----Original Message-----
> > From: Paul E. McKenney [mailto:paulmck@xxxxxxxxxxxxx]
> > Sent: Thursday, December 13, 2018 08:12
> > To: He, Bo <bo.he@xxxxxxxxx>
> > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>; 
> > linux-kernel@xxxxxxxxxxxxxxx; josh@xxxxxxxxxxxxxxxx; 
> > mathieu.desnoyers@xxxxxxxxxxxx; jiangshanlai@xxxxxxxxx; Zhang, Jun 
> > <jun.zhang@xxxxxxxxx>; Xiao, Jin <jin.xiao@xxxxxxxxx>; Zhang, Yanmin 
> > <yanmin.zhang@xxxxxxxxx>; Bai, Jie A <jie.a.bai@xxxxxxxxx>; Sun, Yi J 
> > <yi.j.sun@xxxxxxxxx>
> > Subject: Re: rcu_preempt caused oom
> > 
> > On Wed, Dec 12, 2018 at 11:13:22PM +0000, He, Bo wrote:
> > > I don't see the rcutree.sysrq_rcu parameter in v4.19 kernel, I also checked the latest kernel and the latest tag v4.20-rc6, not see the sysrq_rcu.
> > > Please correct me if I have something wrong.
> > 
> > That would be because I sent you the wrong patch, apologies!  :-/
> > 
> > Please instead see the one below, which does add sysrq_rcu.
> > 
> > 							Thanx, Paul
> > 
> > > -----Original Message-----
> > > From: Paul E. McKenney <paulmck@xxxxxxxxxxxxx>
> > > Sent: Thursday, December 13, 2018 5:03 AM
> > > To: He, Bo <bo.he@xxxxxxxxx>
> > > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>; 
> > > linux-kernel@xxxxxxxxxxxxxxx; josh@xxxxxxxxxxxxxxxx; 
> > > mathieu.desnoyers@xxxxxxxxxxxx; jiangshanlai@xxxxxxxxx; Zhang, Jun 
> > > <jun.zhang@xxxxxxxxx>; Xiao, Jin <jin.xiao@xxxxxxxxx>; Zhang, Yanmin 
> > > <yanmin.zhang@xxxxxxxxx>; Bai, Jie A <jie.a.bai@xxxxxxxxx>
> > > Subject: Re: rcu_preempt caused oom
> > > 
> > > On Wed, Dec 12, 2018 at 07:42:24AM -0800, Paul E. McKenney wrote:
> > > > On Wed, Dec 12, 2018 at 01:21:33PM +0000, He, Bo wrote:
> > > > > we reproduce on two boards, but I still not see the show_rcu_gp_kthreads() dump logs, it seems the patch can't catch the scenario.
> > > > > I double confirmed the CONFIG_PROVE_RCU=y is enabled in the config as it's extracted from the /proc/config.gz.
> > > > 
> > > > Strange.
> > > > 
> > > > Are the systems responsive to sysrq keys once failure occurs?  If 
> > > > so, I will provide you a sysrq-R or some such to dump out the RCU state.
> > > 
> > > Or, as it turns out, sysrq-y if booting with rcutree.sysrq_rcu=1 using the patch below.  Only lightly tested.
> > 
> > ----------------------------------------------------------------------
> > --
> > 
> > commit 04b6245c8458e8725f4169e62912c1fadfdf8141
> > Author: Paul E. McKenney <paulmck@xxxxxxxxxxxxx>
> > Date:   Wed Dec 12 16:10:09 2018 -0800
> > 
> >     rcu: Add sysrq rcu_node-dump capability
> >     
> >     Backported from v4.21/v5.0
> >     
> >     Life is hard if RCU manages to get stuck without triggering RCU CPU
> >     stall warnings or triggering the rcu_check_gp_start_stall() checks
> >     for failing to start a grace period.  This commit therefore adds a
> >     boot-time-selectable sysrq key (commandeering "y") that allows manually
> >     dumping Tree RCU state.  The new rcutree.sysrq_rcu kernel boot parameter
> >     must be set for this sysrq to be available.
> >     
> >     Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxx>
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 
> > 0b760c1369f7..e9392a9d6291 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -61,6 +61,7 @@
> >  #include <linux/trace_events.h>
> >  #include <linux/suspend.h>
> >  #include <linux/ftrace.h>
> > +#include <linux/sysrq.h>
> >  
> >  #include "tree.h"
> >  #include "rcu.h"
> > @@ -128,6 +129,9 @@ int num_rcu_lvl[] = NUM_RCU_LVL_INIT;  int 
> > rcu_num_nodes __read_mostly = NUM_RCU_NODES; /* Total # rcu_nodes in 
> > use. */
> >  /* panic() on RCU Stall sysctl. */
> >  int sysctl_panic_on_rcu_stall __read_mostly;
> > +/* Commandeer a sysrq key to dump RCU's tree. */ static bool 
> > +sysrq_rcu; module_param(sysrq_rcu, bool, 0444);
> >  
> >  /*
> >   * The rcu_scheduler_active variable is initialized to the value @@ 
> > -662,6 +666,27 @@ void show_rcu_gp_kthreads(void)  }  
> > EXPORT_SYMBOL_GPL(show_rcu_gp_kthreads);
> >  
> > +/* Dump grace-period-request information due to commandeered sysrq. 
> > +*/ static void sysrq_show_rcu(int key) {
> > +	show_rcu_gp_kthreads();
> > +}
> > +
> > +static struct sysrq_key_op sysrq_rcudump_op = {
> > +	.handler = sysrq_show_rcu,
> > +	.help_msg = "show-rcu(y)",
> > +	.action_msg = "Show RCU tree",
> > +	.enable_mask = SYSRQ_ENABLE_DUMP,
> > +};
> > +
> > +static int __init rcu_sysrq_init(void) {
> > +	if (sysrq_rcu)
> > +		return register_sysrq_key('y', &sysrq_rcudump_op);
> > +	return 0;
> > +}
> > +early_initcall(rcu_sysrq_init);
> > +
> >  /*
> >   * Send along grace-period-related data for rcutorture diagnostics.
> >   */
> > 
>