On Fri, Jun 28, 2019 at 11:22:16AM -0700, Paul E. McKenney wrote: > On Fri, Jun 28, 2019 at 07:45:45PM +0200, Sebastian Andrzej Siewior wrote: > > On 2019-06-28 10:30:11 [-0700], Paul E. McKenney wrote: > > > > I believe the .blocked field remains set even though we are not any more in a > > > > reader section because of deferred processing of the blocked lists that you > > > > mentioned yesterday. > > > > > > That can indeed happen. However, in current -rcu, that would mean > > > that .deferred_qs is also set, which (if in_irq()) would prevent > > > the raise_softirq_irqsoff() from being invoked. Which was why I was > > > asking the questions about whether in_irq() returns true within threaded > > > interrupts yesterday. If it does, I need to find if there is some way > > > of determining whether rcu_read_unlock_special() is being called from > > > a threaded interrupt in order to suppress the call to raise_softirq() > > > in that case. > > > > Please not that: > > | void irq_exit(void) > > | { > > |… > > in_irq() returns true > > | preempt_count_sub(HARDIRQ_OFFSET); > > in_irq() returns false > > | if (!in_interrupt() && local_softirq_pending()) > > | invoke_softirq(); > > > > -> invoke_softirq() does > > | if (!force_irqthreads) { > > | __do_softirq(); > > | } else { > > | wakeup_softirqd(); > > | } > > > > so for `force_irqthreads' rcu_read_unlock_special() within > > wakeup_softirqd() will see false. > > OK, fair point. How about the following instead, again on -rcu? > > Here is the rationale for the new version of the "if" statement: > > 1. irqs_were_disabled: If interrupts are enabled, we should > instead let the upcoming irq_enable()/local_bh_enable() > do the rescheduling for us. > 2. use_softirq: If we aren't using softirq, then > raise_softirq_irqoff() will be unhelpful. > 3a. in_interrupt(): If this returns true, the subsequent > call to raise_softirq_irqoff() is guaranteed not to > do a wakeup, so that call will be both very cheap and > quite safe. > 3b. Otherwise, if !in_interrupt(), if exp (an expedited RCU grace > period is being blocked), then incurring wakeup overhead > is worthwhile, and if also !.deferred_qs then scheduler locks > cannot be held so the wakeup will be safe. > > Does that make more sense? This makes a lot of sense. It would be nice to stick these comments on top of rcu_read_unlock_special() for future reference. thanks, - Joel > > Thanx, Paul > > ------------------------------------------------------------------------ > > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h > index 82c925df1d92..83333cfe8707 100644 > --- a/kernel/rcu/tree_plugin.h > +++ b/kernel/rcu/tree_plugin.h > @@ -624,8 +624,9 @@ static void rcu_read_unlock_special(struct task_struct *t) > (rdp->grpmask & rnp->expmask) || > tick_nohz_full_cpu(rdp->cpu); > // Need to defer quiescent state until everything is enabled. > - if ((exp || in_irq()) && irqs_were_disabled && use_softirq && > - (in_irq() || !t->rcu_read_unlock_special.b.deferred_qs)) { > + if (irqs_were_disabled && use_softirq && > + (in_interrupt() || > + (exp && !t->rcu_read_unlock_special.b.deferred_qs))) { > // Using softirq, safe to awaken, and we get > // no help from enabling irqs, unlike bh/preempt. > raise_softirq_irqoff(RCU_SOFTIRQ); >