From: "Zhang, Qiang1" <qiang1.zhang@intel.com>
To: Boqun Feng <boqun.feng@gmail.com>,
"quic_neeraju@quicinc.com" <quic_neeraju@quicinc.com>
Cc: "paulmck@kernel.org" <paulmck@kernel.org>,
"frederic@kernel.org" <frederic@kernel.org>,
"rcu@vger.kernel.org" <rcu@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] rcu: Add exp QS check in rcu_exp_handler() for no-preemptible expedited RCU
Date: Wed, 22 Jun 2022 23:34:15 +0000 [thread overview]
Message-ID: <PH0PR11MB58809E7A0BF02907DAA826AADAB29@PH0PR11MB5880.namprd11.prod.outlook.com> (raw)
In-Reply-To: <YrNQpxjIiNpxwyQh@boqun-archlinux>
On Wed, Jun 22, 2022 at 06:35:49PM +0800, Zqiang wrote:
> In CONFIG_PREEMPT=n and CONFIG_PREEMPT_COUNT=y kernel, after a exp
> grace period begins, if detected current CPU enters idle in
> rcu_exp_handler() IPI handler, will immediately report the exp QS of the
> current cpu, at this time, maybe not being in an RCU read-side critical
> section, but need wait until rcu-softirq or sched-clock irq or sched-switch
> occurs on current CPU to check and report exp QS.
>
>I think the idea is OK, however, this "optimization" is based on the
>implementation detail that rcu_read_lock() counts preempt_count when
>CONFIG_PREEMPT_COUNT=y, right? It's a little bit dangerous because the
>preempt_count when CONFIG_PREEMPT_COUNT=y and CONFIG_PREEMPT=n is mostly
>for debugging purposes IIUC, and in other words, _it could be gone_.
>
Yes, for CONFIG_PREEMPT_COUNT=y and CONFIG_PREEMPT=n kernel
The rcu_read_lock/unlock are replaced by preempt_disbale/enable, and the
preempt-count is exists, so can report exp QS when not being an RCU
read-side critical(preempt_count & (PREEMPT_MASK | SOFTIRQ_MASK )return zero).
in IPI handler.
For CONFIG_PREEMPT_COUNT=n and CONFIG_PREEMPT=n kernel,
The rcu_read_lock/unlock is just barrier().
So I add IS_ENABLED(CONFIG_PREEMPT_COUNT) check in code.
Of course, for CONFIG_PREEMPT_COUNT=n kernel, in RCU softirq, the
preempt-count is also checked
/* Report any deferred quiescent states if preemption enabled. */
if (IS_ENABLED(CONFIG_PREEMPT_COUNT) && (!(preempt_count() & PREEMPT_MASK))) {
rcu_preempt_deferred_qs(current);
but the RCU softirq may not be triggered in time and reported exp QS, for
example a kernel loop exist on NO_HZ_FULL CPU
this change, It is to capture the exp QS state earlier and report it.
>Also I'm not aware of any but there could be someone assuming that RCU
>read-side critical sections can be formed without
>rcu_read_{lock,unlock}() in CONFIG_PREEMPT=n kernel. For example, there
>might be "creative" code like the following:
>
> void do_something_only_in_nonpreempt(void)
> {
> int *p;
>
> // This function only gets called in PREEMPT=n kernel,
> // which means everywhere is a RCU read-side critical
> // section, let's save some lines of code.
>
rcu_read_lock();
> p = rcu_dereference_check(gp, !IS_ENABLED(PREEMPT));
> ... // of course no schedule() here.
> <access p>
rcu_read_unlock();
> }
>
Usually access to pointers of type rcu needs to be protected.
Any thoughts?
>Again, I'm not aware of any existing code that does this but we need to
>be sure.
>
>Regards,
>Boqun
>
> This commit add a exp QS check in rcu_exp_handler(), when not being
> in an RCU read-side critical section, report exp QS earlier.
>
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> ---
> kernel/rcu/tree_exp.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> index be667583a554..34f08267410f 100644
> --- a/kernel/rcu/tree_exp.h
> +++ b/kernel/rcu/tree_exp.h
> @@ -828,11 +828,14 @@ static void rcu_exp_handler(void *unused)
> {
> struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> struct rcu_node *rnp = rdp->mynode;
> + bool preempt_bh_disabled =
> + !!(preempt_count() & (PREEMPT_MASK | SOFTIRQ_MASK));
>
> if (!(READ_ONCE(rnp->expmask) & rdp->grpmask) ||
> __this_cpu_read(rcu_data.cpu_no_qs.b.exp))
> return;
> - if (rcu_is_cpu_rrupt_from_idle()) {
> + if (rcu_is_cpu_rrupt_from_idle() ||
> + (IS_ENABLED(CONFIG_PREEMPT_COUNT) && !preempt_bh_disabled)) {
> rcu_report_exp_rdp(this_cpu_ptr(&rcu_data));
> return;
> }
> --
> 2.25.1
>
next prev parent reply other threads:[~2022-06-22 23:34 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-22 10:35 [PATCH] rcu: Add exp QS check in rcu_exp_handler() for no-preemptible expedited RCU Zqiang
2022-06-22 12:26 ` Neeraj Upadhyay
2022-06-22 17:25 ` Boqun Feng
2022-06-22 23:34 ` Zhang, Qiang1 [this message]
2022-06-22 23:36 ` Zhang, Qiang1
2022-06-23 0:34 ` Paul E. McKenney
2022-06-23 2:15 ` Boqun Feng
2022-06-23 3:23 ` Zhang, Qiang1
2022-06-27 21:41 ` Paul E. McKenney
2022-06-28 4:32 ` Zhang, Qiang1
2022-07-05 19:14 ` Paul E. McKenney
2022-07-06 2:06 ` Zhang, Qiang1
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=PH0PR11MB58809E7A0BF02907DAA826AADAB29@PH0PR11MB5880.namprd11.prod.outlook.com \
--to=qiang1.zhang@intel.com \
--cc=boqun.feng@gmail.com \
--cc=frederic@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=paulmck@kernel.org \
--cc=quic_neeraju@quicinc.com \
--cc=rcu@vger.kernel.org \
/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 NNTP newsgroup(s).