[PATCH] ARM: update show_interrupts for online cpu's only

[PATCH] ARM: update show_interrupts for online cpu's only

[Santosh Shilimkar]

Even after CPUx is logically offline, it's interrupt are displayed
in proc entry

This patch fixes the same by use of 'for_each_online_cpu' instead
of 'for_each_present_cpu' in 'show_interrupts' function

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/kernel/irq.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
index 3b3d2c8..cfab369 100644
--- a/arch/arm/kernel/irq.c
+++ b/arch/arm/kernel/irq.c
@@ -60,7 +60,7 @@ int show_interrupts(struct seq_file *p, void *v)
 		char cpuname[12];
 
 		seq_printf(p, "    ");
-		for_each_present_cpu(cpu) {
+		for_each_online_cpu(cpu) {
 			sprintf(cpuname, "CPU%d", cpu);
 			seq_printf(p, " %10s", cpuname);
 		}
@@ -74,7 +74,7 @@ int show_interrupts(struct seq_file *p, void *v)
 			goto unlock;
 
 		seq_printf(p, "%3d: ", i);
-		for_each_present_cpu(cpu)
+		for_each_online_cpu(cpu)
 			seq_printf(p, "%10u ", kstat_irqs_cpu(i, cpu));
 		seq_printf(p, " %10s", irq_desc[i].chip->name ? : "-");
 		seq_printf(p, "  %s", action->name);
-- 
1.6.0.4

[PATCH] ARM: update show_interrupts for online cpu's only

[Russell King - ARM Linux]

On Sat, Jul 31, 2010 at 07:38:53PM +0530, Santosh Shilimkar wrote:
> Even after CPUx is logically offline, it's interrupt are displayed
> in proc entry
> 
> This patch fixes the same by use of 'for_each_online_cpu' instead
> of 'for_each_present_cpu' in 'show_interrupts' function

What's the synchronisation mechanism between CPUs being brought on or
offline and this file being read?

I believe we have a more dynamic approach to CPU hotplug (using it for
dynamic power saving) than x86, so I believe hotplug events can happen
more often on ARM.

[PATCH] ARM: update show_interrupts for online cpu's only

[Shilimkar, Santosh]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Saturday, July 31, 2010 8:06 PM
> To: Shilimkar, Santosh
> Cc: linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH] ARM: update show_interrupts for online cpu's only
> 
> On Sat, Jul 31, 2010 at 07:38:53PM +0530, Santosh Shilimkar wrote:
> > Even after CPUx is logically offline, it's interrupt are displayed
> > in proc entry
> >
> > This patch fixes the same by use of 'for_each_online_cpu' instead
> > of 'for_each_present_cpu' in 'show_interrupts' function
> 
> What's the synchronisation mechanism between CPUs being brought on or
> offline and this file being read?
>
Isn't that taken care by "cpu_online_mask" mask ?
 
> I believe we have a more dynamic approach to CPU hotplug (using it for
> dynamic power saving) than x86, so I believe hotplug events can happen
> more often on ARM.
Most likely yes.

[PATCH] ARM: update show_interrupts for online cpu's only

[Russell King - ARM Linux]

On Sat, Jul 31, 2010 at 08:18:05PM +0530, Shilimkar, Santosh wrote:
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> > Sent: Saturday, July 31, 2010 8:06 PM
> > To: Shilimkar, Santosh
> > Cc: linux-arm-kernel at lists.infradead.org
> > Subject: Re: [PATCH] ARM: update show_interrupts for online cpu's only
> > 
> > On Sat, Jul 31, 2010 at 07:38:53PM +0530, Santosh Shilimkar wrote:
> > > Even after CPUx is logically offline, it's interrupt are displayed
> > > in proc entry
> > >
> > > This patch fixes the same by use of 'for_each_online_cpu' instead
> > > of 'for_each_present_cpu' in 'show_interrupts' function
> > 
> > What's the synchronisation mechanism between CPUs being brought on or
> > offline and this file being read?
> >
> Isn't that taken care by "cpu_online_mask" mask ?

The problem comes when you have:

	for_each_online_cpu(cpu)
		do_something(cpu);

	/* cpu goes offline */

	for_each_online_cpu(cpu)
		do_something_else(cpu);

This causes first iterates over more CPUs than the second.  When you
consider we iterate over the online CPU mask for each interrupt, which
could cross a read() call, it's possible that a CPU can go offline
in the middle of reading /proc/interrupts.

The real question is whether this matters.  If you're reading this
file as part of a daemon which is controlling the hotplug CPUs based
on interrupt load, then probably not.

If you're reading this as part of a daemon involved with balancing IRQs
across several CPUs, and CPU2 of 4 CPUs goes offline, you could get
confused (but hopefully your parser is good enough to spot the format
change and signal an exception.)

I feel rather uneasy about files which can spontaneously change format
on a reader at any time.

[PATCH] ARM: update show_interrupts for online cpu's only

[Shilimkar, Santosh]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Saturday, July 31, 2010 8:35 PM
> To: Shilimkar, Santosh
> Cc: linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH] ARM: update show_interrupts for online cpu's only
> 
> On Sat, Jul 31, 2010 at 08:18:05PM +0530, Shilimkar, Santosh wrote:
> > > -----Original Message-----
> > > From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> > > Sent: Saturday, July 31, 2010 8:06 PM
> > > To: Shilimkar, Santosh
> > > Cc: linux-arm-kernel at lists.infradead.org
> > > Subject: Re: [PATCH] ARM: update show_interrupts for online cpu's only
> > >
> > > On Sat, Jul 31, 2010 at 07:38:53PM +0530, Santosh Shilimkar wrote:
> > > > Even after CPUx is logically offline, it's interrupt are displayed
> > > > in proc entry
> > > >
> > > > This patch fixes the same by use of 'for_each_online_cpu' instead
> > > > of 'for_each_present_cpu' in 'show_interrupts' function
> > >
> > > What's the synchronisation mechanism between CPUs being brought on or
> > > offline and this file being read?
> > >
> > Isn't that taken care by "cpu_online_mask" mask ?
> 
> The problem comes when you have:
> 
> 	for_each_online_cpu(cpu)
> 		do_something(cpu);
> 
> 	/* cpu goes offline */
> 
> 	for_each_online_cpu(cpu)
> 		do_something_else(cpu);
> 
> This causes first iterates over more CPUs than the second.  When you
> consider we iterate over the online CPU mask for each interrupt, which
> could cross a read() call, it's possible that a CPU can go offline
> in the middle of reading /proc/interrupts.
> 
> The real question is whether this matters.  If you're reading this
> file as part of a daemon which is controlling the hotplug CPUs based
> on interrupt load, then probably not.
> 
> If you're reading this as part of a daemon involved with balancing IRQs
> across several CPUs, and CPU2 of 4 CPUs goes offline, you could get
> confused (but hopefully your parser is good enough to spot the format
> change and signal an exception.)
> 
> I feel rather uneasy about files which can spontaneously change format
> on a reader at any time.
I see your point. Thanks for clarifying it.
Do you think this change is such a problem? 
I just though from correctness point of view and if daemons
like irqbalance depend on this entry, it's likely going to get
confused.

Regards,
Santosh