fix rcu vs hotplug race

PATCH In review Dhaval Giani 5 participants 18 messages Jun 23, 2008 – Aug 1, 2008

lore

Share & tools

lore

Dhaval Giani

author

2008-06-23 10:37 UTC #0 lore raw

PATCH 1/1 fix rcu vs hotplug race

On running kernel compiles in parallel with cpu hotplug,

------------[ cut here ]------------
WARNING: at arch/x86/kernel/smp.c:118
native_smp_send_reschedule+0x21/0x36()
Modules linked in:
Pid: 27483, comm: cc1 Not tainted 2.6.26-rc7 #1
 [<c01217d9>] warn_on_slowpath+0x41/0x5d
 [<c01515b7>] ? generic_file_aio_read+0x10f/0x137
 [<c0151340>] ? file_read_actor+0x0/0xf7
 [<c013ae4c>] ? validate_chain+0xaa/0x29c
 [<c013c854>] ? __lock_acquire+0x612/0x666
 [<c013c854>] ? __lock_acquire+0x612/0x666
 [<c013ae4c>] ? validate_chain+0xaa/0x29c
 [<c01715d3>] ? file_kill+0x2d/0x30
 [<c013cbd7>] ? __lock_release+0x4b/0x51
 [<c01715d3>] ? file_kill+0x2d/0x30
 [<c0110355>] native_smp_send_reschedule+0x21/0x36
 [<c014fe8f>] force_quiescent_state+0x47/0x57
 [<c014fef0>] call_rcu+0x51/0x6d
 [<c01713b3>] __fput+0x130/0x158
 [<c0171231>] fput+0x17/0x19
 [<c016fd99>] filp_close+0x4d/0x57
 [<c016fdff>] sys_close+0x5c/0x97
 [<c0103861>] sysenter_past_esp+0x6a/0xb1
 =======================
---[ end trace aa35f3913ddf2d06 ]---

This is because a reschedule is sent to a CPU which is offline. Just ensure that the CPU we send the smp_send_reschedule is actually online.

Signed-off-by Dhaval Giani <[email protected]>

---
 kernel/rcuclassic.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

kernel/rcuclassic.c +2 -1

Index: linux-2.6.26-rc7/kernel/rcuclassic.c
===================================================================
@@ -93,7 +93,8 @@ static void force_quiescent_state(struct
 		cpumask = rcp->cpumask;
 		cpu_clear(rdp->cpu, cpumask);
 		for_each_cpu_mask(cpu, cpumask)
-			smp_send_reschedule(cpu);
+			if (cpu_online(cpu))
+				smp_send_reschedule(cpu);
 	}
 }
 #else

Signature · 2 lines

regards,
Dhaval

Ingo Molnar

2008-06-23 11:04 UTC #1 lore raw

Quoted 42 lines · Dhaval Giani

On running kernel compiles in parallel with cpu hotplug,

------------[ cut here ]------------
WARNING: at arch/x86/kernel/smp.c:118
native_smp_send_reschedule+0x21/0x36()
Modules linked in:
Pid: 27483, comm: cc1 Not tainted 2.6.26-rc7 #1
 [<c01217d9>] warn_on_slowpath+0x41/0x5d
 [<c01515b7>] ? generic_file_aio_read+0x10f/0x137
 [<c0151340>] ? file_read_actor+0x0/0xf7
 [<c013ae4c>] ? validate_chain+0xaa/0x29c
 [<c013c854>] ? __lock_acquire+0x612/0x666
 [<c013c854>] ? __lock_acquire+0x612/0x666
 [<c013ae4c>] ? validate_chain+0xaa/0x29c
 [<c01715d3>] ? file_kill+0x2d/0x30
 [<c013cbd7>] ? __lock_release+0x4b/0x51
 [<c01715d3>] ? file_kill+0x2d/0x30
 [<c0110355>] native_smp_send_reschedule+0x21/0x36
 [<c014fe8f>] force_quiescent_state+0x47/0x57
 [<c014fef0>] call_rcu+0x51/0x6d
 [<c01713b3>] __fput+0x130/0x158
 [<c0171231>] fput+0x17/0x19
 [<c016fd99>] filp_close+0x4d/0x57
 [<c016fdff>] sys_close+0x5c/0x97
 [<c0103861>] sysenter_past_esp+0x6a/0xb1
 =======================
---[ end trace aa35f3913ddf2d06 ]---

This is because a reschedule is sent to a CPU which is offline. Just ensure that the CPU we send the smp_send_reschedule is actually online.

Signed-off-by Dhaval Giani <[email protected]>

---
 kernel/rcuclassic.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6.26-rc7/kernel/rcuclassic.c
===================================================================

