From: Leonardo Bras <leobras@redhat.com>
To: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Leonardo Bras <leobras@redhat.com>,
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
Subject: Re: [RFC PATCH v1 0/2] Avoid rcu_core() if CPU just left guest vcpu
Date: Thu, 9 May 2024 05:16:57 -0300 [thread overview]
Message-ID: <ZjyGefTZ8ThZukNG@LeoBras> (raw)
In-Reply-To: <5fd66909-1250-4a91-aa71-93cb36ed4ad5@paulmck-laptop>
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 about keying off
> > of the last VM-Exit time. It's effectively a broad stroke "this task 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, namely
> to make rcu_pending() refrain from invoking rcu_core() until the grace
> 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 jitter
>
> 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 scheduling-clock
> tick enabled for any reason (such as a second runnable task), and if the
> system was booted with rcutree.use_softirq=0, then RCU can add insult to
> injury by awakening that CPU's rcuc kthread, resulting in yet another
> task and yet more OS jitter due to switching to that task, running 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 guest OS.
> In this case, the rcuc kthread cannot do anything that the main RCU
> grace-period kthread cannot do on its behalf, at least if it is given
> a few additional milliseconds (for example, given the time duration
> specified by rcutree.jiffies_till_first_fqs, give or take scheduling
> delays).
>
> This commit therefore adds a rcutree.nocb_patience_delay kernel boot
> 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 systems
> with constrained memory might choose a smaller value. Systems with
> less-aggressive OS-jitter requirements might choose the default value
> of zero, which keeps the traditional immediate-wakeup behavior, thus
> 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/Documentation/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= [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= [KNL]
> Set threshold of queued RCU callbacks beyond which
> 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 = jiffies;
I think this is probably taken care by the compiler, but just in case I would move the
j = jiffies;
closer to it's use, in order to avoid reading 'jiffies' if rcu_pending
exits before the nohz_full testing.
> + unsigned int patience = 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)
> struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> struct rcu_node *rnp = 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 = rcu_gp_in_progress();
> + if ((user || rcu_is_cpu_rrupt_from_idle() ||
> + (gp_in_progress && time_before(j + patience, rcu_state.gp_start))) &&
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)
> + rcu_nohz_full_cpu())
> return 0;
>
> /* Is the RCU core waiting for a quiescent state from this CPU? */
> - gp_in_progress = rcu_gp_in_progress();
> if (rdp->core_needs_qs && !rdp->cpu_no_qs.b.norm && gp_in_progress)
> 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(void)
> 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 = 0;
> + } else if (nocb_patience_delay > 5 * MSEC_PER_SEC) {
> + pr_info("\tRCU NOCB CPU patience too large (%d), resetting to %ld.\n", nocb_patience_delay, 5 * MSEC_PER_SEC);
> + nocb_patience_delay = 5 * MSEC_PER_SEC;
> + } else if (nocb_patience_delay) {
Here you suggest that we don't print if 'nocb_patience_delay == 0',
as it's the default behavior, right?
I think printing on 0 could be useful to check if the feature exists, even
though we are zeroing it, but this will probably add unnecessary verbosity.
> + 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 convert
ms -> jiffies every rcu_pending():
+ nocb_patience_delay = msecs_to_jiffies(nocb_patience_delay);
> if (!use_softirq)
> pr_info("\tRCU_SOFTIRQ processing moved to rcuc kthreads.\n");
> if (IS_ENABLED(CONFIG_RCU_EQS_DEBUG))
>
Thanks!
Leo
next prev parent reply other threads:[~2024-05-09 8:17 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-28 17:19 [RFC PATCH v1 0/2] Avoid rcu_core() if CPU just left guest vcpu Leonardo Bras
2024-03-28 17:19 ` [RFC PATCH v1 1/2] kvm: Implement guest_exit_last_time() Leonardo Bras
2024-03-28 17:19 ` [RFC PATCH v1 2/2] rcu: Ignore RCU in nohz_full cpus if it was running a guest recently Leonardo Bras
2024-04-01 15:52 ` Paul E. McKenney
2024-04-01 20:21 ` [RFC PATCH v1 0/2] Avoid rcu_core() if CPU just left guest vcpu Sean Christopherson
2024-04-05 13:45 ` Marcelo Tosatti
2024-04-05 14:42 ` Sean Christopherson
2024-04-06 0:03 ` Paul E. McKenney
2024-04-08 17:16 ` Sean Christopherson
2024-04-08 18:42 ` Paul E. McKenney
2024-04-08 20:06 ` Sean Christopherson
2024-04-08 21:02 ` Paul E. McKenney
2024-04-08 21:56 ` Sean Christopherson
2024-04-08 22:35 ` Paul E. McKenney
2024-04-08 23:06 ` Sean Christopherson
2024-04-08 23:20 ` Paul E. McKenney
2024-04-10 2:39 ` Marcelo Tosatti
2024-04-15 19:47 ` Marcelo Tosatti
2024-04-15 21:29 ` Sean Christopherson
2024-04-16 12:36 ` Marcelo Tosatti
2024-04-16 14:07 ` Sean Christopherson
2024-04-17 16:14 ` Marcelo Tosatti
2024-04-17 17:22 ` Sean Christopherson
2024-05-03 20:44 ` Leonardo Bras
2024-05-06 18:47 ` Marcelo Tosatti
2024-05-07 18:05 ` Sean Christopherson
2024-05-07 22:36 ` Leonardo Bras
2024-05-03 18:42 ` Leonardo Bras
2024-05-03 19:09 ` Leonardo Bras
2024-05-03 21:29 ` Sean Christopherson
2024-05-03 22:00 ` Leonardo Bras
2024-05-03 22:00 ` Paul E. McKenney
2024-05-07 17:55 ` Sean Christopherson
2024-05-07 19:15 ` Paul E. McKenney
2024-05-07 21:00 ` Sean Christopherson
2024-05-07 21:37 ` Paul E. McKenney
2024-05-07 23:47 ` Sean Christopherson
2024-05-08 0:08 ` Sean Christopherson
2024-05-08 2:51 ` Leonardo Bras
2024-05-08 3:22 ` Paul E. McKenney
2024-05-08 6:19 ` Leonardo Bras
2024-05-08 14:01 ` Sean Christopherson
2024-05-09 3:32 ` Paul E. McKenney
2024-05-09 8:16 ` Leonardo Bras [this message]
2024-05-09 10:14 ` Leonardo Bras
2024-05-09 23:45 ` Paul E. McKenney
2024-05-10 16:06 ` Leonardo Bras
2024-05-10 16:21 ` Paul E. McKenney
2024-05-10 17:12 ` Leonardo Bras
2024-05-10 17:41 ` Paul E. McKenney
2024-05-10 19:50 ` Leonardo Bras
2024-05-10 21:15 ` Leonardo Bras
2024-05-10 21:38 ` Paul E. McKenney
2024-05-09 22:41 ` Paul E. McKenney
2024-05-09 23:07 ` Leonardo Bras Soares Passos
2024-05-11 2:08 ` Leonardo Bras
2024-05-08 3:20 ` Paul E. McKenney
2024-05-08 4:04 ` Paul E. McKenney
2024-05-08 14:36 ` Paul E. McKenney
2024-05-08 15:35 ` Sean Christopherson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZjyGefTZ8ThZukNG@LeoBras \
--to=leobras@redhat.com \
--cc=boqun.feng@gmail.com \
--cc=frederic@kernel.org \
--cc=jiangshanlai@gmail.com \
--cc=joel@joelfernandes.org \
--cc=josh@joshtriplett.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=mtosatti@redhat.com \
--cc=paulmck@kernel.org \
--cc=pbonzini@redhat.com \
--cc=qiang.zhang1211@gmail.com \
--cc=quic_neeraju@quicinc.com \
--cc=rcu@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=seanjc@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).