Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CAAhV-H58OpQJapV7LDNjZ-vM7nNJrwdkBiPjFcCutO1yRsUshQ@mail.gmail.com>
 <87ttspct76.ffs@tglx> <03fe7084-0509-45fa-87ee-8f8705a221a6@paulmck-laptop>
 <CAAhV-H5Z3s=2_OyA_AJ1-NqXBtNrcs-EmsqYcrjc+qXmJ=SitQ@mail.gmail.com>
 <16827b4e-9823-456d-a6be-157fbfae64c3@paulmck-laptop> <CAAhV-H7uXA=r-w1nN7sBpRTba3LjjZs+wasJfGo7VZ6D9eMBAw@mail.gmail.com>
 <8792da20-a58e-4cc0-b3d2-231d5ade2242@paulmck-laptop> <CAAhV-H5BNPX8Eo3Xdy-jcYY97=xazGU+VVqoDy7qEH+VpVWFJA@mail.gmail.com>
 <24e34f50-32d2-4b67-8ec0-1034c984d035@paulmck-laptop> <CAAhV-H5pfDG_tsRDL4dUYykaQ1ZwQYRDrQccpULBM5+kF4i2fA@mail.gmail.com>
 <20230825232807.GA97898@google.com> <CAEXW_YSock304V471X_A7WrxCWtHJGx3APmSy0k7Lc0o69D9Hg@mail.gmail.com>
In-Reply-To: <CAEXW_YSock304V471X_A7WrxCWtHJGx3APmSy0k7Lc0o69D9Hg@mail.gmail.com>
From:   Huacai Chen <chenhuacai@kernel.org>
Date:   Sun, 27 Aug 2023 13:50:56 +0800
Message-ID: <CAAhV-H6PM_KZj4_h-SdJAaseMDK2nMqqJWL8fWHhL4vUA50bQg@mail.gmail.com>
Subject: Re: [PATCH V4 2/2] rcu: Update jiffies in rcu_cpu_stall_reset()
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     paulmck@kernel.org, Thomas Gleixner <tglx@linutronix.de>,
        Z qiang <qiang.zhang1211@gmail.com>,
        Huacai Chen <chenhuacai@loongson.cn>,
        Frederic Weisbecker <frederic@kernel.org>,
        Neeraj Upadhyay <quic_neeraju@quicinc.com>,
        Josh Triplett <josh@joshtriplett.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        Ingo Molnar <mingo@kernel.org>,
        John Stultz <jstultz@google.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        rcu@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org, Binbin Zhou <zhoubinbin@loongson.cn>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi, Joel,

