srcu: use cpu_online() instead custom check

The current check via srcu_online is slightly racy because after looking at srcu_online there could be an interrupt that interrupted us long enough until the CPU we checked against went offline.

An alternative would be to hold the hotplug rwsem (so the CPUs don't change their state) and then check based on cpu_online() if we queue it on a specific CPU or not. queue_work_on() itself can handle if something is enqueued on an offline CPU but a timer which is enqueued on an offline CPU won't fire until the CPU is back online.

I am not sure if the removal in rcu_init() is okay or not. I assume that SRCU won't enqueue a work item before SRCU is up and ready.

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

kernel/rcu/srcutree.c +4 -18

index 6c9866a854b1..3428a40a813e 100644
@@ -38,6 +38,7 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/srcu.h>
+#include <linux/cpu.h>
 
 #include "rcu.h"
 #include "rcu_segcblist.h"

@@ -458,21 +459,6 @@ static void srcu_gp_start(struct srcu_struct *sp)
 	WARN_ON_ONCE(state != SRCU_STATE_SCAN1);
 }
 
-/*
- * Track online CPUs to guide callback workqueue placement.
- */
-DEFINE_PER_CPU(bool, srcu_online);
-
-void srcu_online_cpu(unsigned int cpu)
-{
-	WRITE_ONCE(per_cpu(srcu_online, cpu), true);
-}
-
-void srcu_offline_cpu(unsigned int cpu)
-{
-	WRITE_ONCE(per_cpu(srcu_online, cpu), false);
-}
-
 /*
  * Place the workqueue handler on the specified CPU if online, otherwise
  * just run it whereever.  This is useful for placing workqueue handlers

@@ -484,12 +470,12 @@ static bool srcu_queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
 {
 	bool ret;
 
-	preempt_disable();
-	if (READ_ONCE(per_cpu(srcu_online, cpu)))
+	cpus_read_lock();
+	if (cpu_online(cpu))
 		ret = queue_delayed_work_on(cpu, wq, dwork, delay);
 	else
 		ret = queue_delayed_work(wq, dwork, delay);
-	preempt_enable();
+	cpus_read_unlock();
 	return ret;
 }
 

kernel/rcu/tree.c +0 -4

index 6868ef417e9f..e2e68250009b 100644
@@ -3767,8 +3767,6 @@ int rcutree_online_cpu(unsigned int cpu)
 		rnp->ffmask |= rdp->grpmask;
 		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
 	}
-	if (IS_ENABLED(CONFIG_TREE_SRCU))
-		srcu_online_cpu(cpu);
 	if (rcu_scheduler_active == RCU_SCHEDULER_INACTIVE)
 		return 0; /* Too early in boot for scheduler work. */
 	sync_sched_exp_online_cleanup(cpu);

@@ -3796,8 +3794,6 @@ int rcutree_offline_cpu(unsigned int cpu)
 	}
 
 	rcutree_affinity_setting(cpu, cpu);
-	if (IS_ENABLED(CONFIG_TREE_SRCU))
-		srcu_offline_cpu(cpu);
 	return 0;
 }
 

Quoted 3 lines

The current check via srcu_online is slightly racy because after looking at srcu_online there could be an interrupt that interrupted us long enough until the CPU we checked against went offline.

I don't see how this can happen, even in -rt. The call to srcu_offline_cpu() happens very early in the CPU removal process, which means that the synchronize_rcu_mult(call_rcu, call_rcu_sched) in sched_cpu_deactivate() would wait for the interrupt to complete. And for the enclosing preempt_disable region to complete.

Or is getting rid of that preempt_disable region the real reason for this change?

Quoted 8 lines

