Re: Allow multiple GP misses before Panic

["Paul E. McKenney" <paulmck@kernel.org>]

On Thu, Aug 13, 2020 at 11:50:47AM -0700, Chao Zhou wrote:
> Thanks Paul for the insights!
> 
> I studied the 3 options and think that #1+#3 offers both flexibility
> to users and coverage of boundary user cases.
> 
> For example, as an user of RCU, we want the warnings to be spilled at
> the default 21 seconds so that we know such events are happening. At
> the same time, we want Panic to happen if the stall is long enough to
> significantly affect available system memory on our system.

OK, good, you want to panic only on long stalls, not on multiple short
stalls.  Makes sense!

By the way, the short stalls sometimes indicate bugs that should
be fixed.  For example, there might be a long loop in the kernel
that needs an added cond_resched().  Fixing those would make your
systems less noisy.

I am not asking you to do anything I don't do myself:

0a3b3c253a1e ("mm/mmap.c: Add cond_resched() for exit_mmap() CPU stalls")
9f47eb5461aa ("fs/btrfs: Add cond_resched() for try_release_extent_mapping() stalls")

> Here is the plan based on our discussion, please advise if not inline
> with the idea:
> 1. modify panic_on_rcu_stall to be the maximum number of consecutive
> warnings to trigger Panic.
>     1) change its name to max_rcu_stall_to_panic,

This change would make sense were it not for the possibility that there
are lots of existing "panic_on_rcu_stall" instances in scripts, boot
parameters, and so on.  We need the old name for compatibility, and with
"0" and "1" having the same meanings as before.  We -can- use other
numbers because those other numbers were not legal before.

So we don't get to change the name.

>     2) default value to 1, which is the same behavior as today's.

The default value must be zero, which needs to mean "don't ever panic".
Setting the value to "1" must mean "panic after the very first RCU CPU
stall warning.  Other values can have other meanings.

But you could use the existing panic_on_rcu_stall to be (say) the number
of stall-warning messages that will be tolerated in a given grace period
before panicking.

Then you could create a new panic_on_rcu_stall_age or similar that
does the time-based panicking.

Either way, people currently using panic_on_rcu_stall need to be able
to continue using panic_on_rcu_stall exactly as they are without seeing
any change in behavior.  We don't break users if we have a choice,
and here we really do have a choice.  So we don't break users.

> 2. use ((struct rcu_state *)->gpnum - (struct rcu_data *)->gpnum) >=
> max_rcu_stall_to_panic as condition to trigger Panic;

This probably won't do what you want.  This would trigger only if
the rcu_data structure's ->gpnum were to lag behind the rcu_state
structure's ->gpnum, which happens now whenever a CPU is idle for
an extended time period.

For the time-based approach (#1 below), the idea would be to compare
against the rcu_state structure's ->gp_start field, and even then only
when a grace period is actually in progress.  One way to do this is
to check against the "gps" local variable in check_cpu_stall() in
kernel/rcu/tree_stall.h.

> 3. reset (struct rcu_data *)->gpnum to (struct rcu_state *)->gpnum
> every time a new grace period starts;

Touching every rcu_data structure at the beginning of each grace
period will slow things down.  This slowdown is avoided in the
current code by having each CPU update its rcu_data structure's
->gpnum when it notices a new grace period.

And ->gpnum and ->completed have been combined into a single ->gp_seq
in recent kernels.

So it is better to compare against ->gp_start (AKA "gps") as discussed
above.

> 4. add a new member (struct rcu_data *)->gpmiss that is incremented at
> each miss to track how many misses so far for statistics/debug
> purpose.

This is for #3 below, correct?

If so, please instead create an ->nstalls_cur_gp or similar in the
rcu_state structure.  Zero this at the beginning of every grace
period (in record_gp_stall_check_time() in kernel/rcu/tree_stall.h)
and increment and check it in both legs of the "if" statement at the
end of check_cpu_stall() in this same file.

> Your insights and advice are highly appreciated.

Please let me know how it goes!

							Thanx, Paul

> Thanks!
> 
> Chao
> 
> On Thu, Aug 13, 2020 at 11:19 AM Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > On Thu, Aug 13, 2020 at 10:22:09AM -0700, Chao Zhou wrote:
> > > Hi,
> > >
> > > Some RCU stalls are transient and a system is fully capable to recover
> > > after that, but we do want Panic after certain amount of GP misses.
> > >
> > > Current module parameter rcu_cpu_stall_panic only turn on/off Panic,
> > > and 1 GP miss will trigger Panic when it is enabled.
> > >
> > > Plan to add a module parameter for users to fine-tune how many GP
> > > misses are allowed before Panic.
> > >
> > > To save our precious time, a diff has been tested on our systems and
> > > it works and solves our problem in transient RCU stall events.
> > >
> > > Your insights and guidance is highly appreciated.
> >
> > Please feel free to post a patch.  I could imagine a number of things
> > you might be doing from your description above:
> >
> > 1.      Having a different time for panic, so that (for example) an
> >         RCU CPU stall warning appears at 21 seconds (in mainline), and
> >         if the grace period still has not ended at some time specified
> >         by some kernel parameter.  For example, one approach would be
> >         to make the existing panic_on_rcu_stall sysctl take an integer
> >         instead of a boolean, and to make that integer specify how old
> >         the stall-warned grace period must be before panic() is invoked.
> >
> > 2.      Instead use the number of RCU CPU stall warning messages to
> >         trigger the panic, so that (for example), the panic would happen
> >         on the tenth message.  Again, the panic_on_rcu_stall sysctl
> >         might be used for this.
> >
> > 3.      Like #2, but reset the count every time a new grace period
> >         starts.  So if the panic_on_rcu_stall sysctl was set to
> >         ten, there would need to be ten RCU CPU stall warnings for
> >         the same grace period before panic() was invoked.
> >
> > Of the above three, #1 and #3 seem the most attractive, with a slight
> > preference for #1.
> >
> > Or did you have something else in mind?
> >
> >                                                         Thanx, Paul