[PATCH] proc/softirqs: only show state for online cpus

[PATCH] proc/softirqs: only show state for online cpus

[Yong Zhang]

Like /proc/interrupts, no need to output data for nobody.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 fs/proc/softirqs.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/proc/softirqs.c b/fs/proc/softirqs.c
index 62604be..2b32b46 100644
--- a/fs/proc/softirqs.c
+++ b/fs/proc/softirqs.c
@@ -11,13 +11,13 @@ static int show_softirqs(struct seq_file *p, void *v)
 	int i, j;
 
 	seq_puts(p, "                    ");
-	for_each_possible_cpu(i)
+	for_each_online_cpu(i)
 		seq_printf(p, "CPU%-8d", i);
 	seq_putc(p, '\n');
 
 	for (i = 0; i < NR_SOFTIRQS; i++) {
 		seq_printf(p, "%12s:", softirq_to_name[i]);
-		for_each_possible_cpu(j)
+		for_each_online_cpu(j)
 			seq_printf(p, " %10u", kstat_softirqs_cpu(i, j));
 		seq_putc(p, '\n');
 	}
-- 
1.7.4.1

Re: [PATCH] proc/softirqs: only show state for online cpus

[KOSAKI Motohiro]

> Like /proc/interrupts, no need to output data for nobody.
> 
> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

If the cpu never be onlined, its statistics always 0. Then, it definitely
no value. In the other hand, if the cpu was offlined dynamically, we don't
know the user want to know the cpus's statistics or not.

Anyway, it's incompatibility change. I don't think this is valuable change.


> ---
>  fs/proc/softirqs.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/proc/softirqs.c b/fs/proc/softirqs.c
> index 62604be..2b32b46 100644
> --- a/fs/proc/softirqs.c
> +++ b/fs/proc/softirqs.c
> @@ -11,13 +11,13 @@ static int show_softirqs(struct seq_file *p, void *v)
>  	int i, j;
>  
>  	seq_puts(p, "                    ");
> -	for_each_possible_cpu(i)
> +	for_each_online_cpu(i)
>  		seq_printf(p, "CPU%-8d", i);
>  	seq_putc(p, '\n');
>  
>  	for (i = 0; i < NR_SOFTIRQS; i++) {
>  		seq_printf(p, "%12s:", softirq_to_name[i]);
> -		for_each_possible_cpu(j)
> +		for_each_online_cpu(j)
>  			seq_printf(p, " %10u", kstat_softirqs_cpu(i, j));
>  		seq_putc(p, '\n');
>  	}

Re: [PATCH] proc/softirqs: only show state for online cpus

[Yong Zhang]

2011/7/26 KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>:
>> Like /proc/interrupts, no need to output data for nobody.
>>
>> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
>> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>
> If the cpu never be onlined, its statistics always 0. Then, it definitely

Yeah, so your screen may contain noise.

> no value. In the other hand, if the cpu was offlined dynamically, we don't
> know the user want to know the cpus's statistics or not.

Same to /proc/interrupts :)

IMHO, if user want to check the value of offline-cpu, maybe that means
he want to check the state of the whole system, /proc/stat should be the
right choice. /proc/{softirqs,interrupts} is just for immediate state.

>
> Anyway, it's incompatibility change.

Yup, I should have marked the patch with RFC :)

Thanks,
Yong

> I don't think this is valuable change.
>
>
>> ---
>>  fs/proc/softirqs.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/proc/softirqs.c b/fs/proc/softirqs.c
>> index 62604be..2b32b46 100644
>> --- a/fs/proc/softirqs.c
>> +++ b/fs/proc/softirqs.c
>> @@ -11,13 +11,13 @@ static int show_softirqs(struct seq_file *p, void *v)
>>       int i, j;
>>
>>       seq_puts(p, "                    ");
>> -     for_each_possible_cpu(i)
>> +     for_each_online_cpu(i)
>>               seq_printf(p, "CPU%-8d", i);
>>       seq_putc(p, '\n');
>>
>>       for (i = 0; i < NR_SOFTIRQS; i++) {
>>               seq_printf(p, "%12s:", softirq_to_name[i]);
>> -             for_each_possible_cpu(j)
>> +             for_each_online_cpu(j)
>>                       seq_printf(p, " %10u", kstat_softirqs_cpu(i, j));
>>               seq_putc(p, '\n');
>>       }
>
>
>



-- 
Only stand for myself

Re: [PATCH] proc/softirqs: only show state for online cpus

[KOSAKI Motohiro]

