Received-SPF: pass (google.com: domain of linux-kernel+bounces-175065-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <ZjqWXPFuoYWWcxP3@google.com> <0e239143-65ed-445a-9782-e905527ea572@paulmck-laptop>
 <Zjq9okodmvkywz82@google.com> <ZjrClk4Lqw_cLO5A@google.com>
 <Zjroo8OsYcVJLsYO@LeoBras> <b44962dd-7b8a-4201-90b7-4c39ba20e28d@paulmck-laptop>
 <ZjsZVUdmDXZOn10l@LeoBras> <ZjuFuZHKUy7n6-sG@google.com> <5fd66909-1250-4a91-aa71-93cb36ed4ad5@paulmck-laptop>
 <ZjyGefTZ8ThZukNG@LeoBras> <4e368040-05b0-46ab-bafa-59710d5de549@paulmck-laptop>
In-Reply-To: <4e368040-05b0-46ab-bafa-59710d5de549@paulmck-laptop>
From: Leonardo Bras Soares Passos <leobras@redhat.com>
Date: Thu, 9 May 2024 20:07:22 -0300
Message-ID: <CAJ6HWG4LyyLuQ9ZZ2ayLRt79_m_6-=ZePhHyQV8VTMPjFG+kYg@mail.gmail.com>
Subject: Re: [RFC PATCH v1 0/2] Avoid rcu_core() if CPU just left guest vcpu
To: paulmck@kernel.org
Cc: Sean Christopherson <seanjc@google.com>, Paolo Bonzini <pbonzini@redhat.com>, 
	Frederic Weisbecker <frederic@kernel.org>, Neeraj Upadhyay <quic_neeraju@quicinc.com>, 
	Joel Fernandes <joel@joelfernandes.org>, Josh Triplett <josh@joshtriplett.org>, 
	Boqun Feng <boqun.feng@gmail.com>, Steven Rostedt <rostedt@goodmis.org>, 
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, Lai Jiangshan <jiangshanlai@gmail.com>, 
	Zqiang <qiang.zhang1211@gmail.com>, Marcelo Tosatti <mtosatti@redhat.com>, kvm@vger.kernel.org, 
	linux-kernel@vger.kernel.org, rcu@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, May 9, 2024 at 7:44=E2=80=AFPM Paul E. McKenney <paulmck@kernel.org=
> wrote:
>
> On Thu, May 09, 2024 at 05:16:57AM -0300, Leonardo Bras wrote:
> > On Wed, May 08, 2024 at 08:32:40PM -0700, Paul E. McKenney wrote:
> > > On Wed, May 08, 2024 at 07:01:29AM -0700, Sean Christopherson wrote:
> > > > On Wed, May 08, 2024, Leonardo Bras wrote:
> > > > > Something just hit me, and maybe I need to propose something more=
 generic.
> > > >
> > > > Yes.  This is what I was trying to get across with my complaints ab=
out keying off
> > > > of the last VM-Exit time.  It's effectively a broad stroke "this ta=
sk will likely
> > > > be quiescent soon" and so the core concept/functionality belongs in=
 common code,
> > > > not KVM.
> > >
> > > OK, we could do something like the following wholly within RCU, namel=
y
> > > to make rcu_pending() refrain from invoking rcu_core() until the grac=
e
> > > period is at least the specified age, defaulting to zero (and to the
> > > current behavior).
> > >
> > > Perhaps something like the patch shown below.
> >
> > That's exactly what I was thinking :)
> >
> > >
> > > Thoughts?
> >
> > Some suggestions below:
> >
> > >
> > >                                                     Thanx, Paul
> > >
> > > ---------------------------------------------------------------------=
---
> > >
> > > commit abc7cd2facdebf85aa075c567321589862f88542
> > > Author: Paul E. McKenney <paulmck@kernel.org>
> > > Date:   Wed May 8 20:11:58 2024 -0700
> > >
> > >     rcu: Add rcutree.nocb_patience_delay to reduce nohz_full OS jitte=
r
> > >
> > >     If a CPU is running either a userspace application or a guest OS =
in
> > >     nohz_full mode, it is possible for a system call to occur just as=
 an
> > >     RCU grace period is starting.  If that CPU also has the schedulin=
g-clock
> > >     tick enabled for any reason (such as a second runnable task), and=
 if the
> > >     system was booted with rcutree.use_softirq=3D0, then RCU can add =
insult to
> > >     injury by awakening that CPU's rcuc kthread, resulting in yet ano=
ther
> > >     task and yet more OS jitter due to switching to that task, runnin=
g it,
> > >     and switching back.
> > >
> > >     In addition, in the common case where that system call is not of
> > >     excessively long duration, awakening the rcuc task is pointless.
> > >     This pointlessness is due to the fact that the CPU will enter an =
extended
> > >     quiescent state upon returning to the userspace application or gu=
est OS.
> > >     In this case, the rcuc kthread cannot do anything that the main R=
CU
> > >     grace-period kthread cannot do on its behalf, at least if it is g=
iven
> > >     a few additional milliseconds (for example, given the time durati=
on
> > >     specified by rcutree.jiffies_till_first_fqs, give or take schedul=
ing
> > >     delays).
> > >
> > >     This commit therefore adds a rcutree.nocb_patience_delay kernel b=
oot
> > >     parameter that specifies the grace period age (in milliseconds)
> > >     before which RCU will refrain from awakening the rcuc kthread.
> > >     Preliminary experiementation suggests a value of 1000, that is,
> > >     one second.  Increasing rcutree.nocb_patience_delay will increase
> > >     grace-period latency and in turn increase memory footprint, so sy=
stems
> > >     with constrained memory might choose a smaller value.  Systems wi=
th
> > >     less-aggressive OS-jitter requirements might choose the default v=
alue
> > >     of zero, which keeps the traditional immediate-wakeup behavior, t=
hus
> > >     avoiding increases in grace-period latency.
> > >
> > >     Link: https://lore.kernel.org/all/20240328171949.743211-1-leobras=
@redhat.com/
> > >
> > >     Reported-by: Leonardo Bras <leobras@redhat.com>
> > >     Suggested-by: Leonardo Bras <leobras@redhat.com>
> > >     Suggested-by: Sean Christopherson <seanjc@google.com>
> > >     Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > >
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Docume=
ntation/admin-guide/kernel-parameters.txt
> > > index 0a3b0fd1910e6..42383986e692b 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -4981,6 +4981,13 @@
> > >                     the ->nocb_bypass queue.  The definition of "too
> > >                     many" is supplied by this kernel boot parameter.
> > >
> > > +   rcutree.nocb_patience_delay=3D [KNL]
> > > +                   On callback-offloaded (rcu_nocbs) CPUs, avoid
> > > +                   disturbing RCU unless the grace period has
> > > +                   reached the specified age in milliseconds.
> > > +                   Defaults to zero.  Large values will be capped
> > > +                   at five seconds.
> > > +
> > >     rcutree.qhimark=3D [KNL]
> > >                     Set threshold of queued RCU callbacks beyond whic=
h
> > >                     batch limiting is disabled.
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 7560e204198bb..6e4b8b43855a0 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -176,6 +176,8 @@ static int gp_init_delay;
> > >  module_param(gp_init_delay, int, 0444);
> > >  static int gp_cleanup_delay;
> > >  module_param(gp_cleanup_delay, int, 0444);
> > > +static int nocb_patience_delay;
> > > +module_param(nocb_patience_delay, int, 0444);
> > >
> > >  // Add delay to rcu_read_unlock() for strict grace periods.
> > >  static int rcu_unlock_delay;
> > > @@ -4334,6 +4336,8 @@ EXPORT_SYMBOL_GPL(cond_synchronize_rcu_full);
> > >  static int rcu_pending(int user)
> > >  {
> > >     bool gp_in_progress;
> > > +   unsigned long j =3D jiffies;
> >
> > I think this is probably taken care by the compiler, but just in case I=
 would move the
> > j =3D jiffies;
> > closer to it's use, in order to avoid reading 'jiffies' if rcu_pending
> > exits before the nohz_full testing.
>
> Good point!  I just removed j and used jiffies directly.
>
> > > +   unsigned int patience =3D msecs_to_jiffies(nocb_patience_delay);
> >
> > What do you think on processsing the new parameter in boot, and saving =
it
> > in terms of jiffies already?
> >
> > It would make it unnecessary to convert ms -> jiffies every time we run
> > rcu_pending.
> >
> > (OOO will probably remove the extra division, but may cause less impact=
 in
> > some arch)
>
> This isn't exactly a fastpath, but it is easy enough to do the conversion
> in rcu_bootup_announce_oddness() and place it into another variable
> (for the benefit of those using drgn or going through crash dumps).
>
> > >     struct rcu_data *rdp =3D this_cpu_ptr(&rcu_data);
> > >     struct rcu_node *rnp =3D rdp->mynode;
> > >
> > > @@ -4347,11 +4351,13 @@ static int rcu_pending(int user)
> > >             return 1;
> > >
> > >     /* Is this a nohz_full CPU in userspace or idle?  (Ignore RCU if =
so.) */
> > > -   if ((user || rcu_is_cpu_rrupt_from_idle()) && rcu_nohz_full_cpu()=
)
> > > +   gp_in_progress =3D rcu_gp_in_progress();
> > > +   if ((user || rcu_is_cpu_rrupt_from_idle() ||
> > > +        (gp_in_progress && time_before(j + patience, rcu_state.gp_st=
art))) &&
> >
> > I think you meant:
> >       time_before(j, rcu_state.gp_start + patience)
> >
> > or else this always fails, as we can never have now to happen before a
> > previously started gp, right?
> >
> > Also, as per rcu_nohz_full_cpu() we probably need it to be read with
> > READ_ONCE():
> >
> >       time_before(j, READ_ONCE(rcu_state.gp_start) + patience)
>
> Good catch on both counts, fixed!
>
> > > +       rcu_nohz_full_cpu())
> > >             return 0;
> > >
> > >     /* Is the RCU core waiting for a quiescent state from this CPU? *=
/
> > > -   gp_in_progress =3D rcu_gp_in_progress();
> > >     if (rdp->core_needs_qs && !rdp->cpu_no_qs.b.norm && gp_in_progres=
s)
> > >             return 1;
> > >
> > > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > > index 340bbefe5f652..174333d0e9507 100644
> > > --- a/kernel/rcu/tree_plugin.h
> > > +++ b/kernel/rcu/tree_plugin.h
> > > @@ -93,6 +93,15 @@ static void __init rcu_bootup_announce_oddness(voi=
d)
> > >             pr_info("\tRCU debug GP init slowdown %d jiffies.\n", gp_=
init_delay);
> > >     if (gp_cleanup_delay)
> > >             pr_info("\tRCU debug GP cleanup slowdown %d jiffies.\n", =
gp_cleanup_delay);
> > > +   if (nocb_patience_delay < 0) {
> > > +           pr_info("\tRCU NOCB CPU patience negative (%d), resetting=
 to zero.\n", nocb_patience_delay);
> > > +           nocb_patience_delay =3D 0;
> > > +   } else if (nocb_patience_delay > 5 * MSEC_PER_SEC) {
> > > +           pr_info("\tRCU NOCB CPU patience too large (%d), resettin=
g to %ld.\n", nocb_patience_delay, 5 * MSEC_PER_SEC);
> > > +           nocb_patience_delay =3D 5 * MSEC_PER_SEC;
> > > +   } else if (nocb_patience_delay) {
> >
> > Here you suggest that we don't print if 'nocb_patience_delay =3D=3D 0',
> > as it's the default behavior, right?
>
> Exactly, in keeping with the function name rcu_bootup_announce_oddness().
>
> This approach allows easy spotting of deviations from default settings,
> which can be very helpful when debugging.
>
> > I think printing on 0 could be useful to check if the feature exists, e=
ven
> > though we are zeroing it, but this will probably add unnecessary verbos=
ity.
>
> It could be quite useful to people learning the RCU implementation,
> and I encourage those people to remove all those "if" statements from
> rcu_bootup_announce_oddness() in order to get the full story.
>
> > > +           pr_info("\tRCU NOCB CPU patience set to %d milliseconds.\=
n", nocb_patience_delay);
> > > +   }
> >
> > Here I suppose something like this can take care of not needing to conv=
ert
> > ms -> jiffies every rcu_pending():
> >
> > +     nocb_patience_delay =3D msecs_to_jiffies(nocb_patience_delay);
>
> Agreed, but I used a separate variable to help people looking at crash
> dumps or using drgn.
>
> And thank you for your review and comments!  Applying these changes
> with attribution.
>

Thank you!
Leo

>                                                         Thanx, Paul
>
> > >     if (!use_softirq)
> > >             pr_info("\tRCU_SOFTIRQ processing moved to rcuc kthreads.=
\n");
> > >     if (IS_ENABLED(CONFIG_RCU_EQS_DEBUG))
> > >
> >
> >
> > Thanks!
> > Leo
> >
>