[RFC,2/3] rcu: Simplify rcu_note_context_switch exit from critical section
diff mbox series

Message ID 20190701040415.219001-2-joel@joelfernandes.org
State New
Headers show
Series
  • [RFC,1/3] rcu: Expedite the rcu quiescent state reporting if help needed
Related show

Commit Message

Joel Fernandes July 1, 2019, 4:04 a.m. UTC
The rcu_preempt_note_context_switch() tries to handle cases where
__rcu_read_unlock() got preempted and then the context switch path does
the reporting of the quiscent state along with clearing any bits in the
rcu_read_unlock_special union.

This can be handled by just calling rcu_deferred_qs() which was added
during the RCU consolidation work and already does these checks.

Tested RCU config TREE03 for an hour which succeeds.

Cc: rcu@vger.kernel.org
Cc: kernel-team@android.com
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree_plugin.h | 9 ---------
 1 file changed, 9 deletions(-)

Comments

Paul E. McKenney July 1, 2019, 8:03 p.m. UTC | #1
On Mon, Jul 01, 2019 at 12:04:14AM -0400, Joel Fernandes (Google) wrote:
> The rcu_preempt_note_context_switch() tries to handle cases where
> __rcu_read_unlock() got preempted and then the context switch path does
> the reporting of the quiscent state along with clearing any bits in the
> rcu_read_unlock_special union.
> 
> This can be handled by just calling rcu_deferred_qs() which was added
> during the RCU consolidation work and already does these checks.
> 
> Tested RCU config TREE03 for an hour which succeeds.
> 
> Cc: rcu@vger.kernel.org
> Cc: kernel-team@android.com
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

My first reaction was "that cannot possibly work", but after a bit of
digging, it really does appear to work just fine.  I therefore expanded
the commit log a bit, so please check it to catch any messups on my part.

Very cool, thank you very much!  ;-)

							Thanx, Paul

------------------------------------------------------------------------

commit ce547cb41ed7662f70d0b07d4c7f7555ba130c61
Author: Joel Fernandes (Google) <joel@joelfernandes.org>
Date:   Mon Jul 1 00:04:14 2019 -0400

    rcu: Simplify rcu_note_context_switch exit from critical section
    
    Because __rcu_read_unlock() can be preempted just before the call to
    rcu_read_unlock_special(), it is possible for a task to be preempted just
    before it would have fully exited its RCU read-side critical section.
    This would result in a needless extension of that critical section until
    that task was resumed, which might in turn result in a needlessly
    long grace period, needless RCU priority boosting, and needless
    force-quiescent-state actions.  Therefore, rcu_note_context_switch()
    invokes __rcu_read_unlock() followed by rcu_preempt_deferred_qs() when
    it detects this situation.  This action by rcu_note_context_switch()
    ends the RCU read-side critical section immediately.
    
    Of course, once the task resumes, it will invoke rcu_read_unlock_special()
    redundantly.  This is harmless because the fact that a preemption
    happened means that interrupts, preemption, and softirqs cannot
    have been disabled, so there would be no deferred quiescent state.
    While ->rcu_read_lock_nesting remains less than zero, none of the
    ->rcu_read_unlock_special.b bits can be set, and they were all zeroed by
    the call to rcu_note_context_switch() at task-preemption time.  Therefore,
    setting ->rcu_read_unlock_special.b.exp_hint to false has no effect.
    
    Therefore, the extra call to rcu_preempt_deferred_qs_irqrestore()
    would return immediately.  With one possible exception, which is
    if an expedited grace period started just as the task was being
    resumed, which could leave ->exp_deferred_qs set.  This will cause
    rcu_preempt_deferred_qs_irqrestore() to invoke rcu_report_exp_rdp(),
    reporting the quiescent state, just as it should.  (Such an expedited
    grace period won't affect the preemption code path due to interrupts
    having already been disabled.)
    
    But when rcu_note_context_switch() invokes __rcu_read_unlock(), it
    is doing so with preemption disabled, hence __rcu_read_unlock() will
    unconditionally defer the quiescent state, only to immediately invoke
    rcu_preempt_deferred_qs(), thus immediately reporting the deferred
    quiescent state.  It turns out to be safe (and faster) to instead
    just invoke rcu_preempt_deferred_qs() without the __rcu_read_unlock()
    middleman.
    
    Because this is the invocation during the preemption (as opposed to
    the invocation just after the resume), at least one of the bits in
    ->rcu_read_unlock_special.b must be set and ->rcu_read_lock_nesting
    must be negative.  This means that rcu_preempt_need_deferred_qs() must
    return true, avoiding the early exit from rcu_preempt_deferred_qs().
    Thus, rcu_preempt_deferred_qs_irqrestore() will be invoked immediately,
    as required.
    
    This commit therefore simplifies the CONFIG_PREEMPT=y version of
    rcu_note_context_switch() by removing the "else if" branch of its
    "if" statement.  This change means that all callers that would have
    invoked rcu_read_unlock_special() followed by rcu_preempt_deferred_qs()
    will now simply invoke rcu_preempt_deferred_qs(), thus avoiding the
    rcu_read_unlock_special() middleman when __rcu_read_unlock() is preempted.
    
    Cc: rcu@vger.kernel.org
    Cc: kernel-team@android.com
    Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
    Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 187dc076c497..214e4689c29d 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -313,15 +313,6 @@ void rcu_note_context_switch(bool preempt)
 				       ? rnp->gp_seq
 				       : rcu_seq_snap(&rnp->gp_seq));
 		rcu_preempt_ctxt_queue(rnp, rdp);
