Fix regression. Make hot unlplug of CPU0 work again.

Fix regression. Make hot unlplug of CPU0 work again.

[Tony Breeds]

Early in the 2.6.23 cycle we broke the ability to offline cpu0
(7ccb4a662462616f6be5053e26b79580e02f1529).  This patch fixes that by
ensuring that the (xics)  default irq server, will not be 0 when taking
cpu0 offline.

Also catches a use of irq, when virq should be used (I think that the
last one).

Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>

---

Unless there is a problem with this patch, it'd be nice to get it into
2.6.23 :)

 arch/powerpc/platforms/pseries/xics.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/xics.c b/arch/powerpc/platforms/pseries/xics.c
index f0b5ff1..2fb8fd0 100644
--- a/arch/powerpc/platforms/pseries/xics.c
+++ b/arch/powerpc/platforms/pseries/xics.c
@@ -837,6 +837,15 @@ void xics_migrate_irqs_away(void)
 	/* Allow IPIs again... */
 	xics_set_cpu_priority(cpu, DEFAULT_PRIORITY);
 
+	/* It would be bad to migrate any IRQs to the CPU we're taking down */
+	if (default_server == cpu) {
+		unsigned int new_server = first_cpu(cpu_online_map);
+
+		default_server = get_hard_smp_processor_id(new_server);
+		printk(KERN_WARNING "%s: default server was %d, reset to %d\n",
+		       __func__, cpu, default_server);
+	}
+
 	for_each_irq(virq) {
 		struct irq_desc *desc;
 		int xics_status[2];
@@ -882,7 +891,7 @@ void xics_migrate_irqs_away(void)
 
 		/* Reset affinity to all cpus */
 		desc->chip->set_affinity(virq, CPU_MASK_ALL);
-		irq_desc[irq].affinity = CPU_MASK_ALL;
+		irq_desc[virq].affinity = CPU_MASK_ALL;
 unlock:
 		spin_unlock_irqrestore(&desc->lock, flags);
 	}

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

Re: Fix regression. Make hot unlplug of CPU0 work again.

[Tony Breeds]

On Fri, Oct 05, 2007 at 01:52:41PM +1000, Tony Breeds wrote:
> Early in the 2.6.23 cycle we broke the ability to offline cpu0
> (7ccb4a662462616f6be5053e26b79580e02f1529).  This patch fixes that by
> ensuring that the (xics)  default irq server, will not be 0 when taking
> cpu0 offline.
> 
> Also catches a use of irq, when virq should be used (I think that the
> last one).

Hmm testing, this on a JS21 shows that it doesn't work.  I guess I'll go
back to the drawing board.

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

Re: Fix regression. Make hot unlplug of CPU0 work again.

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

On Fri, 2007-10-05 at 17:05 +1000, Tony Breeds wrote:
> On Fri, Oct 05, 2007 at 01:52:41PM +1000, Tony Breeds wrote:
> > Early in the 2.6.23 cycle we broke the ability to offline cpu0
> > (7ccb4a662462616f6be5053e26b79580e02f1529).  This patch fixes that by
> > ensuring that the (xics)  default irq server, will not be 0 when taking
> > cpu0 offline.
> > 
> > Also catches a use of irq, when virq should be used (I think that the
> > last one).
> 
> Hmm testing, this on a JS21 shows that it doesn't work.  I guess I'll go
> back to the drawing board.

Maybe we should revert the original patch and go back to the drawing
board for 2.6.24? Making sure we address the initial problem (which was
exposed by kexec I think) and that we don't break cpu hotplug on the
way.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Patch: Fix regression. Make hot unlplug of CPU0 work again.

[Milton Miller]

On Fri, Oct 05, 2007 at 05:05:21PM, Tony Breeds wrote:
>> On Fri, Oct 05, 2007 at 01:52:41PM +1000, Tony Breeds wrote:
>> Early in the 2.6.23 cycle we broke the ability to offline cpu0
>> (7ccb4a662462616f6be5053e26b79580e02f1529).  This patch fixes that by
>> ensuring that the (xics)  default irq server, will not be 0 when taking
>> cpu0 offline.
>> 
>> Also catches a use of irq, when virq should be used (I think that the
>> last one).
> 
> Hmm testing, this on a JS21 shows that it doesn't work.  I guess I'll go
> back to the drawing board.


