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:paul...@linux.ibm.com]
> Sent: Thursday, December 13, 2018 10:43
> To: Zhang, Jun <jun.zh...@intel.com>
> Cc: He, Bo <bo...@intel.com>; Steven Rostedt <rost...@goodmis.org>;
> linux-kernel@vger.kernel.org; j...@joshtriplett.org;
> mathieu.desnoy...@efficios.com; jiangshan...@gmail.com; Xiao, Jin
> <jin.x...@intel.com>; Zhang, Yanmin <yanmin.zh...@intel.com>; Bai, Jie A
> <jie.a....@intel.com>; Sun, Yi J <yi.j....@intel.com>
> 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:paul...@linux.ibm.com]
> > Sent: Thursday, December 13, 2018 08:12
> > To: He, Bo <bo...@intel.com>
> > Cc: Steven Rostedt <rost...@goodmis.org>;
> > linux-kernel@vger.kernel.org; j...@joshtriplett.org;
> > mathieu.desnoy...@efficios.com; jiangshan...@gmail.com; Zhang, Jun
> > <jun.zh...@intel.com>; Xiao, Jin <jin.x...@intel.com>; Zhang, Yanmin
> > <yanmin.zh...@intel.com>; Bai, Jie A <jie.a....@intel.com>; Sun, Yi J
> > <yi.j....@intel.com>
> > 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 <paul...@linux.ibm.com>
> > > Sent: Thursday, December 13, 2018 5:03 AM
> > > To: He, Bo <bo...@intel.com>
> > > Cc: Steven Rostedt <rost...@goodmis.org>;
> > > linux-kernel@vger.kernel.org; j...@joshtriplett.org;
> > > mathieu.desnoy...@efficios.com; jiangshan...@gmail.com; Zhang, Jun
> > > <jun.zh...@intel.com>; Xiao, Jin <jin.x...@intel.com>; Zhang, Yanmin
> > > <yanmin.zh...@intel.com>; Bai, Jie A <jie.a....@intel.com>
> > > 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 <paul...@linux.ibm.com>
> > 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 <paul...@linux.ibm.com>
> >
> > 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.
> > */
> >
>