-	} else if (t->rcu_read_lock_nesting < 0 &&
-		   t->rcu_read_unlock_special.s) {
-
-		/*
-		 * Complete exit from RCU read-side critical section on
-		 * behalf of preempted instance of __rcu_read_unlock().
-		 */
-		rcu_read_unlock_special(t);
-		rcu_preempt_deferred_qs(t);
 	} else {
 		rcu_preempt_deferred_qs(t);
 	}
Joel Fernandes July 1, 2019, 9:33 p.m. UTC | #2
On Mon, Jul 01, 2019 at 01:03:10PM -0700, Paul E. McKenney wrote:
> On Mon, Jul 01, 2019 at 12:04:14AM -0400, Joel Fernandes (Google) wrote:
> > The rcu_preempt_note_context_switch() tries to handle cases where
> > __rcu_read_unlock() got preempted and then the context switch path does
> > the reporting of the quiscent state along with clearing any bits in the
> > rcu_read_unlock_special union.
> > 
> > This can be handled by just calling rcu_deferred_qs() which was added
> > during the RCU consolidation work and already does these checks.
> > 
> > Tested RCU config TREE03 for an hour which succeeds.
> > 
> > Cc: rcu@vger.kernel.org
> > Cc: kernel-team@android.com
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> My first reaction was "that cannot possibly work", but after a bit of
> digging, it really does appear to work just fine.  I therefore expanded
> the commit log a bit, so please check it to catch any messups on my part.
> 
> Very cool, thank you very much!  ;-)

Awesome! Thanks. I am glad you agree with the change and I agree with your
changes to the commit log.

thanks,

 - Joel


> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit ce547cb41ed7662f70d0b07d4c7f7555ba130c61
> Author: Joel Fernandes (Google) <joel@joelfernandes.org>
> Date:   Mon Jul 1 00:04:14 2019 -0400
> 
>     rcu: Simplify rcu_note_context_switch exit from critical section
>     
>     Because __rcu_read_unlock() can be preempted just before the call to
>     rcu_read_unlock_special(), it is possible for a task to be preempted just
>     before it would have fully exited its RCU read-side critical section.
>     This would result in a needless extension of that critical section until
>     that task was resumed, which might in turn result in a needlessly
>     long grace period, needless RCU priority boosting, and needless
>     force-quiescent-state actions.  Therefore, rcu_note_context_switch()
>     invokes __rcu_read_unlock() followed by rcu_preempt_deferred_qs() when
>     it detects this situation.  This action by rcu_note_context_switch()
>     ends the RCU read-side critical section immediately.
>     
>     Of course, once the task resumes, it will invoke rcu_read_unlock_special()
>     redundantly.  This is harmless because the fact that a preemption
>     happened means that interrupts, preemption, and softirqs cannot
>     have been disabled, so there would be no deferred quiescent state.
>     While ->rcu_read_lock_nesting remains less than zero, none of the
>     ->rcu_read_unlock_special.b bits can be set, and they were all zeroed by
>     the call to rcu_note_context_switch() at task-preemption time.  Therefore,
>     setting ->rcu_read_unlock_special.b.exp_hint to false has no effect.
>     
>     Therefore, the extra call to rcu_preempt_deferred_qs_irqrestore()
>     would return immediately.  With one possible exception, which is
>     if an expedited grace period started just as the task was being
>     resumed, which could leave ->exp_deferred_qs set.  This will cause
>     rcu_preempt_deferred_qs_irqrestore() to invoke rcu_report_exp_rdp(),
>     reporting the quiescent state, just as it should.  (Such an expedited
>     grace period won't affect the preemption code path due to interrupts
>     having already been disabled.)
>     
>     But when rcu_note_context_switch() invokes __rcu_read_unlock(), it
>     is doing so with preemption disabled, hence __rcu_read_unlock() will
>     unconditionally defer the quiescent state, only to immediately invoke
>     rcu_preempt_deferred_qs(), thus immediately reporting the deferred
>     quiescent state.  It turns out to be safe (and faster) to instead
>     just invoke rcu_preempt_deferred_qs() without the __rcu_read_unlock()
>     middleman.
>     
>     Because this is the invocation during the preemption (as opposed to
>     the invocation just after the resume), at least one of the bits in
>     ->rcu_read_unlock_special.b must be set and ->rcu_read_lock_nesting
>     must be negative.  This means that rcu_preempt_need_deferred_qs() must
>     return true, avoiding the early exit from rcu_preempt_deferred_qs().
>     Thus, rcu_preempt_deferred_qs_irqrestore() will be invoked immediately,
>     as required.
>     
>     This commit therefore simplifies the CONFIG_PREEMPT=y version of
>     rcu_note_context_switch() by removing the "else if" branch of its
>     "if" statement.  This change means that all callers that would have
>     invoked rcu_read_unlock_special() followed by rcu_preempt_deferred_qs()
>     will now simply invoke rcu_preempt_deferred_qs(), thus avoiding the
>     rcu_read_unlock_special() middleman when __rcu_read_unlock() is preempted.
>     
>     Cc: rcu@vger.kernel.org
>     Cc: kernel-team@android.com
>     Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 187dc076c497..214e4689c29d 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -313,15 +313,6 @@ void rcu_note_context_switch(bool preempt)
>  				       ? rnp->gp_seq
>  				       : rcu_seq_snap(&rnp->gp_seq));
>  		rcu_preempt_ctxt_queue(rnp, rdp);
> -	} else if (t->rcu_read_lock_nesting < 0 &&
> -		   t->rcu_read_unlock_special.s) {
> -
> -		/*
> -		 * Complete exit from RCU read-side critical section on
> -		 * behalf of preempted instance of __rcu_read_unlock().
> -		 */
> -		rcu_read_unlock_special(t);
> -		rcu_preempt_deferred_qs(t);
>  	} else {
>  		rcu_preempt_deferred_qs(t);
>  	}
>
Paul E. McKenney July 1, 2019, 9:42 p.m. UTC | #3
On Mon, Jul 01, 2019 at 05:33:28PM -0400, Joel Fernandes wrote:
> On Mon, Jul 01, 2019 at 01:03:10PM -0700, Paul E. McKenney wrote:
> > On Mon, Jul 01, 2019 at 12:04:14AM -0400, Joel Fernandes (Google) wrote:
> > > The rcu_preempt_note_context_switch() tries to handle cases where
> > > __rcu_read_unlock() got preempted and then the context switch path does
> > > the reporting of the quiscent state along with clearing any bits in the
> > > rcu_read_unlock_special union.
> > > 
> > > This can be handled by just calling rcu_deferred_qs() which was added
> > > during the RCU consolidation work and already does these checks.
> > > 
> > > Tested RCU config TREE03 for an hour which succeeds.
> > > 
> > > Cc: rcu@vger.kernel.org
> > > Cc: kernel-team@android.com
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > 
> > My first reaction was "that cannot possibly work", but after a bit of
> > digging, it really does appear to work just fine.  I therefore expanded
> > the commit log a bit, so please check it to catch any messups on my part.
> > 
> > Very cool, thank you very much!  ;-)
> 
> Awesome! Thanks. I am glad you agree with the change and I agree with your
> changes to the commit log.

Very good, I will push it to -rcu shortly.

							Thanx, Paul