kernel/rcuclassic.c static void force_quiescent_state(struct · line 98

Quoted 7 lines

kernel/rcuclassic.c +2 -1

@@ -93,7 +93,8 @@ static void force_quiescent_state(struct
 		cpumask = rcp->cpumask;
 		cpu_clear(rdp->cpu, cpumask);
 		for_each_cpu_mask(cpu, cpumask)
-			smp_send_reschedule(cpu);
+			if (cpu_online(cpu))
+				smp_send_reschedule(cpu);
 	}

hm, not sure - we might just be fighting the symptom and we might now create a silent resource leak instead. Isnt a full RCU quiescent state forced (on all CPUs) before a CPU is cleared out of cpu_online_map? That way the to-be-offlined CPU should never actually show up in rcp->cpumask.

Ingo

Gautham R Shenoy

2008-06-23 11:48 UTC #2 lore raw

Quoted 40 lines · Ingo Molnar

Quoted 37 lines · Dhaval Giani

On running kernel compiles in parallel with cpu hotplug,

------------[ cut here ]------------
WARNING: at arch/x86/kernel/smp.c:118
native_smp_send_reschedule+0x21/0x36()
Modules linked in:
Pid: 27483, comm: cc1 Not tainted 2.6.26-rc7 #1
 [<c01217d9>] warn_on_slowpath+0x41/0x5d
 [<c01515b7>] ? generic_file_aio_read+0x10f/0x137
 [<c0151340>] ? file_read_actor+0x0/0xf7
 [<c013ae4c>] ? validate_chain+0xaa/0x29c
 [<c013c854>] ? __lock_acquire+0x612/0x666
 [<c013c854>] ? __lock_acquire+0x612/0x666
 [<c013ae4c>] ? validate_chain+0xaa/0x29c
 [<c01715d3>] ? file_kill+0x2d/0x30
 [<c013cbd7>] ? __lock_release+0x4b/0x51
 [<c01715d3>] ? file_kill+0x2d/0x30
 [<c0110355>] native_smp_send_reschedule+0x21/0x36
 [<c014fe8f>] force_quiescent_state+0x47/0x57
 [<c014fef0>] call_rcu+0x51/0x6d
 [<c01713b3>] __fput+0x130/0x158
 [<c0171231>] fput+0x17/0x19
 [<c016fd99>] filp_close+0x4d/0x57
 [<c016fdff>] sys_close+0x5c/0x97
 [<c0103861>] sysenter_past_esp+0x6a/0xb1
 =======================
---[ end trace aa35f3913ddf2d06 ]---

This is because a reschedule is sent to a CPU which is offline. Just ensure that the CPU we send the smp_send_reschedule is actually online.

Signed-off-by Dhaval Giani <[email protected]>

---
 kernel/rcuclassic.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

Quoted 13 lines

kernel/rcuclassic.c static void force_quiescent_state(struct · line 98
Quoted 12 lines
Index: linux-2.6.26-rc7/kernel/rcuclassic.c
===================================================================
kernel/rcuclassic.c +2 -1
@@ -93,7 +93,8 @@ static void force_quiescent_state(struct
 		cpumask = rcp->cpumask;
 		cpu_clear(rdp->cpu, cpumask);
 		for_each_cpu_mask(cpu, cpumask)
-			smp_send_reschedule(cpu);
+			if (cpu_online(cpu))
+				smp_send_reschedule(cpu);
 	}

Hi Ingo,

Quoted 5 lines

hm, not sure - we might just be fighting the symptom and we might now create a silent resource leak instead. Isnt a full RCU quiescent state forced (on all CPUs) before a CPU is cleared out of cpu_online_map? That way the to-be-offlined CPU should never actually show up in rcp->cpumask.

No, this does not happen currently. The rcp->cpumask is always initialized to cpu_online_map&~nohz_cpu_mask when we start a new batch. Hence, before the batch ends, if a cpu goes offline we _can_ have a stale rcp->cpumask, till the RCU subsystem has handled it's CPU_DEAD notification.

Thus for a tiny interval, the rcp->cpumask would contain the offlined CPU. One of the alternatives is probably to handle this using CPU_DYING notifier instead of CPU_DEAD where we can call __rcu_offline_cpu().

The warn_on that dhaval was hitting was because of some cpu-offline that was called just before we did a local_irq_save inside call_rcu(). But at that time, the rcp->cpumask was still stale, and hence we ended up sending a smp_reschedule() to an offlined cpu. So the check may not create any resource leak.

But probably there's a better way to fix this.

Quoted 2 lines

Ingo

Signature · 2 lines

Thanks and Regards
gautham

Ingo Molnar

2008-06-24 11:02 UTC #3 lore raw

Quoted 17 lines · Gautham R Shenoy

Quoted 5 lines
hm, not sure - we might just be fighting the symptom and we might now create a silent resource leak instead. Isnt a full RCU quiescent state forced (on all CPUs) before a CPU is cleared out of cpu_online_map? That way the to-be-offlined CPU should never actually show up in rcp->cpumask.
No, this does not happen currently. The rcp->cpumask is always initialized to cpu_online_map&~nohz_cpu_mask when we start a new batch. Hence, before the batch ends, if a cpu goes offline we _can_ have a stale rcp->cpumask, till the RCU subsystem has handled it's CPU_DEAD notification.
Thus for a tiny interval, the rcp->cpumask would contain the offlined CPU. One of the alternatives is probably to handle this using CPU_DYING notifier instead of CPU_DEAD where we can call __rcu_offline_cpu().

Quoted 5 lines

The warn_on that dhaval was hitting was because of some cpu-offline that was called just before we did a local_irq_save inside call_rcu(). But at that time, the rcp->cpumask was still stale, and hence we ended up sending a smp_reschedule() to an offlined cpu. So the check may not create any resource leak.

the check may not - but the problem it highlights might and with the patch we'd end up hiding potential problems in this area.

Paul, what do you think about this mixed CPU hotplug plus RCU workload?

Ingo

Paul E. McKenney

2008-06-26 15:27 UTC #4 lore raw

Quoted 26 lines · Ingo Molnar

Quoted 17 lines · Gautham R Shenoy
Quoted 5 lines
hm, not sure - we might just be fighting the symptom and we might now create a silent resource leak instead. Isnt a full RCU quiescent state forced (on all CPUs) before a CPU is cleared out of cpu_online_map? That way the to-be-offlined CPU should never actually show up in rcp->cpumask.
No, this does not happen currently. The rcp->cpumask is always initialized to cpu_online_map&~nohz_cpu_mask when we start a new batch. Hence, before the batch ends, if a cpu goes offline we _can_ have a stale rcp->cpumask, till the RCU subsystem has handled it's CPU_DEAD notification.
Thus for a tiny interval, the rcp->cpumask would contain the offlined CPU. One of the alternatives is probably to handle this using CPU_DYING notifier instead of CPU_DEAD where we can call __rcu_offline_cpu().
Quoted 5 lines
The warn_on that dhaval was hitting was because of some cpu-offline that was called just before we did a local_irq_save inside call_rcu(). But at that time, the rcp->cpumask was still stale, and hence we ended up sending a smp_reschedule() to an offlined cpu. So the check may not create any resource leak.

Quoted 4 lines

the check may not - but the problem it highlights might and with the patch we'd end up hiding potential problems in this area.
Paul, what do you think about this mixed CPU hotplug plus RCU workload?

RCU most certainly needs to work correctly in face of arbitrary sequences of CPU-hotplug events, and should therefore be tested with arbitrary CPU-hotplug tests. And RCU also most certainly needs to refrain from issuing spurious warning messages that might over time be ignored, possibly causing someone to miss a real bug. My concern with this patch is in the second spurious-warning area.

Not sure I answered the actual question, though...

Thanx, Paul

Gautham R Shenoy

2008-06-27 04:46 UTC #5 lore raw

Quoted 30 lines · Paul E. McKenney

Quoted 29 lines · Ingo Molnar
Quoted 17 lines · Gautham R Shenoy
Quoted 5 lines
hm, not sure - we might just be fighting the symptom and we might now create a silent resource leak instead. Isnt a full RCU quiescent state forced (on all CPUs) before a CPU is cleared out of cpu_online_map? That way the to-be-offlined CPU should never actually show up in rcp->cpumask.
No, this does not happen currently. The rcp->cpumask is always initialized to cpu_online_map&~nohz_cpu_mask when we start a new batch. Hence, before the batch ends, if a cpu goes offline we _can_ have a stale rcp->cpumask, till the RCU subsystem has handled it's CPU_DEAD notification.
Thus for a tiny interval, the rcp->cpumask would contain the offlined CPU. One of the alternatives is probably to handle this using CPU_DYING notifier instead of CPU_DEAD where we can call __rcu_offline_cpu().
Quoted 5 lines
The warn_on that dhaval was hitting was because of some cpu-offline that was called just before we did a local_irq_save inside call_rcu(). But at that time, the rcp->cpumask was still stale, and hence we ended up sending a smp_reschedule() to an offlined cpu. So the check may not create any resource leak.
the check may not - but the problem it highlights might and with the patch we'd end up hiding potential problems in this area.

Quoted 8 lines

Quoted 1 line
Paul, what do you think about this mixed CPU hotplug plus RCU workload?
RCU most certainly needs to work correctly in face of arbitrary sequences of CPU-hotplug events, and should therefore be tested with arbitrary CPU-hotplug tests. And RCU also most certainly needs to refrain from issuing spurious warning messages that might over time be ignored, possibly causing someone to miss a real bug. My concern with this patch is in the second spurious-warning area.

IMHO the warning is a spurious one.
Here's the timeline.
CPU_A CPU_B
--------------------------------------------------------------
cpu_down(): .
. .
. .
stop_machine(): /* disables preemption, .
* and irqs */ .
. .
. .
take_cpu_down(); .
. .
. .
. .
cpu_disable(); /*this removes cpu .

		*from cpu_online_map 		.
		*/				.

. .
. .
restart_machine(); /* enables irqs */ .
------WINDOW DURING WHICH rcp->cpumask is stale ---------------
. call_rcu();
. /* disables irqs here */
. .force_quiescent_state();
.CPU_DEAD: .for_each_cpu(rcp->cpumask)
. . smp_send_reschedule();
. .
. . WARN_ON() for offlined CPU!
.
.
.
rcu_cpu_notify:
.
-------- WINDOW ENDS ------------------------------------------ rcu_offline_cpu() /* Which calls cpu_quiet()
* which removes
* cpu from rcp->cpumask.
*/

If a new batch was started just before calling stop_machine_run(), the "tobe-offlined" cpu is still present in rcp-cpumask.

During a cpu-offline, from take_cpu_down(),
we queue an rt-prio idle task
as the next task to be picked by the scheduler.
We also call cpu_disable() which will disable any further interrupts and remove the cpu's bit from the cpu_online_map.

Once the stop_machine_run() successfully calls take_cpu_down(), it calls schedule(). That's the last time a schedule is called on the offlined cpu, and hence the last time when rdp->passed_quiesc will be set to 1 through rcu_qsctr_inc().

But the cpu_quiet() will be on this cpu will be called only when the next RCU_SOFTIRQ occurs on this CPU. So at this time, the offlined CPU is still set in rcp->cpumask.

Now coming back to the idle_task which truely offlines the CPU, it does check for a pending RCU and raises the softirq, since it will find rdp->passed_quiesc to be 0 in this case. However, since the cpu is offline I am not sure if the softirq will trigger on the CPU.

Even if it doesn't the rcu_offline_cpu() will find that rcp->completed is not the same as rcp->cur, which means that our cpu could be holding up the grace period progression. Hence we call cpu_quiet() and move ahead.

But because of the window explained in the timeline, we could still have a call_rcu() before the RCU subsystem executes it's CPU_DEAD notification, and we send smp_send_reschedule() to offlined cpu while trying to force the quiescent states. The appended patch adds comments and prevents checking for offlined cpu everytime.

Author: Gautham R Shenoy <[email protected]>
Date:   Fri Jun 27 09:33:55 2008 +0530

cpu_online_map is updated by the _cpu_down()
using stop_machine_run(). Since force_quiescent_state is invoked from irqs disabled section, stop_machine_run() won't be executing while a cpu is executing force_quiescent_state(). Hence the cpu_online_map is stable while we're in the irq disabled section.

However, a cpu might have been offlined _just_ before we disabled irqs while entering force_quiescent_state(). And rcu subsystem might not yet have handled the CPU_DEAD notification, leading to the offlined cpu's bit being set in the rcp->cpumask.

Hence cpumask = (rcp->cpumask & cpu_online_map) to prevent sending smp_reschedule() to an offlined CPU.

Signed-off-by Gautham R Shenoy <[email protected]>

Cc Dhaval Giani <[email protected]>

kernel/rcuclassic.c +15 -1

index f4ffbd0..a38895a 100644
@@ -89,8 +89,22 @@ static void force_quiescent_state(struct rcu_data *rdp,
 		/*
 		 * Don't send IPI to itself. With irqs disabled,
 		 * rdp->cpu is the current cpu.
+		 *
+		 * cpu_online_map is updated by the _cpu_down()
+		 * using stop_machine_run(). Since we're in irqs disabled
+		 * section, stop_machine_run() is not exectuting, hence
+		 * the cpu_online_map is stable.
+		 *
+		 * However,  a cpu might have been offlined _just_ before
+		 * we disabled irqs while entering here.
+		 * And rcu subsystem might not yet have handled the CPU_DEAD
+		 * notification, leading to the offlined cpu's bit
+		 * being set in the rcp->cpumask.
+		 *
+		 * Hence cpumask = (rcp->cpumask & cpu_online_map) to prevent
+		 * sending smp_reschedule() to an offlined CPU.
 		 */
-		cpumask = rcp->cpumask;
+		cpus_and(cpumask, rcp->cpumask, cpu_online_map);
 		cpu_clear(rdp->cpu, cpumask);
 		for_each_cpu_mask(cpu, cpumask)
 			smp_send_reschedule(cpu);

Quoted 4 lines

Not sure I answered the actual question, though...
Thanx, Paul

Signature · 2 lines

Thanks and Regards
gautham

Dipankar Sarma

2008-06-27 05:22 UTC #6 lore raw

Quoted 18 lines · Gautham R Shenoy

IMHO the warning is a spurious one.
Here's the timeline.
CPU_A CPU_B
--------------------------------------------------------------
cpu_down(): .
. .
. .
stop_machine(): /* disables preemption, .
* and irqs */ .
. .
. .
take_cpu_down(); .
. .
. .
. .
cpu_disable(); /*this removes cpu .
		*from cpu_online_map 		.
		*/				.

Quoted 12 lines

. .
. .
restart_machine(); /* enables irqs */ .
------WINDOW DURING WHICH rcp->cpumask is stale ---------------
. call_rcu();
. /* disables irqs here */
. .force_quiescent_state();
.CPU_DEAD: .for_each_cpu(rcp->cpumask)
. . smp_send_reschedule();
. .
. . WARN_ON() for offlined CPU!
.

Exactly. The call_rcu()s are coming from a different subsystem and can happen anytime during the CPU hotplug path. So, RCU subsystem doesn't have anything to do to keep rcu->cpumask consistent. It is *safe* even if we miss poking a cpu or two while forcing quiescent state in all CPUs. The worst that can happen is a delay in grace period. No correctness problem here.

Thanks
Dipankar

Dhaval Giani

author

2008-06-27 05:52 UTC #7 lore raw

Quoted 32 lines · Dipankar Sarma

Quoted 18 lines · Gautham R Shenoy
IMHO the warning is a spurious one.
Here's the timeline.
CPU_A CPU_B
--------------------------------------------------------------
cpu_down(): .
. .
. .
stop_machine(): /* disables preemption, .
* and irqs */ .
. .
. .
take_cpu_down(); .
. .
. .
. .
cpu_disable(); /*this removes cpu .
		*from cpu_online_map 		.
		*/				.
Quoted 12 lines
. .
. .
restart_machine(); /* enables irqs */ .
------WINDOW DURING WHICH rcp->cpumask is stale ---------------
. call_rcu();
. /* disables irqs here */
. .force_quiescent_state();
.CPU_DEAD: .for_each_cpu(rcp->cpumask)
. . smp_send_reschedule();
. .
. . WARN_ON() for offlined CPU!
.

Quoted 7 lines

Exactly. The call_rcu()s are coming from a different subsystem and can happen anytime during the CPU hotplug path. So, RCU subsystem doesn't have anything to do to keep rcu->cpumask consistent. It is *safe* even if we miss poking a cpu or two while forcing quiescent state in all CPUs. The worst that can happen is a delay in grace period. No correctness problem here.

One question. What is preventing a CPU from clearing its mask after we have checked whether it is online but before we have called into smp_send_reschedule?

Signature · 2 lines

regards,
Dhaval

Paul E. McKenney

2008-06-27 14:58 UTC #8 lore raw

Quoted 41 lines · Dhaval Giani

Quoted 32 lines · Dipankar Sarma
Quoted 18 lines · Gautham R Shenoy
IMHO the warning is a spurious one.
Here's the timeline.
CPU_A CPU_B
--------------------------------------------------------------
cpu_down(): .
. .
. .
stop_machine(): /* disables preemption, .
* and irqs */ .
. .
. .
take_cpu_down(); .
. .
. .
. .
cpu_disable(); /*this removes cpu .
		*from cpu_online_map 		.
		*/				.
Quoted 12 lines
. .
. .
restart_machine(); /* enables irqs */ .
------WINDOW DURING WHICH rcp->cpumask is stale ---------------
. call_rcu();
. /* disables irqs here */
. .force_quiescent_state();
.CPU_DEAD: .for_each_cpu(rcp->cpumask)
. . smp_send_reschedule();
. .
. . WARN_ON() for offlined CPU!
.
Quoted 7 lines
Exactly. The call_rcu()s are coming from a different subsystem and can happen anytime during the CPU hotplug path. So, RCU subsystem doesn't have anything to do to keep rcu->cpumask consistent. It is *safe* even if we miss poking a cpu or two while forcing quiescent state in all CPUs. The worst that can happen is a delay in grace period. No correctness problem here.

Quoted 3 lines

One question. What is preventing a CPU from clearing its mask after we have checked whether it is online but before we have called into smp_send_reschedule?

This is my concern as well. Gautham, at which point in the above timeline is the offlining CPU marked DYING? Before stop_machine(), right?

If so, can't we just disable irqs, check for DYING or DEAD, and invoke smp_send_reschedule() only if not DYING or DEAD?

Thanx, Paul

Gautham R Shenoy

2008-07-01 05:38 UTC #9 lore raw

Quoted 42 lines · Paul E. McKenney

Quoted 41 lines · Dhaval Giani
Quoted 39 lines · Dipankar Sarma
Quoted 18 lines · Gautham R Shenoy
IMHO the warning is a spurious one.
Here's the timeline.
CPU_A CPU_B
--------------------------------------------------------------
cpu_down(): .
. .
. .
stop_machine(): /* disables preemption, .
* and irqs */ .
. .
. .
take_cpu_down(); .
. .
. .
. .
cpu_disable(); /*this removes cpu .
		*from cpu_online_map 		.
		*/				.
Quoted 12 lines
. .
. .
restart_machine(); /* enables irqs */ .
------WINDOW DURING WHICH rcp->cpumask is stale ---------------
. call_rcu();
. /* disables irqs here */
. .force_quiescent_state();
.CPU_DEAD: .for_each_cpu(rcp->cpumask)
. . smp_send_reschedule();
. .
. . WARN_ON() for offlined CPU!
.
Exactly. The call_rcu()s are coming from a different subsystem and can happen anytime during the CPU hotplug path. So, RCU subsystem doesn't have anything to do to keep rcu->cpumask consistent. It is *safe* even if we miss poking a cpu or two while forcing quiescent state in all CPUs. The worst that can happen is a delay in grace period. No correctness problem here.

Quoted 6 lines

Quoted 3 lines
One question. What is preventing a CPU from clearing its mask after we have checked whether it is online but before we have called into smp_send_reschedule?
This is my concern as well. Gautham, at which point in the above timeline is the offlining CPU marked DYING? Before stop_machine(), right?

No :) The offlining CPU is marked DYING after stop_machine(), inside take_cpu_down() which is the work we want to execute after stopping the machine.

it's like
_cpu_down()

|
|-> stop_machine_run();
|   |
|   |-> stop_machine(); /* All CPUs irqs disabled. */
|   |
|   |-> take_cpu_down() --> sets state to CPU_DYING. disables irqs on
|   |				offlined cpu
|   |
|   |-> restart_machine(); /* All CPUs irqs reenabled */
|
|-> send_CPU_DEAD_notification.

The very fact that a thread is running with irqs disabled means that stop_machine_run() thread cannot start executing the work it has been assinged to execute. Because for Machine to be stopped, stop_machine() needs to create n-1 high priority threads on n-1 online cpus, which will disable interrupts and preemption, and stop the machine. Then it will run the task assigned to it on the ith cpu, which in this case is the cpu to be offlined.

So, it's the design of stop_machine() that's preventing someone from updating the cpu_online_map while force_quiescent_state() is performing the
cpu_is_online() check. Becase we always call force_quiescent_state() with irqs disabled :)

Quoted 8 lines

If so, can't we just disable irqs, check for DYING or DEAD, and invoke smp_send_reschedule() only if not DYING or DEAD?
Thanx, Paul

Signature · 2 lines

Thanks and Regards
gautham

Ingo Molnar

Acked-by Gautham R Shenoy

2008-07-01 06:16 UTC #10 lore raw

Quoted 4 lines · Gautham R Shenoy

So, it's the design of stop_machine() that's preventing someone from updating the cpu_online_map while force_quiescent_state() is performing the cpu_is_online() check. Becase we always call force_quiescent_state() with irqs disabled :)