(2011/07/26 15:14), Yong Zhang wrote:
> 2011/7/26 KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>:
>>> Like /proc/interrupts, no need to output data for nobody.
>>>
>>> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
>>> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>>
>> If the cpu never be onlined, its statistics always 0. Then, it definitely
> 
> Yeah, so your screen may contain noise.

One question. Is this big matter?
Who see /proc/softirqs and /proc/interrupts directly? (i.e. by 'cat' command).


>> no value. In the other hand, if the cpu was offlined dynamically, we don't
>> know the user want to know the cpus's statistics or not.
> 
> Same to /proc/interrupts :)
> 
> IMHO, if user want to check the value of offline-cpu, maybe that means
> he want to check the state of the whole system, /proc/stat should be the
> right choice. /proc/{softirqs,interrupts} is just for immediate state.
>
>> Anyway, it's incompatibility change.
> 
> Yup, I should have marked the patch with RFC :)

And I should have remarked I don't dislike this patch so strongly, so
if kobayashi-san who original /proc/softirqs author ack you, I'm going
to second him.


Offtopic, /proc/interrupt should be protected by get_online_cpus().
Otherwise the header (i.e. cpu number) and the actual statistics fields
can be mismatched likes following. Am I missing something?

           CPU0       CPU1       CPU2
  0:   14286646   14747155          0   IO-APIC-edge      timer
  1:          6          6   IO-APIC-edge      i8042

                                 ^
                                 |
                               cpu2 was offlined dynamically at the same time.

Re: [PATCH] proc/softirqs: only show state for online cpus

[Yong Zhang]

On Tue, Jul 26, 2011 at 2:38 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
> (2011/07/26 15:14), Yong Zhang wrote:
>> 2011/7/26 KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>:
>>>> Like /proc/interrupts, no need to output data for nobody.
>>>>
>>>> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>> Cc: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
>>>> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>>>
>>> If the cpu never be onlined, its statistics always 0. Then, it definitely
>>
>> Yeah, so your screen may contain noise.
>
> One question. Is this big matter?

Actually it doesn't :)

> Who see /proc/softirqs and /proc/interrupts directly? (i.e. by 'cat' command).

By accident I noticed it by accident when running rt kernel. My screen
is full of '0'.
You know my usage is just for testing, maybe the real user is script-like.

>
>
>>> no value. In the other hand, if the cpu was offlined dynamically, we don't
>>> know the user want to know the cpus's statistics or not.
>>
>> Same to /proc/interrupts :)
>>
>> IMHO, if user want to check the value of offline-cpu, maybe that means
>> he want to check the state of the whole system, /proc/stat should be the
>> right choice. /proc/{softirqs,interrupts} is just for immediate state.
>>
>>> Anyway, it's incompatibility change.
>>
>> Yup, I should have marked the patch with RFC :)
>
> And I should have remarked I don't dislike this patch so strongly, so
> if kobayashi-san who original /proc/softirqs author ack you, I'm going
> to second him.

Hmmm, so let kobayashi-san decide it.

>
>
> Offtopic, /proc/interrupt should be protected by get_online_cpus().
> Otherwise the header (i.e. cpu number) and the actual statistics fields
> can be mismatched likes following. Am I missing something?

I think you are right. The reader could be preempted by cpu hotplug.

After searching the whole tree, only s390 take cpu_hotplug.lock,
but its usage is not currect:

arch/s390/kernel/irq.c:
int show_interrupts(struct seq_file *p, void *v)
{
          get_online_cpus();
          .........
          put_online_cpus();
}

Because the reader will call show_interrupts nr_irqs times.
So get_online_cpus()/put_online_cpus() should be put upper,
maybe interrupts_open(). How do you think about it?

Thanks,
Yong


-- 
Only stand for myself

Re: [PATCH] proc/softirqs: only show state for online cpus

[Keika Kobayashi]

