On Thu, Sep 15, 2022 at 01:58:25PM +0800, Pingfan Liu wrote:
> As Paul pointed out "The tick_dep_clear() is SMP-safe because it uses
> atomic operations, but the problem is that if there are multiple
> nohz_full CPUs going offline concurrently, the first CPU to invoke
> rcutree_dead_cpu() will turn the tick off. This might require an
> atomically manipulated counter to mediate the calls to
> rcutree_dead_cpu(). "
>
> This patch introduces a new member ->dying to rcu_node, which reflects
> the number of concurrent offlining cpu. TICK_DEP_BIT_RCU is set by
> the first entrance and cleared by the last.
>
> Note: now, tick_dep_set() is put under the rnp->lock, but since it takes
> no lock, no extra locking order is introduced.
>
> Suggested-by: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
> Signed-off-by: Pingfan Liu <kernelfans@xxxxxxxxx>
> Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
> Cc: David Woodhouse <dwmw@xxxxxxxxxxxx>
> Cc: Frederic Weisbecker <frederic@xxxxxxxxxx>
> Cc: Neeraj Upadhyay <quic_neeraju@xxxxxxxxxxx>
> Cc: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> Cc: Lai Jiangshan <jiangshanlai@xxxxxxxxx>
> Cc: Joel Fernandes <joel@xxxxxxxxxxxxxxxxx>
> Cc: "Jason A. Donenfeld" <Jason@xxxxxxxxx>
> To: rcu@xxxxxxxxxxxxxxx
> ---
> kernel/rcu/tree.c | 19 ++++++++++++++-----
> kernel/rcu/tree.h | 1 +
> 2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 8a829b64f5b2..f8bd0fc5fd2f 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2164,13 +2164,19 @@ int rcutree_dead_cpu(unsigned int cpu)
> {
> struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
> struct rcu_node *rnp = rdp->mynode; /* Outgoing CPU's rdp & rnp. */
> + unsigned long flags;
> + u8 dying;
>
> if (!IS_ENABLED(CONFIG_HOTPLUG_CPU))
> return 0;
>
> WRITE_ONCE(rcu_state.n_online_cpus, rcu_state.n_online_cpus - 1);
> - // Stop-machine done, so allow nohz_full to disable tick.
> - tick_dep_clear(TICK_DEP_BIT_RCU);
> + raw_spin_lock_irqsave_rcu_node(rnp, flags);
> + dying = --rnp->dying;
> + if (!dying)
> + // Stop-machine done, so allow nohz_full to disable tick.
> + tick_dep_clear(TICK_DEP_BIT_RCU);
> + raw_spin_lock_irqsave_rcu_node(rnp, flags);
Note this is only locking the rdp's node, not the root node.
Therefore if CPU 0 and CPU 256 are going off at the same time and they
don't belong to the same node, the above won't protect against concurrent
TICK_DEP_BIT_RCU set/clear.
My suspicion is that we don't need this TICK_DEP_BIT_RCU tick dependency
anymore. I believe it was there because of issues that were fixed with:
53e87e3cdc15 (timers/nohz: Last resort update jiffies on nohz_full IRQ entry)
and:
a1ff03cd6fb9 (tick: Detect and fix jiffies update stall)
It's unfortunately just suspicion because the reason for that tick dependency
is unclear but I believe it should be safe to remove now.
Thanks.