Reviewing the first patch, xics_set_affinity no longer looks at the
cpu_mask arg, instead get_irq_server reads it from the irq descriptor.

Signed-off-by: Milton Miller <miltonm@bga.com>
--- 
On top of tonys patch (13926)

I don't have a system to test hotplug, so this is only compile tested.

A more complete fix might be to pass the cpu_mask struct to get_irq_server,
but kernel/irq/manage.c currently sets the descriptor first.

Index: kernel/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- kernel.orig/arch/powerpc/platforms/pseries/xics.c	2007-10-05 11:37:01.000000000 -0500
+++ kernel/arch/powerpc/platforms/pseries/xics.c	2007-10-05 11:37:16.000000000 -0500
@@ -890,8 +890,8 @@ void xics_migrate_irqs_away(void)
 		       virq, cpu);
 
 		/* Reset affinity to all cpus */
-		desc->chip->set_affinity(virq, CPU_MASK_ALL);
 		irq_desc[virq].affinity = CPU_MASK_ALL;
+		desc->chip->set_affinity(virq, CPU_MASK_ALL);
 unlock:
 		spin_unlock_irqrestore(&desc->lock, flags);
 	}

[PATCH v2] Fix regression. Make hot unlplug of CPU0 work again.

[Tony Breeds]

Early in the 2.6.23 cycle we broke the ability to offline cpu0
(7ccb4a662462616f6be5053e26b79580e02f1529).  This patch fixes that by
ensuring that the (xics)  default irq server, will not be 0 when taking
cpu0 offline.

Also catches a use of irq, when virq should be used (I think that's the
last one).

This patch also include the fix from Milton which makes JS21 work
aswell. In the commit message for that patch Milton writes:
	xics_set_affinity no longer looks at the cpu_mask arg, instead
	get_irq_server reads it from the irq descriptor.

Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
Signed-off-by: Milton Miller <miltonm@bga.com>

---
Milton also says in his patch:
> A more complete fix might be to pass the cpu_mask struct to get_irq_server,
> but kernel/irq/manage.c currently sets the descriptor first.

 arch/powerpc/platforms/pseries/xics.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/xics.c b/arch/powerpc/platforms/pseries/xics.c
index f0b5ff1..217ae5d 100644
--- a/arch/powerpc/platforms/pseries/xics.c
+++ b/arch/powerpc/platforms/pseries/xics.c
@@ -837,6 +837,15 @@ void xics_migrate_irqs_away(void)
 	/* Allow IPIs again... */
 	xics_set_cpu_priority(cpu, DEFAULT_PRIORITY);
 
+	/* It would be bad to migrate any IRQs to the CPU we're taking down */
+	if (default_server == cpu) {
+		unsigned int new_server = first_cpu(cpu_online_map);
+
+		default_server = get_hard_smp_processor_id(new_server);
+		printk(KERN_WARNING "%s: default server was %d, reset to %d\n",
+		       __func__, cpu, default_server);
+	}
+
 	for_each_irq(virq) {
 		struct irq_desc *desc;
 		int xics_status[2];
@@ -881,8 +890,8 @@ void xics_migrate_irqs_away(void)
 		       virq, cpu);
 
 		/* Reset affinity to all cpus */
+		irq_desc[virq].affinity = CPU_MASK_ALL;
 		desc->chip->set_affinity(virq, CPU_MASK_ALL);
-		irq_desc[irq].affinity = CPU_MASK_ALL;
 unlock:
 		spin_unlock_irqrestore(&desc->lock, flags);
 	}

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

Re: [PATCH v2] Fix regression. Make hot unlplug of CPU0 work again.

[Michael Neuling]

In message <20071011073040.GD9814@bakeyournoodle.com> you wrote:
> 
> Early in the 2.6.23 cycle we broke the ability to offline cpu0
> (7ccb4a662462616f6be5053e26b79580e02f1529).  This patch fixes that by
> ensuring that the (xics)  default irq server, will not be 0 when taking
> cpu0 offline.
> 
> Also catches a use of irq, when virq should be used (I think that's the
> last one).
> 
> This patch also include the fix from Milton which makes JS21 work
> aswell. In the commit message for that patch Milton writes:
> 	xics_set_affinity no longer looks at the cpu_mask arg, instead
> 	get_irq_server reads it from the irq descriptor.