(2011/07/26 16:29), Yong Zhang wrote:
> On Tue, Jul 26, 2011 at 2:38 PM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
>> (2011/07/26 15:14), Yong Zhang wrote:
>>> 2011/7/26 KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>:
>>>>> Like /proc/interrupts, no need to output data for nobody.
>>>>>
>>>>> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>> Cc: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
>>>>> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>>>>
>>>> If the cpu never be onlined, its statistics always 0. Then, it definitely
>>>
>>> Yeah, so your screen may contain noise.
>>
>> One question. Is this big matter?
> 
> Actually it doesn't :)
> 
>> Who see /proc/softirqs and /proc/interrupts directly? (i.e. by 'cat' command).
> 
> By accident I noticed it by accident when running rt kernel. My screen
> is full of '0'.
> You know my usage is just for testing, maybe the real user is script-like.
> 
>>
>>
>>>> no value. In the other hand, if the cpu was offlined dynamically, we don't
>>>> know the user want to know the cpus's statistics or not.
>>>
>>> Same to /proc/interrupts :)
>>>
>>> IMHO, if user want to check the value of offline-cpu, maybe that means
>>> he want to check the state of the whole system, /proc/stat should be the
>>> right choice. /proc/{softirqs,interrupts} is just for immediate state.
>>>
>>>> Anyway, it's incompatibility change.
>>>
>>> Yup, I should have marked the patch with RFC :)
>>
>> And I should have remarked I don't dislike this patch so strongly, so
>> if kobayashi-san who original /proc/softirqs author ack you, I'm going
>> to second him.
> 
> Hmmm, so let kobayashi-san decide it.

for_each_online_cpu() was in my first patch, like /proc/softirq.

But Andrew said
-- 
Probably for_each_possible_cpu() is best - people might want to see how
many softirqs happened on a CPU which was recently offlined.
-- 
It makes sense. 

We would like to collect this information
for trouble-shooting.
I think for_each_possible_cpu() is better.

At that time, I suggested to change
from for_each_online_cpu() to for_each_possible_cpu(),
in /proc/interrupts.

In conclusion, we decided to remain /proc/interrupts.
because it had been the way for a long time.



>> Offtopic, /proc/interrupt should be protected by get_online_cpus().
>> Otherwise the header (i.e. cpu number) and the actual statistics fields
>> can be mismatched likes following. Am I missing something?
> 
> I think you are right. The reader could be preempted by cpu hotplug.
> 
> After searching the whole tree, only s390 take cpu_hotplug.lock,
> but its usage is not currect:
> 
> arch/s390/kernel/irq.c:
> int show_interrupts(struct seq_file *p, void *v)
> {
>           get_online_cpus();
>           .........
>           put_online_cpus();
> }
> 
> Because the reader will call show_interrupts nr_irqs times.
> So get_online_cpus()/put_online_cpus() should be put upper,
> maybe interrupts_open(). How do you think about it?
> 
> Thanks,
> Yong
> 
> 


-- 
――――――――――――――――――――――
NEC通信システム
  技術管理本部　Linux技術センター
    小林　恵果
Mail   : kobayashi.kk@ncos.nec.co.jp
Tel    : 04-7185-6956（内線 : 8-26-35686）
――――――――――――――――――――――

Re: [PATCH] proc/softirqs: only show state for online cpus

[KOSAKI Motohiro]

>> Offtopic, /proc/interrupt should be protected by get_online_cpus().
>> Otherwise the header (i.e. cpu number) and the actual statistics fields
>> can be mismatched likes following. Am I missing something?
> 
> I think you are right. The reader could be preempted by cpu hotplug.
> 
> After searching the whole tree, only s390 take cpu_hotplug.lock,
> but its usage is not currect:
> 
> arch/s390/kernel/irq.c:
> int show_interrupts(struct seq_file *p, void *v)
> {
>           get_online_cpus();
>           .........
>           put_online_cpus();
> }
> 
> Because the reader will call show_interrupts nr_irqs times.
> So get_online_cpus()/put_online_cpus() should be put upper,
> maybe interrupts_open(). How do you think about it?

I agree with you.

Re: [PATCH] proc/softirqs: only show state for online cpus

[Yong Zhang]

On Tue, Jul 26, 2011 at 3:46 PM, Keika Kobayashi
<kobayashi.kk@ncos.nec.co.jp> wrote:
>
> for_each_online_cpu() was in my first patch, like /proc/softirq.
>
> But Andrew said
> --
> Probably for_each_possible_cpu() is best - people might want to see how
> many softirqs happened on a CPU which was recently offlined.
> --
> It makes sense.

Ah, thanks for the clarification.

>
> We would like to collect this information
> for trouble-shooting.
> I think for_each_possible_cpu() is better.
>
> At that time, I suggested to change
> from for_each_online_cpu() to for_each_possible_cpu(),
> in /proc/interrupts.

+1
Thus we could also avoid the issue pointed by KOSAKI Motonhiro.

>
> In conclusion, we decided to remain /proc/interrupts.
> because it had been the way for a long time.

fair enough :)
I will make a patch for /proc/interrupts instead.

Thanks,
Yong



-- 
Only stand for myself

Re: [PATCH] proc/softirqs: only show state for online cpus

[Heiko Carstens]