An alternative would be to hold the hotplug rwsem (so the CPUs don't change their state) and then check based on cpu_online() if we queue it on a specific CPU or not. queue_work_on() itself can handle if something is enqueued on an offline CPU but a timer which is enqueued on an offline CPU won't fire until the CPU is back online.

I am not sure if the removal in rcu_init() is okay or not. I assume that SRCU won't enqueue a work item before SRCU is up and ready.

That was the case before the current merge window, but use of call_srcu() by tracing means that SRCU needs to be able to deal with call_srcu() long before any initialization has happened. The actual callbacks won't be invoked until much later, after the scheduler and workqueues are completely up and running, but call_srcu() can be invoked very early.

But I am not seeing any removal in rcu_init() in this patch, so I might be missing something.

Thanx, Paul

Quoted 4 lines · Paul E. McKenney

Quoted 3 lines

The current check via srcu_online is slightly racy because after looking at srcu_online there could be an interrupt that interrupted us long enough until the CPU we checked against went offline.

Quoted 5 lines

I don't see how this can happen, even in -rt. The call to srcu_offline_cpu() happens very early in the CPU removal process, which means that the synchronize_rcu_mult(call_rcu, call_rcu_sched) in sched_cpu_deactivate() would wait for the interrupt to complete. And for the enclosing preempt_disable region to complete.

Is this again a hidden RCU detail that preempt_disable() on CPU4 is enough to ensure that CPU2 does not get marked offline between?

Quoted 2 lines

Or is getting rid of that preempt_disable region the real reason for this change?

Well, that preempt_disable() + queue_(delayed_)work() does not work -RT. But looking further, that preempt_disable() while looking at online CPUs didn't look good.

Quoted 9 lines

Quoted 8 lines

An alternative would be to hold the hotplug rwsem (so the CPUs don't change their state) and then check based on cpu_online() if we queue it on a specific CPU or not. queue_work_on() itself can handle if something is enqueued on an offline CPU but a timer which is enqueued on an offline CPU won't fire until the CPU is back online.

I am not sure if the removal in rcu_init() is okay or not. I assume that SRCU won't enqueue a work item before SRCU is up and ready.

Quoted 8 lines

That was the case before the current merge window, but use of call_srcu() by tracing means that SRCU needs to be able to deal with call_srcu() long before any initialization has happened. The actual callbacks won't be invoked until much later, after the scheduler and workqueues are completely up and running, but call_srcu() can be invoked very early.

But I am not seeing any removal in rcu_init() in this patch, so I might be missing something.

The description is not up-to-date. There was this hunk:

|@@ -4236,8 +4232,6 @@ void __init rcu_init(void)
|       for_each_online_cpu(cpu) {
|               rcutree_prepare_cpu(cpu);
|               rcu_cpu_starting(cpu);
|-              if (IS_ENABLED(CONFIG_TREE_SRCU))
|-                      srcu_online_cpu(cpu);
|       }
| }

which got removed in v4.16.

Quoted 2 lines

Is this again a hidden RCU detail that preempt_disable() on CPU4 is enough to ensure that CPU2 does not get marked offline between?

The call_rcu_sched parameter to synchronize_rcu_mult() makes this work. This synchronize_rcu_mult() call is in sched_cpu_deactivate(), so it is a hidden sched/RCU detail, I guess.

Or am I missing the point of your question?

Quoted 6 lines

Quoted 2 lines

Or is getting rid of that preempt_disable region the real reason for this change?

Well, that preempt_disable() + queue_(delayed_)work() does not work -RT. But looking further, that preempt_disable() while looking at online CPUs didn't look good.

That is why it is invoked from the very early CPU-hotplug notifier. That early in the process, the preempt_disable() does prevent the current CPU from being taken offline twice: Once due to synchronize_rcu_mult(), and once due to the stop-machine call.

Quoted 11 lines

The description is not up-to-date. There was this hunk:

|@@ -4236,8 +4232,6 @@ void __init rcu_init(void)
|       for_each_online_cpu(cpu) {
|               rcutree_prepare_cpu(cpu);
|               rcu_cpu_starting(cpu);
|-              if (IS_ENABLED(CONFIG_TREE_SRCU))
|-                      srcu_online_cpu(cpu);
|       }
| }

which got removed in v4.16.

Ah! Here is the current rcu_init() code:

	for_each_online_cpu(cpu) {
		rcutree_prepare_cpu(cpu);
		rcu_cpu_starting(cpu);
		rcutree_online_cpu(cpu);
	}

And rcutree_online_cpu() calls srcu_online_cpu() when CONFIG_TREE_SRCU is enabled, so no need for the direct call from rcu_init().

Thanx, Paul