This doesn't fix the problem for me.  

If I offline CPU0, then online it again, it's fine, but doing the same
for CPU1 kills the machine.

Mikey

> 
> Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
> Signed-off-by: Milton Miller <miltonm@bga.com>
> 
> ---
> Milton also says in his patch:
> > A more complete fix might be to pass the cpu_mask struct to get_irq_server,
> > but kernel/irq/manage.c currently sets the descriptor first.
> 
>  arch/powerpc/platforms/pseries/xics.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/xics.c b/arch/powerpc/platforms/p
series/xics.c
> index f0b5ff1..217ae5d 100644
> --- a/arch/powerpc/platforms/pseries/xics.c
> +++ b/arch/powerpc/platforms/pseries/xics.c
> @@ -837,6 +837,15 @@ void xics_migrate_irqs_away(void)
>  	/* Allow IPIs again... */
>  	xics_set_cpu_priority(cpu, DEFAULT_PRIORITY);
>  
> +	/* It would be bad to migrate any IRQs to the CPU we're taking down */
> +	if (default_server == cpu) {
> +		unsigned int new_server = first_cpu(cpu_online_map);
> +
> +		default_server = get_hard_smp_processor_id(new_server);
> +		printk(KERN_WARNING "%s: default server was %d, reset to %d\n",
> +		       __func__, cpu, default_server);
> +	}
> +
>  	for_each_irq(virq) {
>  		struct irq_desc *desc;
>  		int xics_status[2];
> @@ -881,8 +890,8 @@ void xics_migrate_irqs_away(void)
>  		       virq, cpu);
>  
>  		/* Reset affinity to all cpus */
> +		irq_desc[virq].affinity = CPU_MASK_ALL;
>  		desc->chip->set_affinity(virq, CPU_MASK_ALL);
> -		irq_desc[irq].affinity = CPU_MASK_ALL;
>  unlock:
>  		spin_unlock_irqrestore(&desc->lock, flags);
>  	}
> 
> Yours Tony
> 
>   linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
>   Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
> 

Re: [PATCH v2] Fix regression. Make hot unlplug of CPU0 work again.

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2099 bytes --]

On Thu, 2007-10-11 at 17:30 +1000, Tony Breeds wrote:
> Early in the 2.6.23 cycle we broke the ability to offline cpu0
> (7ccb4a662462616f6be5053e26b79580e02f1529).  This patch fixes that by
> ensuring that the (xics)  default irq server, will not be 0 when taking
> cpu0 offline.
> 
> Also catches a use of irq, when virq should be used (I think that's the
> last one).
> 
> This patch also include the fix from Milton which makes JS21 work
> aswell. In the commit message for that patch Milton writes:
> 	xics_set_affinity no longer looks at the cpu_mask arg, instead
> 	get_irq_server reads it from the irq descriptor.
> 
> Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
> Signed-off-by: Milton Miller <miltonm@bga.com>
> 
> ---
> Milton also says in his patch:
> > A more complete fix might be to pass the cpu_mask struct to get_irq_server,
> > but kernel/irq/manage.c currently sets the descriptor first.
> 
>  arch/powerpc/platforms/pseries/xics.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/xics.c b/arch/powerpc/platforms/pseries/xics.c
> index f0b5ff1..217ae5d 100644
> --- a/arch/powerpc/platforms/pseries/xics.c
> +++ b/arch/powerpc/platforms/pseries/xics.c
> @@ -837,6 +837,15 @@ void xics_migrate_irqs_away(void)
>  	/* Allow IPIs again... */
>  	xics_set_cpu_priority(cpu, DEFAULT_PRIORITY);
>  
> +	/* It would be bad to migrate any IRQs to the CPU we're taking down */
> +	if (default_server == cpu) {
> +		unsigned int new_server = first_cpu(cpu_online_map);
> +
> +		default_server = get_hard_smp_processor_id(new_server);
> +		printk(KERN_WARNING "%s: default server was %d, reset to %d\n",
> +		       __func__, cpu, default_server);

WARNING? It's not like the user can do anything about it.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]