[PATCH] Fix interrupt distribution in ppc970

[PATCH] Fix interrupt distribution in ppc970

[Mohan Kumar M]

Hello,

We have encountered a strange problem with kdump in ppc970 based
machines. When a kdump kernel is booted with maxcpus=1 parameter,
interrupt is distributed to non-existent cpus also. It created lot of
interrupt missing problems.

This problem can be resolved by passing the additional kernel parameter
"noirqdistrib", which uses boot cpu id as interrupt distribution server.

To overcome this problem, I have created the patch, which
checks for the condition if the machine is ppc970 based and maxcpus
kernel parameter is specified. If the condition is met, the default
distribution server is assigned to be current boot cpu instead of
assigning from the gserver#s property.

Tested on PPC970 based box and POWER5 based box.

Any comment, feedback?

Patch is generated over 2.6.19-git7.


o Kdump with maxcpus kernel parameter in PPC970xx creates interrupt
  distribution problem, so set default distribution server to the
  current cpu (i.e. boot cpu)

Signed-off-by: Mohan Kumar M <mohan@in.ibm.com>

---
 arch/powerpc/platforms/pseries/xics.c |   35 +++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

Index: linux-2.6.19-git7/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- linux-2.6.19-git7.orig/arch/powerpc/platforms/pseries/xics.c
+++ linux-2.6.19-git7/arch/powerpc/platforms/pseries/xics.c
@@ -680,6 +680,27 @@ static struct device_node *cpuid_to_of_n
 	return NULL;
 }
 