On Sun, Aug 27, 2023 at 11:27=E2=80=AFAM Joel Fernandes <joel@joelfernandes=
.org> wrote:
>
> On Fri, Aug 25, 2023 at 7:28=E2=80=AFPM Joel Fernandes <joel@joelfernande=
s.org> wrote:
> >
> > On Fri, Aug 25, 2023 at 07:15:44PM +0800, Huacai Chen wrote:
> > > Hi, Paul,
> > >
> > > On Fri, Aug 25, 2023 at 2:28=E2=80=AFAM Paul E. McKenney <paulmck@ker=
nel.org> wrote:
> [..]
> > > > > > > > > > > > On Thu, Aug 17, 2023 at 3:27=E2=80=AFAM Joel Fernan=
des <joel@joelfernandes.org> 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 ji=
ffies lock held
> > > > > > > > > > > region then you still dead lock.
> > > > > > > > > > >
> > > > > > > > > > > >> I dislike that..
> > > > > > > > > > > > Is this acceptable?
> > > > > > > > > > > >
> > > > > > > > > > > > void rcu_cpu_stall_reset(void)
> > > > > > > > > > > > {
> > > > > > > > > > > >         unsigned long delta;
> > > > > > > > > > > >
> > > > > > > > > > > >         delta =3D nsecs_to_jiffies(ktime_get_ns() -=
 ktime_get_coarse_ns());
> > > > > > > > > > > >
> > > > > > > > > > > >         WRITE_ONCE(rcu_state.jiffies_stall,
> > > > > > > > > > > >                    jiffies + delta + rcu_jiffies_ti=
ll_stall_check());
> > > > > > > > > > > > }
> > > > > > > > > > > >
> > > > > > > > > > > > This can update jiffies_stall without updating jiff=
ies (but has the
> > > > > > > > > > > > same effect).
> > > > > > > > > > >
> > > > > > > > > > > Now you traded the potential dead lock on jiffies loc=
k 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 t=
he incomplete and
> > > > > > > > > > > uncompiled below. NMI safe and therefore livelock pro=
of time interfaces
> > > > > > > > > > > exist for a reason.
> > > > > > > > > >
> > > > > > > > > > Just for completeness, another approach, with its own a=
dvantages
> > > > > > > > > > and disadvantage, is to add something like ULONG_MAX/4 =
to
> > > > > > > > > > rcu_state.jiffies_stall, but also set a counter indicat=
ing that this
> > > > > > > > > > has been done.  Then RCU's force-quiescent processing c=
ould 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, b=
ut "live by the
> > > > > > > > > > heuristic, die by the heuristic".  ;-)
> > > > > > > > > >
> > > > > > > > > > It would be good to have some indication when gdb exite=
d, 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*H=
Z, but Joel
> > > > > > > > > dislikes that.
> > > > > > > >
> > > > > > > > I am not seeing the ULONG_MAX/2, so could you please point =
me to that
> > > > > > > > original code?
> > > > > > >
> > > > > > > Maybe I misunderstand something, I say the original code mean=
s code
> > > > > > > before commit a80be428fbc1f1f3bc9ed924 ("rcu: Do not disable =
GP stall
> > > > > > > detection in rcu_cpu_stall_reset()").
> > > > > >
> > > > > > Yes, my suggestion would essentially revert that patch.  It wou=
ld
> > > > > > compensate by resetting rcu_state.jiffies_stall after a few cal=
ls
> > > > > > to rcu_gp_fqs().
> > > > > >
> > > > > > Alternatively, we could simply provide a way for gdb users to m=
anually
> > > > > > disable RCU CPU stall warnings at the beginning of their debug =
sessions
> > > > > > and to manually re-enable them when they are done.
> > > > >
> > > > > This problem is not KGDB-specific (though it is firstly found in =
the
> > > > > KGDB case), so I want to fix it in the rcu code rather than in th=
e
> > > > > kgdb code.
> > > >
> > > > Sure, for example, there is also PowerPC XMON.
> > > >
> > > > But this problem also is not RCU-specific.  There are also hardlock=
ups,
> > > > softlockups, workqueue lockups, networking timeouts, and who knows =
what
> > > > all else.
> > > >
> > > > Plus, and again to Thomas's point, gdb breakpoints can happen anywh=
ere.
> > > > For example, immediately after RCU computes the RCU CPU stall time =
for
> > > > a new grace period, and right before it stores it.  The gdb callout
> > > > updates rcu_state.jiffies_stall, but that update is overwritten wit=
h a
> > > > stale value as soon as the system starts back up.
> > > >
> > > > Low probabillity, to be sure, but there are quite a few places in
> > > > the kernel right after a read from some timebase or another, and ma=
ny
> > > > (perhaps all) of these can see similar stale-time-use problems.
> > > >
> > > > The only way I know of to avoid these sorts of false positives is f=
or
> > > > the user to manually suppress all timeouts (perhaps using a kernel-=
boot
> > > > parameter for your early-boot case), do the gdb work, and then unsu=
ppress
> > > > all stalls.  Even that won't work for networking, because the other
> > > > system's clock will be running throughout.
> > > >
> > > > In other words, from what I know now, there is no perfect solution.
> > > > Therefore, there are sharp limits to the complexity of any solution=
 that
> > > > I will be willing to accept.
> > > I think the simplest solution is (I hope Joel will not angry):
> >
> > Not angry at all, just want to help. ;-). The problem is the 300*HZ sol=
ution
> > will also effect the VM workloads which also do a similar reset.  Allow=
 me few
> > days to see if I can take a shot at fixing it slightly differently. I a=
m
> > trying Paul's idea of setting jiffies at a later time. I think it is do=
able.
> > I think the advantage of doing this is it will make stall detection mor=
e
> > robust in this face of these gaps in jiffie update. And that solution d=
oes
> > not even need us to rely on ktime (and all the issues that come with th=
at).
> >
>
> I wrote a patch similar to Paul's idea and sent it out for review, the
> advantage being it purely is based on jiffies. Could you try it out
> and let me know?
If you can cc my gmail <chenhuacai@gmail.com>, that could be better.

I have read your patch, maybe the counter (nr_fqs_jiffies_stall)
should be atomic_t and we should use atomic operation to decrement its
value. Because rcu_gp_fqs() can be run concurrently, and we may miss
the (nr_fqs =3D=3D 1) condition.

Huacai
>
> thanks,
>
>  - Joel