linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] irqbalance, powerpc: add IRQs without settable SMP affinity to banned list
@ 2010-09-22  6:04 Michael Neuling
  2010-09-23  8:22 ` Michael Ellerman
  0 siblings, 1 reply; 7+ messages in thread
From: Michael Neuling @ 2010-09-22  6:04 UTC (permalink / raw)
  To: Arjen Van De Ven, Neil Horman; +Cc: linuxppc-dev, linux-kernel

On pseries powerpc, IPIs are registered with an IRQ number so
/proc/interrupts looks like this on a 2 core/2 thread machine:

           CPU0       CPU1       CPU2       CPU3
 16:    3164282    3290514    1138794     983121   XICS             Level        IPI
 18:    2605674          0     304994          0   XICS             Level        lan0
 30:     400057          0     169209          0   XICS             Level        ibmvscsi
LOC:     133734      77250     106425      91951   Local timer interrupts
SPU:          0          0          0          0   Spurious interrupts
CNT:          0          0          0          0   Performance monitoring interrupts
MCE:          0          0          0          0   Machine check exceptions

Unfortunately this means irqbalance attempts to set the affinity of IPIs
which is not possible.  So in the above case, when irqbalance is in
performance mode due to heavy IPI, lan0 and ibmvscsi activity, it
sometimes attempts to put the IPIs on one core (CPU0&1) and lan0 and
ibmvscsi on the other core (CPU2&3).  This is suboptimal as we want lan0
and ibmvscsi to be on separate cores and IPIs to be ignored.

When irqblance attempts writes to the IPI smp_affinity (ie.
/proc/irq/16/smp_affinity in the above example) it fails but irqbalance
ignores currently ignores this.

This patch catches these write fails and in this case adds that IRQ
number to the banned IRQ list.  This will catch the above IPI case and
any other IRQ where the SMP affinity can't be set.

Tested on POWER6, POWER7 and x86.

Signed-off-by: Michael Neuling <mikey@neuling.org>

Index: irqbalance/irqlist.c
===================================================================
--- irqbalance.orig/irqlist.c
+++ irqbalance/irqlist.c
@@ -67,7 +67,7 @@
 	DIR *dir;
 	struct dirent *entry;
 	char *c, *c2;
-	int nr , count = 0;
+	int nr , count = 0, can_set = 1;
 	char buf[PATH_MAX];
 	sprintf(buf, "/proc/irq/%i", number);
 	dir = opendir(buf);
@@ -80,7 +80,7 @@
 			size_t size = 0;
 			FILE *file;
 			sprintf(buf, "/proc/irq/%i/smp_affinity", number);
-			file = fopen(buf, "r");
+			file = fopen(buf, "r+");
 			if (!file)
 				continue;
 			if (getline(&line, &size, file)==0) {
@@ -89,7 +89,14 @@
 				continue;
 			}
 			cpumask_parse_user(line, strlen(line), irq->mask);
-			fclose(file);
+			/*
+			 * Check that we can write the affinity, if
+			 * not take it out of the list.
+			 */
+			if (fwrite(line, strlen(line) - 1, 1, file) == 0)
+				can_set = 0;
+			if (fclose(file))
+				can_set = 0;
 			free(line);
 		} else if (strcmp(entry->d_name,"allowed_affinity")==0) {
 			char *line = NULL;
@@ -122,7 +129,7 @@
 			count++;
 
 	/* if there is no choice in the allowed mask, don't bother to balance */
-	if (count<2)
+	if ((count<2) || (can_set == 0))
 		 irq->balance_level = BALANCE_NONE;
 		
 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] irqbalance, powerpc: add IRQs without settable SMP affinity to banned list
  2010-09-22  6:04 [PATCH] irqbalance, powerpc: add IRQs without settable SMP affinity to banned list Michael Neuling
@ 2010-09-23  8:22 ` Michael Ellerman
  2010-09-23 10:57   ` Michael Neuling
  0 siblings, 1 reply; 7+ messages in thread
From: Michael Ellerman @ 2010-09-23  8:22 UTC (permalink / raw)
  To: Michael Neuling; +Cc: linuxppc-dev, Arjen Van De Ven, linux-kernel, Neil Horman

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

On Wed, 2010-09-22 at 16:04 +1000, Michael Neuling wrote:
> When irqblance attempts writes to the IPI smp_affinity (ie.
> /proc/irq/16/smp_affinity in the above example) it fails but irqbalance
> ignores currently ignores this.
> 
> This patch catches these write fails and in this case adds that IRQ
> number to the banned IRQ list.  This will catch the above IPI case and
> any other IRQ where the SMP affinity can't be set.

Cool!

> Index: irqbalance/irqlist.c
> ===================================================================
> --- irqbalance.orig/irqlist.c
> +++ irqbalance/irqlist.c
> @@ -67,7 +67,7 @@
>  	DIR *dir;
>  	struct dirent *entry;
>  	char *c, *c2;
> -	int nr , count = 0;
> +	int nr , count = 0, can_set = 1;
>  	char buf[PATH_MAX];
>  	sprintf(buf, "/proc/irq/%i", number);
>  	dir = opendir(buf);
> @@ -80,7 +80,7 @@
>  			size_t size = 0;
>  			FILE *file;
>  			sprintf(buf, "/proc/irq/%i/smp_affinity", number);
> -			file = fopen(buf, "r");
> +			file = fopen(buf, "r+");
>  			if (!file)
>  				continue;
>  			if (getline(&line, &size, file)==0) {
> @@ -89,7 +89,14 @@
>  				continue;
>  			}
>  			cpumask_parse_user(line, strlen(line), irq->mask);
> -			fclose(file);
> +			/*
> +			 * Check that we can write the affinity, if
> +			 * not take it out of the list.
> +			 */
> +			if (fwrite(line, strlen(line) - 1, 1, file) == 0)

if (fputs(line, file) == EOF)

?

cheers


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] irqbalance, powerpc: add IRQs without settable SMP affinity to banned list
  2010-09-23  8:22 ` Michael Ellerman
