Fix CPU hotplug IRQ migration

Now that the GIC takes care of selecting a target interrupt from the affinity mask, we don't need all this complexity in the core code anymore. Just detect when we need to break affinity.

Signed-off-by Russell King<[email protected]>

---
  arch/arm/kernel/irq.c |   10 ++++------
  1 files changed, 4 insertions(+), 6 deletions(-)

arch/arm/kernel/irq.c +0 -0

index d7aa5c9..ab63c05 100644
@@ -133,17 +133,15 @@ int __init arch_probe_nr_irqs(void)
 

Review

Quoted 2 lines · Russell King - ARM Linux

(quoted diff) +2 -0

+	if (cpumask_any_and(affinity, cpu_online_mask)>= nr_cpu_ids) {
+		affinity cpu_online_mask;

I noticed, you missed '=' above and same is corrected in PATCH 4/4. Should be fixed in this patch itself to avoid git-bisect breakage.

This series fixes a bunch of issues with IRQ migration:

1. Preventing affinity changes causing IRQs to be directed to off-line CPUs.
2. Preventing oopses with non-GIC interrupt controllers.
3. Preventing per-CPU IRQs being candidates for migration.
4. Removing the abuse of irqdesc's node member.

This prevents crashes on OMAP4430 SDP when non-default IRQ affinity settings are used and a CPU which owns some IRQs is offlined.

With this patch set applied, there is no reason core code can't handle CPU0 being offlined - on OMAP4, the only remaining issue is CPU0 being woken from WFI and falsely passing the non-spurious wake-up test.

+ Colin,

Quoted 10 lines · Russell King - ARM Linux

This series fixes a bunch of issues with IRQ migration:

1. Preventing affinity changes causing IRQs to be directed to off-line CPUs.
2. Preventing oopses with non-GIC interrupt controllers.
3. Preventing per-CPU IRQs being candidates for migration.
4. Removing the abuse of irqdesc's node member.

This prevents crashes on OMAP4430 SDP when non-default IRQ affinity settings are used and a CPU which owns some IRQs is offlined.

Firstly thanks for fixing the IRQ migration and affinity issues with hotplug code. Colin found the hotplug irq affinity issue last week.

Colin quoted ..
----------------------------------
The irq affinity mask is not kept up to date by the ARM irq code. In particular, migrate_one_irq is not called when the irq node does not match the cpu going offline, and when it is called, it doesn't update the irq affinity mask before calling the irq chip's set_affinity function. This causes the use of the affinity mask in the mask and unmask functions to be unreliable, possibly unmasking an interrupt on a cpu that is offline.
----------------------------------

Need to check if this series addresses above issue as well.

Quoted 3 lines

With this patch set applied, there is no reason core code can't handle CPU0 being offlined - on OMAP4, the only remaining issue is CPU0 being woken from WFI and falsely passing the non-spurious wake-up test.

There is one as commented in other thread. The Secure code runs only on CPU0 so if you offline the CPU0, all the secure services are not available.

On Fri, Jul 22, 2011 at 11:05:01AM +0530, Santosh Shilimkar wrote:

Secure code is called on CPU1 too, so I don't think your statement is accurate. Eg:

	omap_modify_auxcoreboot0(0x0, 0x200)

	omap_read_auxcoreboot0()

both result in calling secure code on CPU1.

Quoted 17 lines · Santosh Shilimkar

+ Colin,

Quoted 10 lines · Russell King - ARM Linux

This series fixes a bunch of issues with IRQ migration:

1. Preventing affinity changes causing IRQs to be directed to off-line CPUs.
2. Preventing oopses with non-GIC interrupt controllers.
3. Preventing per-CPU IRQs being candidates for migration.
4. Removing the abuse of irqdesc's node member.

This prevents crashes on OMAP4430 SDP when non-default IRQ affinity settings are used and a CPU which owns some IRQs is offlined.

Firstly thanks for fixing the IRQ migration and affinity issues with hotplug code. Colin found the hotplug irq affinity issue last week.

Quoted 11 lines

Colin quoted ..
----------------------------------
The irq affinity mask is not kept up to date by the ARM irq code. ?In particular, migrate_one_irq is not called when the irq node does not match the cpu going offline, and when it is called, it doesn't update the irq affinity mask before calling the irq chip's set_affinity function. This causes the use of the affinity mask in the mask and unmask functions to be unreliable, possibly unmasking an interrupt on a cpu that is offline.
----------------------------------

I don't have a copy of that message... where was it posted?

Quoted 1 line

Need to check if this series addresses above issue as well.

It does not, and it is wrong to change the affinity mask of the interrupt due to a CPU going offline. ?Think about it.

If you offline CPU0, which migrates all IRQs to CPU1. ?You then update the affinity mask to exclude CPU0. ?Now you online CPU0, and offline CPU1. You now get a pile of kernel messages about breaking the _apparant_ users affinity settings for the interrupts, because you have to move them off CPU1 which is the only CPU they are now affine to.

The affinity mask is a policy setting from userspace. ?It is not an absolute setting which the kernel must keep updated.

And actually you _can't_ tell what CPU the interrupt is routed to from the affinity mask - the affinity mask gives a hint as to which CPU_s_ (plural) are permitted to receive the interrupt. ?The fact we choose - at the moment - the first CPU to route stuff to from the masks is merely an implementation detail which must not be relied upon.

So, Colin is wrong on the affinity mask issue. ?The correct behaviour is that the affinity at any point is the set of CPUs in the current affinity mask _and_ the CPU online mask. ?If that is an empty set, then any online CPU will be chosen to handle the interrupt.

Quoted 6 lines

Quoted 3 lines

With this patch set applied, there is no reason core code can't handle CPU0 being offlined - on OMAP4, the only remaining issue is CPU0 being woken from WFI and falsely passing the non-spurious wake-up test.

There is one as commented in other thread. The Secure code runs only on CPU0 so if you offline the CPU0, all the secure services are not available.

Secure code is called on CPU1 too, so I don't think your statement is accurate. ?Eg:

? ? ? ?omap_modify_auxcoreboot0(0x0, 0x200)

? ? ? ?omap_read_auxcoreboot0()

both result in calling secure code on CPU1.

+ Colin,

Quoted 8 lines · Russell King - ARM Linux

This series fixes a bunch of issues with IRQ migration:

1. Preventing affinity changes causing IRQs to be directed to off-line CPUs.
2. Preventing oopses with non-GIC interrupt controllers.
3. Preventing per-CPU IRQs being candidates for migration.
4. Removing the abuse of irqdesc's node member.

Quoted 3 lines

This prevents crashes on OMAP4430 SDP when non-default IRQ affinity settings are used and a CPU which owns some IRQs is offlined.

Firstly thanks for fixing the IRQ migration and affinity issues with hotplug code. Colin found the hotplug irq affinity issue last week.

After CPU1 offline, the IRQ is handled by CPU0 even though masks say's it's CPU0. Mask should have

been corrected by hotplug code but as per your comments this is just userspace settings and shouldn't dictate which CPU handles the IRQ.

The interesting part is once you online CPU now,
IRQ44 continues to be on CPU0.

You think above behavior is fine? My i686 UBUNTU box, I don't see above behaviour. The hotplug code updates the IRQ mask to available CPU.

Secondly, GIC smp_set_affinity is kind of set_target_cpu function.

How can we relay the target CPU information to gic_arch_extn, so that they can update their settings as per GIC.

Quoted 2 lines · Santosh Shilimkar

After CPU1 offline, the IRQ is handled by CPU0 even though masks say's it's CPU0. Mask should have

ITYM CPU1 here.

Quoted 6 lines

been corrected by hotplug code but as per your comments this is just userspace settings and shouldn't dictate which CPU handles the IRQ.

The interesting part is once you online CPU now,
IRQ44 continues to be on CPU0.

Yes, we don't migrate IRQs back onto their affine CPUs when they come back online - I don't think anyone does.

Quoted 3 lines

You think above behavior is fine? My i686 UBUNTU box, I don't see above behaviour. The hotplug code updates the IRQ mask to available CPU.

Yes, I noticed that, and the code is really weird - and I think buggy - there for two reasons:

1. If you bind an interrupt to CPUs 1, and you take CPU 1 offline, the result is that the interrupt now is bound to _all_ CPUs even when CPU 1 comes back online.

2. irq_set_affinity() returns a code to indicate whether the affinity mask is to be updated (see __irq_set_affinity_locked - IRQ_SET_MASK_OK or IRQ_SET_MASK_OK_NOCOPY which defines whether irq_data->affinity is updated.

To have the individual irq_set_affinity() directly set irq_data->affinity like x86 does seems to go against the intentions of __irq_set_affinity_locked().

Quoted 1 line

Secondly, GIC smp_set_affinity is kind of set_target_cpu function.

No, the gic_set_affinity() has always been a "set this interrupt to be routed to one of these IRQs", except when our IRQ migration code used to call it with a specific CPU. That's an exceptional (and incorrect) case though.

Why? The interface is that the affinity mask contains a set of CPUs, and that's how generic code will call it when you write to the /proc/irq/*/smp_affinity files. If you have a 4-CPU system, and you echo '3' into one of those files, gic_set_affinity() will be called with a mask containing CPU0,1 and not 2,3.

The fact that gic_set_affinity() has always chosen the first CPU in the mask is an implementation detail, one which should stay private to gic_set_affinity().

Quoted 2 lines

How can we relay the target CPU information to gic_arch_extn, so that they can update their settings as per GIC.

That was my point to Colin - the set_affinity() interface, nor irq_data->affinity really isn't suitable for doing that.

One way we _could_ do it is have the GIC code recompose a CPU mask containing exactly one CPU - but if we ever end up with NR_CPUS> 32 the CPU mask idea may become unmanagable - it may be better to pass the exact CPU number down. It depends how its going to be used, and so far I can see no examples of that.

Will make you OMAP4 IRQ code available for this particular aspect and may be we can take it from there. There are two main problems I am seeing currently.

1)Which CPU gic_arch_extn_[mask/unmask] operate on? Can we assume that whichever CPU the call being executed should be the target CPU ? This might not be safe assumption though. For the extn, there is no other way to know the target CPU for mask/unmask of an IRQ

Quoted 9 lines · Santosh Shilimkar

Will make you OMAP4 IRQ code available for this particular aspect and may be we can take it from there. There are two main problems I am seeing currently.

1)Which CPU gic_arch_extn_[mask/unmask] operate on? Can we assume that whichever CPU the call being executed should be the target CPU ? This might not be safe assumption though. For the extn, there is no other way to know the target CPU for mask/unmask of an IRQ