Paul, do you concur? I'll apply the commit in the form below to tip/core/urgent if Paul agrees.

Ingo

----------------------------------->
Subject: cpu-hotplug + rcu: fix spurious warning
From: Dhaval Giani <[email protected]>
Date: Mon, 23 Jun 2008 16:07:00 +0530

On running kernel compiles in parallel with cpu hotplug,

------------[ cut here ]------------
WARNING: at arch/x86/kernel/smp.c:118
native_smp_send_reschedule+0x21/0x36()
Modules linked in:
Pid: 27483, comm: cc1 Not tainted 2.6.26-rc7 #1
 [<c01217d9>] warn_on_slowpath+0x41/0x5d
 [<c01515b7>] ? generic_file_aio_read+0x10f/0x137
 [<c0151340>] ? file_read_actor+0x0/0xf7
 [<c013ae4c>] ? validate_chain+0xaa/0x29c
 [<c013c854>] ? __lock_acquire+0x612/0x666
 [<c013c854>] ? __lock_acquire+0x612/0x666
 [<c013ae4c>] ? validate_chain+0xaa/0x29c
 [<c01715d3>] ? file_kill+0x2d/0x30
 [<c013cbd7>] ? __lock_release+0x4b/0x51
 [<c01715d3>] ? file_kill+0x2d/0x30
 [<c0110355>] native_smp_send_reschedule+0x21/0x36
 [<c014fe8f>] force_quiescent_state+0x47/0x57
 [<c014fef0>] call_rcu+0x51/0x6d
 [<c01713b3>] __fput+0x130/0x158
 [<c0171231>] fput+0x17/0x19
 [<c016fd99>] filp_close+0x4d/0x57
 [<c016fdff>] sys_close+0x5c/0x97
 [<c0103861>] sysenter_past_esp+0x6a/0xb1
 =======================
