Re: [PATCH tip/core/rcu 2/2] rcu: Check for wakeup-safe conditions in rcu_read_unlock_special()

[Paul E. McKenney]

On Wed, Apr 03, 2019 at 09:25:50AM -0700, Paul E. McKenney wrote:
> On Wed, Apr 03, 2019 at 11:50:46AM +0200, Peter Zijlstra wrote:
> > On Tue, Apr 02, 2019 at 06:18:53AM -0700, Paul E. McKenney wrote:
> > > On Tue, Apr 02, 2019 at 09:09:53AM +0200, Peter Zijlstra wrote:
> > > > On Mon, Apr 01, 2019 at 10:22:57AM -0700, Paul E. McKenney wrote:
> > 
> > > > > Or am I missing something that gets the scheduler on the job faster?
> > > > 
> > > > Oh urgh, yah. So normally we only twiddle with the need_resched state:
> > > > 
> > > >  - while preempt_disabl(), such that preempt_enable() will reschedule
> > > >  - from interrupt context, such that interrupt return will reschedule
> > > > 
> > > > But the usage here 'violates' those rules and then there is an
> > > > unspecified latency between setting the state and it getting observed,
> > > > but no longer than 1 tick I would think.
> > > 
> > > In general, yes, which is fine (famous last words) for normal grace
> > > periods but not so good for expedited grace periods.
> > > 
> > > > I don't think we can go NOHZ with need_resched set, because the moment
> > > > we hit the idle loop with that set, we _will_ reschedule.
> > > 
> > > Agreed, and I believe that transitioning to usermode execution also
> > > gives the scheduler a chance to take action.
> > > 
> > > The one exception to this is when a nohz_full CPU running in nohz_full
> > > mode does a system call that decides to execute for a very long time.
> > > Last I checked, the scheduling-clock interrupt did -not- get retriggered
> > > in this case, and the delay could be indefinite, as in bad even for
> > > normal grace periods.
> > 
> > Right, there is that.
> > 
> > > > So in that respect the irq_work suggestion I made would fix things
> > > > properly.
> > > 
> > > But wouldn't the current use of set_tsk_need_resched(current) followed by
> > > set_preempt_need_resched() work just as well in that case?  The scheduler
> > > would react to these at the next scheduler-clock interrupt on their
> > > own, right?  Or am I being scheduler-naive again?
> > 
> > Well, you have that unspecified delay. By forcing the (self) interrupt
> > you enforce a timely response.
> 
> Good point!  I will give this a go, thank you!

How about as shown below?

							Thanx, Paul

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

commit 687c00c91c9edbaf5309402689bce644dd140590
Author: Paul E. McKenney <paulmck@xxxxxxxxxxxxx>
Date:   Thu Apr 4 12:19:25 2019 -0700

    rcu: Use irq_work to get scheduler's attention in clean context
    
    When rcu_read_unlock_special() is invoked with interrupts disabled, is
    either not in an interrupt handler or is not using RCU_SOFTIRQ, is not
    the first RCU read-side critical section in the chain, and either there
    is an expedited grace period in flight or this is a NO_HZ_FULL kernel,
    the end of the grace period can be unduly delayed.  The reason for this
    is that it is not safe to do wakeups in this situation.
    
    This commit fixes this problem by using the irq_work subsystem to
    force a later interrupt handler in a clean environment.  Because
    set_tsk_need_resched(current) and set_preempt_need_resched() are
    invoked prior to this, the scheduler will force a context switch
    upon return from this interrupt (though perhaps at the end of any
    interrupted preempt-disable or BH-disable region of code), which will
    invoke rcu_note_context_switch() (again in a clean environment), which
    will in turn give RCU the chance to report the deferred quiescent state.
    
    Of course, by then this task might be within another RCU read-side
    critical section.  But that will be detected at that time and reporting
    will be further deferred to the outermost rcu_read_unlock().  See
    rcu_preempt_need_deferred_qs() and rcu_preempt_deferred_qs() for more
    details on the checking.
    
    Suggested-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
    Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxx>

diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index b9c5d1af8451..dc3c53cb9608 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -161,6 +161,8 @@ struct rcu_data {
 					/*  ticks this CPU has handled */
 					/*  during and after the last grace */
 					/* period it is aware of. */
+	struct irq_work defer_qs_iw;	/* Obtain later scheduler attention. */
+	bool defer_qs_iw_pending;	/* Scheduler attention pending? */
 
 	/* 2) batch handling */
 	struct rcu_segcblist cblist;	/* Segmented callback list, with */
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index d90a262ba04b..80ee4d3f3891 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -587,6 +587,17 @@ static void rcu_preempt_deferred_qs(struct task_struct *t)
 		t->rcu_read_lock_nesting += RCU_NEST_BIAS;
 }
 
+/*
+ * Minimal handler to give the scheduler a chance to re-evaluate.
+ */
+static void rcu_preempt_deferred_qs_handler(struct irq_work *iwp)
+{
+	struct rcu_data *rdp;
+
+	rdp = container_of(iwp, struct rcu_data, defer_qs_iw);
+	rdp->defer_qs_iw_pending = false;
+}
+
 /*
  * Handle special cases during rcu_read_unlock(), such as needing to
  * notify RCU core processing or task having blocked during the RCU
@@ -630,6 +641,15 @@ static void rcu_read_unlock_special(struct task_struct *t)
 			// 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) &&
+			    !rdp->defer_qs_iw_pending && exp) {
+				// Get scheduler to re-evaluate and call hooks.
+				// If !IRQ_WORK, FQS scan will eventually IPI.
+				init_irq_work(&rdp->defer_qs_iw,
+					      rcu_preempt_deferred_qs_handler);
+				rdp->defer_qs_iw_pending = true;
+				irq_work_queue_on(&rdp->defer_qs_iw, rdp->cpu);
+			}
 		}
 		t->rcu_read_unlock_special.b.deferred_qs = true;
 		local_irq_restore(flags);