@ 2010-09-23 10:57   ` Michael Neuling
  2010-09-23 13:13     ` Neil Horman
  0 siblings, 1 reply; 7+ messages in thread
From: Michael Neuling @ 2010-09-23 10:57 UTC (permalink / raw)
  To: michael
  Cc: nhorman, Neil Horman, linux-kernel, linuxppc-dev,
	Arjen Van De Ven, arjan


> > +		if (fwrite(line, strlen(line) - 1, 1, file) == 0)
> 
> if (fputs(line, file) == EOF)

Good point thanks... new patch below

Mikey

irqbalance, powerpc: add IRQs without settable SMP affinity to banned list

On pseries powerpc, IPIs are registered with an IRQ number so
/proc/interrupts looks like this on a 2 core/2 thread machine:

           CPU0       CPU1       CPU2       CPU3
 16:    3164282    3290514    1138794     983121   XICS             Level        IPI
 18:    2605674          0     304994          0   XICS             Level        lan0
 30:     400057          0     169209          0   XICS             Level        ibmvscsi
LOC:     133734      77250     106425      91951   Local timer interrupts
SPU:          0          0          0          0   Spurious interrupts
CNT:          0          0          0          0   Performance monitoring interrupts
MCE:          0          0          0          0   Machine check exceptions

Unfortunately this means irqbalance attempts to set the affinity of IPIs
which is not possible.  So in the above case, when irqbalance is in
performance mode due to heavy IPI, lan0 and ibmvscsi activity, it
sometimes attempts to put the IPIs on one core (CPU0&1) and lan0 and
ibmvscsi on the other core (CPU2&3).  This is suboptimal as we want lan0
and ibmvscsi to be on separate cores and IPIs to be ignored.

