prevent sparc64 from invoking irq handlers on offline CPUs

Make sparc64 refrain from clearing a given to-be-offlined CPU's bit in the cpu_online_mask until it has processed pending irqs. This change prevents other CPUs from being blindsided by an apparently offline CPU nevertheless changing globally visible state.

---

 smp.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 743ccad..b83a683 100644
@@ -1305,10 +1305,6 @@ int __cpu_disable(void)
 	c->core_id = 0;
 	c->proc_id = -1;
 
-	spin_lock(&call_lock);
-	cpu_clear(cpu, cpu_online_map);
-	spin_unlock(&call_lock);
-
 	smp_wmb();
 
 	/* Make sure no interrupts point to this cpu.  */

@@ -1318,6 +1314,10 @@ int __cpu_disable(void)
 	mdelay(1);
 	local_irq_disable();
 
+	spin_lock(&call_lock);
+	cpu_clear(cpu, cpu_online_map);
+	spin_unlock(&call_lock);
+
 	return 0;
 }
 

From: "Paul E. McKenney" <[email protected]>
Date: Sun, 31 Aug 2008 10:33:49 -0700

I wonder what the 'call_lock' thing protects :-)

That lock is a cobweb from the sparc64 code before I switched it over to use the generic smp_call_function() code in kernel/smp.c

So this lock doesn't protect anything any more.

kernel/smp.c has a call_function_lock, which isn't marked static but isn't declared in any header file.

My instinct is that the intention is that I could use this lock for the synchronization previously provided by sparc64's local "call_lock", and it even seems the author of kernel/smp.c intended this kind of usage.

Anyways, if this code is still using the worthless call_lock, it isn't protecting against anything.

So I'd like to hold off on this patch until this locking issue is resolved.

I didn't look till now. ;-)

It is a static defined in arch/sparc64/kernel/smp.c, and is used only when setting and clearing bits in cpu_online_mask.

It is exported via ipi_call_lock(), ipi_call_unlock(), and friends. A few architectures use it to exclude some of the IPI code while setting (but not clearing) bits in cpu_online_map. These particular architectures have a phase during CPU offlining where they drain pending interrupts, so perhaps that is why they only worry about onlining?

Agreed.

OK, it is your architecture. But in the meantime, sparc64 can take interrupts on CPUs whose cpu_online_map bits have been cleared.

Thanx, Paul

From: "Paul E. McKenney" <[email protected]>
Date: Tue, 2 Sep 2008 17:42:11 -0700

Paul, here is how I resolved this in my tree.

First, I applied a patch that killed that 'call_lock' and replaced the accesses with ipi_call_lock() and ipi_call_unlock().

Then I sed'd up your patch so that it applies properly after that change.

I still think there will be a problem here on sparc64. I had the online map clearing there happening first because the fixup_irqs() thing doesn't drain interrupts. It just makes sure that "device" interrupts no longer point at the cpu. So all new device interrupts after fixup_irqs() will not go to the cpu.

Then we do the:

	local_irq_enable();
	mdelay(1);
	local_irq_disable();

thing to process any interrupts which were sent while we were retargetting the device IRQs.

I also intended this to drain the cross-call interrupts too, that's why I cleared the cpu_online_map() bit before fixup_irqs() and the above "enable/disable" sequence runs.

With your change in there now, IPIs won't get drained and the system might get stuck as a result.

I wonder if it would work if we cleared the cpu_online_map right before the "enable/disable" sequence, but after fixup_irqs()?

Paul, what do you think?

Given that __cpu_disable runs in the context of __stop_machine(), the only new IPIs will be from irqs being drained out of the current CPU, correct?

Here are the situations I can think of (no doubt betraying my ignorance of modern processor irq hardware in general and of sparc64 in particular):

o Pending device irq. There should be a limited number of these,
and the fixup_irqs() call prevents any more from appearing.

o Pending scheduling-clock interrupts. Does fixup_irqs() turn
these off as well? (It does if the scheduling-clock interrupt is one of the 0..NR_IRQS irqs.) On the other hand, leaving one of these pending should not be a problem (famous last words).

o Pending IPIs. There should again be a limited number of these.
Except that an IPI handler could possibly IPI this CPU, as could a device irq handler, I suppose. (We cannot receive additional IPIs from other CPUs, as they are spinning with irqs disabled.)

o Timer irqs. Not sure what happens to add_timer() calls from
a CPU that is going offline. The hope would be that they get queued to some other CPU?

Now, an IPI handler cannot be allowed to send a synchronous IPI to anyone, because the other CPUs are spinning with irqs disabled until __cpu_disable() returns. And in any context, a handler for a synchronous IPI cannot be allowed to send a synchronous IPI to any set of CPUs that might include the sender of the currently running handler, as doing so would result in deadlock.

Therefore, one approach would be for the IPI subsystem to use a cpu_ipi_map rather than the current cpu_online_map. The cpu_ipi_map could be cleared before re-enabling irqs, and then the cpu_online_map could be cleared after the final disabling of irqs. Yes, this would mean that sending IPIs to an online CPU would be disallowed when that CPU is going offline, but then again you have to be careful when sending IPIs anyway, especially synchronous IPIs where you spin waiting for a response.

Another approach would be to assume that any chain of IPI-myself handlers is finite, and to assume that base-level execution will be blocked by the resulting irqs until the chain terminates. Given this finite-IPI-myself-chain assumption, you by definition would have no IPI irqs pending when the mdelay(1) returned.

Yes, I can imagine irq hardware that would have many nanoseconds of delay built in, which would allow the loop to exit with IPIs still pending. One way of resolving this would be to have a per-CPU IPI-received counter, and to spin as follows:

	local_irq_enable();
	for (;;) {
		tmp = __get_cpu_var(ipi_received_counter);
		mdelay(1);
		if (tmp = __get_cpu_var(ipi_received_counter)
			break;
	}
	local_irq_disable();
	cpu_clear(cpu, cpu_online_map);

This assumes that the hardware IPI-pending latency is no longer than however long mdelay(1) spins.

Seem reasonable, or are these cures worse than the disease?

Thanx, Paul

From: "Paul E. McKenney" <[email protected]>
Date: Wed, 3 Sep 2008 08:42:17 -0700

Correct.

No, the timer interrupts are controlled differently, as the IRQ source lives inside of the CPU rather in some external entity.

I need to fix that by invoking tick_ops->disable_irq() here. I'll take care of this.

And IPI handler runs in HW irq context, therefore such an IPI-creates-an-IPI should not be allowed, at least not directly.

Actually the restriction seems to be that an IPI cannot be sent when "irqs_disabled()", hmmm...

This case is interesting, and I'm no sure what happens here.

Exactly.

It turns out that there is a timer_cpu_modifier() that invokes migrate_timers() upon CPU_DEAD or CPU_DEAD_FROZEN. And migrate_timers() looks like it does what its name says. And I believe that CPU_DEAD happens after sparc64's local_irq_enable() window, so we should be OK.

Thanx, Paul

Does this apply to smp_send_reschedule() as well?
A [rare] codepath in the current rcu code does to trigger quiescent states on remote cpus.

--
Manfred

From: Manfred Spraul <[email protected]>
Date: Tue, 09 Sep 2008 20:49:04 +0200

It does work on sparc64. Such an IPI doesn't do a wait so there would be no deadlock issues either.