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

[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 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>

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>