Re: [PATCH tip/core/rcu 06/12] rcu: Do full report for .need_qs for strict GPs

[Paul E. McKenney]

On Thu, Aug 13, 2020 at 02:50:27AM +0200, Jann Horn wrote:
> On Thu, Aug 13, 2020 at 12:57 AM <paulmck@xxxxxxxxxx> wrote:
> > The rcu_preempt_deferred_qs_irqrestore() function is invoked at
> > the end of an RCU read-side critical section (for example, directly
> > from rcu_read_unlock()) and, if .need_qs is set, invokes rcu_qs() to
> > report the new quiescent state.  This works, except that rcu_qs() only
> > updates per-CPU state, leaving reporting of the actual quiescent state
> > to a later call to rcu_report_qs_rdp(), for example from within a later
> > RCU_SOFTIRQ instance.  Although this approach is exactly what you want if
> > you are more concerned about efficiency than about short grace periods,
> > in CONFIG_RCU_STRICT_GRACE_PERIOD=y kernels, short grace periods are
> > the name of the game.
> >
> > This commit therefore makes rcu_preempt_deferred_qs_irqrestore() directly
> > invoke rcu_report_qs_rdp() in CONFIG_RCU_STRICT_GRACE_PERIOD=y, thus
> > shortening grace periods.
> 
> Ooh, I'm very happy about this series! :)

Glad you like it!  And I hope that it helps!

One usability concern is whether rcutree.rcu_unlock_delay needs to be
applied only some small fraction of the time in order to allow the delay
to be large (a couple hundred microseconds?)  while still avoiding doing
too much more damage to timing and performance than absolutely necessary.

> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index 7ed55c5..1761ff4 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -459,8 +459,12 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
> >                 return;
> >         }
> >         t->rcu_read_unlock_special.s = 0;
> > -       if (special.b.need_qs)
> > -               rcu_qs();
> > +       if (special.b.need_qs) {
> > +               if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD))
> > +                       rcu_report_qs_rdp(rdp->cpu, rdp);
> 
> Not an issue with this patch specifically, but: I'm looking at
> rcu_report_qs_rdp(), and some of the parts that I do vaguely
> understand look a bit off to me.
> 
> rcu_report_qs_rdp() is given a CPU number as first argument, but never
> actually uses that argument. (And the only existing caller also passes
> in rdp->cpu, just like this patch.) I guess that argument can go away?
> 
> The comment above rcu_report_qs_rdp() claims that it "must be called
> from the specified CPU", but there is a branch in there that
> specifically checks whether that is true ("if (rdp->cpu ==
> smp_processor_id())"). As far as I can tell, rcu_report_qs_rdp() is,
> as the comment says, indeed never invoked with another CPU's rcu_data
> (only invoked via rcu_core() -> rcu_check_quiescent_state() ->
> rcu_report_qs_rdp(), and rcu_core() looks up "rdp =
> raw_cpu_ptr(&rcu_data)"). So perhaps if there is a check for whether
> rdp belongs to the current CPU, that check should have a WARN_ON(), or
> something like that, since it indicates that the API contract
> specified in the comment was violated?

It looks like you are correct, and that the first parameter can be
dropped, the "if" you mention replaced by a WARN_ON_ONCE(), and
the body of that "if" be unconditional.  I have it on my list, and
if it still looks correct in the cold hard light of dawn, I will
apply it with your Reported-by.

And thank you very much for looking it over!

						Thanx, Paul