---[ end trace aa35f3913ddf2d06 ]---

This is because a reschedule is sent to a CPU which is offline. Just ensure that the CPU we send the smp_send_reschedule is actually online.

It's the design of stop_machine() that's preventing someone from updating the cpu_online_map while force_quiescent_state() is performing the cpu_is_online() check. Becase we always call force_quiescent_state() with irqs disabled :)

Signed-off-by Dhaval Giani <[email protected]>

Acked-by Gautham R Shenoy <[email protected]>

Cc [email protected]

Cc Dipankar Sarma <[email protected]>

Cc [email protected]

Cc Peter Zijlstra <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/rcuclassic.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

kernel/rcuclassic.c +2 -1

Index: tip/kernel/rcuclassic.c
===================================================================
@@ -93,7 +93,8 @@ static void force_quiescent_state(struct
 		cpumask = rcp->cpumask;
 		cpu_clear(rdp->cpu, cpumask);
 		for_each_cpu_mask(cpu, cpumask)
-			smp_send_reschedule(cpu);
+			if (cpu_online(cpu))
+				smp_send_reschedule(cpu);
 	}
 }
 #else

Dhaval Giani

author

2008-07-01 06:29 UTC #11 lore raw

Quoted 8 lines · Ingo Molnar

Quoted 4 lines · Gautham R Shenoy
So, it's the design of stop_machine() that's preventing someone from updating the cpu_online_map while force_quiescent_state() is performing the cpu_is_online() check. Becase we always call force_quiescent_state() with irqs disabled :)

