Re: [RFC PATCH RT 3/4] rcu: unlock special: Treat irq and preempt disabled the same

[Paul E. McKenney]

On Fri, Jun 21, 2019 at 05:26:06PM -0700, Paul E. McKenney wrote:
> On Thu, Jun 20, 2019 at 06:08:19PM -0500, Scott Wood wrote:
> > On Thu, 2019-06-20 at 15:25 -0700, Paul E. McKenney wrote:
> > > On Thu, Jun 20, 2019 at 04:59:30PM -0500, Scott Wood wrote:
> > > > On Thu, 2019-06-20 at 14:10 -0700, Paul E. McKenney wrote:
> > > > > On Tue, Jun 18, 2019 at 08:19:07PM -0500, Scott Wood wrote:
> > > > > > [Note: Just before posting this I noticed that the invoke_rcu_core
> > > > > > stuff
> > > > > >  is part of the latest RCU pull request, and it has a patch that
> > > > > >  addresses this in a more complicated way that appears to deal with
> > > > > > the
> > > > > >  bare irq-disabled sequence as well.
> > > > > 
> > > > > Far easier to deal with it than to debug the lack of it.  ;-)
> > > > > 
> > > > > >  Assuming we need/want to support such sequences, is the
> > > > > >  invoke_rcu_core() call actually going to result in scheduling any
> > > > > >  sooner?  resched_curr() just does the same setting of need_resched
> > > > > >  when it's the same cpu.
> > > > > > ]
> > > > > 
> > > > > Yes, invoke_rcu_core() can in some cases invoke the scheduler sooner.
> > > > > Setting the CPU-local bits might not have effect until the next
> > > > > interrupt.
> > > > 
> > > > Maybe I'm missing something, but I don't see how (in the non-use_softirq
> > > > case).  It just calls wake_up_process(), which in resched_curr() will
> > > > set
> > > > need_resched but not do an IPI-to-self.
> > > 
> > > The common non-rt case will be use_softirq.  Or are you referring
> > > specifically to this block of code in current -rcu?
> > > 
> > > 		} else if (exp && irqs_were_disabled && !use_softirq &&
> > > 			   !t->rcu_read_unlock_special.b.deferred_qs) {
> > > 			// Safe to awaken and we get no help from enabling
> > > 			// irqs, unlike bh/preempt.
> > > 			invoke_rcu_core();
> > 
> > Yes, that one.  If that block is removed the else path should be sufficient,
> > now that an IPI-to-self has been added.
> 
> I will give it a try and let you know what happens.

How about the following?

								Thanx, Paul

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

commit 2fd23b1b649bf7e0754fa1dfce01e945bc62f4af
Author: Paul E. McKenney <paulmck@xxxxxxxxxxxxx>
Date:   Sat Jun 22 12:05:54 2019 -0700

    rcu: Simplify rcu_read_unlock_special() deferred wakeups
    
    In !use_softirq runs, we clearly cannot rely on raise_softirq() and
    its lightweight bit setting, so we must instead do some form of wakeup.
    In the absence of a self-IPI when interrupts are disabled, these wakeups
    can be delayed until the next interrupt occurs.  This means that calling
    invoke_rcu_core() doesn't actually do any expediting.
    
    In this case, it is better to take the "else" clause, which sets the
    current CPU's resched bits and, if there is an expedited grace period
    in flight, uses IRQ-work to force the needed self-IPI.  This commit
    therefore removes the "else if" clause that calls invoke_rcu_core().
    
    Reported-by: Scott Wood <swood@xxxxxxxxxx>
    Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxx>

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 841060fce33c..c631413f457f 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -629,17 +629,12 @@ static void rcu_read_unlock_special(struct task_struct *t)
 			// Using softirq, safe to awaken, and we get
 			// no help from enabling irqs, unlike bh/preempt.
 			raise_softirq_irqoff(RCU_SOFTIRQ);
-		} else if (exp && irqs_were_disabled && !use_softirq &&
-			   !t->rcu_read_unlock_special.b.deferred_qs) {
-			// Safe to awaken and we get no help from enabling
-			// irqs, unlike bh/preempt.
-			invoke_rcu_core();
 		} else {
 			// Enabling BH or preempt does reschedule, so...
 			// Also if no expediting or NO_HZ_FULL, slow is OK.
 			set_tsk_need_resched(current);
 			set_preempt_need_resched();
-			if (IS_ENABLED(CONFIG_IRQ_WORK) &&
+			if (IS_ENABLED(CONFIG_IRQ_WORK) && irqs_were_disabled &&
 			    !rdp->defer_qs_iw_pending && exp) {
 				// Get scheduler to re-evaluate and call hooks.
 				// If !IRQ_WORK, FQS scan will eventually IPI.