When irqblance attempts writes to the IPI smp_affinity (ie.
/proc/irq/16/smp_affinity in the above example) it fails but irqbalance
ignores currently ignores this.

This patch catches these write fails and in this case adds that IRQ
number to the banned IRQ list.  This will catch the above IPI case and
any other IRQ where the SMP affinity can't be set.

Tested on POWER6, POWER7 and x86.

Signed-off-by: Michael Neuling <mikey@neuling.org>

Index: irqbalance/irqlist.c
===================================================================
--- irqbalance.orig/irqlist.c
+++ irqbalance/irqlist.c
@@ -67,7 +67,7 @@
 	DIR *dir;
 	struct dirent *entry;
 	char *c, *c2;
-	int nr , count = 0;
+	int nr , count = 0, can_set = 1;
 	char buf[PATH_MAX];
 	sprintf(buf, "/proc/irq/%i", number);
 	dir = opendir(buf);
@@ -80,7 +80,7 @@
 			size_t size = 0;
 			FILE *file;
 			sprintf(buf, "/proc/irq/%i/smp_affinity", number);
-			file = fopen(buf, "r");
+			file = fopen(buf, "r+");
 			if (!file)
 				continue;
 			if (getline(&line, &size, file)==0) {
@@ -89,7 +89,14 @@
 				continue;
 			}
 			cpumask_parse_user(line, strlen(line), irq->mask);
