Date:   Sun, 1 Jul 2018 15:27:49 -0700
From:   "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:     Joel Fernandes <joel@...lfernandes.org>
Cc:     linux-kernel@...r.kernel.org, mingo@...nel.org,
        jiangshanlai@...il.com, dipankar@...ibm.com,
        akpm@...ux-foundation.org, mathieu.desnoyers@...icios.com,
        josh@...htriplett.org, tglx@...utronix.de, peterz@...radead.org,
        rostedt@...dmis.org, dhowells@...hat.com, edumazet@...gle.com,
        fweisbec@...il.com, oleg@...hat.com
Subject: Re: [PATCH RFC tip/core/rcu 1/2] rcu: Defer reporting RCU-preempt
 quiescent states when disabled

On Sun, Jul 01, 2018 at 11:38:28AM -0700, Joel Fernandes wrote:
> Hi Paul,
> 
> On Wed, Jun 27, 2018 at 01:49:14PM -0700, Paul E. McKenney wrote:
> [...]
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index c1b17f5b9361..ff5c70eae47d 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -371,6 +371,9 @@ static void rcu_preempt_note_context_switch(bool preempt)
> >  		 * behalf of preempted instance of __rcu_read_unlock().
> >  		 */
> >  		rcu_read_unlock_special(t);
> > +		rcu_preempt_deferred_qs(t);
> > +	} else {
> > +		rcu_preempt_deferred_qs(t);
> >  	}
> >  
> >  	/*
> > @@ -464,54 +467,51 @@ static bool rcu_preempt_has_tasks(struct rcu_node *rnp)
> >  }
> >  
> >  /*
> > - * Handle special cases during rcu_read_unlock(), such as needing to
> > - * notify RCU core processing or task having blocked during the RCU
> > - * read-side critical section.
> > + * Report deferred quiescent states.  The deferral time can
> > + * be quite short, for example, in the case of the call from
> > + * rcu_read_unlock_special().
> >   */
> > -static void rcu_read_unlock_special(struct task_struct *t)
> > +static void
> > +rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
> >  {
> >  	bool empty_exp;
> >  	bool empty_norm;
> >  	bool empty_exp_now;
> > -	unsigned long flags;
> >  	struct list_head *np;
> >  	bool drop_boost_mutex = false;
> >  	struct rcu_data *rdp;
> >  	struct rcu_node *rnp;
> >  	union rcu_special special;
> >  
> > -	/* NMI handlers cannot block and cannot safely manipulate state. */
> > -	if (in_nmi())
> > -		return;
> > -
> > -	local_irq_save(flags);
> > -
> >  	/*
> >  	 * If RCU core is waiting for this CPU to exit its critical section,
> >  	 * report the fact that it has exited.  Because irqs are disabled,
> >  	 * t->rcu_read_unlock_special cannot change.
> >  	 */
> >  	special = t->rcu_read_unlock_special;
> > +	rdp = this_cpu_ptr(rcu_state_p->rda);
> > +	if (!special.s && !rdp->deferred_qs) {
> > +		local_irq_restore(flags);
> > +		return;
> > +	}
> >  	if (special.b.need_qs) {
> >  		rcu_preempt_qs();
> >  		t->rcu_read_unlock_special.b.need_qs = false;
> > -		if (!t->rcu_read_unlock_special.s) {
> > +		if (!t->rcu_read_unlock_special.s && !rdp->deferred_qs) {
> >  			local_irq_restore(flags);
> >  			return;
> >  		}
> >  	}
> >  
> >  	/*
> > -	 * Respond to a request for an expedited grace period, but only if
> > -	 * we were not preempted, meaning that we were running on the same
> > -	 * CPU throughout.  If we were preempted, the exp_need_qs flag
> > -	 * would have been cleared at the time of the first preemption,
> > -	 * and the quiescent state would be reported when we were dequeued.
> > +	 * Respond to a request by an expedited grace period for a
> > +	 * quiescent state from this CPU.  Note that requests from
> > +	 * tasks are handled when removing the task from the
> > +	 * blocked-tasks list below.
> >  	 */
> > -	if (special.b.exp_need_qs) {
> > -		WARN_ON_ONCE(special.b.blocked);
> > +	if (special.b.exp_need_qs || rdp->deferred_qs) {
> >  		t->rcu_read_unlock_special.b.exp_need_qs = false;
> > -		rdp = this_cpu_ptr(rcu_state_p->rda);
> > +		rdp->deferred_qs = false;
> >  		rcu_report_exp_rdp(rcu_state_p, rdp, true);
> >  		if (!t->rcu_read_unlock_special.s) {
> >  			local_irq_restore(flags);
> > @@ -519,19 +519,6 @@ static void rcu_read_unlock_special(struct task_struct *t)
> >  		}
> >  	}
> >  
> > -	/* Hardware IRQ handlers cannot block, complain if they get here. */
> > -	if (in_irq() || in_serving_softirq()) {
> > -		lockdep_rcu_suspicious(__FILE__, __LINE__,
> > -				       "rcu_read_unlock() from irq or softirq with blocking in critical section!!!\n");
> > -		pr_alert("->rcu_read_unlock_special: %#x (b: %d, enq: %d nq: %d)\n",
> > -			 t->rcu_read_unlock_special.s,
> > -			 t->rcu_read_unlock_special.b.blocked,
> > -			 t->rcu_read_unlock_special.b.exp_need_qs,
> > -			 t->rcu_read_unlock_special.b.need_qs);
> > -		local_irq_restore(flags);
> > -		return;
> > -	}
> > -
> >  	/* Clean up if blocked during RCU read-side critical section. */
> >  	if (special.b.blocked) {
> >  		t->rcu_read_unlock_special.b.blocked = false;
> > @@ -602,6 +589,66 @@ static void rcu_read_unlock_special(struct task_struct *t)
> >  	}
> >  }
> >  
> > +/*
> > + * Is a deferred quiescent-state pending, and are we also not in
> > + * an RCU read-side critical section?  It is the caller's responsibility
> > + * to ensure it is otherwise safe to report any deferred quiescent
> > + * states.  The reason for this is that it is safe to report a
> > + * quiescent state during context switch even though preemption
> > + * is disabled.  This function cannot be expected to understand these
> > + * nuances, so the caller must handle them.
> > + */
> > +static bool rcu_preempt_need_deferred_qs(struct task_struct *t)
> > +{
> > +	return (this_cpu_ptr(&rcu_preempt_data)->deferred_qs ||
> > +		READ_ONCE(t->rcu_read_unlock_special.s)) &&
> > +	       !t->rcu_read_lock_nesting;
> > +}
> > +
> > +/*
> > + * Report a deferred quiescent state if needed and safe to do so.
> > + * As with rcu_preempt_need_deferred_qs(), "safe" involves only
> > + * not being in an RCU read-side critical section.  The caller must
> > + * evaluate safety in terms of interrupt, softirq, and preemption
> > + * disabling.
> > + */
> > +static void rcu_preempt_deferred_qs(struct task_struct *t)
> > +{
> > +	unsigned long flags;
> > +
> > +	if (!rcu_preempt_need_deferred_qs(t))
> > +		return;
> > +	local_irq_save(flags);
> > +	rcu_preempt_deferred_qs_irqrestore(t, flags);
> > +}
> > +
> > +/*
> > + * Handle special cases during rcu_read_unlock(), such as needing to
> > + * notify RCU core processing or task having blocked during the RCU
> > + * read-side critical section.
> > + */
> > +static void rcu_read_unlock_special(struct task_struct *t)
> > +{
> > +	unsigned long flags;
> > +	bool preempt_bh_were_disabled = !!(preempt_count() & ~HARDIRQ_MASK);
> > +	bool irqs_were_disabled;
> > +
> > +	/* NMI handlers cannot block and cannot safely manipulate state. */
> > +	if (in_nmi())
> > +		return;
> > +
> > +	local_irq_save(flags);
> > +	irqs_were_disabled = irqs_disabled_flags(flags);
> > +	if ((preempt_bh_were_disabled || irqs_were_disabled) &&
> > +	    t->rcu_read_unlock_special.b.blocked) {
> > +		/* Need to defer quiescent state until everything is enabled. */
> > +		raise_softirq_irqoff(RCU_SOFTIRQ);
> > +		local_irq_restore(flags);
> > +		return;
> > +	}
> > +	rcu_preempt_deferred_qs_irqrestore(t, flags);
> > +}
> > +
> >  /*
> >   * Dump detailed information for all tasks blocking the current RCU
> >   * grace period on the specified rcu_node structure.
> > @@ -737,10 +784,20 @@ static void rcu_preempt_check_callbacks(void)
> >  	struct rcu_state *rsp = &rcu_preempt_state;
> >  	struct task_struct *t = current;
> >  
> > -	if (t->rcu_read_lock_nesting == 0) {
> > -		rcu_preempt_qs();
> > +	if (t->rcu_read_lock_nesting > 0 ||
> > +	    (preempt_count() & (PREEMPT_MASK | SOFTIRQ_MASK))) {
> > +		/* No QS, force context switch if deferred. */
> > +		if (rcu_preempt_need_deferred_qs(t))
> > +			resched_cpu(smp_processor_id());
> 
> 
> Hi Paul,
> 
> I had a similar idea of checking the preempt_count() sometime back but didn't
> believe this path can be called with preempt enabled (for some reason ;-)).
> Now that I've convinced myself that's possible, what do you think about
> taking advantage of the opportunity to report a RCU-sched qs like below from
> rcu_check_callbacks ?
> 
> Did some basic testing, can roll into a patch later if you're Ok with it.

The problem here is that the code patch above cannot be called
with CONFIG_PREEMPT_COUNT=n, but the code below can.  And if
CONFIG_PREEMPT_COUNT=n, the return value from preempt_count() can be
misleading.

Or am I missing something here?

							Thanx, Paul

> thanks.
> 
> ---8<-----------------------
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index fb440baf8ac6..caa1e68f4168 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2683,6 +2683,12 @@ void rcu_check_callbacks(int user)
>  		rcu_note_voluntary_context_switch(current);
> 
>  	} else if (!in_softirq()) {
> +		/*
> +		 * Report RCU-sched qs if not in an RCU-sched read-side
> +		 * critical section.
> +		 */
> +		if (!(preempt_count() & PREEMPT_MASK))
> +			rcu_sched_qs();
> 
>  		/*
>  		 * Get here if this CPU did not take its interrupt from
> 

Powered by blists - more mailing lists