> thanks,
> 
>  - Joel
> 
> 
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > commit ce547cb41ed7662f70d0b07d4c7f7555ba130c61
> > Author: Joel Fernandes (Google) <joel@joelfernandes.org>
> > Date:   Mon Jul 1 00:04:14 2019 -0400
> > 
> >     rcu: Simplify rcu_note_context_switch exit from critical section
> >     
> >     Because __rcu_read_unlock() can be preempted just before the call to
> >     rcu_read_unlock_special(), it is possible for a task to be preempted just
> >     before it would have fully exited its RCU read-side critical section.
> >     This would result in a needless extension of that critical section until
> >     that task was resumed, which might in turn result in a needlessly
> >     long grace period, needless RCU priority boosting, and needless
> >     force-quiescent-state actions.  Therefore, rcu_note_context_switch()
> >     invokes __rcu_read_unlock() followed by rcu_preempt_deferred_qs() when
> >     it detects this situation.  This action by rcu_note_context_switch()
> >     ends the RCU read-side critical section immediately.
> >     
> >     Of course, once the task resumes, it will invoke rcu_read_unlock_special()
> >     redundantly.  This is harmless because the fact that a preemption
> >     happened means that interrupts, preemption, and softirqs cannot
> >     have been disabled, so there would be no deferred quiescent state.
> >     While ->rcu_read_lock_nesting remains less than zero, none of the
> >     ->rcu_read_unlock_special.b bits can be set, and they were all zeroed by
> >     the call to rcu_note_context_switch() at task-preemption time.  Therefore,
> >     setting ->rcu_read_unlock_special.b.exp_hint to false has no effect.
> >     
> >     Therefore, the extra call to rcu_preempt_deferred_qs_irqrestore()
> >     would return immediately.  With one possible exception, which is
> >     if an expedited grace period started just as the task was being
> >     resumed, which could leave ->exp_deferred_qs set.  This will cause
> >     rcu_preempt_deferred_qs_irqrestore() to invoke rcu_report_exp_rdp(),
> >     reporting the quiescent state, just as it should.  (Such an expedited
> >     grace period won't affect the preemption code path due to interrupts
> >     having already been disabled.)
> >     
> >     But when rcu_note_context_switch() invokes __rcu_read_unlock(), it
> >     is doing so with preemption disabled, hence __rcu_read_unlock() will
> >     unconditionally defer the quiescent state, only to immediately invoke
> >     rcu_preempt_deferred_qs(), thus immediately reporting the deferred
> >     quiescent state.  It turns out to be safe (and faster) to instead
> >     just invoke rcu_preempt_deferred_qs() without the __rcu_read_unlock()
> >     middleman.
> >     
> >     Because this is the invocation during the preemption (as opposed to
> >     the invocation just after the resume), at least one of the bits in
> >     ->rcu_read_unlock_special.b must be set and ->rcu_read_lock_nesting
> >     must be negative.  This means that rcu_preempt_need_deferred_qs() must
> >     return true, avoiding the early exit from rcu_preempt_deferred_qs().
> >     Thus, rcu_preempt_deferred_qs_irqrestore() will be invoked immediately,
> >     as required.
> >     
> >     This commit therefore simplifies the CONFIG_PREEMPT=y version of
> >     rcu_note_context_switch() by removing the "else if" branch of its
> >     "if" statement.  This change means that all callers that would have
> >     invoked rcu_read_unlock_special() followed by rcu_preempt_deferred_qs()
> >     will now simply invoke rcu_preempt_deferred_qs(), thus avoiding the
> >     rcu_read_unlock_special() middleman when __rcu_read_unlock() is preempted.
> >     
> >     Cc: rcu@vger.kernel.org
> >     Cc: kernel-team@android.com
> >     Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >     Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
> > 
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index 187dc076c497..214e4689c29d 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -313,15 +313,6 @@ void rcu_note_context_switch(bool preempt)
> >  				       ? rnp->gp_seq
> >  				       : rcu_seq_snap(&rnp->gp_seq));
> >  		rcu_preempt_ctxt_queue(rnp, rdp);
> > -	} else if (t->rcu_read_lock_nesting < 0 &&
> > -		   t->rcu_read_unlock_special.s) {
> > -
> > -		/*
> > -		 * Complete exit from RCU read-side critical section on
> > -		 * behalf of preempted instance of __rcu_read_unlock().
> > -		 */
> > -		rcu_read_unlock_special(t);
> > -		rcu_preempt_deferred_qs(t);
> >  	} else {
> >  		rcu_preempt_deferred_qs(t);
> >  	}
> > 
>

Patch
diff mbox series

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index bff6410fac06..ebb4d46a6267 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -313,15 +313,6 @@  void rcu_note_context_switch(bool preempt)
 				       ? rnp->gp_seq
 				       : rcu_seq_snap(&rnp->gp_seq));
 		rcu_preempt_ctxt_queue(rnp, rdp);
-	} else if (t->rcu_read_lock_nesting < 0 &&
-		   t->rcu_read_unlock_special.s) {
-
-		/*
-		 * Complete exit from RCU read-side critical section on
-		 * behalf of preempted instance of __rcu_read_unlock().
-		 */
-		rcu_read_unlock_special(t);
-		rcu_preempt_deferred_qs(t);
 	} else {
 		rcu_preempt_deferred_qs(t);
 	}