Quoted 4 lines

Paul, do you concur? I'll apply the commit in the form below to tip/core/urgent if Paul agrees.
Ingo

Ingo,

I believe Gautham's fix at http://lkml.org/lkml/2008/6/27/9 is better and also explains it better.

Thanks.

Signature · 2 lines

regards,
Dhaval

Ingo Molnar

2008-07-01 06:41 UTC #12 lore raw

Quoted 4 lines · Dhaval Giani

Ingo,
I believe Gautham's fix at http://lkml.org/lkml/2008/6/27/9 is better and also explains it better.

ah, indeed - picked that one up instead.

Ingo

Ingo Molnar

2008-07-01 06:53 UTC #13 lore raw

Quoted 6 lines · Ingo Molnar

Quoted 4 lines
Ingo,
I believe Gautham's fix at http://lkml.org/lkml/2008/6/27/9 is better and also explains it better.
ah, indeed - picked that one up instead.

this is the patch i picked up:

-------------------------->
Subject: rcu: fix hotplug vs rcu race
From: Gautham R Shenoy <[email protected]>
Date: Fri, 27 Jun 2008 10:17:38 +0530

Dhaval Giani reported this warning during cpu hotplug stress-tests:

| On running kernel compiles in parallel with cpu hotplug:
|
| WARNING: at arch/x86/kernel/smp.c:118
| native_smp_send_reschedule+0x21/0x36()
| Modules linked in:
| Pid: 27483, comm: cc1 Not tainted 2.6.26-rc7 #1
| [...]
|  [<c0110355>] native_smp_send_reschedule+0x21/0x36
|  [<c014fe8f>] force_quiescent_state+0x47/0x57
|  [<c014fef0>] call_rcu+0x51/0x6d
|  [<c01713b3>] __fput+0x130/0x158
|  [<c0171231>] fput+0x17/0x19
|  [<c016fd99>] filp_close+0x4d/0x57
|  [<c016fdff>] sys_close+0x5c/0x97