-			fclose(file);
+			/*
+			 * Check that we can write the affinity, if
+			 * not take it out of the list.
+			 */
+			if (fputs(line, file) == EOF)
+				can_set = 0;
+			if (fclose(file))
+				can_set = 0;
 			free(line);
 		} else if (strcmp(entry->d_name,"allowed_affinity")==0) {
 			char *line = NULL;
@@ -122,7 +129,7 @@
 			count++;
 
 	/* if there is no choice in the allowed mask, don't bother to balance */
-	if (count<2)
+	if ((count<2) || (can_set == 0))
 		 irq->balance_level = BALANCE_NONE;
 		
 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] irqbalance, powerpc: add IRQs without settable SMP affinity to banned list
  2010-09-23 10:57   ` Michael Neuling
@ 2010-09-23 13:13     ` Neil Horman
  2010-09-24  5:03       ` Michael Ellerman
  0 siblings, 1 reply; 7+ messages in thread
From: Neil Horman @ 2010-09-23 13:13 UTC (permalink / raw)
  To: Michael Neuling
  Cc: linuxppc-dev, linux-kernel, Neil Horman, Arjen Van De Ven, arjan

On Thu, Sep 23, 2010 at 08:57:20PM +1000, Michael Neuling wrote:
> 
> > > +		if (fwrite(line, strlen(line) - 1, 1, file) == 0)
> > 
> > if (fputs(line, file) == EOF)
> 
> Good point thanks... new patch below
> 
> Mikey
> 
> irqbalance, powerpc: add IRQs without settable SMP affinity to banned list
> 
> On pseries powerpc, IPIs are registered with an IRQ number so
> /proc/interrupts looks like this on a 2 core/2 thread machine:
> 
>            CPU0       CPU1       CPU2       CPU3
>  16:    3164282    3290514    1138794     983121   XICS             Level        IPI
>  18:    2605674          0     304994          0   XICS             Level        lan0
>  30:     400057          0     169209          0   XICS             Level        ibmvscsi
> LOC:     133734      77250     106425      91951   Local timer interrupts
> SPU:          0          0          0          0   Spurious interrupts
> CNT:          0          0          0          0   Performance monitoring interrupts
> MCE:          0          0          0          0   Machine check exceptions
> 
> Unfortunately this means irqbalance attempts to set the affinity of IPIs
> which is not possible.  So in the above case, when irqbalance is in
> performance mode due to heavy IPI, lan0 and ibmvscsi activity, it
> sometimes attempts to put the IPIs on one core (CPU0&1) and lan0 and
> ibmvscsi on the other core (CPU2&3).  This is suboptimal as we want lan0
> and ibmvscsi to be on separate cores and IPIs to be ignored.
> 
> When irqblance attempts writes to the IPI smp_affinity (ie.
> /proc/irq/16/smp_affinity in the above example) it fails but irqbalance
> ignores currently ignores this.
> 
> This patch catches these write fails and in this case adds that IRQ
> number to the banned IRQ list.  This will catch the above IPI case and
> any other IRQ where the SMP affinity can't be set.
> 
> Tested on POWER6, POWER7 and x86.
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> 
> Index: irqbalance/irqlist.c
> ===================================================================
> --- irqbalance.orig/irqlist.c
> +++ irqbalance/irqlist.c
> @@ -67,7 +67,7 @@
>  	DIR *dir;
>  	struct dirent *entry;
>  	char *c, *c2;
> -	int nr , count = 0;
> +	int nr , count = 0, can_set = 1;
>  	char buf[PATH_MAX];
>  	sprintf(buf, "/proc/irq/%i", number);
>  	dir = opendir(buf);
> @@ -80,7 +80,7 @@
>  			size_t size = 0;
>  			FILE *file;
>  			sprintf(buf, "/proc/irq/%i/smp_affinity", number);
> -			file = fopen(buf, "r");
> +			file = fopen(buf, "r+");
>  			if (!file)
>  				continue;
>  			if (getline(&line, &size, file)==0) {
> @@ -89,7 +89,14 @@
>  				continue;
>  			}
>  			cpumask_parse_user(line, strlen(line), irq->mask);
> -			fclose(file);
> +			/*
> +			 * Check that we can write the affinity, if
> +			 * not take it out of the list.
> +			 */
> +			if (fputs(line, file) == EOF)
> +				can_set = 0;
This is maybe a nit, but writing to the affinity file can fail for a few
different reasons, some of them permanent, some transient.  For instance, if
we're in a memory constrained condition temporarily irq_affinity_proc_write
might return -ENOMEM.  Might it be better to modify this code so that, instead
of using fputs to merge the various errors into an EOF, we use some other write
method that lets us better determine the error and selectively ban the interrupt
only for those errors which we consider permanent?

Otherwise this looks fine to me.

Thanks
Neil

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] irqbalance, powerpc: add IRQs without settable SMP affinity to banned list
  2010-09-23 13:13     ` Neil Horman
@ 2010-09-24  5:03       ` Michael Ellerman
  2010-09-24  6:56         ` Michael Neuling
  0 siblings, 1 reply; 7+ messages in thread
From: Michael Ellerman @ 2010-09-24  5:03 UTC (permalink / raw)
  To: Neil Horman
  Cc: Michael Neuling, linuxppc-dev, linux-kernel, Neil Horman,
	Arjen Van De Ven, arjan

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

