Messages in this thread | | | Date | Wed, 7 Mar 2018 07:03:43 -0800 | From | "Paul E. McKenney" <> | Subject | Re: [RFC] rcu: Prevent expedite reporting within RCU read-side section |
| |
On Wed, Mar 07, 2018 at 03:25:36PM +0900, Byungchul Park wrote: > 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@lge.com> > >>>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.
You mean rcu_report_exp_cpu_mult()? If so, which calls to this function do you believe should be removed?
(Please note that there are likely to be changes in this area soon, but still, I would like to understand what might be make more efficient.)
Thanx, Paul
> >>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@lge.com> > >>>--- > >>> 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 >
| |