+static int is_processor_970(void)
+{
+	unsigned long rval;
+	int rc;
+	rval = PVR_VER(mfspr(SPRN_PVR));
+
+	switch(rval) {
+		case PV_970:
+		case PV_970FX:
+		case PV_970MP:
+		case PV_970GX:
+			rc = 1;
+			break;
+		default:
+			rc = 0;
+			break;
+	}
+
+	return rc;
+}
+
 void __init xics_init_IRQ(void)
 {
 	int i, j;
@@ -688,6 +709,7 @@ void __init xics_init_IRQ(void)
 	const u32 *ireg, *isize;
 	int found = 0;
 	u32 hcpuid;
+	int fix_970_intr_distrib = 0;
 
 	ppc64_boot_msg(0x20, "XICS Init");
 
@@ -707,6 +729,13 @@ void __init xics_init_IRQ(void)
 
 	xics_init_host();
 
+	/* Kdump with maxcpus kernel parameter in PPC970xx creates interrupt
+	 * distribution problem, so set default distribution server to the
+	 * current cpu (i.e. boot cpu)
+	 */
+	if (strstr(saved_command_line, "maxcpus=") && is_processor_970())
+		fix_970_intr_distrib = 1;
+
 	/* Find the server numbers for the boot cpu. */
 	np = cpuid_to_of_node(boot_cpuid);
 	BUG_ON(!np);
@@ -724,7 +753,11 @@ void __init xics_init_IRQ(void)
 	for (j = 0; j < i; j += 2) {
 		if (ireg[j] == hcpuid) {
 			default_server = hcpuid;
-			default_distrib_server = ireg[j+1];
+
+			if (fix_970_intr_distrib)
+				default_distrib_server = hcpuid;
+			else
+				default_distrib_server = ireg[j+1];
 
 			isize = get_property(np,
 					"ibm,interrupt-server#-size", NULL);

Re: [PATCH] Fix interrupt distribution in ppc970

[Paul Mackerras]

Mohan Kumar M writes:

> To overcome this problem, I have created the patch, which
> checks for the condition if the machine is ppc970 based and maxcpus
> kernel parameter is specified. If the condition is met, the default
> distribution server is assigned to be current boot cpu instead of
> assigning from the gserver#s property.

It feels hacky to be basing this sort of thing on the PVR.  I would
like to understand the real problem better.  Is it that we are
sometimes setting default_distrib_server to point to a CPU which is
not running?  If so, why and how?  Is this something that is specific
to the firmware on IBM 970-based blade systems?  Is there in fact a
firmware bug that this works around?  Why don't we see it on non-970
based systems?

Paul.

Re: [PATCH] Fix interrupt distribution in ppc970

[Mohan Kumar M]

On Mon, Dec 18, 2006 at 03:37:36PM +1100, Paul Mackerras wrote:
> Mohan Kumar M writes:
> 
> > To overcome this problem, I have created the patch, which
> > checks for the condition if the machine is ppc970 based and maxcpus
> > kernel parameter is specified. If the condition is met, the default
> > distribution server is assigned to be current boot cpu instead of
> > assigning from the gserver#s property.
> 
> It feels hacky to be basing this sort of thing on the PVR.  I would
> like to understand the real problem better.  Is it that we are
> sometimes setting default_distrib_server to point to a CPU which is
> not running?  If so, why and how?  Is this something that is specific
> to the firmware on IBM 970-based blade systems?  Is there in fact a
> firmware bug that this works around?  Why don't we see it on non-970
> based systems?
>

Yes, It may be related to firmware issue also. I am trying to upgrade my
JS20 box with the latest firmware. I will test this case after a
successful firmware upgrade and update you with the results.

Regards,
Mohan.

> Paul.

Re: [PATCH] Fix interrupt distribution in ppc970

[Mohan Kumar M]

On Mon, Dec 18, 2006 at 03:37:36PM +1100, Paul Mackerras wrote:
> Mohan Kumar M writes:
> 
> > To overcome this problem, I have created the patch, which
> > checks for the condition if the machine is ppc970 based and maxcpus
> > kernel parameter is specified. If the condition is met, the default
> > distribution server is assigned to be current boot cpu instead of
> > assigning from the gserver#s property.
> 
> It feels hacky to be basing this sort of thing on the PVR.  I would
> like to understand the real problem better.  Is it that we are
> sometimes setting default_distrib_server to point to a CPU which is
> not running?  If so, why and how?

At the time of kexec/kdump shutdown sequence all secondary processors
are removed from the Global Interrupt Queue by calling the corresponding
rtas-set-indicator call. So even though default_distrib_server is 0xff
PIC should distribute the interrupts only to the processors available in
GIQ (please correct me if I am wrong), but its not happening in our
JS20 box. Our JS20 firmware version is FW04310120 dated 07/26/04.

> Is this something that is specific
> to the firmware on IBM 970-based blade systems?  Is there in fact a
> firmware bug that this works around?

Looks like a firmware bug, since I am not able to reproduce this problem
on a JS21 box with the firmware version MB240_470 dated 03/23/2006. I am
planning to upgrade JS20 with latest firmware and test with maxcpus=1

> Why don't we see it on non-970
> based systems?

I have seen secondary thread related interrupt distribution problems on
POWER5 boxes and I fixed it sometime back. Since POWER4 does not have
SMT there is no interrupt distribution problem.

> 
> Paul.

Re: [Fastboot] [PATCH] Fix interrupt distribution in ppc970

[Mohan Kumar M]

On Mon, Dec 18, 2006 at 04:27:06PM +0530, Mohan Kumar M wrote:
> On Mon, Dec 18, 2006 at 03:37:36PM +1100, Paul Mackerras wrote:
> > 
> > It feels hacky to be basing this sort of thing on the PVR.  I would
> > like to understand the real problem better.  Is it that we are
> > sometimes setting default_distrib_server to point to a CPU which is
> > not running?  If so, why and how?
> 
> At the time of kexec/kdump shutdown sequence all secondary processors
> are removed from the Global Interrupt Queue by calling the corresponding
> rtas-set-indicator call. So even though default_distrib_server is 0xff
> PIC should distribute the interrupts only to the processors available in
> GIQ (please correct me if I am wrong), but its not happening in our
> JS20 box. Our JS20 firmware version is FW04310120 dated 07/26/04.
>
> > Is this something that is specific
> > to the firmware on IBM 970-based blade systems?  Is there in fact a
> > firmware bug that this works around?
> 
> Looks like a firmware bug, since I am not able to reproduce this problem
> on a JS21 box with the firmware version MB240_470 dated 03/23/2006. I am
> planning to upgrade JS20 with latest firmware and test with maxcpus=1
>

Even after updating the JS20 box with the latest firmware FW06470160
dated 11/21/06, we are facing the same problems with "maxcpus=1" kernel
parameter. As mentioned earlier, we do not have this problem on JS21
hardware. JS20 has PPC970 cpus while JS21 has PPC970MP cpus. Could it be
a reason? Still we are not able to conclude whether it is related to
firmware or not.

Paul, can you tell us which approach we can follow to solve this
problem? The patch I have sent earlier was hackish. So do we need to use
"noirqdistrib" kernel parameter in addition to "maxcpus=1" to avoid
these problems?

Mohan.

 
> > Why don't we see it on non-970
> > based systems?
> 
> I have seen secondary thread related interrupt distribution problems on
> POWER5 boxes and I fixed it sometime back. Since POWER4 does not have
> SMT there is no interrupt distribution problem.
> 
> > 
> > Paul.

Re: [Fastboot] [PATCH] Fix interrupt distribution in ppc970

[Doug Maxey]

On Tue, 02 Jan 2007 17:12:12 +0530, Mohan Kumar M wrote:
> Even after updating the JS20 box with the latest firmware FW06470160
> dated 11/21/06, we are facing the same problems with "maxcpus=1" kernel
> parameter.

The codebases for the firmware diverged between JS20 and JS21 back in '04.
So it is more than just possible there is a firmware feature in play.

Re: [PATCH] Fix interrupt distribution in ppc970

[Mohan Kumar M]

Hi,

Here comes the revised version of patch to fix the interrupt missing
problem when a kdump kernel is booted with "maxcpus=1" kernel parameter.

In the xics initialization code a check is made to detemine whether
maxcpus kernel parameter is present and if its present then
default_distrib_server variable is initialized to the current boot cpu
id (by default_server variable). So that when ever a kernel is booted
with maxcpus kernel parameter all interrupts are routed to the boot cpu
only.

Tested on POWER5 and JS20 systems.

Any comment?

Signed-off-by: Mohan Kumar M <mohan@in.ibm.com>

---
 arch/powerpc/platforms/pseries/xics.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: linux-2.6.20/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- linux-2.6.20.orig/arch/powerpc/platforms/pseries/xics.c
+++ linux-2.6.20/arch/powerpc/platforms/pseries/xics.c
@@ -734,6 +734,13 @@ void __init xics_init_IRQ(void)
 skip_gserver_check:
 	of_node_put(np);
 
+	/* Kdump with maxcpus parameter in PPC970xx creates interrupt
+	 * distribution problems. So assign current boot cpu id to
+	 * interrupt distribution server
+	 */
+	if (strstr(saved_command_line, "maxcpus="))
+		default_distrib_server = default_server;
+
 	if (firmware_has_feature(FW_FEATURE_LPAR))
 		ppc_md.get_irq = xics_get_irq_lpar;
 	else

Re: [PATCH] Fix interrupt distribution in ppc970

[Michael Ellerman]

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

On Tue, 2007-03-06 at 19:27 +0530, Mohan Kumar M wrote:
> Hi,
> 
> Here comes the revised version of patch to fix the interrupt missing
> problem when a kdump kernel is booted with "maxcpus=1" kernel parameter.
> 
> In the xics initialization code a check is made to detemine whether
> maxcpus kernel parameter is present and if its present then
> default_distrib_server variable is initialized to the current boot cpu
> id (by default_server variable). So that when ever a kernel is booted
> with maxcpus kernel parameter all interrupts are routed to the boot cpu
> only.
> 
> Tested on POWER5 and JS20 systems.

First, I don't know why we keep telling people to use maxcpus=1 for
kexec/kdump - it's causing bugs, and I don't know of any that it fixes?

Second, the way you've written this is not so good. The xics code should
not be checking that "maxcpus" exists on the command line, it should be
checking that the distrib server points to a cpu that is online - using
cpu_online() etc.

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 interrupt distribution in ppc970

[Mohan Kumar M]

On Tue, Mar 06, 2007 at 03:16:55PM +0100, Michael Ellerman wrote:
> On Tue, 2007-03-06 at 19:27 +0530, Mohan Kumar M wrote:
> > Hi,
> > 
> > Here comes the revised version of patch to fix the interrupt missing
> > problem when a kdump kernel is booted with "maxcpus=1" kernel parameter.
> > 
> > In the xics initialization code a check is made to detemine whether
> > maxcpus kernel parameter is present and if its present then
> > default_distrib_server variable is initialized to the current boot cpu
> > id (by default_server variable). So that when ever a kernel is booted
> > with maxcpus kernel parameter all interrupts are routed to the boot cpu
> > only.
> > 
> > Tested on POWER5 and JS20 systems.
> 
> First, I don't know why we keep telling people to use maxcpus=1 for
> kexec/kdump - it's causing bugs, and I don't know of any that it fixes?

Some distros want to use maxcpus=1 for kdump.

> 
> Second, the way you've written this is not so good. The xics code should
> not be checking that "maxcpus" exists on the command line, it should be
> checking that the distrib server points to a cpu that is online - using
> cpu_online() etc.
>

In get_irq_server function in xics.c, "noirqdistrib" command line
parameter is indirectly checked for routing the interrupts either to a
specific cpu or to all cpus. So I think checking for maxcpus= command
line parameter in xics.c is not a problem.

Also during the xics_init_IRQ function other cpus (secondary cpus) will
not be online, they are made online at a later stage. So using
cpu_online() function at xics_init_IRQ will return true only for boot
cpu id.
 
> 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

Re: [PATCH] Fix interrupt distribution in ppc970

[Michael Ellerman]

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

On Tue, 2007-03-06 at 22:25 +0530, Mohan Kumar M wrote:
> On Tue, Mar 06, 2007 at 03:16:55PM +0100, Michael Ellerman wrote:
> > On Tue, 2007-03-06 at 19:27 +0530, Mohan Kumar M wrote:
> > > Hi,
> > > 
> > > Here comes the revised version of patch to fix the interrupt missing
> > > problem when a kdump kernel is booted with "maxcpus=1" kernel parameter.
> > > 
> > > In the xics initialization code a check is made to detemine whether
> > > maxcpus kernel parameter is present and if its present then
> > > default_distrib_server variable is initialized to the current boot cpu
> > > id (by default_server variable). So that when ever a kernel is booted
> > > with maxcpus kernel parameter all interrupts are routed to the boot cpu
> > > only.
> > > 
> > > Tested on POWER5 and JS20 systems.
> > 
> > First, I don't know why we keep telling people to use maxcpus=1 for
> > kexec/kdump - it's causing bugs, and I don't know of any that it fixes?
> 
> Some distros want to use maxcpus=1 for kdump.

Yeah OK.

> > Second, the way you've written this is not so good. The xics code should
> > not be checking that "maxcpus" exists on the command line, it should be
> > checking that the distrib server points to a cpu that is online - using
> > cpu_online() etc.
> >
> 
> In get_irq_server function in xics.c, "noirqdistrib" command line
> parameter is indirectly checked for routing the interrupts either to a
> specific cpu or to all cpus. So I think checking for maxcpus= command
> line parameter in xics.c is not a problem.

No, it checks a flag, it doesn't run strstr on the saved command line.

> Also during the xics_init_IRQ function other cpus (secondary cpus) will
> not be online, they are made online at a later stage. So using
> cpu_online() function at xics_init_IRQ will return true only for boot
> cpu id.

OK, so you should be able to use cpus possible map or something similar.

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 interrupt distribution in ppc970

[Mohan Kumar M]

On Tue, Mar 06, 2007 at 06:37:14PM +0100, Michael Ellerman wrote:
> On Tue, 2007-03-06 at 22:25 +0530, Mohan Kumar M wrote:
> > On Tue, Mar 06, 2007 at 03:16:55PM +0100, Michael Ellerman wrote:
> > > First, I don't know why we keep telling people to use maxcpus=1 for
> > > kexec/kdump - it's causing bugs, and I don't know of any that it fixes?
> > 
> > Some distros want to use maxcpus=1 for kdump.
> 
> Yeah OK.
> 
> > > Second, the way you've written this is not so good. The xics code should
> > > not be checking that "maxcpus" exists on the command line, it should be
> > > checking that the distrib server points to a cpu that is online - using
> > > cpu_online() etc.
> > >
> > 
> > In get_irq_server function in xics.c, "noirqdistrib" command line
> > parameter is indirectly checked for routing the interrupts either to a
> > specific cpu or to all cpus. So I think checking for maxcpus= command
> > line parameter in xics.c is not a problem.
> 
> No, it checks a flag, it doesn't run strstr on the saved command line.
>
What if I use a flag (or existing global variable if any) to check for
the presence of maxcpus kernel paramter?
 
> > Also during the xics_init_IRQ function other cpus (secondary cpus) will
> > not be online, they are made online at a later stage. So using
> > cpu_online() function at xics_init_IRQ will return true only for boot
> > cpu id.
> 
> OK, so you should be able to use cpus possible map or something similar.
>
Hmm, I think cpus possible map will have an entry for the offline cpu
also.
 
> 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

Re: [PATCH] Fix interrupt distribution in ppc970

[Michael Ellerman]

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

On Wed, 2007-03-07 at 10:23 +0530, Mohan Kumar M wrote:
> On Tue, Mar 06, 2007 at 06:37:14PM +0100, Michael Ellerman wrote:
> > On Tue, 2007-03-06 at 22:25 +0530, Mohan Kumar M wrote:
> > > On Tue, Mar 06, 2007 at 03:16:55PM +0100, Michael Ellerman wrote:
> > > > First, I don't know why we keep telling people to use maxcpus=1 for
> > > > kexec/kdump - it's causing bugs, and I don't know of any that it fixes?
> > > 
> > > Some distros want to use maxcpus=1 for kdump.
> > 
> > Yeah OK.
> > 
> > > > Second, the way you've written this is not so good. The xics code should
> > > > not be checking that "maxcpus" exists on the command line, it should be
> > > > checking that the distrib server points to a cpu that is online - using
> > > > cpu_online() etc.
> > > >
> > > 
> > > In get_irq_server function in xics.c, "noirqdistrib" command line
> > > parameter is indirectly checked for routing the interrupts either to a
> > > specific cpu or to all cpus. So I think checking for maxcpus= command
> > > line parameter in xics.c is not a problem.
> > 
> > No, it checks a flag, it doesn't run strstr on the saved command line.
> >
> What if I use a flag (or existing global variable if any) to check for
> the presence of maxcpus kernel paramter?

There's already maxcpus in init/main.c, that would probably be better,
though still ugly.

> > > Also during the xics_init_IRQ function other cpus (secondary cpus) will
> > > not be online, they are made online at a later stage. So using
> > > cpu_online() function at xics_init_IRQ will return true only for boot
> > > cpu id.
> > 
> > OK, so you should be able to use cpus possible map or something similar.
> >
> Hmm, I think cpus possible map will have an entry for the offline cpu
> also.

You're right, it will, did I mention that maxcpus= is broken :)

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 interrupt distribution in ppc970

[Mohan Kumar M]

On Wed, Mar 07, 2007 at 11:52:32AM +0100, Michael Ellerman wrote:
> There's already maxcpus in init/main.c, that would probably be better,
> though still ugly.
>
Based on Mike's suggestions, I modified the patch. The attached patch
refers max_cpus variable to check whether the kernel is booted with
maxcpus=1 parameter and if maxcpus=1 is specified the patch assigns only
the current boot cpu to be the default distribution server.

Patch is generated over 2.6.20 kernel, cleanly applies to 2.6.21-rc5
kernel.

Any suggestion, comment?


Signed-off-by: Mohan Kumar M <mohan@in.ibm.com>
---
 arch/powerpc/platforms/pseries/xics.c |    9 +++++++++
 init/main.c                           |    2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

Index: linux-2.6.20/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- linux-2.6.20.orig/arch/powerpc/platforms/pseries/xics.c
+++ linux-2.6.20/arch/powerpc/platforms/pseries/xics.c
@@ -679,6 +679,8 @@ static struct device_node *cpuid_to_of_n
 	return NULL;
 }
 
+extern unsigned int max_cpus;
+
 void __init xics_init_IRQ(void)
 {
 	int i, j;
@@ -734,6 +736,13 @@ void __init xics_init_IRQ(void)
 skip_gserver_check:
 	of_node_put(np);
 
+	/* Kdump with maxcpus parameter in PPC970xx creates interrupt
+	 * distribution problems. So assign current boot cpu id to
+	 * interrupt distribution server
+	 */
+	if (max_cpus == 1)
+		default_distrib_server = default_server;
+
 	if (firmware_has_feature(FW_FEATURE_LPAR))
 		ppc_md.get_irq = xics_get_irq_lpar;
 	else
Index: linux-2.6.20/init/main.c
===================================================================
--- linux-2.6.20.orig/init/main.c
+++ linux-2.6.20/init/main.c
@@ -128,7 +128,7 @@ static char *execute_command;
 static char *ramdisk_execute_command;
 
 /* Setup configured maximum number of CPUs to activate */
-static unsigned int max_cpus = NR_CPUS;
+unsigned int max_cpus = NR_CPUS;
 
 /*
  * If set, this is an indication to the drivers that reset the underlying
 

Re: [PATCH] Fix interrupt distribution in ppc970

[Michael Ellerman]

On Mon, 2007-04-09 at 14:27 +0530, Mohan Kumar M wrote:
> On Wed, Mar 07, 2007 at 11:52:32AM +0100, Michael Ellerman wrote:
> > There's already maxcpus in init/main.c, that would probably be better,
> > though still ugly.
> >
> Based on Mike's suggestions, I modified the patch. The attached patch
> refers max_cpus variable to check whether the kernel is booted with
> maxcpus=1 parameter and if maxcpus=1 is specified the patch assigns only
> the current boot cpu to be the default distribution server.
> 
> Patch is generated over 2.6.20 kernel, cleanly applies to 2.6.21-rc5
> kernel.
> 
> Any suggestion, comment?

So the core of the problem is that if we haven't onlined all cpus then
we can't use the default_distrib_server value given to us by firmware,
because some of the cpus in that queue won't be online.

We can detect this situation by comparing the number of cpus that are
online vs the number that are present (not possible). This might even
work if you boot with maxcpus=1 and then hotplug the rest in.

How about this:

Index: powerpc/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- powerpc.orig/arch/powerpc/platforms/pseries/xics.c
+++ powerpc/arch/powerpc/platforms/pseries/xics.c
@@ -167,7 +167,10 @@ static int get_irq_server(unsigned int v
 		return default_server;
 
 	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
-		server = default_distrib_server;
+		if (num_online_cpus() == num_present_cpus())
+			server = default_distrib_server;
+		else
+			server = default_server;
 	} else {
 		cpus_and(tmp, cpu_online_map, cpumask);
 
@@ -415,7 +418,10 @@ static void xics_set_affinity(unsigned i
 
 	/* For the moment only implement delivery to all cpus or one cpu */
 	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
-		newmask = default_distrib_server;
+		if (num_online_cpus() == num_present_cpus())
+			newmask = default_distrib_server;
+		else
+			newmask = default_server;
 	} else {
 		cpus_and(tmp, cpu_online_map, cpumask);
 		if (cpus_empty(tmp))

Re: [PATCH] Fix interrupt distribution in ppc970

[Mohan Kumar M]

On Tue, Apr 10, 2007 at 05:06:03PM +1000, Michael Ellerman wrote:
> So the core of the problem is that if we haven't onlined all cpus then
> we can't use the default_distrib_server value given to us by firmware,
> because some of the cpus in that queue won't be online.
> 
> We can detect this situation by comparing the number of cpus that are
> online vs the number that are present (not possible). This might even
> work if you boot with maxcpus=1 and then hotplug the rest in.
> 
> How about this:
> 

Right, This also solves the problem.

> Index: powerpc/arch/powerpc/platforms/pseries/xics.c
> ===================================================================
> --- powerpc.orig/arch/powerpc/platforms/pseries/xics.c
> +++ powerpc/arch/powerpc/platforms/pseries/xics.c
> @@ -167,7 +167,10 @@ static int get_irq_server(unsigned int v
>  		return default_server;
> 
>  	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		server = default_distrib_server;
> +		if (num_online_cpus() == num_present_cpus())
> +			server = default_distrib_server;
> +		else
> +			server = default_server;
>  	} else {
>  		cpus_and(tmp, cpu_online_map, cpumask);
> 
> @@ -415,7 +418,10 @@ static void xics_set_affinity(unsigned i
> 
>  	/* For the moment only implement delivery to all cpus or one cpu */
>  	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		newmask = default_distrib_server;
> +		if (num_online_cpus() == num_present_cpus())
> +			newmask = default_distrib_server;
> +		else
> +			newmask = default_server;
>  	} else {
>  		cpus_and(tmp, cpu_online_map, cpumask);
>  		if (cpus_empty(tmp))
> 

Re: [PATCH] Fix interrupt distribution in ppc970

[Milton Miller]

On Apr 10, 2007, at 2:06 AM, Michael Ellerman wrote:

> On Mon, 2007-04-09 at 14:27 +0530, Mohan Kumar M wrote:
>> On Wed, Mar 07, 2007 at 11:52:32AM +0100, Michael Ellerman wrote:
>>> There's already maxcpus in init/main.c, that would probably be 
>>> better,
>>> though still ugly.
>>>
>> Based on Mike's suggestions, I modified the patch. The attached patch
>> refers max_cpus variable to check whether the kernel is booted with
>> maxcpus=1 parameter and if maxcpus=1 is specified the patch assigns 
>> only
>> the current boot cpu to be the default distribution server.
>
> So the core of the problem is that if we haven't onlined all cpus then
> we can't use the default_distrib_server value given to us by firmware,
> because some of the cpus in that queue won't be online.
>
> We can detect this situation by comparing the number of cpus that are
> online vs the number that are present (not possible). This might even
> work if you boot with maxcpus=1 and then hotplug the rest in.
>
> How about this:
>
> Index: powerpc/arch/powerpc/platforms/pseries/xics.c
> ===================================================================
> --- powerpc.orig/arch/powerpc/platforms/pseries/xics.c
> +++ powerpc/arch/powerpc/platforms/pseries/xics.c
> @@ -167,7 +167,10 @@ static int get_irq_server(unsigned int v
>  		return default_server;
>
>  	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		server = default_distrib_server;
> +		if (num_online_cpus() == num_present_cpus())
> +			server = default_distrib_server;
> +		else
> +			server = default_server;
>  	} else {
>  		cpus_and(tmp, cpu_online_map, cpumask);

This means we are doing population counts of two masks, when we really
just care that they are the same.   How about using
cpus_equal(cpu_online_mask, cpu_present_mask)?

>
> @@ -415,7 +418,10 @@ static void xics_set_affinity(unsigned i
>
>  	/* For the moment only implement delivery to all cpus or one cpu */
>  	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		newmask = default_distrib_server;
> +		if (num_online_cpus() == num_present_cpus())
> +			newmask = default_distrib_server;
> +		else
> +			newmask = default_server;
>  	} else {
>  		cpus_and(tmp, cpu_online_map, cpumask);
>  		if (cpus_empty(tmp))

This shows how close these two functions are.  The difference is
what happens when cpus_empty is true -- we default in one and
ignore in the other.

How about adding another arg to get_server, that says to fail
or default for the empty case, with failure being -1?

I'll try to code this up, but it might be a day or two until i get
the time.

milton

Re: [PATCH] Fix interrupt distribution in ppc970

[Michael Ellerman]

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

On Tue, 2007-04-10 at 11:59 -0500, Milton Miller wrote:
> On Apr 10, 2007, at 2:06 AM, Michael Ellerman wrote:
> 
> > On Mon, 2007-04-09 at 14:27 +0530, Mohan Kumar M wrote:
> >> On Wed, Mar 07, 2007 at 11:52:32AM +0100, Michael Ellerman wrote:
> >>> There's already maxcpus in init/main.c, that would probably be 
> >>> better,
> >>> though still ugly.
> >>>
> >> Based on Mike's suggestions, I modified the patch. The attached patch
> >> refers max_cpus variable to check whether the kernel is booted with
> >> maxcpus=1 parameter and if maxcpus=1 is specified the patch assigns 
> >> only
> >> the current boot cpu to be the default distribution server.
> >
> > So the core of the problem is that if we haven't onlined all cpus then
> > we can't use the default_distrib_server value given to us by firmware,
> > because some of the cpus in that queue won't be online.
> >
> > We can detect this situation by comparing the number of cpus that are
> > online vs the number that are present (not possible). This might even
> > work if you boot with maxcpus=1 and then hotplug the rest in.
> >
> > How about this:
> >
> > Index: powerpc/arch/powerpc/platforms/pseries/xics.c
> > ===================================================================
> > --- powerpc.orig/arch/powerpc/platforms/pseries/xics.c
> > +++ powerpc/arch/powerpc/platforms/pseries/xics.c
> > @@ -167,7 +167,10 @@ static int get_irq_server(unsigned int v
> >  		return default_server;
> >
> >  	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> > -		server = default_distrib_server;
> > +		if (num_online_cpus() == num_present_cpus())
> > +			server = default_distrib_server;
> > +		else
> > +			server = default_server;
> >  	} else {
> >  		cpus_and(tmp, cpu_online_map, cpumask);
> 
> This means we are doing population counts of two masks, when we really
> just care that they are the same.   How about using
> cpus_equal(cpu_online_mask, cpu_present_mask)?

Yep that's sensible.

> >
> > @@ -415,7 +418,10 @@ static void xics_set_affinity(unsigned i
> >
> >  	/* For the moment only implement delivery to all cpus or one cpu */
> >  	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> > -		newmask = default_distrib_server;
> > +		if (num_online_cpus() == num_present_cpus())
> > +			newmask = default_distrib_server;
> > +		else
> > +			newmask = default_server;
> >  	} else {
> >  		cpus_and(tmp, cpu_online_map, cpumask);
> >  		if (cpus_empty(tmp))
> 
> This shows how close these two functions are.  The difference is
> what happens when cpus_empty is true -- we default in one and
> ignore in the other.
> 
> How about adding another arg to get_server, that says to fail
> or default for the empty case, with failure being -1?
> 
> I'll try to code this up, but it might be a day or two until i get
> the time.

Yeah it annoyed me that the code was so similar, but it is a small but
important semantic difference.

I'll let you code it up, you know that code better than me.

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 interrupt distribution in ppc970

[Mohan Kumar M]

On Wed, Apr 11, 2007 at 11:16:41AM +1000, Michael Ellerman wrote:
> On Tue, 2007-04-10 at 11:59 -0500, Milton Miller wrote:
> > 
> > This means we are doing population counts of two masks, when we really
> > just care that they are the same.   How about using
> > cpus_equal(cpu_online_mask, cpu_present_mask)?
> 
> Yep that's sensible.
> 
> > This shows how close these two functions are.  The difference is
> > what happens when cpus_empty is true -- we default in one and
> > ignore in the other.
> > 
> > How about adding another arg to get_server, that says to fail
> > or default for the empty case, with failure being -1?
> > 
> > I'll try to code this up, but it might be a day or two until i get
> > the time.

I modified the patch based on suggestions given by Milton and Mike.
Milton is this patch okay?

Tested on 2.6.21-rc7.

=====
It is observed that in some PPC970 based machines, When the kernel is
booted with maxcpus=1, interrupts were distributed to non online cpus
also. So a condition is included to check whether the cpu online
map and cpu present map are equal or not. If they are equal
default_distrib_server is used as the interrupt server otherwise
default_server(ie boot cpu) is used as the interrupt server.

In addition to this, if an interrupt is assigned to a specific cpu (ie
smp affinity) and if that cpu is not online, the earlier code used to
return the default_distrib_server as interrupt server. This patch
introduces an additional parameter to the get_irq function ie
strict_check, based on this parameter, if the cpu is not online either
default_distrib_server or -1 is returned.

Cc: Milton Miller <miltonm@bga.com>,
    Michael Ellerman <michael@ellerman.id.au>
Signed-off-by: Mohan Kumar M <mohan@in.ibm.com>
---
 arch/powerpc/platforms/pseries/xics.c |   25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

Index: linux-2.6.21-rc4/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- linux-2.6.21-rc4.orig/arch/powerpc/platforms/pseries/xics.c
+++ linux-2.6.21-rc4/arch/powerpc/platforms/pseries/xics.c
@@ -156,9 +156,9 @@ static inline void lpar_qirr_info(int n_
 
 
 #ifdef CONFIG_SMP
-static int get_irq_server(unsigned int virq)
+static int get_irq_server(unsigned int virq, unsigned int strict_check)
 {
-	unsigned int server;
+	int server;
 	/* For the moment only implement delivery to all cpus or one cpu */
 	cpumask_t cpumask = irq_desc[virq].affinity;
 	cpumask_t tmp = CPU_MASK_NONE;
@@ -167,21 +167,25 @@ static int get_irq_server(unsigned int v
 		return default_server;
 
 	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
-		server = default_distrib_server;
+		if (cpus_equal(cpu_online_map, cpu_present_map))
+			server = default_distrib_server;
+		else
+			server = default_server;
 	} else {
 		cpus_and(tmp, cpu_online_map, cpumask);
 
-		if (cpus_empty(tmp))
+		if (cpus_empty(tmp) && !strict_check)
 			server = default_distrib_server;
+		else if(cpus_empty(tmp) && strict_check)
+			server = -1;
 		else
 			server = get_hard_smp_processor_id(first_cpu(tmp));
 	}
 
 	return server;
-
 }
 #else
-static int get_irq_server(unsigned int virq)
+static int get_irq_server(unsigned int virq, unsigned int strict_check)
 {
 	return default_server;
 }
@@ -192,7 +196,7 @@ static void xics_unmask_irq(unsigned int
 {
 	unsigned int irq;
 	int call_status;
-	unsigned int server;
+	int server;
 
 	pr_debug("xics: unmask virq %d\n", virq);
 
@@ -201,7 +205,7 @@ static void xics_unmask_irq(unsigned int
 	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
 		return;
 
-	server = get_irq_server(virq);
+	server = get_irq_server(virq, 0);
 
 	call_status = rtas_call(ibm_set_xive, 3, 1, NULL, irq, server,
 				DEFAULT_PRIORITY);
@@ -415,7 +419,10 @@ static void xics_set_affinity(unsigned i
 
 	/* For the moment only implement delivery to all cpus or one cpu */
 	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
-		newmask = default_distrib_server;
+		if (cpus_equal(cpu_online_map, cpu_present_map))
+			newmask = default_distrib_server;
+		else
+			newmask = default_server;
 	} else {
 		cpus_and(tmp, cpu_online_map, cpumask);
 		if (cpus_empty(tmp))

Re: [PATCH] Fix interrupt distribution in ppc970

[Milton Miller]

On Apr 19, 2007, at 6:52 AM, Mohan Kumar M wrote:

> On Wed, Apr 11, 2007 at 11:16:41AM +1000, Michael Ellerman wrote:
>> On Tue, 2007-04-10 at 11:59 -0500, Milton Miller wrote:
>>>
>>> This means we are doing population counts of two masks, when we 
>>> really
>>> just care that they are the same.   How about using
>>> cpus_equal(cpu_online_mask, cpu_present_mask)?
>>
>> Yep that's sensible.
>>
>>> This shows how close these two functions are.  The difference is
>>> what happens when cpus_empty is true -- we default in one and
>>> ignore in the other.
>>>
>>> How about adding another arg to get_server, that says to fail
>>> or default for the empty case, with failure being -1?
>
> I modified the patch based on suggestions given by Milton and Mike.
> Milton is this patch okay?

Well, its part of the way there.

>
> Tested on 2.6.21-rc7.
> Index: linux-2.6.21-rc4/arch/powerpc/platforms/pseries/xics.c
> ===================================================================
> --- linux-2.6.21-rc4.orig/arch/powerpc/platforms/pseries/xics.c
> +++ linux-2.6.21-rc4/arch/powerpc/platforms/pseries/xics.c
> @@ -156,9 +156,9 @@ static inline void lpar_qirr_info(int n_
>
>
>  #ifdef CONFIG_SMP
> -static int get_irq_server(unsigned int virq)
> +static int get_irq_server(unsigned int virq, unsigned int 
> strict_check)
>  {
> -	unsigned int server;
> +	int server;
>  	/* For the moment only implement delivery to all cpus or one cpu */
>  	cpumask_t cpumask = irq_desc[virq].affinity;
>  	cpumask_t tmp = CPU_MASK_NONE;
> @@ -167,21 +167,25 @@ static int get_irq_server(unsigned int v
>  		return default_server;
>
>  	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		server = default_distrib_server;
> +		if (cpus_equal(cpu_online_map, cpu_present_map))
> +			server = default_distrib_server;
> +		else
> +			server = default_server;
>  	} else {
>  		cpus_and(tmp, cpu_online_map, cpumask);
>
> -		if (cpus_empty(tmp))
> +		if (cpus_empty(tmp) && !strict_check)
>  			server = default_distrib_server;
> +		else if(cpus_empty(tmp) && strict_check)
> +			server = -1;
>  		else
>  			server = get_hard_smp_processor_id(first_cpu(tmp));
>  	}

Don't test cpus_empty twice, instead do a nested if.

Actually, looking at this a bit, you need your default_distrib or
default_server check again.

How about reorder to be if (!cpus_equal(...ALL)) {
     cpus_and
     if !cpus_empty(tmp
         return get_hard(first);
     if (strict)
         return -1;
}

if (cpus_equal(online, preseent)
return distrib
return default_server

For that matter, can we just call first_cpu, and check its <= NUM_CPUS
elminating the call to cpus_empty ?   Only call get_hard_cpu if its
valid.

...
> @@ -415,7 +419,10 @@ static void xics_set_affinity(unsigned i
...
>  	/* For the moment only implement delivery to all cpus or one cpu */
>  	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		newmask = default_distrib_server;
> +		if (cpus_equal(cpu_online_map, cpu_present_map))
>

this was supposed to be the call with strict = 1

milton

Re: [PATCH] Fix interrupt distribution in ppc970

[Mohan Kumar M]

On Fri, Apr 20, 2007 at 12:45:15AM -0500, Milton Miller wrote:
[snip]
> 
> Don't test cpus_empty twice, instead do a nested if.
> 
> Actually, looking at this a bit, you need your default_distrib or
> default_server check again.
> 
> How about reorder to be if (!cpus_equal(...ALL)) {
>     cpus_and
>     if !cpus_empty(tmp
>         return get_hard(first);
>     if (strict)
>         return -1;
> }
> 
> if (cpus_equal(online, preseent)
> return distrib
> return default_server
> 
> For that matter, can we just call first_cpu, and check its <= NUM_CPUS
> elminating the call to cpus_empty ?   Only call get_hard_cpu if its
> valid.
>

Milton, I hope this patch meets all your requirements.

Cc: Milton Miller <miltonm@bga.com>,
    Michael Ellerman <michael@ellerman.id.au>
Signed-off-by: Mohan Kumar M <mohan@in.ibm.com>
---
 arch/powerpc/platforms/pseries/xics.c |   39 ++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 15 deletions(-)

Index: linux-2.6.21-rc4/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- linux-2.6.21-rc4.orig/arch/powerpc/platforms/pseries/xics.c
+++ linux-2.6.21-rc4/arch/powerpc/platforms/pseries/xics.c
@@ -156,9 +156,9 @@ static inline void lpar_qirr_info(int n_
 
 
 #ifdef CONFIG_SMP
-static int get_irq_server(unsigned int virq)
+static int get_irq_server(unsigned int virq, unsigned int strict_check)
 {
-	unsigned int server;
+	int server;
 	/* For the moment only implement delivery to all cpus or one cpu */
 	cpumask_t cpumask = irq_desc[virq].affinity;
 	cpumask_t tmp = CPU_MASK_NONE;
@@ -166,22 +166,28 @@ static int get_irq_server(unsigned int v
 	if (!distribute_irqs)
 		return default_server;
 
-	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
-		server = default_distrib_server;
-	} else {
+	if (!cpus_equal(cpumask, CPU_MASK_ALL)) {
 		cpus_and(tmp, cpu_online_map, cpumask);
 
-		if (cpus_empty(tmp))
-			server = default_distrib_server;
+		server = first_cpu(tmp);
+
+		if (server < NR_CPUS)
+			return get_hard_smp_processor_id(server);
+		else {
+			if(strict_check)
+				return (-1);
+			else
+				return default_distrib_server;
+		}
+	} else {
+		if (cpus_equal(cpu_online_map, cpu_present_map))
+			return default_distrib_server;
 		else
-			server = get_hard_smp_processor_id(first_cpu(tmp));
+			return default_server;
 	}
-
-	return server;
-
 }
 #else
-static int get_irq_server(unsigned int virq)
+static int get_irq_server(unsigned int virq, unsigned int strict_check)
 {
 	return default_server;
 }
@@ -192,7 +198,7 @@ static void xics_unmask_irq(unsigned int
 {
 	unsigned int irq;
 	int call_status;
-	unsigned int server;
+	int server;
 
 	pr_debug("xics: unmask virq %d\n", virq);
 
@@ -201,7 +207,7 @@ static void xics_unmask_irq(unsigned int
 	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
 		return;
 
-	server = get_irq_server(virq);
+	server = get_irq_server(virq, 0);
 
 	call_status = rtas_call(ibm_set_xive, 3, 1, NULL, irq, server,
 				DEFAULT_PRIORITY);
@@ -415,7 +421,10 @@ static void xics_set_affinity(unsigned i
 
 	/* For the moment only implement delivery to all cpus or one cpu */
 	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
-		newmask = default_distrib_server;
+		if (cpus_equal(cpu_online_map, cpu_present_map))
+			newmask = default_distrib_server;
+		else
+			newmask = default_server;
 	} else {
 		cpus_and(tmp, cpu_online_map, cpumask);
 		if (cpus_empty(tmp))


=================== 
> ...
> >@@ -415,7 +419,10 @@ static void xics_set_affinity(unsigned i
> ...
> > 	/* For the moment only implement delivery to all cpus or one cpu */
> > 	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> >-		newmask = default_distrib_server;
> >+		if (cpus_equal(cpu_online_map, cpu_present_map))
> >
> 
> this was supposed to be the call with strict = 1

Do you mean to use 'strict_check' argument in xics_set_affinity?
set_affinity call is declared in linux/irq.h, so if modifying
xics_set_affinity will affect other arch's set_affinity also.

Regards,
Mohan

Re: [PATCH] Fix interrupt distribution in ppc970

[Milton Miller]

On Apr 26, 2007, at 4:24 AM, Mohan Kumar M wrote:
> On Fri, Apr 20, 2007 at 12:45:15AM -0500, Milton Miller wrote:
[snip]
> Milton, I hope this patch meets all your requirements.

Closer, much better.
> Cc: Milton Miller <miltonm@bga.com>,
>     Michael Ellerman <michael@ellerman.id.au>
> Signed-off-by: Mohan Kumar M <mohan@in.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/xics.c |   39 
> ++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
>
> Index: linux-2.6.21-rc4/arch/powerpc/platforms/pseries/xics.c
> ===================================================================
> --- linux-2.6.21-rc4.orig/arch/powerpc/platforms/pseries/xics.c
> +++ linux-2.6.21-rc4/arch/powerpc/platforms/pseries/xics.c
> @@ -156,9 +156,9 @@ static inline void lpar_qirr_info(int n_
[snipping]
> +	if (!cpus_equal(cpumask, CPU_MASK_ALL)) {
>  		cpus_and(tmp, cpu_online_map, cpumask);
>
> +		server = first_cpu(tmp);
> +
> +		if (server < NR_CPUS)
> +			return get_hard_smp_processor_id(server);
> +		else {
> +			if(strict_check)
> +				return (-1);

No parens around the return value.  That is, use return -1;

> +			else
> +				return default_distrib_server;
> +		}
...

> @@ -415,7 +421,10 @@ static void xics_set_affinity(unsigned i
>
>  	/* For the moment only implement delivery to all cpus or one cpu */
>  	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		newmask = default_distrib_server;
> +		if (cpus_equal(cpu_online_map, cpu_present_map))
> +			newmask = default_distrib_server;
> +		else
> +			newmask = default_server;
>  	} else {
>  		cpus_and(tmp, cpu_online_map, cpumask);
>  		if (cpus_empty(tmp))
>
>
> ===================
>> ...
>>> @@ -415,7 +419,10 @@ static void xics_set_affinity(unsigned i
>> ...
>>> 	/* For the moment only implement delivery to all cpus or one cpu */
>>> 	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
>>> -		newmask = default_distrib_server;
>>> +		if (cpus_equal(cpu_online_map, cpu_present_map))
>>>
>>
>> this was supposed to be the call with strict = 1
>
> Do you mean to use 'strict_check' argument in xics_set_affinity?
> set_affinity call is declared in linux/irq.h, so if modifying
> xics_set_affinity will affect other arch's set_affinity also.

Yes.   The whole point of
> -static int get_irq_server(unsigned int virq)
> +static int get_irq_server(unsigned int virq, unsigned int 
> strict_check)
was to factor out the common code in this function.

I wasn't trying to change the prototype of xics_set_affinity.

Looking at the code a bit, I think part of the confusion is that newmask
is horribly misnamed.   Please rename it to server or irqserver.
Obtain its value by calling get_irq_server.  If the server returned
is -1 (in strict mode), don't call rtas (just return like today).
I guess a printk could be in order since the function is void, and
only root can request the change.

milton

Re: [PATCH] Fix interrupt distribution in ppc970

[Mohan Kumar M]

On Thu, Apr 26, 2007 at 09:42:50AM -0500, Milton Miller wrote:
> Yes.   The whole point of
> >-static int get_irq_server(unsigned int virq)
> >+static int get_irq_server(unsigned int virq, unsigned int 
> >strict_check)
> was to factor out the common code in this function.
> 
> I wasn't trying to change the prototype of xics_set_affinity.
> 
> Looking at the code a bit, I think part of the confusion is that newmask
> is horribly misnamed.   Please rename it to server or irqserver.
> Obtain its value by calling get_irq_server.  If the server returned
> is -1 (in strict mode), don't call rtas (just return like today).
> I guess a printk could be in order since the function is void, and
> only root can request the change.

Milton,

How about this patch?


It is observed that in some PPC970 based machines, When the kernel is
booted with maxcpus=1, interrupts were distributed to offline cpus
also. So a condition is included to check whether the cpu online
map and cpu present map are equal or not. If they are equal
default_distrib_server is used as the interrupt server otherwise
default_server(ie boot cpu) is used as the interrupt server.

In addition to this, if an interrupt is assigned to a specific cpu (ie
smp affinity) and if that cpu is not online, the earlier code used to
return the default_distrib_server as interrupt server. This patch
introduces an additional paramter to the get_irq function ie
strict_check, based on this parameter, if the cpu is not online either
default_distrib_server or -1 is returned.

Cc: Milton Miller <miltonm@bga.com>,
    Michael Ellerman <michael@ellerman.id.au>
Signed-off-by: Mohan Kumar M <mohan@in.ibm.com>
---
 arch/powerpc/platforms/pseries/xics.c |   63 +++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 24 deletions(-)

Index: linux-2.6.21.1/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- linux-2.6.21.1.orig/arch/powerpc/platforms/pseries/xics.c
+++ linux-2.6.21.1/arch/powerpc/platforms/pseries/xics.c
@@ -156,9 +156,9 @@ static inline void lpar_qirr_info(int n_
 
 
 #ifdef CONFIG_SMP
-static int get_irq_server(unsigned int virq)
+static int get_irq_server(unsigned int virq, unsigned int strict_check)
 {
-	unsigned int server;
+	int server;
 	/* For the moment only implement delivery to all cpus or one cpu */
 	cpumask_t cpumask = irq_desc[virq].affinity;
 	cpumask_t tmp = CPU_MASK_NONE;
@@ -166,22 +166,28 @@ static int get_irq_server(unsigned int v
 	if (!distribute_irqs)
 		return default_server;
 
-	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
-		server = default_distrib_server;
-	} else {
+	if (!cpus_equal(cpumask, CPU_MASK_ALL)) {
 		cpus_and(tmp, cpu_online_map, cpumask);
 
-		if (cpus_empty(tmp))
-			server = default_distrib_server;
+		server = first_cpu(tmp);
+
+		if (server < NR_CPUS)
+			return get_hard_smp_processor_id(server);
+		else {
+			if (strict_check)
+				return -1;
+			else
+				return default_distrib_server;
+		}
+	} else {
+		if (cpus_equal(cpu_online_map, cpu_present_map))
+			return default_distrib_server;
 		else
-			server = get_hard_smp_processor_id(first_cpu(tmp));
+			return default_server;
 	}
-
-	return server;
-
 }
 #else
-static int get_irq_server(unsigned int virq)
+static int get_irq_server(unsigned int virq, unsigned int strict_check)
 {
 	return default_server;
 }
@@ -192,7 +198,7 @@ static void xics_unmask_irq(unsigned int
 {
 	unsigned int irq;
 	int call_status;
-	unsigned int server;
+	int server;
 
 	pr_debug("xics: unmask virq %d\n", virq);
 
@@ -201,7 +207,7 @@ static void xics_unmask_irq(unsigned int
 	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
 		return;
 
-	server = get_irq_server(virq);
+	server = get_irq_server(virq, 0);
 
 	call_status = rtas_call(ibm_set_xive, 3, 1, NULL, irq, server,
 				DEFAULT_PRIORITY);
@@ -398,8 +404,7 @@ static void xics_set_affinity(unsigned i
 	unsigned int irq;
 	int status;
 	int xics_status[2];
-	unsigned long newmask;
-	cpumask_t tmp = CPU_MASK_NONE;
+	int irq_server;
 
 	irq = (unsigned int)irq_map[virq].hwirq;
 	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
@@ -413,18 +418,28 @@ static void xics_set_affinity(unsigned i
 		return;
 	}
 
-	/* For the moment only implement delivery to all cpus or one cpu */
-	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
-		newmask = default_distrib_server;
-	} else {
-		cpus_and(tmp, cpu_online_map, cpumask);
-		if (cpus_empty(tmp))
+	/* Get current irq_server for the given irq */
+	irq_server = get_irq_server(irq, 1);
+	if (irq_server == -1) {
+		printk(KERN_ERR "xics_set_affinity: Invalid cpumask\n");
+		return;
+	}
+
+	/* For the moment only implement delivery to all cpus or one cpu.
+	 * Compare the irq_server with the new cpumask. If the irq_server
+	 * is specified in cpumask, do the required rtas_call, otherwise
+	 * return by printing an error message
+	 */
+	if (!cpus_equal(cpumask, CPU_MASK_ALL)) {
+		if (!cpu_isset(irq_server, cpumask)) {
+			printk(KERN_ERR "xics_set_affinity: Invalid "
+							"cpumask\n");
 			return;
-		newmask = get_hard_smp_processor_id(first_cpu(tmp));
+		}
 	}
 
 	status = rtas_call(ibm_set_xive, 3, 1, NULL,
-				irq, newmask, xics_status[1]);
+				irq, irq_server, xics_status[1]);
 
 	if (status) {
 		printk(KERN_ERR "xics_set_affinity: irq=%u ibm,set-xive "

Re: [PATCH] Fix interrupt distribution in ppc970

[Milton Miller]

On May 3, 2007, at 9:47 AM, Mohan Kumar M wrote:
> On Thu, Apr 26, 2007 at 09:42:50AM -0500, Milton Miller wrote:
>> Yes.   The whole point of
>>> -static int get_irq_server(unsigned int virq)
>>> +static int get_irq_server(unsigned int virq, unsigned int
>>> strict_check)
>> was to factor out the common code in this function.
>
> Milton,
>
> How about this patch?

Getting closer, still not right.

>
> Index: linux-2.6.21.1/arch/powerpc/platforms/pseries/xics.c
> ===================================================================
> --- linux-2.6.21.1.orig/arch/powerpc/platforms/pseries/xics.c
> +++ linux-2.6.21.1/arch/powerpc/platforms/pseries/xics.c
> @@ -156,9 +156,9 @@ static inline void lpar_qirr_info(int n_
>
>
>  #ifdef CONFIG_SMP
> -static int get_irq_server(unsigned int virq)
> +static int get_irq_server(unsigned int virq, unsigned int 
> strict_check)
>  {
> -	unsigned int server;
> +	int server;
>  	/* For the moment only implement delivery to all cpus or one cpu */
>  	cpumask_t cpumask = irq_desc[virq].affinity;
>  	cpumask_t tmp = CPU_MASK_NONE;
> @@ -166,22 +166,28 @@ static int get_irq_server(unsigned int v
>  	if (!distribute_irqs)
>  		return default_server;
>
> -	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		server = default_distrib_server;
> -	} else {
> +	if (!cpus_equal(cpumask, CPU_MASK_ALL)) {
>  		cpus_and(tmp, cpu_online_map, cpumask);
>
> -		if (cpus_empty(tmp))
> -			server = default_distrib_server;
> +		server = first_cpu(tmp);
> +
> +		if (server < NR_CPUS)
> +			return get_hard_smp_processor_id(server);
> +		else {
> +			if (strict_check)
> +				return -1;


> +			else
> +				return default_distrib_server;
> +		}
> +	} else {

Take out the above 4 lines, so that the return always has cpu_online
vs cpu_present factored in, by falling through from specific mask to
default server selection.

> +		if (cpus_equal(cpu_online_map, cpu_present_map))
> +			return default_distrib_server;
>  		else
> -			server = get_hard_smp_processor_id(first_cpu(tmp));
> +			return default_server;
>  	}

[This matching brace will go and indent will change.]

> -
> -	return server;
> -
>  }
...

> @@ -398,8 +404,7 @@ static void xics_set_affinity(unsigned i
>  	unsigned int irq;
>  	int status;
>  	int xics_status[2];
> -	unsigned long newmask;
> -	cpumask_t tmp = CPU_MASK_NONE;
> +	int irq_server;
>
>  	irq = (unsigned int)irq_map[virq].hwirq;
>  	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
> @@ -413,18 +418,28 @@ static void xics_set_affinity(unsigned i
>  		return;
>  	}
>
> -	/* For the moment only implement delivery to all cpus or one cpu */
> -	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		newmask = default_distrib_server;
> -	} else {
> -		cpus_and(tmp, cpu_online_map, cpumask);
> -		if (cpus_empty(tmp))
> +	/* Get current irq_server for the given irq */
> +	irq_server = get_irq_server(irq, 1);
> +	if (irq_server == -1) {
> +		printk(KERN_ERR "xics_set_affinity: Invalid cpumask\n");

WARNING or NOTICE would be fine.  The interrupt number should be 
printed.
and maybe "No online cpus in <cpumask> for irq %d" would be better (I
think there is a print cpumask helper).

> +		return;
> +	}
> +
> +	/* For the moment only implement delivery to all cpus or one cpu.
> +	 * Compare the irq_server with the new cpumask. If the irq_server
> +	 * is specified in cpumask, do the required rtas_call, otherwise
> +	 * return by printing an error message
> +	 */
> +	if (!cpus_equal(cpumask, CPU_MASK_ALL)) {
> +		if (!cpu_isset(irq_server, cpumask)) {
> +			printk(KERN_ERR "xics_set_affinity: Invalid "
> +							"cpumask\n");
>  			return;

I don't understand what you are trying to do here.  We already
chose the mask, and printed an error if we got -1 due to strict.
I think this can just be dropped?

> -		newmask = get_hard_smp_processor_id(first_cpu(tmp));
> +		}
>  	}
>
>  	status = rtas_call(ibm_set_xive, 3, 1, NULL,
> -				irq, newmask, xics_status[1]);
> +				irq, irq_server, xics_status[1]);
>
>  	if (status) {
>  		printk(KERN_ERR "xics_set_affinity: irq=%u ibm,set-xive "


milton

Re: [PATCH] Fix interrupt distribution in ppc970

[Mohan Kumar M]

Milton,

How about this patch?

=============================================================
In some of the PPC970 based systems, interrupt would be distributed to
offline cpus also even when booted with "maxcpus=1". So check whether
cpu online map and cpu present map are equal or not. If they are equal
default_distrib_server is used as interrupt server otherwise boot cpu
(default_server) used as interrupt server. 

In addition to this, if an interrupt is assigned to a specific cpu (ie
smp affinity) and if that cpu is not online, the earlier code used to
return the default_distrib_server as interrupt server. This patch
introduces an additional paramter to the get_irq function ie
strict_check, based on this parameter, if the cpu is not online either
default_distrib_server or -1 is returned.

Cc: Milton Miller <miltonm@bga.com>,
    Michael Ellerman <michael@ellerman.id.au>
Signed-off-by: Mohan Kumar M <mohan@in.ibm.com>
---
 arch/powerpc/platforms/pseries/xics.c |   54 ++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 25 deletions(-)

Index: linux-2.6.21.1/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- linux-2.6.21.1.orig/arch/powerpc/platforms/pseries/xics.c
+++ linux-2.6.21.1/arch/powerpc/platforms/pseries/xics.c
@@ -156,9 +156,9 @@ static inline void lpar_qirr_info(int n_
 
 
 #ifdef CONFIG_SMP
-static int get_irq_server(unsigned int virq)
+static int get_irq_server(unsigned int virq, unsigned int strict_check)
 {
-	unsigned int server;
+	int server;
 	/* For the moment only implement delivery to all cpus or one cpu */
 	cpumask_t cpumask = irq_desc[virq].affinity;
 	cpumask_t tmp = CPU_MASK_NONE;
@@ -166,22 +166,24 @@ static int get_irq_server(unsigned int v
 	if (!distribute_irqs)
 		return default_server;
 
-	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
-		server = default_distrib_server;
-	} else {
+	if (!cpus_equal(cpumask, CPU_MASK_ALL)) {
 		cpus_and(tmp, cpu_online_map, cpumask);
 
-		if (cpus_empty(tmp))
-			server = default_distrib_server;
-		else
-			server = get_hard_smp_processor_id(first_cpu(tmp));
-	}
+		server = first_cpu(tmp);
 
-	return server;
+		if (server < NR_CPUS)
+			return get_hard_smp_processor_id(server);
+		else if (strict_check)
+				return -1;
+	}
 
+	if (cpus_equal(cpu_online_map, cpu_present_map))
+		return default_distrib_server;
+	else
+		return default_server;
 }
 #else
-static int get_irq_server(unsigned int virq)
+static int get_irq_server(unsigned int virq, unsigned int strict_check)
 {
 	return default_server;
 }
@@ -192,7 +194,7 @@ static void xics_unmask_irq(unsigned int
 {
 	unsigned int irq;
 	int call_status;
-	unsigned int server;
+	int server;
 
 	pr_debug("xics: unmask virq %d\n", virq);
 
@@ -201,7 +203,7 @@ static void xics_unmask_irq(unsigned int
 	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
 		return;
 
-	server = get_irq_server(virq);
+	server = get_irq_server(virq, 0);
 
 	call_status = rtas_call(ibm_set_xive, 3, 1, NULL, irq, server,
 				DEFAULT_PRIORITY);
@@ -398,8 +400,7 @@ static void xics_set_affinity(unsigned i
 	unsigned int irq;
 	int status;
 	int xics_status[2];
-	unsigned long newmask;
-	cpumask_t tmp = CPU_MASK_NONE;
+	int irq_server;
 
 	irq = (unsigned int)irq_map[virq].hwirq;
 	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
@@ -413,18 +414,21 @@ static void xics_set_affinity(unsigned i
 		return;
 	}
 
-	/* For the moment only implement delivery to all cpus or one cpu */
-	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
-		newmask = default_distrib_server;
-	} else {
-		cpus_and(tmp, cpu_online_map, cpumask);
-		if (cpus_empty(tmp))
-			return;
-		newmask = get_hard_smp_processor_id(first_cpu(tmp));
+	/*
+	 * For the moment only implement delivery to all cpus or one cpu.
+	 * Get current irq_server for the given irq
+	 */
+	irq_server = get_irq_server(irq, 1);
+	if (irq_server == -1) {
+		char cpulist[128];
+		cpulist_scnprintf(cpulist, sizeof(cpulist), cpumask);
+		printk(KERN_WARNING "xics_set_affinity: No online cpus in "
+				"the mask %s for irq %d\n", cpulist, virq);
+		return;
 	}
 
 	status = rtas_call(ibm_set_xive, 3, 1, NULL,
-				irq, newmask, xics_status[1]);
+				irq, irq_server, xics_status[1]);
 
 	if (status) {
 		printk(KERN_ERR "xics_set_affinity: irq=%u ibm,set-xive "

Re: [PATCH] Fix interrupt distribution in ppc970

[Milton Miller]

On Jun 4, 2007, at 5:54 AM, Mohan Kumar M wrote:

> Milton,
>
> How about this patch?

I'd make a few minor changes (see below), but I'll ack it.

I've checked it compiles and boots but not the server selection.

> =============================================================
> In some of the PPC970 based systems, interrupt would be distributed to
> offline cpus also even when booted with "maxcpus=1". So check whether
> cpu online map and cpu present map are equal or not. If they are equal
> default_distrib_server is used as interrupt server otherwise boot cpu
> (default_server) used as interrupt server.
>
> In addition to this, if an interrupt is assigned to a specific cpu (ie
> smp affinity) and if that cpu is not online, the earlier code used to
> return the default_distrib_server as interrupt server. This patch
> introduces an additional paramter to the get_irq function ie
> strict_check, based on this parameter, if the cpu is not online either
> default_distrib_server or -1 is returned.
>
> Cc: Milton Miller <miltonm@bga.com>,
>     Michael Ellerman <michael@ellerman.id.au>
> Signed-off-by: Mohan Kumar M <mohan@in.ibm.com>

Acked-By: Milton Miller <miltonm@bga.com>

> @@ -166,22 +166,24 @@ static int get_irq_server(unsigned int v
>  	if (!distribute_irqs)
>  		return default_server;
>
> -	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		server = default_distrib_server;
> -	} else {
> +	if (!cpus_equal(cpumask, CPU_MASK_ALL)) {
>  		cpus_and(tmp, cpu_online_map, cpumask);
>
> -		if (cpus_empty(tmp))
> -			server = default_distrib_server;
> -		else
> -			server = get_hard_smp_processor_id(first_cpu(tmp));
> -	}
> +		server = first_cpu(tmp);
>
> -	return server;
> +		if (server < NR_CPUS)
> +			return get_hard_smp_processor_id(server);
> +		else if (strict_check)
> +				return -1;

The indent on the return is 1 tab too many.
See next comment.

> +	}
>
> +	if (cpus_equal(cpu_online_map, cpu_present_map))
> +		return default_distrib_server;
> +	else
> +		return default_server;

I'd actually remove both occurrences of "else" bringing this and the 
previous return back a tabstop.  The else is unnecessary because the if 
statement is return .  (Seperate the new if from the previous with a 
blank line in both cases).

>  }
>  #else
> -static int get_irq_server(unsigned int virq)
> +static int get_irq_server(unsigned int virq, unsigned int 
> strict_check)
>  {
>  	return default_server;
>  }

Re: [PATCH] Fix interrupt distribution in ppc970

[Mohan Kumar M]

I updated the patch with correct tab spacing and removed unnecessary
"else".


In some of the PPC970 based systems, interrupt would be distributed to
offline cpus also even when booted with "maxcpus=1". So check whether
cpu online map and cpu present map are equal or not. If they are equal
default_distrib_server is used as interrupt server otherwise boot cpu
(default_server) used as interrupt server. 

In addition to this, if an interrupt is assigned to a specific cpu (ie
smp affinity) and if that cpu is not online, the earlier code used to
return the default_distrib_server as interrupt server. This patch
introduces an additional paramter to the get_irq function ie
strict_check, based on this parameter, if the cpu is not online either
default_distrib_server or -1 is returned.

Cc: Milton Miller <miltonm@bga.com>,
    Michael Ellerman <michael@ellerman.id.au>
Signed-off-by: Mohan Kumar M <mohan@in.ibm.com>
---
 arch/powerpc/platforms/pseries/xics.c |   53 ++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 24 deletions(-)

Index: linux-2.6.21.1/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- linux-2.6.21.1.orig/arch/powerpc/platforms/pseries/xics.c
+++ linux-2.6.21.1/arch/powerpc/platforms/pseries/xics.c
@@ -156,9 +156,9 @@ static inline void lpar_qirr_info(int n_
 
 
 #ifdef CONFIG_SMP
-static int get_irq_server(unsigned int virq)
+static int get_irq_server(unsigned int virq, unsigned int strict_check)
 {
-	unsigned int server;
+	int server;
 	/* For the moment only implement delivery to all cpus or one cpu */
 	cpumask_t cpumask = irq_desc[virq].affinity;
 	cpumask_t tmp = CPU_MASK_NONE;
@@ -166,22 +166,25 @@ static int get_irq_server(unsigned int v
 	if (!distribute_irqs)
 		return default_server;
 
-	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
-		server = default_distrib_server;
-	} else {
+	if (!cpus_equal(cpumask, CPU_MASK_ALL)) {
 		cpus_and(tmp, cpu_online_map, cpumask);
 
-		if (cpus_empty(tmp))
-			server = default_distrib_server;
-		else
-			server = get_hard_smp_processor_id(first_cpu(tmp));
+		server = first_cpu(tmp);
+
+		if (server < NR_CPUS)
+			return get_hard_smp_processor_id(server);
+
+		if (strict_check)
+			return -1;
 	}
 
-	return server;
+	if (cpus_equal(cpu_online_map, cpu_present_map))
+		return default_distrib_server;
 
+	return default_server;
 }
 #else
-static int get_irq_server(unsigned int virq)
+static int get_irq_server(unsigned int virq, unsigned int strict_check)
 {
 	return default_server;
 }
@@ -192,7 +195,7 @@ static void xics_unmask_irq(unsigned int
 {
 	unsigned int irq;
 	int call_status;
-	unsigned int server;
+	int server;
 
 	pr_debug("xics: unmask virq %d\n", virq);
 
@@ -201,7 +204,7 @@ static void xics_unmask_irq(unsigned int
 	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
 		return;
 
-	server = get_irq_server(virq);
+	server = get_irq_server(virq, 0);
 
 	call_status = rtas_call(ibm_set_xive, 3, 1, NULL, irq, server,
 				DEFAULT_PRIORITY);
@@ -398,8 +401,7 @@ static void xics_set_affinity(unsigned i
 	unsigned int irq;
 	int status;
 	int xics_status[2];
-	unsigned long newmask;
-	cpumask_t tmp = CPU_MASK_NONE;
+	int irq_server;
 
 	irq = (unsigned int)irq_map[virq].hwirq;
 	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
@@ -413,18 +415,21 @@ static void xics_set_affinity(unsigned i
 		return;
 	}
 
-	/* For the moment only implement delivery to all cpus or one cpu */
-	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
-		newmask = default_distrib_server;
-	} else {
-		cpus_and(tmp, cpu_online_map, cpumask);
-		if (cpus_empty(tmp))
-			return;
-		newmask = get_hard_smp_processor_id(first_cpu(tmp));
+	/*
+	 * For the moment only implement delivery to all cpus or one cpu.
+	 * Get current irq_server for the given irq
+	 */
+	irq_server = get_irq_server(irq, 1);
+	if (irq_server == -1) {
+		char cpulist[128];
+		cpulist_scnprintf(cpulist, sizeof(cpulist), cpumask);
+		printk(KERN_WARNING "xics_set_affinity: No online cpus in "
+				"the mask %s for irq %d\n", cpulist, virq);
+		return;
 	}
 
 	status = rtas_call(ibm_set_xive, 3, 1, NULL,
-				irq, newmask, xics_status[1]);
+				irq, irq_server, xics_status[1]);
 
 	if (status) {
 		printk(KERN_ERR "xics_set_affinity: irq=%u ibm,set-xive "

Re: [PATCH] Fix interrupt distribution in ppc970

[Milton Miller]

On Jun 6, 2007, at 6:31 AM, Mohan Kumar M wrote:
> I updated the patch with correct tab spacing and removed unnecessary
> "else".


> In some of the PPC970 based systems, interrupt would be distributed to
> offline cpus also even when booted with "maxcpus=1". So check whether
> cpu online map and cpu present map are equal or not. If they are equal
> default_distrib_server is used as interrupt server otherwise boot cpu
> (default_server) used as interrupt server.
>
> In addition to this, if an interrupt is assigned to a specific cpu (ie
> smp affinity) and if that cpu is not online, the earlier code used to
> return the default_distrib_server as interrupt server. This patch
> introduces an additional paramter to the get_irq function ie
> strict_check, based on this parameter, if the cpu is not online either
> default_distrib_server or -1 is returned.


The code is structured cleanly.  However, when testing this patch, I 
found (1) you printed the mask as a cpulist instead of a cpumask.  
Since the user writes a cpumask to /proc/irq/xx/smp_affinity, it would 
make more sense to print a mask in the error message.

However, this is all mute because (2) the common in /kenrel/irq/proc.c 
checks that a cpu in the mask is online and returns -EINVAL to the user 
without calling the ->set_affinity hook (we have no select_smp_affinity 
hook arch code).   Unless there is another path to call ->set_affinity, 
we can only trigger the case of no online cpu by racing between setting 
the affinity and taking a cpu offline.

Does anyone know of another path to set the affinity?  If not I would 
remove this extra logic and change the behavior from ignore to set to 
default server.


milton

>  #ifdef CONFIG_SMP
> -static int get_irq_server(unsigned int virq)
> +static int get_irq_server(unsigned int virq, unsigned int 
> strict_check)
>  {
> -	unsigned int server;
> +	int server;
>  	/* For the moment only implement delivery to all cpus or one cpu */
>  	cpumask_t cpumask = irq_desc[virq].affinity;
>  	cpumask_t tmp = CPU_MASK_NONE;
> @@ -166,22 +166,25 @@ static int get_irq_server(unsigned int v
>  	if (!distribute_irqs)
>  		return default_server;
>
> -	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		server = default_distrib_server;
> -	} else {
> +	if (!cpus_equal(cpumask, CPU_MASK_ALL)) {
>  		cpus_and(tmp, cpu_online_map, cpumask);
>
> -		if (cpus_empty(tmp))
> -			server = default_distrib_server;
> -		else
> -			server = get_hard_smp_processor_id(first_cpu(tmp));
> +		server = first_cpu(tmp);
> +
> +		if (server < NR_CPUS)
> +			return get_hard_smp_processor_id(server);
> +
> +		if (strict_check)
> +			return -1;
>  	}
>
> -	return server;
> +	if (cpus_equal(cpu_online_map, cpu_present_map))
> +		return default_distrib_server;
>
> +	return default_server;
>  }
>

...

> +	/*
> +	 * For the moment only implement delivery to all cpus or one cpu.
> +	 * Get current irq_server for the given irq
> +	 */
> +	irq_server = get_irq_server(irq, 1);
> +	if (irq_server == -1) {
> +		char cpulist[128];
> +		cpulist_scnprintf(cpulist, sizeof(cpulist), cpumask);
> +		printk(KERN_WARNING "xics_set_affinity: No online cpus in "
> +				"the mask %s for irq %d\n", cpulist, virq);
> +		return;
>  	}

Re: [PATCH] Fix interrupt distribution in ppc970

[Mohan Kumar M]

On Sun, Jun 10, 2007 at 08:58:10PM -0500, Milton Miller wrote:
> The code is structured cleanly.  However, when testing this patch, I 
> found (1) you printed the mask as a cpulist instead of a cpumask.  
> Since the user writes a cpumask to /proc/irq/xx/smp_affinity, it would 
> make more sense to print a mask in the error message.
>

I can change it to use cpumask print instead of cpulist print.
 
> However, this is all mute because (2) the common in /kenrel/irq/proc.c 
> checks that a cpu in the mask is online and returns -EINVAL to the user 
> without calling the ->set_affinity hook (we have no select_smp_affinity 
> hook arch code).   Unless there is another path to call ->set_affinity, 
> we can only trigger the case of no online cpu by racing between setting 
> the affinity and taking a cpu offline.
>

As you said, we can remove the extra check in get_irq_server function.

Any other thoughts?

 
> Does anyone know of another path to set the affinity?  If not I would 
> remove this extra logic and change the behavior from ignore to set to 
> default server.
> 

Re: [PATCH] Fix interrupt distribution in ppc970

[Mohan Kumar M]

On Sun, Jun 10, 2007 at 08:58:10PM -0500, Milton Miller wrote:
> The code is structured cleanly.  However, when testing this patch, I 
> found (1) you printed the mask as a cpulist instead of a cpumask.  
> Since the user writes a cpumask to /proc/irq/xx/smp_affinity, it would 
> make more sense to print a mask in the error message.

Attached patch uses cpumask_scnprintf function.

> 
> However, this is all mute because (2) the common in /kenrel/irq/proc.c 
> checks that a cpu in the mask is online and returns -EINVAL to the user 
> without calling the ->set_affinity hook (we have no select_smp_affinity 
> hook arch code).   Unless there is another path to call ->set_affinity, 
> we can only trigger the case of no online cpu by racing between setting 
> the affinity and taking a cpu offline.
>
> Does anyone know of another path to set the affinity?  If not I would 
> remove this extra logic and change the behavior from ignore to set to 
> default server.
>

tick_setup_device function in kernel/time/tick-common.c file calls
irq_set_affinity function without validating the new cpumask with the
cpu online mask. So IMHO the check in get_irq_server is required.

================
In some of the PPC970 based systems, interrupt would be distributed to
offline cpus also even when booted with "maxcpus=1". So check whether
cpu online map and cpu present map are equal or not. If they are equal
default_distrib_server is used as interrupt server otherwise boot cpu
(default_server) used as interrupt server. 

In addition to this, if an interrupt is assigned to a specific cpu (ie
smp affinity) and if that cpu is not online, the earlier code used to
return the default_distrib_server as interrupt server. This patch
introduces an additional paramter to the get_irq function ie
strict_check, based on this parameter, if the cpu is not online either
default_distrib_server or -1 is returned.

Cc: Milton Miller <miltonm@bga.com>,
    Michael Ellerman <michael@ellerman.id.au>
Signed-off-by: Mohan Kumar M <mohan@in.ibm.com>
---
 arch/powerpc/platforms/pseries/xics.c |   53 ++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 24 deletions(-)

Index: linux-2.6.21.1/arch/powerpc/platforms/pseries/xics.c
===================================================================
--- linux-2.6.21.1.orig/arch/powerpc/platforms/pseries/xics.c
+++ linux-2.6.21.1/arch/powerpc/platforms/pseries/xics.c
@@ -156,9 +156,9 @@ static inline void lpar_qirr_info(int n_
 
 
 #ifdef CONFIG_SMP
-static int get_irq_server(unsigned int virq)
+static int get_irq_server(unsigned int virq, unsigned int strict_check)
 {
-	unsigned int server;
+	int server;
 	/* For the moment only implement delivery to all cpus or one cpu */
 	cpumask_t cpumask = irq_desc[virq].affinity;
 	cpumask_t tmp = CPU_MASK_NONE;
@@ -166,22 +166,25 @@ static int get_irq_server(unsigned int v
 	if (!distribute_irqs)
 		return default_server;
 
-	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
-		server = default_distrib_server;
-	} else {
+	if (!cpus_equal(cpumask, CPU_MASK_ALL)) {
 		cpus_and(tmp, cpu_online_map, cpumask);
 
-		if (cpus_empty(tmp))
-			server = default_distrib_server;
-		else
-			server = get_hard_smp_processor_id(first_cpu(tmp));
+		server = first_cpu(tmp);
+
+		if (server < NR_CPUS)
+			return get_hard_smp_processor_id(server);
+
+		if (strict_check)
+			return -1;
 	}
 
-	return server;
+	if (cpus_equal(cpu_online_map, cpu_present_map))
+		return default_distrib_server;
 
+	return default_server;
 }
 #else
-static int get_irq_server(unsigned int virq)
+static int get_irq_server(unsigned int virq, unsigned int strict_check)
 {
 	return default_server;
 }
@@ -192,7 +195,7 @@ static void xics_unmask_irq(unsigned int
 {
 	unsigned int irq;
 	int call_status;
-	unsigned int server;
+	int server;
 
 	pr_debug("xics: unmask virq %d\n", virq);
 
@@ -201,7 +204,7 @@ static void xics_unmask_irq(unsigned int
 	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
 		return;
 
-	server = get_irq_server(virq);
+	server = get_irq_server(virq, 0);
 
 	call_status = rtas_call(ibm_set_xive, 3, 1, NULL, irq, server,
 				DEFAULT_PRIORITY);
@@ -398,8 +401,7 @@ static void xics_set_affinity(unsigned i
 	unsigned int irq;
 	int status;
 	int xics_status[2];
-	unsigned long newmask;
-	cpumask_t tmp = CPU_MASK_NONE;
+	int irq_server;
 
 	irq = (unsigned int)irq_map[virq].hwirq;
 	if (irq == XICS_IPI || irq == XICS_IRQ_SPURIOUS)
@@ -413,18 +415,21 @@ static void xics_set_affinity(unsigned i
 		return;
 	}
 
-	/* For the moment only implement delivery to all cpus or one cpu */
-	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
-		newmask = default_distrib_server;
-	} else {
-		cpus_and(tmp, cpu_online_map, cpumask);
-		if (cpus_empty(tmp))
-			return;
-		newmask = get_hard_smp_processor_id(first_cpu(tmp));
+	/*
+	 * For the moment only implement delivery to all cpus or one cpu.
+	 * Get current irq_server for the given irq
+	 */
+	irq_server = get_irq_server(irq, 1);
+	if (irq_server == -1) {
+		char cpulist[128];
+		cpumask_scnprintf(cpulist, sizeof(cpulist), cpumask);
+		printk(KERN_WARNING "xics_set_affinity: No online cpus in "
+				"the mask %s for irq %d\n", cpulist, virq);
+		return;
 	}
 
 	status = rtas_call(ibm_set_xive, 3, 1, NULL,
-				irq, newmask, xics_status[1]);
+				irq, irq_server, xics_status[1]);
 
 	if (status) {
 		printk(KERN_ERR "xics_set_affinity: irq=%u ibm,set-xive "

Re: [PATCH] Fix interrupt distribution in ppc970

[Milton Miller]

On Jun 12, 2007, at 9:51 AM, Mohan Kumar M wrote:
>
> Attached patch uses cpumask_scnprintf function.


> In some of the PPC970 based systems, interrupt would be distributed to
> offline cpus also even when booted with "maxcpus=1". So check whether
> cpu online map and cpu present map are equal or not. If they are equal
> default_distrib_server is used as interrupt server otherwise boot cpu
> (default_server) used as interrupt server.
>
> In addition to this, if an interrupt is assigned to a specific cpu (ie
> smp affinity) and if that cpu is not online, the earlier code used to
> return the default_distrib_server as interrupt server. This patch
> introduces an additional paramter to the get_irq function ie
> strict_check, based on this parameter, if the cpu is not online either
> default_distrib_server or -1 is returned.
>
> Cc: Milton Miller <miltonm@bga.com>,
>     Michael Ellerman <michael@ellerman.id.au>
> Signed-off-by: Mohan Kumar M <mohan@in.ibm.com>
>

Acked-by: Milton Miller <miltonm@bga.com>

Re: [Fastboot] [PATCH] Fix interrupt distribution in ppc970

[Vivek Goyal]

On Tue, Mar 06, 2007 at 03:16:55PM +0100, Michael Ellerman wrote:
> On Tue, 2007-03-06 at 19:27 +0530, Mohan Kumar M wrote:
> > Hi,
> > 
> > Here comes the revised version of patch to fix the interrupt missing
> > problem when a kdump kernel is booted with "maxcpus=1" kernel parameter.
> > 
> > In the xics initialization code a check is made to detemine whether
> > maxcpus kernel parameter is present and if its present then
> > default_distrib_server variable is initialized to the current boot cpu
> > id (by default_server variable). So that when ever a kernel is booted
> > with maxcpus kernel parameter all interrupts are routed to the boot cpu
> > only.
> > 
> > Tested on POWER5 and JS20 systems.
> 
> First, I don't know why we keep telling people to use maxcpus=1 for
> kexec/kdump - it's causing bugs, and I don't know of any that it fixes?
> 

Logically speaking, there is no need to bring up all the cpus in the
system to capture the dump. A single cpu can do the job, may be in
relatively lesser memory. Just because we see bugs with maxcpus=1, does
not mean we should try to bring up all the cpus in second kernel. I think
we should try to clean maxcpus=1 path instead.

Thanks
Vivek

Re: [Fastboot] [PATCH] Fix interrupt distribution in ppc970

[Michael Ellerman]

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

On Wed, 2007-03-07 at 11:36 +0530, Vivek Goyal wrote:
> On Tue, Mar 06, 2007 at 03:16:55PM +0100, Michael Ellerman wrote:
> > On Tue, 2007-03-06 at 19:27 +0530, Mohan Kumar M wrote:
> > > Hi,
> > > 
> > > Here comes the revised version of patch to fix the interrupt missing
> > > problem when a kdump kernel is booted with "maxcpus=1" kernel parameter.
> > > 
> > > In the xics initialization code a check is made to detemine whether
> > > maxcpus kernel parameter is present and if its present then
> > > default_distrib_server variable is initialized to the current boot cpu
> > > id (by default_server variable). So that when ever a kernel is booted
> > > with maxcpus kernel parameter all interrupts are routed to the boot cpu
> > > only.
> > > 
> > > Tested on POWER5 and JS20 systems.
> > 
> > First, I don't know why we keep telling people to use maxcpus=1 for
> > kexec/kdump - it's causing bugs, and I don't know of any that it fixes?
> > 
> 
> Logically speaking, there is no need to bring up all the cpus in the
> system to capture the dump. A single cpu can do the job, may be in
> relatively lesser memory. Just because we see bugs with maxcpus=1, does
> not mean we should try to bring up all the cpus in second kernel. I think
> we should try to clean maxcpus=1 path instead.

Sure it'd be nice. But in reality I think the memory reductions are
miniscule, it's still an SMP kernel, it still allocates per-cpu data for
every cpu, they're still spinning. The only thing that maxcpus does is
not bring them online ... and cause lots of bugs.

But the distros have made the call, so I'll just shutup now :)

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 interrupt distribution in ppc970

[Nathan Lynch]

Mohan Kumar M wrote:
> Hi,
> 
> Here comes the revised version of patch to fix the interrupt missing
> problem when a kdump kernel is booted with "maxcpus=1" kernel parameter.
> 
> In the xics initialization code a check is made to detemine whether
> maxcpus kernel parameter is present and if its present then
> default_distrib_server variable is initialized to the current boot cpu
> id (by default_server variable). So that when ever a kernel is booted
> with maxcpus kernel parameter all interrupts are routed to the boot cpu
> only.
> 
> Tested on POWER5 and JS20 systems.
> 
> Any comment?

Was the root cause of this problem ever determined?  It sure looked
like a firmware or hardware problem since it's known to occur only on
JS20.

Re: [PATCH] Fix interrupt distribution in ppc970

[Mohan Kumar M]

On Tue, Mar 06, 2007 at 04:05:46PM -0600, Nathan Lynch wrote:
> Was the root cause of this problem ever determined?  It sure looked
> like a firmware or hardware problem since it's known to occur only on
> JS20.

When I am trying to boot the kernel with maxcpus=1 from yaboot every
thing works fine. I can recreate this problem only with kexec/kdump
booting. So it may not be a firmware issue.

There may be a problem in the device tree that we pass from first kernel
to kexec'ed kernels?

Regards,
Mohan.

Re: [PATCH] Fix interrupt distribution in ppc970

[Benjamin Herrenschmidt]

On Tue, 2007-03-06 at 16:05 -0600, Nathan Lynch wrote:
> Mohan Kumar M wrote:
> > Hi,
> > 
> > Here comes the revised version of patch to fix the interrupt missing
> > problem when a kdump kernel is booted with "maxcpus=1" kernel parameter.
> > 
> > In the xics initialization code a check is made to detemine whether
> > maxcpus kernel parameter is present and if its present then
> > default_distrib_server variable is initialized to the current boot cpu
> > id (by default_server variable). So that when ever a kernel is booted
> > with maxcpus kernel parameter all interrupts are routed to the boot cpu
> > only.
> > 
> > Tested on POWER5 and JS20 systems.
> > 
> > Any comment?
> 
> Was the root cause of this problem ever determined?  It sure looked
> like a firmware or hardware problem since it's known to occur only on
> JS20.

js20 and js21 have in common that the firmware "emulates" the XICS on
top of an MPIC, which can cause interesting issues ... like the
requirement of issuing EOI's from the CPU that got the interrupt, among
ohters.

Ben.

Re: [PATCH] Fix interrupt distribution in ppc970

[Mohan Kumar M]

On Wed, Mar 07, 2007 at 09:52:47AM +0100, Benjamin Herrenschmidt wrote:
> On Tue, 2007-03-06 at 16:05 -0600, Nathan Lynch wrote:
> > Was the root cause of this problem ever determined?  It sure looked
> > like a firmware or hardware problem since it's known to occur only on
> > JS20.
> 
> js20 and js21 have in common that the firmware "emulates" the XICS on
> top of an MPIC, which can cause interesting issues ... like the
> requirement of issuing EOI's from the CPU that got the interrupt, among
> ohters.
>

There is no interrupt missing problems in JS21. Only JS20 boxes face this
problem. The only difference between JS20 and JS21 is that JS20 does not
have Global Interrupt Queue while JS21 has GIQ.

Regards,
Mohan.