On Sun, Jul 01, 2018 at 08:11:32PM -0700, Paul E. McKenney wrote:
> On Sun, Jul 01, 2018 at 05:35:53PM -0700, Joel Fernandes wrote:
> > On Sun, Jul 01, 2018 at 03:27:49PM -0700, Paul E. McKenney wrote:
> > [...]
> > > > > +/*
> > > > > + * 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?
> >
> > That is true! so then I could also test if PREEMPT_RCU is enabled like
> > you're
> > doing in the other path.
> >
> > thanks!
> >
> > ---8<-----------------------
> >
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index fb440baf8ac6..03a460921dca 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 (IS_ENABLED(PREEMPT_RCU) && !(preempt_count() &
> > PREEMPT_MASK))
>
> For more precision, s/PREEMPT_RCU/CONFIG_PREEMPT_COUNT/
>
> Hmmm... I recently queued a patch that redefines the RCU-bh update-side
> API in terms of the consolidated RCU implementation, so this "else"
> clause no longer exists. One approach would be to fold this condition
> (with the addition of SOFTIRQ_MASK) into the previous "if" condition,
> but that would call rcu_note_voluntary_context_switch() at bad times.
> So maybe this becomes a new "else if" clause.
>
> Another complication is an upcoming step that redefines the RCU-sched
> update-side API in terms of the consolidated RCU implementation, which
> will likely restructure this "if" statement yet again.
>
> So I will try to fold this idea in (with attribution). If I don't get
> it in place in a week or two, please remind me. Of course, one good way
> to remind me is to supply a patch against whatever this turns into. ;-)
Sounds good, I will keep these complications in mind and remind you in some
time and/or supply a patch doing the same. Will continue going through the
new code in your tree and let you know anything I find.
Cheers, thanks!
- Joel
