Re: [PATCH V4 2/2] rcu: Update jiffies in rcu_cpu_stall_reset()

[Paul E. McKenney]

On Thu, Aug 24, 2023 at 10:50:41AM +0800, Huacai Chen wrote:
> Hi, Paul,
> 
> On Thu, Aug 24, 2023 at 6:41 AM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> >
> > On Thu, Aug 24, 2023 at 12:03:25AM +0200, Thomas Gleixner wrote:
> > > On Thu, Aug 17 2023 at 16:06, Huacai Chen wrote:
> > > > On Thu, Aug 17, 2023 at 3:27 AM Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
> > > >> > If  do_update_jiffies_64() cannot be used in NMI context,
> > > >>
> > > >> Can you not make the jiffies update conditional on whether it is
> > > >> called within NMI context?
> > >
> > > Which solves what? If KGDB has a breakpoint in the jiffies lock held
> > > region then you still dead lock.
> > >
> > > >> I dislike that..
> > > > Is this acceptable?
> > > >
> > > > void rcu_cpu_stall_reset(void)
> > > > {
> > > >         unsigned long delta;
> > > >
> > > >         delta = nsecs_to_jiffies(ktime_get_ns() - ktime_get_coarse_ns());
> > > >
> > > >         WRITE_ONCE(rcu_state.jiffies_stall,
> > > >                    jiffies + delta + rcu_jiffies_till_stall_check());
> > > > }
> > > >
> > > > This can update jiffies_stall without updating jiffies (but has the
> > > > same effect).
> > >
> > > Now you traded the potential dead lock on jiffies lock for a potential
> > > live lock vs. tk_core.seq. Not really an improvement, right?
> > >
> > > The only way you can do the above is something like the incomplete and
> > > uncompiled below. NMI safe and therefore livelock proof time interfaces
> > > exist for a reason.
> >
> > Just for completeness, another approach, with its own advantages
> > and disadvantage, is to add something like ULONG_MAX/4 to
> > rcu_state.jiffies_stall, but also set a counter indicating that this
> > has been done.  Then RCU's force-quiescent processing could decrement
> > that counter (if non-zero) and reset rcu_state.jiffies_stall when it
> > does reach zero.
> >
> > Setting the counter to three should cover most cases, but "live by the
> > heuristic, die by the heuristic".  ;-)
> >
> > It would be good to have some indication when gdb exited, but things
> > like the gdb "next" command can make that "interesting" when applied to
> > a long-running function.
> 
> The original code is adding ULONG_MAX/2, so adding ULONG_MAX/4 may
> make no much difference? The simplest way is adding 300*HZ, but Joel
> dislikes that.

I am not seeing the ULONG_MAX/2, so could you please point me to that
original code?

The advantage of ULONG_MAX/4 over ULONG_MAX/2 is that the time_after()
and time_before() macros have ULONG_MAX/4 slop in either direction
before giving you the wrong answer.  You can get nearly the same result
using ULONG_MAX/2, but it requires a bit more care.  And even on 32-bit
HZ=1000 systems, ULONG_MAX/4 gets you more than 12 days of gdb session
or jiffies-update delay before you start getting false positives.

Then things can be reset after (say) 3 calls to rcu_gp_fqs() and
also the current reset at the beginning of a grace period, which
is in record_gp_stall_check_time().

It would be better if RCU could get notified at both ends of the debug
session, but given gdb commands such as "next", along with Thomas's
point about gdb breakpoints being pretty much anywhere, this might or
might not be so helpful in real life.  But worth looking into.

							Thanx, Paul

> Huacai
> 
> >
> >                                                         Thanx, Paul
> >
> > > Thanks,
> > >
> > >         tglx
> > > ---
> > > --- a/kernel/time/tick-sched.c
> > > +++ b/kernel/time/tick-sched.c
> > > @@ -51,6 +51,13 @@ struct tick_sched *tick_get_tick_sched(i
> > >   */
> > >  static ktime_t last_jiffies_update;
> > >
> > > +unsigned long tick_estimate_stale_jiffies(void)
> > > +{
> > > +     ktime_t delta = ktime_get_mono_fast_ns() - READ_ONCE(last_jiffies_update);
> > > +
> > > +     return delta < 0 ? 0 : div_s64(delta, TICK_NSEC);
> > > +}
> > > +
> > >  /*
> > >   * Must be called with interrupts disabled !
> > >   */
> > >
> > >