Re: [PATCH V3] rcu: Update jiffies locally in rcu_cpu_stall_reset()

[Huacai Chen]

On Tue, Aug 22, 2023 at 11:58 PM Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Aug 22, 2023 at 11:48 AM Huacai Chen <chenhuacai@xxxxxxxxxx> wrote:
> >
> > Hi, Joel,
> >
> > On Tue, Aug 22, 2023 at 11:34 PM Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Tue, Aug 22, 2023 at 12:02:48PM +0800, Huacai Chen wrote:
> > > > The KGDB initial breakpoint gets an rcu stall warning after commit
> > > > a80be428fbc1f1f3bc9ed924 ("rcu: Do not disable GP stall detection in
> > > > rcu_cpu_stall_reset()").
> > > >
> > > > [   53.452051] rcu: INFO: rcu_preempt self-detected stall on CPU
> > > [...]
> > > >
> > > > [1] https://lore.kernel.org/rcu/20230814020045.51950-1-chenhuacai@xxxxxxxxxxx/T/#t
> > > >
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > Fixes: a80be428fbc1f1f3bc9ed924 ("rcu: Do not disable GP stall detection in rcu_cpu_stall_reset()")
> > > > Reported-off-by: Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
> > > > Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx>
> > > > ---
> > > > V2: Use NMI safe functions.
> > > > V3: Add comments to explain why.
> > > >
> > > >  kernel/rcu/tree_stall.h | 17 ++++++++++++++++-
> > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
> > > > index b10b8349bb2a..e4e53113d062 100644
> > > > --- a/kernel/rcu/tree_stall.h
> > > > +++ b/kernel/rcu/tree_stall.h
> > > > @@ -150,11 +150,26 @@ static void panic_on_rcu_stall(void)
> > > >   * rcu_cpu_stall_reset - restart stall-warning timeout for current grace period
> > > >   *
> > > >   * The caller must disable hard irqs.
> > > > + *
> > > > + * The jiffies updating may be delayed for a very long time due to tickless and
> > > > + * irq disabled, especially in the KGDB case, so we need to add the delayed time
> > > > + * (delta) to rcu_state.jiffies_stall.
> > > > + *
> > > > + * This function may be called in NMI context, so we cannot use ktime_get_ns()
> > > > + * and ktime_get_coarse_ns(). Instead, we use their inaccurate but safe friends
> > > > + * ktime_get_mono_fast_ns() and ktime_get_seconds() which will cause rcu_state.
> > > > + * jiffies_stall to be a little large than expected (harmless and safer).
> > > >   */
> > > >  void rcu_cpu_stall_reset(void)
> > > >  {
> > > > +     u64 curr, last, delta;
> > > > +
> > > > +     curr = ktime_get_mono_fast_ns();
> > > > +     last = ktime_get_seconds() * NSEC_PER_SEC;
> > > > +     delta = nsecs_to_jiffies(curr - last);
> > > > +
> > > >       WRITE_ONCE(rcu_state.jiffies_stall,
> > > > -                jiffies + rcu_jiffies_till_stall_check());
> > > > +                jiffies + delta + rcu_jiffies_till_stall_check());
> > > >  }
> > >
> > > I prefer the following diff on top of your patch to take advantage of UBSAN
> > > detecting overflows.
> > >
> > > If you take my diff, feel free to add:
> > > Reviewed-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> > >
> > > ---8<-----------------------
> > >
> > > diff --git a/kernel/rcu/tree_stall.h b/kernel/rcu/tree_stall.h
> > > index 5e9e4779bdf1..3398cf2d19c5 100644
> > > --- a/kernel/rcu/tree_stall.h
> > > +++ b/kernel/rcu/tree_stall.h
> > > @@ -162,14 +162,15 @@ static void panic_on_rcu_stall(void)
> > >   */
> > >  void rcu_cpu_stall_reset(void)
> > >  {
> > > -       u64 curr, last, delta;
> > > +       ktime_t last, delta_ns;
> > > +       u64 delta_jiff;
> > >
> > > -       curr = ktime_get_mono_fast_ns();
> > >         last = ktime_get_seconds() * NSEC_PER_SEC;
> > > -       delta = nsecs_to_jiffies(curr - last);
> > > +       delta_ns = ktime_sub(ktime_get_mono_fast_ns(), last);
> > Though ktime_t is the same as s64/u64 now,
>
> No, you are incorrect.  ktime_t is a signed int not u64. I prefer to
> use types designed for a specific purpose where possible. It makes the
> code safer.
Yes ktime_t is s64 now, I said s64/u64 because there is some existing
code to mix them (of course not very good).

>
> > but I think we'd better to
> > not mix them. Then, ktime_get() and ktime_get_coarse() return ktime_t;
> > ktime_get_ns(), ktime_get_coarse_ns() and ktime_get_mono_fast_ns()
> > return s64/u64 (means nanoseconds); ktime_get_seconds() returns
> > seconds but ktime_get_seconds() * NSEC_PER_SEC is also nanoseconds.
> > So, the type definition in your diff is not suitable,
> > ktime_sub(ktime_get_mono_fast_ns(), last) is not suitable too.
> >
>
> Is there a technical reason for your concern? ktime_get_mono_fast_ns()
> is assigned to ktime_t in other places. I am not seeing what the
> problem is.
My concern is ktime_t may not always be s64 in future, so there are
ktime_to_ns() and some other similar APIs to convert from one to
another.

Huacai
>
> Thanks.