IMHO the warning is a spurious one.

cpu_online_map is updated by the _cpu_down() using stop_machine_run(). Since force_quiescent_state is invoked from irqs disabled section, stop_machine_run() won't be executing while a cpu is executing force_quiescent_state(). Hence the cpu_online_map is stable while we're in the irq disabled section.

Hence cpumask = (rcp->cpumask & cpu_online_map) to prevent sending smp_reschedule() to an offlined CPU.

Here's the timeline:

		*from cpu_online_map 		.
		*/				.

Ingo Molnar

2008-07-01 07:53 UTC #14 lore raw

Quoted 10 lines · Ingo Molnar

Quoted 6 lines · Ingo Molnar
Quoted 4 lines
Ingo,
I believe Gautham's fix at http://lkml.org/lkml/2008/6/27/9 is better and also explains it better.
ah, indeed - picked that one up instead.
this is the patch i picked up:

for some reason my mail to lkml was cut in half - here it is again:

Quoted 6 lines · Ingo Molnar

Quoted 4 lines
Ingo,
I believe Gautham's fix at http://lkml.org/lkml/2008/6/27/9 is better and also explains it better.
ah, indeed - picked that one up instead.

this is the patch i picked up:

-------------------------->
Subject: rcu: fix hotplug vs rcu race
From: Gautham R Shenoy <[email protected]>
Date: Fri, 27 Jun 2008 10:17:38 +0530

Dhaval Giani reported this warning during cpu hotplug stress-tests:

| On running kernel compiles in parallel with cpu hotplug:
|
| WARNING: at arch/x86/kernel/smp.c:118
| native_smp_send_reschedule+0x21/0x36()
| Modules linked in:
| Pid: 27483, comm: cc1 Not tainted 2.6.26-rc7 #1
| [...]
|  [<c0110355>] native_smp_send_reschedule+0x21/0x36
|  [<c014fe8f>] force_quiescent_state+0x47/0x57
|  [<c014fef0>] call_rcu+0x51/0x6d
|  [<c01713b3>] __fput+0x130/0x158
|  [<c0171231>] fput+0x17/0x19
|  [<c016fd99>] filp_close+0x4d/0x57
|  [<c016fdff>] sys_close+0x5c/0x97

IMHO the warning is a spurious one.