On Tue, Jul 26, 2011 at 03:29:43PM +0800, Yong Zhang wrote:
> > Offtopic, /proc/interrupt should be protected by get_online_cpus().
> > Otherwise the header (i.e. cpu number) and the actual statistics fields
> > can be mismatched likes following. Am I missing something?
> 
> I think you are right. The reader could be preempted by cpu hotplug.
> 
> After searching the whole tree, only s390 take cpu_hotplug.lock,
> but its usage is not currect:
> 
> arch/s390/kernel/irq.c:
> int show_interrupts(struct seq_file *p, void *v)
> {
>           get_online_cpus();
>           .........
>           put_online_cpus();
> }
> 
> Because the reader will call show_interrupts nr_irqs times.
> So get_online_cpus()/put_online_cpus() should be put upper,
> maybe interrupts_open(). How do you think about it?

Indeed, it's broken. You're going to submit a patch?

Re: [PATCH] proc/softirqs: only show state for online cpus

[Yong Zhang]

On Wed, Jul 27, 2011 at 12:34 AM, Heiko Carstens
<heiko.carstens@de.ibm.com> wrote:
> On Tue, Jul 26, 2011 at 03:29:43PM +0800, Yong Zhang wrote:
>> > Offtopic, /proc/interrupt should be protected by get_online_cpus().
>> > Otherwise the header (i.e. cpu number) and the actual statistics fields
>> > can be mismatched likes following. Am I missing something?
>>
>> I think you are right. The reader could be preempted by cpu hotplug.
>>
>> After searching the whole tree, only s390 take cpu_hotplug.lock,
>> but its usage is not currect:
>>
>> arch/s390/kernel/irq.c:
>> int show_interrupts(struct seq_file *p, void *v)
>> {
>>           get_online_cpus();
>>           .........
>>           put_online_cpus();
>> }
>>
>> Because the reader will call show_interrupts nr_irqs times.
>> So get_online_cpus()/put_online_cpus() should be put upper,
>> maybe interrupts_open(). How do you think about it?
>
> Indeed, it's broken. You're going to submit a patch?
>

Yeah, coming soon :)

Thanks,
Yong



-- 
Only stand for myself

[PATCH 1/2] proc/insterrupts: make it cpu hotplug safe

[Yong Zhang]

KOSAKI Motonhiro noticed that the reader of /proc/interrupts
could be preempted by cpu hotplug, thus the reader can get
broken result due to show_interrupts() iterate every online
cpu without any protection.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 fs/proc/interrupts.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/fs/proc/interrupts.c b/fs/proc/interrupts.c
index 05029c0..d5642a6 100644
--- a/fs/proc/interrupts.c
+++ b/fs/proc/interrupts.c
@@ -4,6 +4,7 @@
 #include <linux/irqnr.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
