Re: [RFC] rcu: Prevent expedite reporting within RCU read-side section
From: Byungchul Park
Date: Wed Mar 07 2018 - 01:27:27 EST
On 3/7/2018 2:55 PM, Byungchul Park wrote:
On 3/6/2018 10:42 PM, Boqun Feng wrote:
On Tue, Mar 06, 2018 at 02:31:58PM +0900, Byungchul Park wrote:
Hello Paul and RCU folks,
I am afraid I correctly understand and fix it. But I really wonder why
sync_rcu_exp_handler() reports the quiescent state even in the case that
current task is within a RCU read-side section. Do I miss something?
If I correctly understand it and you agree with it, I can add more logic
which make it more expedited by boosting current or making it urgent
when we fail to report the quiescent state on the IPI.
----->8-----
ÂFrom 0b0191f506c19ce331a1fdb7c2c5a00fb23fbcf2 Mon Sep 17 00:00:00 2001
From: Byungchul Park <byungchul.park@xxxxxxx>
Date: Tue, 6 Mar 2018 13:54:41 +0900
Subject: [RFC] rcu: Prevent expedite reporting within RCU read-side
section
We report the quiescent state for this cpu if it's out of RCU read-side
section at the moment IPI was just fired during the expedite process.
However, current code reports the quiescent state even in the case:
ÂÂÂ 1) the current task is still within a RCU read-side section
ÂÂÂ 2) the current task has been blocked within the RCU read-side
section
If this happens, the task will queue itself in
rcu_preempt_note_context_switch() using rcu_preempt_ctxt_queue(). The gp
kthread will wait for this task to dequeue itself. IOW, we have other
mechanism to wait for this task other than bottom-up qs reporting tree.
So I think we are fine here.
Right. Basically we consider both the quiscent state within the current
task and queued tasks on rcu nodes that you mentioned, to control grace
periods when PREEMPT kernel is used.
Actually my concern was if it's safe to clear the bit of 'expmask' on
the IPI for all possible cases, even though anyway blocked tasks would
try to prevent the grace period from ending.
I worried if something subtle might cause problems, but the code looks
fine on second thought as you said. Thank you for your explanation.
In addition, by making quiescent states reported and bits of expmask
cleared only when it's out of rcu read sections, of course keeping
other mechanism unchanged like what you mentioned, I think we can avoid
unnecessary locking ops and other statements, keeping the code still
sane, even though the benefit might be small.
For example, by removing some evitable calls to rcu_report_cpu_mult()
either directly or indirectly. I'm not sure if RCU maintainers think
it's worthy tho.
Regards,
Boqun
Since we don't get to the quiescent state yet in the case, we shouldn't
report it but check it another time.
Signed-off-by: Byungchul Park <byungchul.park@xxxxxxx>
---
 kernel/rcu/tree_exp.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index 73e1d3d..cc69d14 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -731,13 +731,13 @@ static void sync_rcu_exp_handler(void *info)
ÂÂÂÂÂ /*
ÂÂÂÂÂÂ * We are either exiting an RCU read-side critical section
(negative
ÂÂÂÂÂÂ * values of t->rcu_read_lock_nesting) or are not in one at all
- * (zero value of t->rcu_read_lock_nesting). Or we are in an RCU
-ÂÂÂÂ * read-side critical section that blocked before this expedited
- * grace period started. Either way, we can immediately report
-ÂÂÂÂ * the quiescent state.
+ÂÂÂÂ * (zero value of t->rcu_read_lock_nesting). We can immediately
+ÂÂÂÂ * report the quiescent state.
ÂÂÂÂÂÂ */
-ÂÂÂ rdp = this_cpu_ptr(rsp->rda);
-ÂÂÂ rcu_report_exp_rdp(rsp, rdp, true);
+ÂÂÂ if (t->rcu_read_lock_nesting <= 0) {
+ÂÂÂÂÂÂÂ rdp = this_cpu_ptr(rsp->rda);
+ÂÂÂÂÂÂÂ rcu_report_exp_rdp(rsp, rdp, true);
+ÂÂÂ }
 }
 /**
--
1.9.1
--
Thanks,
Byungchul