cpu_online_map is updated by the _cpu_down() using stop_machine_run(). Since force_quiescent_state is invoked from irqs disabled section, stop_machine_run() won't be executing while a cpu is executing force_quiescent_state(). Hence the cpu_online_map is stable while we're in the irq disabled section.

Hence cpumask = (rcp->cpumask & cpu_online_map) to prevent sending smp_reschedule() to an offlined CPU.

Here's the timeline:

		*from cpu_online_map 		.
		*/				.

Ingo Molnar

Acked-by Dhaval Giani

2008-07-01 08:34 UTC #15 lore raw

Quoted 9 lines · Ingo Molnar

------WINDOW DURING WHICH rcp->cpumask is stale ---------------
. call_rcu();
. /* disables irqs here */
. .force_quiescent_state();
.CPU_DEAD: .for_each_cpu(rcp->cpumask)
. . smp_send_reschedule();
. .
. . WARN_ON() for offlined CPU!
--

hm, for some reason the "-- WINDOW ENDS" line has cut off processing somewhere along the mail route. Here it is again, with that line removed.

----------->
commit 8558f8f81680a43d383abd1b5f23d3501fedfa65

Author: Gautham R Shenoy <[email protected]>
Date:   Fri Jun 27 10:17:38 2008 +0530

rcu: fix hotplug vs rcu race

Dhaval Giani reported this warning during cpu hotplug stress-tests:

IMHO the warning is a spurious one.

cpu_online_map is updated by the _cpu_down() using stop_machine_run(). Since force_quiescent_state is invoked from irqs disabled section, stop_machine_run() won't be executing while a cpu is executing force_quiescent_state(). Hence the cpu_online_map is stable while we're in the irq disabled section.

Hence cpumask = (rcp->cpumask & cpu_online_map) to prevent sending smp_reschedule() to an offlined CPU.

Here's the timeline:

CPU_A CPU_B
--------------------------------------------------------------
cpu_down(): .
. .
. .
stop_machine(): /* disables preemption, .
* and irqs */ .
. .
. .
take_cpu_down(); .
. .
. .
. .
cpu_disable(); /*this removes cpu .
*from cpu_online_map .
*/ .
. .
. .
restart_machine(); /* enables irqs */ .
------WINDOW DURING WHICH rcp->cpumask is stale ---------------
. call_rcu();
. /* disables irqs here */
. .force_quiescent_state();
.CPU_DEAD: .for_each_cpu(rcp->cpumask)
. . smp_send_reschedule();
. .
. . WARN_ON() for offlined CPU!
.
.
.
rcu_cpu_notify:
.
[ -- line removed -- ]
rcu_offline_cpu() /* Which calls cpu_quiet()
* which removes
* cpu from rcp->cpumask.
*/

If a new batch was started just before calling stop_machine_run(), the "tobe-offlined" cpu is still present in rcp-cpumask.

During a cpu-offline, from take_cpu_down(), we queue an rt-prio idle task as the next task to be picked by the scheduler. We also call cpu_disable() which will disable any further interrupts and remove the cpu's bit from the cpu_online_map.

But the cpu_quiet() will be on this cpu will be called only when the next RCU_SOFTIRQ occurs on this CPU. So at this time, the offlined CPU is still set in rcp->cpumask.

cpu_online_map is updated by the _cpu_down() using stop_machine_run(). Since force_quiescent_state is invoked from irqs disabled section, stop_machine_run() won't be executing while a cpu is executing force_quiescent_state(). Hence the cpu_online_map is stable while we're in the irq disabled section.

Reported-by Dhaval Giani <[email protected]>

Signed-off-by Gautham R Shenoy <[email protected]>

Acked-by Dhaval Giani <[email protected]>

Cc Dipankar Sarma <[email protected]>

Cc [email protected]

Cc Peter Zijlstra <[email protected]>

Cc Rusty Russel <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

kernel/rcuclassic.c +15 -1