+#include <linux/cpu.h>
 
 /*
  * /proc/interrupts
@@ -35,14 +36,21 @@ static const struct seq_operations int_seq_ops = {
 
 static int interrupts_open(struct inode *inode, struct file *filp)
 {
+	get_online_cpus();
 	return seq_open(filp, &int_seq_ops);
 }
 
+static int interrupts_release(struct inode *inode, struct file *filp)
+{
+	put_online_cpus();
+	return seq_release(inode, filp);
+}
+
 static const struct file_operations proc_interrupts_operations = {
 	.open		= interrupts_open,
 	.read		= seq_read,
 	.llseek		= seq_lseek,
-	.release	= seq_release,
+	.release	= interrupts_release,
 };
 
 static int __init proc_interrupts_init(void)
-- 
1.7.4.1

[PATCH 2/2] [S390] irq: fix show_interrupts() vs cpu hotplug

[Yong Zhang]

The current usage of get_online_cpus()/put_online_cpus()
in show_interrupts() is not correct since show_interrupts()
will be called nr_irqs times, during that period, cpu hotplug
could also happen.
Since have moved the protection to upper(patch#1), the pair of
get_online_cpus()/put_online_cpus() could be removed here.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 arch/s390/kernel/irq.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kernel/irq.c b/arch/s390/kernel/irq.c
index e3264f6..57c036d 100644
--- a/arch/s390/kernel/irq.c
+++ b/arch/s390/kernel/irq.c
@@ -63,7 +63,6 @@ int show_interrupts(struct seq_file *p, void *v)
 {
 	int i = *(loff_t *) v, j;
 
-	get_online_cpus();
 	if (i == 0) {
 		seq_puts(p, "           ");
 		for_each_online_cpu(j)
@@ -83,7 +82,6 @@ int show_interrupts(struct seq_file *p, void *v)
 			seq_printf(p, "  %s", intrclass_names[i].desc);
                 seq_putc(p, '\n');
         }
-	put_online_cpus();
         return 0;
 }
 
-- 
1.7.4.1

Re: [PATCH 1/2] proc/insterrupts: make it cpu hotplug safe

[KOSAKI Motohiro]

(2011/07/27 13:56), Yong Zhang wrote:
> KOSAKI Motonhiro noticed that the reader of /proc/interrupts
> could be preempted by cpu hotplug, thus the reader can get
> broken result due to show_interrupts() iterate every online
> cpu without any protection.
> 
> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>

Looks good. but I have a question. On last thread, kobayashi-san
suggested to use for_each_possible_cpu() and you wrote "+1".

>> At that time, I suggested to change
>> from for_each_online_cpu() to for_each_possible_cpu(),
>> in /proc/interrupts.
>+1
>Thus we could also avoid the issue pointed by KOSAKI Motonhiro.

Why do you decide to use another way?

Re: [PATCH 1/2] proc/insterrupts: make it cpu hotplug safe

[Yong Zhang]

2011/7/27 KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>:
> (2011/07/27 13:56), Yong Zhang wrote:
>> KOSAKI Motonhiro noticed that the reader of /proc/interrupts
>> could be preempted by cpu hotplug, thus the reader can get
>> broken result due to show_interrupts() iterate every online
>> cpu without any protection.
>>
>> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
>> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
>
> Looks good. but I have a question. On last thread, kobayashi-san
> suggested to use for_each_possible_cpu() and you wrote "+1".

Yeah, for_each_possible_cpu() will make code more cleaner.
so I give it my support.

>
>>> At that time, I suggested to change
>>> from for_each_online_cpu() to for_each_possible_cpu(),
>>> in /proc/interrupts.
>>+1
>>Thus we could also avoid the issue pointed by KOSAKI Motonhiro.
>
> Why do you decide to use another way?

But, as kobayashi-san has also said:
In conclusion, we decided to remain /proc/interrupts.
because it had been the way for a long time.

So I don't want to raise an argument again :)

Thanks,
Yong


-- 
Only stand for myself

Re: [PATCH 1/2] proc/insterrupts: make it cpu hotplug safe

[KOSAKI Motohiro]

(2011/07/27 14:47), Yong Zhang wrote:
> 2011/7/27 KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>:
>> (2011/07/27 13:56), Yong Zhang wrote:
>>> KOSAKI Motonhiro noticed that the reader of /proc/interrupts
>>> could be preempted by cpu hotplug, thus the reader can get
>>> broken result due to show_interrupts() iterate every online
>>> cpu without any protection.
>>>
>>> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
>>> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>>> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
>>
>> Looks good. but I have a question. On last thread, kobayashi-san
>> suggested to use for_each_possible_cpu() and you wrote "+1".
> 
> Yeah, for_each_possible_cpu() will make code more cleaner.
> so I give it my support.
> 
>>
>>>> At that time, I suggested to change
>>>> from for_each_online_cpu() to for_each_possible_cpu(),
>>>> in /proc/interrupts.
>>> +1
>>> Thus we could also avoid the issue pointed by KOSAKI Motonhiro.
>>
>> Why do you decide to use another way?
> 
> But, as kobayashi-san has also said:
> In conclusion, we decided to remain /proc/interrupts.
> because it had been the way for a long time.
> 
> So I don't want to raise an argument again :)

Fair enough. thanks.

	Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Re: [PATCH 1/2] proc/insterrupts: make it cpu hotplug safe

[Heiko Carstens]

On Wed, Jul 27, 2011 at 12:56:22PM +0800, Yong Zhang wrote:
> KOSAKI Motonhiro noticed that the reader of /proc/interrupts
> could be preempted by cpu hotplug, thus the reader can get
> broken result due to show_interrupts() iterate every online
> cpu without any protection.
> 
> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Keika Kobayashi <kobayashi.kk@ncos.nec.co.jp>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>

Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>

Re: [PATCH 2/2] [S390] irq: fix show_interrupts() vs cpu hotplug

[Heiko Carstens]

On Wed, Jul 27, 2011 at 12:56:23PM +0800, Yong Zhang wrote:
> The current usage of get_online_cpus()/put_online_cpus()
> in show_interrupts() is not correct since show_interrupts()
> will be called nr_irqs times, during that period, cpu hotplug
> could also happen.
> Since have moved the protection to upper(patch#1), the pair of
> get_online_cpus()/put_online_cpus() could be removed here.
> 
> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>

Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>