On Thu, 2010-09-23 at 09:13 -0400, Neil Horman wrote:
> On Thu, Sep 23, 2010 at 08:57:20PM +1000, Michael Neuling wrote:
> > 
> > > > +		if (fwrite(line, strlen(line) - 1, 1, file) == 0)
> > > 
> > > if (fputs(line, file) == EOF)
> > 
> > Good point thanks... new patch below
> > 
> > Mikey
> > 
> > irqbalance, powerpc: add IRQs without settable SMP affinity to banned list
> > 
> > On pseries powerpc, IPIs are registered with an IRQ number so
> > /proc/interrupts looks like this on a 2 core/2 thread machine:
> > 
> >            CPU0       CPU1       CPU2       CPU3
> >  16:    3164282    3290514    1138794     983121   XICS             Level        IPI
> >  18:    2605674          0     304994          0   XICS             Level        lan0
> >  30:     400057          0     169209          0   XICS             Level        ibmvscsi
> > LOC:     133734      77250     106425      91951   Local timer interrupts
> > SPU:          0          0          0          0   Spurious interrupts
> > CNT:          0          0          0          0   Performance monitoring interrupts
> > MCE:          0          0          0          0   Machine check exceptions
> > 
> > Unfortunately this means irqbalance attempts to set the affinity of IPIs
> > which is not possible.  So in the above case, when irqbalance is in
> > performance mode due to heavy IPI, lan0 and ibmvscsi activity, it
> > sometimes attempts to put the IPIs on one core (CPU0&1) and lan0 and
> > ibmvscsi on the other core (CPU2&3).  This is suboptimal as we want lan0
> > and ibmvscsi to be on separate cores and IPIs to be ignored.
> > 
> > When irqblance attempts writes to the IPI smp_affinity (ie.
> > /proc/irq/16/smp_affinity in the above example) it fails but irqbalance
> > ignores currently ignores this.
> > 
> > This patch catches these write fails and in this case adds that IRQ
> > number to the banned IRQ list.  This will catch the above IPI case and
> > any other IRQ where the SMP affinity can't be set.
> > 
> > Tested on POWER6, POWER7 and x86.
> > 
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > 
> > Index: irqbalance/irqlist.c
> > ===================================================================
> > --- irqbalance.orig/irqlist.c
> > +++ irqbalance/irqlist.c
> > @@ -67,7 +67,7 @@
> >  	DIR *dir;
> >  	struct dirent *entry;
> >  	char *c, *c2;
> > -	int nr , count = 0;
> > +	int nr , count = 0, can_set = 1;
> >  	char buf[PATH_MAX];
> >  	sprintf(buf, "/proc/irq/%i", number);
> >  	dir = opendir(buf);
> > @@ -80,7 +80,7 @@
> >  			size_t size = 0;
> >  			FILE *file;
> >  			sprintf(buf, "/proc/irq/%i/smp_affinity", number);
> > -			file = fopen(buf, "r");
> > +			file = fopen(buf, "r+");
> >  			if (!file)
> >  				continue;
> >  			if (getline(&line, &size, file)==0) {
> > @@ -89,7 +89,14 @@
> >  				continue;
> >  			}
> >  			cpumask_parse_user(line, strlen(line), irq->mask);
> > -			fclose(file);
> > +			/*
> > +			 * Check that we can write the affinity, if
> > +			 * not take it out of the list.
> > +			 */
> > +			if (fputs(line, file) == EOF)
> > +				can_set = 0;

> This is maybe a nit, but writing to the affinity file can fail for a few
> different reasons, some of them permanent, some transient.  For instance, if
> we're in a memory constrained condition temporarily irq_affinity_proc_write
> might return -ENOMEM.  

Yeah true, usually followed shortly by your kernel going so far into
swap you never get it back, or OOMing, but I guess it's possible.

> Might it be better to modify this code so that, instead
> of using fputs to merge the various errors into an EOF, we use some other write
> method that lets us better determine the error and selectively ban the interrupt
> only for those errors which we consider permanent?

Yep. It seems fputs() gives you know way to get the actual error from
write(), so it looks we'll need to switch to open/write, but that's
probably not so terrible.

