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?