This goes back to the mistake that was made with the initial set of GIC PPI patches.

You can't assume anything about which CPU the mask/unmask functions end up running on. Between code calling request_irq() and the relevant unmask function being called, we can end up scheduling, either because of a mutex being waited for or because of a preemption event. That means we can end up on a different CPU to that which we started out on.

So, the only thing you can do is remember the CPU number which it was last associated with - a simple 'unsigned int' should do in the extensions data structures, one per IRQ.

However, my point is that doing this:

	gic_arch_extn.irq_set_affinity(d, cpumask_of(cpu), force);

and then:

	cpu = cpumask_first(affinity);

in the extensions code is a very long winded way of communicating a single CPU number down into the IRQ extension code.

This series fixes a bunch of issues with IRQ migration:

1. Preventing affinity changes causing IRQs to be directed to off-line CPUs.
2. Preventing oopses with non-GIC interrupt controllers.
3. Preventing per-CPU IRQs being candidates for migration.
4. Removing the abuse of irqdesc's node member.

This prevents crashes on OMAP4430 SDP when non-default IRQ affinity settings are used and a CPU which owns some IRQs is offlined.

With this patch set applied, there is no reason core code can't handle CPU0 being offlined - on OMAP4, the only remaining issue is CPU0 being woken from WFI and falsely passing the non-spurious wake-up test.