cheers


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] irqbalance, powerpc: add IRQs without settable SMP affinity to banned list
  2010-09-24  5:03       ` Michael Ellerman
@ 2010-09-24  6:56         ` Michael Neuling
  2010-09-24 10:37           ` Neil Horman
  0 siblings, 1 reply; 7+ messages in thread
From: Michael Neuling @ 2010-09-24  6:56 UTC (permalink / raw)
  To: michael
  Cc: Neil Horman, linuxppc-dev, linux-kernel, Neil Horman,
	Arjen Van De Ven, arjan


> > >  			size_t size =3D 0;
> > >  			FILE *file;
> > >  			sprintf(buf, "/proc/irq/%i/smp_affinity", number);
> > > -			file =3D fopen(buf, "r");
> > > +			file =3D fopen(buf, "r+");
> > >  			if (!file)
> > >  				continue;
> > >  			if (getline(&line, &size, file)=3D=3D0) {
> > > @@ -89,7 +89,14 @@
> > >  				continue;
> > >  			}
> > >  			cpumask_parse_user(line, strlen(line), irq->mask);
> > > -			fclose(file);
> > > +			/*
> > > +			 * Check that we can write the affinity, if
> > > +			 * not take it out of the list.
> > > +			 */
> > > +			if (fputs(line, file) =3D=3D EOF)
> > > +				can_set =3D 0;
> 
> > This is maybe a nit, but writing to the affinity file can fail for a few
> > different reasons, some of them permanent, some transient.  For instance,=
>  if
> > we're in a memory constrained condition temporarily irq_affinity_proc_wri=
> te
> > might return -ENOMEM. =20
> 
> Yeah true, usually followed shortly by your kernel going so far into
> swap you never get it back, or OOMing, but I guess it's possible.
> 
> > Might it be better to modify this code so that, instead
> > of using fputs to merge the various errors into an EOF, we use some other=
>  write
> > method that lets us better determine the error and selectively ban the in=
> terrupt
> > only for those errors which we consider permanent?
> 
> Yep. It seems fputs() gives you know way to get the actual error from
> write(), so it looks we'll need to switch to open/write, but that's
> probably not so terrible.

fclose inherits the error from fputs and it sets errno correctly.  Below
uses this to catch only EIO errors and mark them for the banned list.

Mikey

irqbalance, powerpc: add IRQs without settable SMP affinity to banned list

On pseries powerpc, IPIs are registered with an IRQ number so
/proc/interrupts looks like this on a 2 core/2 thread machine:

           CPU0       CPU1       CPU2       CPU3
 16:    3164282    3290514    1138794     983121   XICS             Level        IPI
 18:    2605674          0     304994          0   XICS             Level        lan0
 30:     400057          0     169209          0   XICS             Level        ibmvscsi
LOC:     133734      77250     106425      91951   Local timer interrupts
SPU:          0          0          0          0   Spurious interrupts
CNT:          0          0          0          0   Performance monitoring interrupts
MCE:          0          0          0          0   Machine check exceptions

Unfortunately this means irqbalance attempts to set the affinity of IPIs
which is not possible.  So in the above case, when irqbalance is in
performance mode due to heavy IPI, lan0 and ibmvscsi activity, it
sometimes attempts to put the IPIs on one core (CPU0&1) and lan0 and
ibmvscsi on the other core (CPU2&3).  This is suboptimal as we want lan0
and ibmvscsi to be on separate cores and IPIs to be ignored.

When irqblance attempts writes to the IPI smp_affinity (ie.
/proc/irq/16/smp_affinity in the above example) it fails with an EIO but
irqbalance currently ignores this.

This patch catches these write fails and in this case adds that IRQ
number to the banned IRQ list.  This will catch the above IPI case and
any other IRQ where the SMP affinity can't be set.

Tested on POWER6, POWER7 and x86.

Signed-off-by: Michael Neuling <mikey@neuling.org>
 
Index: irqbalance/irqlist.c
===================================================================
--- irqbalance.orig/irqlist.c
+++ irqbalance/irqlist.c
@@ -28,6 +28,7 @@
 #include <unistd.h>
 #include <sys/types.h>
 #include <dirent.h>
+#include <errno.h>
 
 #include "types.h"
 #include "irqbalance.h"
@@ -67,7 +68,7 @@
 	DIR *dir;
 	struct dirent *entry;
 	char *c, *c2;
-	int nr , count = 0;
+	int nr , count = 0, can_set = 1;
 	char buf[PATH_MAX];
 	sprintf(buf, "/proc/irq/%i", number);
 	dir = opendir(buf);
@@ -80,7 +81,7 @@
 			size_t size = 0;
 			FILE *file;
 			sprintf(buf, "/proc/irq/%i/smp_affinity", number);
-			file = fopen(buf, "r");
+			file = fopen(buf, "r+");
 			if (!file)
 				continue;
 			if (getline(&line, &size, file)==0) {
@@ -89,7 +90,13 @@
 				continue;
 			}
 			cpumask_parse_user(line, strlen(line), irq->mask);
-			fclose(file);
+			/*
+			 * Check that we can write the affinity, if
+			 * not take it out of the list.
+			 */
+			fputs(line, file);
+			if (fclose(file) && errno == EIO)
+				can_set = 0;
 			free(line);
 		} else if (strcmp(entry->d_name,"allowed_affinity")==0) {
 			char *line = NULL;
@@ -122,7 +129,7 @@
 			count++;
 
 	/* if there is no choice in the allowed mask, don't bother to balance */
-	if (count<2)
+	if ((count<2) || (can_set == 0))
 		 irq->balance_level = BALANCE_NONE;
 		
 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] irqbalance, powerpc: add IRQs without settable SMP affinity to banned list
  2010-09-24  6:56         ` Michael Neuling
