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

[Jann Horn]

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! :)

> 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?