Quoted 8 lines · Paul E. McKenney

Quoted 2 lines

Is this again a hidden RCU detail that preempt_disable() on CPU4 is enough to ensure that CPU2 does not get marked offline between?

The call_rcu_sched parameter to synchronize_rcu_mult() makes this work. This synchronize_rcu_mult() call is in sched_cpu_deactivate(), so it is a hidden sched/RCU detail, I guess.

Or am I missing the point of your question?

No, this answers it.

Quoted 11 lines

Quoted 6 lines

Quoted 2 lines

Or is getting rid of that preempt_disable region the real reason for this change?

Well, that preempt_disable() + queue_(delayed_)work() does not work -RT. But looking further, that preempt_disable() while looking at online CPUs didn't look good.

That is why it is invoked from the very early CPU-hotplug notifier. That early in the process, the preempt_disable() does prevent the current CPU from being taken offline twice: Once due to synchronize_rcu_mult(), and once due to the stop-machine call.

:)

Quoted 14 lines

Quoted 11 lines

The description is not up-to-date. There was this hunk:

|@@ -4236,8 +4232,6 @@ void __init rcu_init(void)
|       for_each_online_cpu(cpu) {
|               rcutree_prepare_cpu(cpu);
|               rcu_cpu_starting(cpu);
|-              if (IS_ENABLED(CONFIG_TREE_SRCU))
|-                      srcu_online_cpu(cpu);
|       }
| }

which got removed in v4.16.

Ah! Here is the current rcu_init() code:

Quoted 8 lines

	for_each_online_cpu(cpu) {
		rcutree_prepare_cpu(cpu);
		rcu_cpu_starting(cpu);
		rcutree_online_cpu(cpu);
	}

And rcutree_online_cpu() calls srcu_online_cpu() when CONFIG_TREE_SRCU is enabled, so no need for the direct call from rcu_init().

So if a CPU goes down, the timer gets migrated to another CPU. If the CPU is already offline the timer can be programmed and nothing happens. If timer_add_on() would return an error we could have fallback code. Looking at the users of queue_delayed_work_on() there are only two using it really (the others are using smp_processor_id()) and one of them is using get_online_cpus().
It does not look like there a lot of users affected. Would be reasonable to avoid adding timers to offlined CPUs?

Just to make sure I understand, this is the call to queue_delayed_work_on() from srcu_queue_delayed_work_on(), right?

And if I am guessing correctly, you would like to get rid of the constraint requiring CPUHP_RCUTREE_PREP to precede CPUHP_TIMERS_PREPARE?

If so, the swait_event_idle_timeout_exclusive() in rcu_gp_fqs_loop() in kernel/rcu/tree.c also requires this ordering. There are probably other pieces of code needing this.

Plus the reason for running this on a specific CPU is that the workqueue item is processing that CPU's per-CPU variables, including invoking that CPU's callbacks. The item is srcu_invoke_callbacks().

Thanx, Paul

Quoted 2 lines · Paul E. McKenney

Just to make sure I understand, this is the call to queue_delayed_work_on() from srcu_queue_delayed_work_on(), right?

correct.

Quoted 2 lines

And if I am guessing correctly, you would like to get rid of the constraint requiring CPUHP_RCUTREE_PREP to precede CPUHP_TIMERS_PREPARE?

no, my problem is the preempt_disable() around queue_delayed_work_on(). If the CPUs goes offline _after_ queue_delayed_work_on() then the timer gets migrated and work item should show up on another CPU. If the CPU is offline at queue_delayed_work_on() time then the timer gets enqueued and won't fire until the CPU is back online and I *think* that is the reason behind this "is CPU online" check.

Quoted 7 lines

If so, the swait_event_idle_timeout_exclusive() in rcu_gp_fqs_loop() in kernel/rcu/tree.c also requires this ordering. There are probably other pieces of code needing this.

Plus the reason for running this on a specific CPU is that the workqueue item is processing that CPU's per-CPU variables, including invoking that CPU's callbacks. The item is srcu_invoke_callbacks().

The SRCU callback is invoking per-CPU variables? Like this_cpu_ptr()? But if the CPU is offline then you fallback to queue_delayed_work()?