[PATCH] prevent sparc64 from invoking irq handlers on offline CPUs

[PATCH] prevent sparc64 from invoking irq handlers on offline CPUs

[Paul E. McKenney]

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.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---

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

diff --git a/arch/sparc64/kernel/smp.c b/arch/sparc64/kernel/smp.c
index 743ccad..b83a683 100644
--- a/arch/sparc64/kernel/smp.c
+++ b/arch/sparc64/kernel/smp.c
@@ -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;
 }
 

Re: [PATCH] prevent sparc64 from invoking irq handlers on offline CPUs

[David Miller]

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Sun, 31 Aug 2008 10:33:49 -0700

> 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.
> 
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

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.

Re: [PATCH] prevent sparc64 from invoking irq handlers on offline CPUs

[Paul E. McKenney]

On Tue, Sep 02, 2008 at 05:16:30PM -0700, David Miller wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Date: Sun, 31 Aug 2008 10:33:49 -0700
> 
> > 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.
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> I wonder what the 'call_lock' thing protects :-)

I didn't look till now.  ;-)

> 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.

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

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

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?

> 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.

Agreed.

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

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

Re: [PATCH] prevent sparc64 from invoking irq handlers on offline CPUs

[David Miller]

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Tue, 2 Sep 2008 17:42:11 -0700

> On Tue, Sep 02, 2008 at 05:16:30PM -0700, David Miller wrote:
> > So I'd like to hold off on this patch until this locking issue is
> > resolved.
> 
> OK, it is your architecture.  But in the meantime, sparc64 can take
> interrupts on CPUs whose cpu_online_map bits have been cleared.

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?

Re: [PATCH] prevent sparc64 from invoking irq handlers on offline CPUs

[Paul E. McKenney]

On Wed, Sep 03, 2008 at 02:21:38AM -0700, David Miller wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Date: Tue, 2 Sep 2008 17:42:11 -0700
> 
> > On Tue, Sep 02, 2008 at 05:16:30PM -0700, David Miller wrote:
> > > So I'd like to hold off on this patch until this locking issue is
> > > resolved.
> > 
> > OK, it is your architecture.  But in the meantime, sparc64 can take
> > interrupts on CPUs whose cpu_online_map bits have been cleared.
> 
> 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

Re: [PATCH] prevent sparc64 from invoking irq handlers on offline CPUs

[David Miller]

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Wed, 3 Sep 2008 08:42:17 -0700

> 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.

Correct.

> 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).

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.

> 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.)

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...

> 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?

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

> 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.

Exactly.

Re: [PATCH] prevent sparc64 from invoking irq handlers on offline CPUs

[Paul E. McKenney]

On Mon, Sep 08, 2008 at 05:17:08PM -0700, David Miller wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Date: Wed, 3 Sep 2008 08:42:17 -0700
> > 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?
> 
> This case is interesting, and I'm no sure what happens here.

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

Re: [PATCH] prevent sparc64 from invoking irq handlers on offline CPUs

[Manfred Spraul]

David Miller wrote:
> 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...
>
>   
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

Re: [PATCH] prevent sparc64 from invoking irq handlers on offline CPUs

[David Miller]

From: Manfred Spraul <manfred@colorfullife.com>
Date: Tue, 09 Sep 2008 20:49:04 +0200

> David Miller wrote:
> > 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...
> >
> >   
> 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.

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