@ 2010-09-24 10:37           ` Neil Horman
  0 siblings, 0 replies; 7+ messages in thread
From: Neil Horman @ 2010-09-24 10:37 UTC (permalink / raw)
  To: Michael Neuling
  Cc: linuxppc-dev, linux-kernel, Neil Horman, Arjen Van De Ven, arjan

On Fri, Sep 24, 2010 at 04:56:34PM +1000, Michael Neuling wrote:
> 
> > > >  			size_t size =3D 0;
> > > >  			FILE *file;
> > > >  			sprintf(buf, "/proc/irq/%i/smp_affinity", number);
> > > > -			file =3D fopen(buf, "r");
> > > > +			file =3D fopen(buf, "r+");
> > > >  			if (!file)
> > > >  				continue;
> > > >  			if (getline(&line, &size, file)=3D=3D0) {
> > > > @@ -89,7 +89,14 @@
> > > >  				continue;
> > > >  			}
> > > >  			cpumask_parse_user(line, strlen(line), irq->mask);
> > > > -			fclose(file);
> > > > +			/*
> > > > +			 * Check that we can write the affinity, if
> > > > +			 * not take it out of the list.
> > > > +			 */
> > > > +			if (fputs(line, file) =3D=3D EOF)
> > > > +				can_set =3D 0;
> > 
> > > This is maybe a nit, but writing to the affinity file can fail for a few
> > > different reasons, some of them permanent, some transient.  For instance,=
> >  if
> > > we're in a memory constrained condition temporarily irq_affinity_proc_wri=
> > te
> > > might return -ENOMEM. =20
> > 
> > Yeah true, usually followed shortly by your kernel going so far into
> > swap you never get it back, or OOMing, but I guess it's possible.
> > 
> > > Might it be better to modify this code so that, instead
> > > of using fputs to merge the various errors into an EOF, we use some other=
> >  write
> > > method that lets us better determine the error and selectively ban the in=
> > terrupt
> > > only for those errors which we consider permanent?
> > 
> > Yep. It seems fputs() gives you know way to get the actual error from
> > write(), so it looks we'll need to switch to open/write, but that's
> > probably not so terrible.
> 
> fclose inherits the error from fputs and it sets errno correctly.  Below
> uses this to catch only EIO errors and mark them for the banned list.
> 
> Mikey
> 
> irqbalance, powerpc: add IRQs without settable SMP affinity to banned list
> 
> On pseries powerpc, IPIs are registered with an IRQ number so
> /proc/interrupts looks like this on a 2 core/2 thread machine:
> 
>            CPU0       CPU1       CPU2       CPU3
>  16:    3164282    3290514    1138794     983121   XICS             Level        IPI
>  18:    2605674          0     304994          0   XICS             Level        lan0
>  30:     400057          0     169209          0   XICS             Level        ibmvscsi
> LOC:     133734      77250     106425      91951   Local timer interrupts
> SPU:          0          0          0          0   Spurious interrupts
> CNT:          0          0          0          0   Performance monitoring interrupts
> MCE:          0          0          0          0   Machine check exceptions
> 
> Unfortunately this means irqbalance attempts to set the affinity of IPIs
> which is not possible.  So in the above case, when irqbalance is in
> performance mode due to heavy IPI, lan0 and ibmvscsi activity, it
> sometimes attempts to put the IPIs on one core (CPU0&1) and lan0 and
> ibmvscsi on the other core (CPU2&3).  This is suboptimal as we want lan0
> and ibmvscsi to be on separate cores and IPIs to be ignored.
> 
> When irqblance attempts writes to the IPI smp_affinity (ie.
> /proc/irq/16/smp_affinity in the above example) it fails with an EIO but
> irqbalance currently ignores this.
> 
> This patch catches these write fails and in this case adds that IRQ
> number to the banned IRQ list.  This will catch the above IPI case and
> any other IRQ where the SMP affinity can't be set.
> 
> Tested on POWER6, POWER7 and x86.
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>
>  
> Index: irqbalance/irqlist.c
> ===================================================================
> --- irqbalance.orig/irqlist.c
> +++ irqbalance/irqlist.c
> @@ -28,6 +28,7 @@
>  #include <unistd.h>
>  #include <sys/types.h>
>  #include <dirent.h>
> +#include <errno.h>
>  
>  #include "types.h"
>  #include "irqbalance.h"
> @@ -67,7 +68,7 @@
>  	DIR *dir;
>  	struct dirent *entry;
>  	char *c, *c2;
> -	int nr , count = 0;
> +	int nr , count = 0, can_set = 1;
>  	char buf[PATH_MAX];
>  	sprintf(buf, "/proc/irq/%i", number);
>  	dir = opendir(buf);
> @@ -80,7 +81,7 @@
>  			size_t size = 0;
>  			FILE *file;
>  			sprintf(buf, "/proc/irq/%i/smp_affinity", number);
> -			file = fopen(buf, "r");
> +			file = fopen(buf, "r+");
>  			if (!file)
>  				continue;
>  			if (getline(&line, &size, file)==0) {
> @@ -89,7 +90,13 @@
>  				continue;
>  			}
>  			cpumask_parse_user(line, strlen(line), irq->mask);
> -			fclose(file);
> +			/*
> +			 * Check that we can write the affinity, if
> +			 * not take it out of the list.
> +			 */
> +			fputs(line, file);
> +			if (fclose(file) && errno == EIO)
> +				can_set = 0;
>  			free(line);
>  		} else if (strcmp(entry->d_name,"allowed_affinity")==0) {
>  			char *line = NULL;
> @@ -122,7 +129,7 @@
>  			count++;
>  
>  	/* if there is no choice in the allowed mask, don't bother to balance */
> -	if (count<2)
> +	if ((count<2) || (can_set == 0))
>  		 irq->balance_level = BALANCE_NONE;
>  		
>  
> 
Thank you, this looks good to me, I'll integrate this shortly.
Neil

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2010-09-24 10:40 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-22  6:04 [PATCH] irqbalance, powerpc: add IRQs without settable SMP affinity to banned list Michael Neuling
2010-09-23  8:22 ` Michael Ellerman
2010-09-23 10:57   ` Michael Neuling
2010-09-23 13:13     ` Neil Horman
2010-09-24  5:03       ` Michael Ellerman
2010-09-24  6:56         ` Michael Neuling
2010-09-24 10:37           ` Neil Horman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).