index f4ffbd0..a38895a 100644
@@ -89,8 +89,22 @@ static void force_quiescent_state(struct rcu_data *rdp,
 		/*
 		 * Don't send IPI to itself. With irqs disabled,
 		 * rdp->cpu is the current cpu.
+		 *
+		 * cpu_online_map is updated by the _cpu_down()
+		 * using stop_machine_run(). Since we're in irqs disabled
+		 * section, stop_machine_run() is not exectuting, hence
+		 * the cpu_online_map is stable.
+		 *
+		 * However,  a cpu might have been offlined _just_ before
+		 * we disabled irqs while entering here.
+		 * And rcu subsystem might not yet have handled the CPU_DEAD
+		 * notification, leading to the offlined cpu's bit
+		 * being set in the rcp->cpumask.
+		 *
+		 * Hence cpumask = (rcp->cpumask & cpu_online_map) to prevent
+		 * sending smp_reschedule() to an offlined CPU.
 		 */
-		cpumask = rcp->cpumask;
+		cpus_and(cpumask, rcp->cpumask, cpu_online_map);
 		cpu_clear(rdp->cpu, cpumask);
 		for_each_cpu_mask(cpu, cpumask)
 			smp_send_reschedule(cpu);

Paul E. McKenney

2008-07-01 19:46 UTC #16 lore raw

In reply to Gautham R Shenoy

Quoted 76 lines · Gautham R Shenoy

Quoted 42 lines · Paul E. McKenney
Quoted 41 lines · Dhaval Giani
Quoted 39 lines · Dipankar Sarma
Quoted 18 lines · Gautham R Shenoy
IMHO the warning is a spurious one.
Here's the timeline.
CPU_A CPU_B
--------------------------------------------------------------
cpu_down(): .
. .
. .
stop_machine(): /* disables preemption, .
* and irqs */ .
. .
. .
take_cpu_down(); .
. .
. .
. .
cpu_disable(); /*this removes cpu .
		*from cpu_online_map 		.
		*/				.
Quoted 12 lines
. .
. .
restart_machine(); /* enables irqs */ .
------WINDOW DURING WHICH rcp->cpumask is stale ---------------
. call_rcu();
. /* disables irqs here */
. .force_quiescent_state();
.CPU_DEAD: .for_each_cpu(rcp->cpumask)
. . smp_send_reschedule();
. .
. . WARN_ON() for offlined CPU!
.
Exactly. The call_rcu()s are coming from a different subsystem and can happen anytime during the CPU hotplug path. So, RCU subsystem doesn't have anything to do to keep rcu->cpumask consistent. It is *safe* even if we miss poking a cpu or two while forcing quiescent state in all CPUs. The worst that can happen is a delay in grace period. No correctness problem here.
Quoted 6 lines
Quoted 3 lines
One question. What is preventing a CPU from clearing its mask after we have checked whether it is online but before we have called into smp_send_reschedule?
This is my concern as well. Gautham, at which point in the above timeline is the offlining CPU marked DYING? Before stop_machine(), right?
No :) The offlining CPU is marked DYING after stop_machine(), inside take_cpu_down() which is the work we want to execute after stopping the machine.
it's like
_cpu_down()
|
|-> stop_machine_run();
|   |
|   |-> stop_machine(); /* All CPUs irqs disabled. */
|   |
|   |-> take_cpu_down() --> sets state to CPU_DYING. disables irqs on
|   |				offlined cpu
|   |
|   |-> restart_machine(); /* All CPUs irqs reenabled */
|
|-> send_CPU_DEAD_notification.
The very fact that a thread is running with irqs disabled means that stop_machine_run() thread cannot start executing the work it has been assinged to execute. Because for Machine to be stopped, stop_machine() needs to create n-1 high priority threads on n-1 online cpus, which will disable interrupts and preemption, and stop the machine. Then it will run the task assigned to it on the ith cpu, which in this case is the cpu to be offlined.

Quoted 5 lines

So, it's the design of stop_machine() that's preventing someone from updating the cpu_online_map while force_quiescent_state() is performing the
cpu_is_online() check. Becase we always call force_quiescent_state() with irqs disabled :)

Got it, so the patch looks good.

Thanx, Paul

Quoted 11 lines

Quoted 7 lines
If so, can't we just disable irqs, check for DYING or DEAD, and invoke smp_send_reschedule() only if not DYING or DEAD?
Thanx, Paul
Signature · 2 lines
Thanks and Regards
gautham

Paul E. McKenney

2008-08-01 21:01 UTC #17 lore raw

In reply to Gautham R Shenoy

Quoted 76 lines · Gautham R Shenoy

Quoted 42 lines · Paul E. McKenney
Quoted 41 lines · Dhaval Giani
Quoted 39 lines · Dipankar Sarma
Quoted 18 lines · Gautham R Shenoy
IMHO the warning is a spurious one.
Here's the timeline.
CPU_A CPU_B
--------------------------------------------------------------
cpu_down(): .
. .
. .
stop_machine(): /* disables preemption, .
* and irqs */ .
. .
. .
take_cpu_down(); .
. .
. .
. .
cpu_disable(); /*this removes cpu .
		*from cpu_online_map 		.
		*/				.
Quoted 12 lines
. .
. .
restart_machine(); /* enables irqs */ .
------WINDOW DURING WHICH rcp->cpumask is stale ---------------
. call_rcu();
. /* disables irqs here */
. .force_quiescent_state();
.CPU_DEAD: .for_each_cpu(rcp->cpumask)
. . smp_send_reschedule();
. .
. . WARN_ON() for offlined CPU!
.
Exactly. The call_rcu()s are coming from a different subsystem and can happen anytime during the CPU hotplug path. So, RCU subsystem doesn't have anything to do to keep rcu->cpumask consistent. It is *safe* even if we miss poking a cpu or two while forcing quiescent state in all CPUs. The worst that can happen is a delay in grace period. No correctness problem here.
Quoted 6 lines
Quoted 3 lines
One question. What is preventing a CPU from clearing its mask after we have checked whether it is online but before we have called into smp_send_reschedule?
This is my concern as well. Gautham, at which point in the above timeline is the offlining CPU marked DYING? Before stop_machine(), right?
No :) The offlining CPU is marked DYING after stop_machine(), inside take_cpu_down() which is the work we want to execute after stopping the machine.
it's like
_cpu_down()
|
|-> stop_machine_run();
|   |
|   |-> stop_machine(); /* All CPUs irqs disabled. */
|   |
|   |-> take_cpu_down() --> sets state to CPU_DYING. disables irqs on
|   |				offlined cpu
|   |
|   |-> restart_machine(); /* All CPUs irqs reenabled */
|
|-> send_CPU_DEAD_notification.
The very fact that a thread is running with irqs disabled means that stop_machine_run() thread cannot start executing the work it has been assinged to execute. Because for Machine to be stopped, stop_machine() needs to create n-1 high priority threads on n-1 online cpus, which will disable interrupts and preemption, and stop the machine. Then it will run the task assigned to it on the ith cpu, which in this case is the cpu to be offlined.

Quoted 5 lines

So, it's the design of stop_machine() that's preventing someone from updating the cpu_online_map while force_quiescent_state() is performing the
cpu_is_online() check. Becase we always call force_quiescent_state() with irqs disabled :)

Got it, so the patch looks good.

Thanx, Paul

Quoted 11 lines

Quoted 7 lines
If so, can't we just disable irqs, check for DYING or DEAD, and invoke smp_send_reschedule() only if not DYING or DEAD?
Thanx, Paul
Signature · 2 lines
Thanks and Regards
gautham