linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] irq: clear kstat_irqs
@ 2009-02-06 21:50 Yinghai Lu
  2009-02-06 22:00 ` Andrew Morton
  0 siblings, 1 reply; 13+ messages in thread
From: Yinghai Lu @ 2009-02-06 21:50 UTC (permalink / raw)
  To: Ingo Molnar, Thomas Gleixner, H. Peter Anvin, Andrew Morton; +Cc: linux-kernel


Impact: get correct kstat_irqs for msi etc

need to call clear_kstat_irqs(), so when reuse that irq_desc, could get correct
kstat in /proc/interrupts

also could make /proc/interrupts don't have none-<NULL> entries

don't need to worry about arch that doesn't support genirq, because they
will not call dynamic_irq_cleanup().

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 kernel/irq/chip.c      |    1 +
 kernel/irq/handle.c    |   11 +++++++++++
 kernel/irq/internals.h |    1 +
 3 files changed, 13 insertions(+)

Index: linux-2.6/kernel/irq/chip.c
===================================================================
--- linux-2.6.orig/kernel/irq/chip.c
+++ linux-2.6/kernel/irq/chip.c
@@ -81,6 +81,7 @@ void dynamic_irq_cleanup(unsigned int ir
 	desc->handle_irq = handle_bad_irq;
 	desc->chip = &no_irq_chip;
 	desc->name = NULL;
+	clear_kstat_irqs(desc);
 	spin_unlock_irqrestore(&desc->lock, flags);
 }
 
Index: linux-2.6/kernel/irq/handle.c
===================================================================
--- linux-2.6.orig/kernel/irq/handle.c
+++ linux-2.6/kernel/irq/handle.c
@@ -270,6 +270,17 @@ struct irq_desc *irq_to_desc_alloc_cpu(u
 }
 #endif /* !CONFIG_SPARSE_IRQ */
 
+void clear_kstat_irqs(struct irq_desc *desc)
+{
+	unsigned long bytes;
+	char *ptr;
+
+	ptr = (char *)desc->kstat_irqs;
+	/* Compute how many bytes we need to clear */
+	bytes = nr_cpu_ids * sizeof(unsigned int);
+	memset(ptr, 0, bytes);
+}
+
 /*
  * What should we do if we get a hw irq event on an illegal vector?
  * Each architecture has to answer this themself.
Index: linux-2.6/kernel/irq/internals.h
===================================================================
--- linux-2.6.orig/kernel/irq/internals.h
+++ linux-2.6/kernel/irq/internals.h
@@ -15,6 +15,7 @@ extern int __irq_set_trigger(struct irq_
 
 extern struct lock_class_key irq_desc_lock_class;
 extern void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr);
+extern void clear_kstat_irqs(struct irq_desc *desc);
 extern spinlock_t sparse_irq_lock;
 
 #ifdef CONFIG_SPARSE_IRQ

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

* Re: [PATCH] irq: clear kstat_irqs
  2009-02-06 21:50 [PATCH] irq: clear kstat_irqs Yinghai Lu
@ 2009-02-06 22:00 ` Andrew Morton
  2009-02-06 22:08   ` [PATCH] irq: clear kstat_irqs v2 Yinghai Lu
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2009-02-06 22:00 UTC (permalink / raw)
  To: Yinghai Lu; +Cc: mingo, tglx, hpa, linux-kernel

On Fri, 06 Feb 2009 13:50:47 -0800
Yinghai Lu <yinghai@kernel.org> wrote:

> +void clear_kstat_irqs(struct irq_desc *desc)
> +{
> +	unsigned long bytes;

size_t would be more formally correct.

> +	char *ptr;
> +
> +	ptr = (char *)desc->kstat_irqs;
> +	/* Compute how many bytes we need to clear */
> +	bytes = nr_cpu_ids * sizeof(unsigned int);

This is fragile.  For example, if someone changes ->kstat_irqs to long
then this code will secretly and subtly break.

If it used sizeof(*(desc->kstat_irqs)) then everything would magically
continue to work.

> +	memset(ptr, 0, bytes);
> +}

The whole function could be written as

	memset(desc->kstat_irqs, 0, nr_cpu_ids * sizeof(*(desc->kstat_irqs)));


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

* [PATCH] irq: clear kstat_irqs v2
  2009-02-06 22:00 ` Andrew Morton
@ 2009-02-06 22:08   ` Yinghai Lu
  2009-02-07  7:49     ` [PATCH] irq: optimize init_kstat_irqs/init_copy_kstat_irqs Yinghai Lu
  0 siblings, 1 reply; 13+ messages in thread
From: Yinghai Lu @ 2009-02-06 22:08 UTC (permalink / raw)
  To: Andrew Morton, mingo, tglx, hpa; +Cc: linux-kernel


Impact: get correct kstat_irqs for msi/msi-x etc

need to call clear_kstat_irqs(), so when reuse that irq_desc, could get correct
kstat in /proc/interrupts

also could make /proc/interrupts don't have none-<NULL> entries

don't need to worry about arch that doesn't support genirq, because they
will not call dynamic_irq_cleanup().

v2: simplify and make clear_kstat_irqs more rebust accord to Andrew

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 kernel/irq/chip.c      |    1 +
 kernel/irq/handle.c    |    5 +++++
 kernel/irq/internals.h |    1 +
 3 files changed, 7 insertions(+)

Index: linux-2.6/kernel/irq/chip.c
===================================================================
--- linux-2.6.orig/kernel/irq/chip.c
+++ linux-2.6/kernel/irq/chip.c
@@ -81,6 +81,7 @@ void dynamic_irq_cleanup(unsigned int ir
 	desc->handle_irq = handle_bad_irq;
 	desc->chip = &no_irq_chip;
 	desc->name = NULL;
+	clear_kstat_irqs(desc);
 	spin_unlock_irqrestore(&desc->lock, flags);
 }
 
Index: linux-2.6/kernel/irq/handle.c
===================================================================
--- linux-2.6.orig/kernel/irq/handle.c
+++ linux-2.6/kernel/irq/handle.c
@@ -270,6 +270,11 @@ struct irq_desc *irq_to_desc_alloc_cpu(u
 }
 #endif /* !CONFIG_SPARSE_IRQ */
 
+void clear_kstat_irqs(struct irq_desc *desc)
+{
+	memset(desc->kstat_irqs, 0, nr_cpu_ids * sizeof(*(desc->kstat_irqs)));
+}
+
 /*
  * What should we do if we get a hw irq event on an illegal vector?
  * Each architecture has to answer this themself.
Index: linux-2.6/kernel/irq/internals.h
===================================================================
--- linux-2.6.orig/kernel/irq/internals.h
+++ linux-2.6/kernel/irq/internals.h
@@ -15,6 +15,7 @@ extern int __irq_set_trigger(struct irq_
 
 extern struct lock_class_key irq_desc_lock_class;
 extern void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr);
+extern void clear_kstat_irqs(struct irq_desc *desc);
 extern spinlock_t sparse_irq_lock;
 
 #ifdef CONFIG_SPARSE_IRQ

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

* [PATCH] irq: optimize init_kstat_irqs/init_copy_kstat_irqs
  2009-02-06 22:08   ` [PATCH] irq: clear kstat_irqs v2 Yinghai Lu
@ 2009-02-07  7:49     ` Yinghai Lu
  2009-02-07  8:26       ` Andrew Morton
  0 siblings, 1 reply; 13+ messages in thread
From: Yinghai Lu @ 2009-02-07  7:49 UTC (permalink / raw)
  To: mingo, tglx, hpa, Andrew Morton; +Cc: linux-kernel


Impact: even later type of kstat_irqs is changed

simplify and make init_kstat_irqs etc more type prof according to Andrew

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 kernel/irq/handle.c       |   20 +++++++++++---------
 kernel/irq/numa_migrate.c |   11 +++--------
 2 files changed, 14 insertions(+), 17 deletions(-)

Index: linux-2.6/kernel/irq/handle.c
===================================================================
--- linux-2.6.orig/kernel/irq/handle.c
+++ linux-2.6/kernel/irq/handle.c
@@ -82,19 +82,21 @@ static struct irq_desc irq_desc_init = {
 
 void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr)
 {
-	unsigned long bytes;
-	char *ptr;
 	int node;
-
-	/* Compute how many bytes we need per irq and allocate them */
-	bytes = nr * sizeof(unsigned int);
+	void *ptr;
 
 	node = cpu_to_node(cpu);
-	ptr = kzalloc_node(bytes, GFP_ATOMIC, node);
-	printk(KERN_DEBUG "  alloc kstat_irqs on cpu %d node %d\n", cpu, node);
+	ptr = kzalloc_node(nr * sizeof(*desc->kstat_irqs), GFP_ATOMIC, node);
 
-	if (ptr)
-		desc->kstat_irqs = (unsigned int *)ptr;
+	/*
+	 * don't overwite if can not get new one
+	 * init_copy_kstat_irqs() could still use old one
+	 */
+	if (ptr) {
+		printk(KERN_DEBUG "  alloc kstat_irqs on cpu %d node %d\n",
+			 cpu, node);
+		desc->kstat_irqs = ptr;
+	}
 }
 
 static void init_one_irq_desc(int irq, struct irq_desc *desc, int cpu)
Index: linux-2.6/kernel/irq/numa_migrate.c
===================================================================
--- linux-2.6.orig/kernel/irq/numa_migrate.c
+++ linux-2.6/kernel/irq/numa_migrate.c
@@ -17,16 +17,11 @@ static void init_copy_kstat_irqs(struct
 				 struct irq_desc *desc,
 				 int cpu, int nr)
 {
-	unsigned long bytes;
-
 	init_kstat_irqs(desc, cpu, nr);
 
-	if (desc->kstat_irqs != old_desc->kstat_irqs) {
-		/* Compute how many bytes we need per irq and allocate them */
-		bytes = nr * sizeof(unsigned int);
-
-		memcpy(desc->kstat_irqs, old_desc->kstat_irqs, bytes);
-	}
+	if (desc->kstat_irqs != old_desc->kstat_irqs)
+		memcpy(desc->kstat_irqs, old_desc->kstat_irqs,
+			 nr * sizeof(*desc->kstat_irqs));
 }
 
 static void free_kstat_irqs(struct irq_desc *old_desc, struct irq_desc *desc)

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

* Re: [PATCH] irq: optimize init_kstat_irqs/init_copy_kstat_irqs
  2009-02-07  7:49     ` [PATCH] irq: optimize init_kstat_irqs/init_copy_kstat_irqs Yinghai Lu
@ 2009-02-07  8:26       ` Andrew Morton
  2009-02-07  9:01         ` Yinghai Lu
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2009-02-07  8:26 UTC (permalink / raw)
  To: Yinghai Lu; +Cc: mingo, tglx, hpa, linux-kernel

On Fri, 06 Feb 2009 23:49:41 -0800 Yinghai Lu <yinghai@kernel.org> wrote:

> 
> Impact: even later type of kstat_irqs is changed
> 
> simplify and make init_kstat_irqs etc more type prof according to Andrew
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  kernel/irq/handle.c       |   20 +++++++++++---------
>  kernel/irq/numa_migrate.c |   11 +++--------
>  2 files changed, 14 insertions(+), 17 deletions(-)
> 
> Index: linux-2.6/kernel/irq/handle.c
> ===================================================================
> --- linux-2.6.orig/kernel/irq/handle.c
> +++ linux-2.6/kernel/irq/handle.c
> @@ -82,19 +82,21 @@ static struct irq_desc irq_desc_init = {
>  
>  void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr)
>  {
> -	unsigned long bytes;
> -	char *ptr;
>  	int node;
> -
> -	/* Compute how many bytes we need per irq and allocate them */
> -	bytes = nr * sizeof(unsigned int);
> +	void *ptr;
>  
>  	node = cpu_to_node(cpu);
> -	ptr = kzalloc_node(bytes, GFP_ATOMIC, node);
> -	printk(KERN_DEBUG "  alloc kstat_irqs on cpu %d node %d\n", cpu, node);
> +	ptr = kzalloc_node(nr * sizeof(*desc->kstat_irqs), GFP_ATOMIC, node);
>  
> -	if (ptr)
> -		desc->kstat_irqs = (unsigned int *)ptr;
> +	/*
> +	 * don't overwite if can not get new one
> +	 * init_copy_kstat_irqs() could still use old one
> +	 */
> +	if (ptr) {
> +		printk(KERN_DEBUG "  alloc kstat_irqs on cpu %d node %d\n",
> +			 cpu, node);
> +		desc->kstat_irqs = ptr;
> +	}

	else
		init_one_irq_desc() goes BUG.

>  }

Sorry, but it's just not acceptable for irq_to_desc_alloc_cpu() and
init_one_irq_desc() to go BUG if a GFP_ATOMIC allocation attempt
failed.

If this code is only called on kernel boot then OK, that's acceptable. 
But if that is the case then all this code should be marked
__init/__initdata.  And ack_apic_edge() sure doesn't look like a
boot-time-only function.

This is basic stuff which even a cursory review should have picked up.

Also, please go through all this code and convert WARN_ON(1) into
WARN(), and convert BUG_ON(1) into BUG().



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

* Re: [PATCH] irq: optimize init_kstat_irqs/init_copy_kstat_irqs
  2009-02-07  8:26       ` Andrew Morton
@ 2009-02-07  9:01         ` Yinghai Lu
  2009-02-07  9:06           ` Andrew Morton
  0 siblings, 1 reply; 13+ messages in thread
From: Yinghai Lu @ 2009-02-07  9:01 UTC (permalink / raw)
  To: Andrew Morton; +Cc: mingo, tglx, hpa, linux-kernel

On Sat, Feb 7, 2009 at 12:26 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 06 Feb 2009 23:49:41 -0800 Yinghai Lu <yinghai@kernel.org> wrote:
>
>>
>> Impact: even later type of kstat_irqs is changed
>>
>> simplify and make init_kstat_irqs etc more type prof according to Andrew
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> ---
>>  kernel/irq/handle.c       |   20 +++++++++++---------
>>  kernel/irq/numa_migrate.c |   11 +++--------
>>  2 files changed, 14 insertions(+), 17 deletions(-)
>>
>> Index: linux-2.6/kernel/irq/handle.c
>> ===================================================================
>> --- linux-2.6.orig/kernel/irq/handle.c
>> +++ linux-2.6/kernel/irq/handle.c
>> @@ -82,19 +82,21 @@ static struct irq_desc irq_desc_init = {
>>
>>  void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr)
>>  {
>> -     unsigned long bytes;
>> -     char *ptr;
>>       int node;
>> -
>> -     /* Compute how many bytes we need per irq and allocate them */
>> -     bytes = nr * sizeof(unsigned int);
>> +     void *ptr;
>>
>>       node = cpu_to_node(cpu);
>> -     ptr = kzalloc_node(bytes, GFP_ATOMIC, node);
>> -     printk(KERN_DEBUG "  alloc kstat_irqs on cpu %d node %d\n", cpu, node);
>> +     ptr = kzalloc_node(nr * sizeof(*desc->kstat_irqs), GFP_ATOMIC, node);
>>
>> -     if (ptr)
>> -             desc->kstat_irqs = (unsigned int *)ptr;
>> +     /*
>> +      * don't overwite if can not get new one
>> +      * init_copy_kstat_irqs() could still use old one
>> +      */
>> +     if (ptr) {
>> +             printk(KERN_DEBUG "  alloc kstat_irqs on cpu %d node %d\n",
>> +                      cpu, node);
>> +             desc->kstat_irqs = ptr;
>> +     }
>
>        else
>                init_one_irq_desc() goes BUG.
>
>>  }
>
> Sorry, but it's just not acceptable for irq_to_desc_alloc_cpu() and
> init_one_irq_desc() to go BUG if a GFP_ATOMIC allocation attempt
> failed.
>
> If this code is only called on kernel boot then OK, that's acceptable.
> But if that is the case then all this code should be marked
> __init/__initdata.  And ack_apic_edge() sure doesn't look like a
> boot-time-only function.

add kzalloc_node_safe()?

>
> This is basic stuff which even a cursory review should have picked up.
>
> Also, please go through all this code and convert WARN_ON(1) into
> WARN(), and convert BUG_ON(1) into BUG().

sure, will do it.

YH

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

* Re: [PATCH] irq: optimize init_kstat_irqs/init_copy_kstat_irqs
  2009-02-07  9:01         ` Yinghai Lu
@ 2009-02-07  9:06           ` Andrew Morton
  2009-02-09  8:11             ` Ingo Molnar
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2009-02-07  9:06 UTC (permalink / raw)
  To: Yinghai Lu; +Cc: mingo, tglx, hpa, linux-kernel

On Sat, 7 Feb 2009 01:01:03 -0800 Yinghai Lu <yinghai@kernel.org> wrote:

> 
> add kzalloc_node_safe()?

I cannot find that function.



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

* Re: [PATCH] irq: optimize init_kstat_irqs/init_copy_kstat_irqs
  2009-02-07  9:06           ` Andrew Morton
@ 2009-02-09  8:11             ` Ingo Molnar
  2009-02-09  8:19               ` Andrew Morton
  0 siblings, 1 reply; 13+ messages in thread
From: Ingo Molnar @ 2009-02-09  8:11 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Yinghai Lu, tglx, hpa, linux-kernel


* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Sat, 7 Feb 2009 01:01:03 -0800 Yinghai Lu <yinghai@kernel.org> wrote:
> 
> > 
> > add kzalloc_node_safe()?
> 
> I cannot find that function.

His suggestion is to provide that allocator variant.

	Ingo

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

* Re: [PATCH] irq: optimize init_kstat_irqs/init_copy_kstat_irqs
  2009-02-09  8:11             ` Ingo Molnar
@ 2009-02-09  8:19               ` Andrew Morton
  2009-02-09  8:37                 ` Ingo Molnar
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2009-02-09  8:19 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: Yinghai Lu, tglx, hpa, linux-kernel

On Mon, 9 Feb 2009 09:11:24 +0100 Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Sat, 7 Feb 2009 01:01:03 -0800 Yinghai Lu <yinghai@kernel.org> wrote:
> > 
> > > 
> > > add kzalloc_node_safe()?
> > 
> > I cannot find that function.
> 
> His suggestion is to provide that allocator variant.
> 

Oh.

It isn't possible to write a kzalloc_node_safe(GFP_ATOMIC).  Or at
least, we've never worked out a way.

Maybe I'm confused again.

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

* Re: [PATCH] irq: optimize init_kstat_irqs/init_copy_kstat_irqs
  2009-02-09  8:19               ` Andrew Morton
@ 2009-02-09  8:37                 ` Ingo Molnar
  2009-02-09  8:43                   ` Andrew Morton
  2009-02-09  8:44                   ` Yinghai Lu
  0 siblings, 2 replies; 13+ messages in thread
From: Ingo Molnar @ 2009-02-09  8:37 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Yinghai Lu, tglx, hpa, linux-kernel


* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 9 Feb 2009 09:11:24 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > * Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > On Sat, 7 Feb 2009 01:01:03 -0800 Yinghai Lu <yinghai@kernel.org> wrote:
> > > 
> > > > 
> > > > add kzalloc_node_safe()?
> > > 
> > > I cannot find that function.
> > 
> > His suggestion is to provide that allocator variant.
> > 
> 
> Oh.
> 
> It isn't possible to write a kzalloc_node_safe(GFP_ATOMIC).  Or at
> least, we've never worked out a way.
> 
> Maybe I'm confused again.

Indeed - duh - more morning tea needed.

Yinghai, why are those allocations GFP_ATOMIC to begin with? These:

earth4:~/tip> grep GFP_ATOMIC kernel/irq/*.c
kernel/irq/handle.c:	ptr = kzalloc_node(nr * sizeof(*desc->kstat_irqs), GFP_ATOMIC, node);
kernel/irq/handle.c:	desc = kzalloc_node(sizeof(*desc), GFP_ATOMIC, node);
kernel/irq/manage.c:	action = kmalloc(sizeof(struct irqaction), GFP_ATOMIC);

Should all be GFP_KERNEL. Wherever they are within a spinlocked section the code 
should be restructured. All descriptor data structures should be preallocated at 
__setup_irq() time. If we ever need to allocate dynamically later on, in the middle 
of some difficult codepath that's a structure bug in the code.

and this one:

kernel/irq/numa_migrate.c:	desc = kzalloc_node(sizeof(*desc), GFP_ATOMIC, node);

should fail the migration silently if GFP_ATOMIC returns NULL.

	Ingo

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

* Re: [PATCH] irq: optimize init_kstat_irqs/init_copy_kstat_irqs
  2009-02-09  8:37                 ` Ingo Molnar
@ 2009-02-09  8:43                   ` Andrew Morton
  2009-02-09 11:34                     ` Ingo Molnar
  2009-02-09  8:44                   ` Yinghai Lu
  1 sibling, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2009-02-09  8:43 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: Yinghai Lu, tglx, hpa, linux-kernel

On Mon, 9 Feb 2009 09:37:39 +0100 Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Mon, 9 Feb 2009 09:11:24 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> > 
> > > 
> > > * Andrew Morton <akpm@linux-foundation.org> wrote:
> > > 
> > > > On Sat, 7 Feb 2009 01:01:03 -0800 Yinghai Lu <yinghai@kernel.org> wrote:
> > > > 
> > > > > 
> > > > > add kzalloc_node_safe()?
> > > > 
> > > > I cannot find that function.
> > > 
> > > His suggestion is to provide that allocator variant.
> > > 
> > 
> > Oh.
> > 
> > It isn't possible to write a kzalloc_node_safe(GFP_ATOMIC).  Or at
> > least, we've never worked out a way.
> > 
> > Maybe I'm confused again.
> 
> Indeed - duh - more morning tea needed.
> 
> Yinghai, why are those allocations GFP_ATOMIC to begin with? These:
> 
> earth4:~/tip> grep GFP_ATOMIC kernel/irq/*.c
> kernel/irq/handle.c:	ptr = kzalloc_node(nr * sizeof(*desc->kstat_irqs), GFP_ATOMIC, node);
> kernel/irq/handle.c:	desc = kzalloc_node(sizeof(*desc), GFP_ATOMIC, node);
> kernel/irq/manage.c:	action = kmalloc(sizeof(struct irqaction), GFP_ATOMIC);
> 
> Should all be GFP_KERNEL. Wherever they are within a spinlocked section the code 
> should be restructured. All descriptor data structures should be preallocated at 
> __setup_irq() time. If we ever need to allocate dynamically later on, in the middle 
> of some difficult codepath that's a structure bug in the code.

yup, something along those lines.

> and this one:
> 
> kernel/irq/numa_migrate.c:	desc = kzalloc_node(sizeof(*desc), GFP_ATOMIC, node);
> 
> should fail the migration silently if GFP_ATOMIC returns NULL.

Silent failure sounds bad?

The allocation attempt will spew a page-allocation-failure backtrace
anyway, so people will still get alarmed.

You might instead choose to suppress that warning with __GFP_NOWARN and
instead add a more meaningful warning at the calling codesite.



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

* Re: [PATCH] irq: optimize init_kstat_irqs/init_copy_kstat_irqs
  2009-02-09  8:37                 ` Ingo Molnar
  2009-02-09  8:43                   ` Andrew Morton
@ 2009-02-09  8:44                   ` Yinghai Lu
  1 sibling, 0 replies; 13+ messages in thread
From: Yinghai Lu @ 2009-02-09  8:44 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: Andrew Morton, tglx, hpa, linux-kernel

Ingo Molnar wrote:
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
>> On Mon, 9 Feb 2009 09:11:24 +0100 Ingo Molnar <mingo@elte.hu> wrote:
>>
>>> * Andrew Morton <akpm@linux-foundation.org> wrote:
>>>
>>>> On Sat, 7 Feb 2009 01:01:03 -0800 Yinghai Lu <yinghai@kernel.org> wrote:
>>>>
>>>>> add kzalloc_node_safe()?
>>>> I cannot find that function.
>>> His suggestion is to provide that allocator variant.
>>>
>> Oh.
>>
>> It isn't possible to write a kzalloc_node_safe(GFP_ATOMIC).  Or at
>> least, we've never worked out a way.
>>
>> Maybe I'm confused again.
> 
> Indeed - duh - more morning tea needed.
> 
> Yinghai, why are those allocations GFP_ATOMIC to begin with? These:
> 
> earth4:~/tip> grep GFP_ATOMIC kernel/irq/*.c
> kernel/irq/handle.c:	ptr = kzalloc_node(nr * sizeof(*desc->kstat_irqs), GFP_ATOMIC, node);
> kernel/irq/handle.c:	desc = kzalloc_node(sizeof(*desc), GFP_ATOMIC, node);
> kernel/irq/manage.c:	action = kmalloc(sizeof(struct irqaction), GFP_ATOMIC);
> 
> Should all be GFP_KERNEL. Wherever they are within a spinlocked section the code 
> should be restructured. All descriptor data structures should be preallocated at 
> __setup_irq() time. If we ever need to allocate dynamically later on, in the middle 
> of some difficult codepath that's a structure bug in the code.

ok.

> 
> and this one:
> 
> kernel/irq/numa_migrate.c:	desc = kzalloc_node(sizeof(*desc), GFP_ATOMIC, node);
> 
> should fail the migration silently if GFP_ATOMIC returns NULL.
it will reuse the old one.

YH

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

* Re: [PATCH] irq: optimize init_kstat_irqs/init_copy_kstat_irqs
  2009-02-09  8:43                   ` Andrew Morton
@ 2009-02-09 11:34                     ` Ingo Molnar
  0 siblings, 0 replies; 13+ messages in thread
From: Ingo Molnar @ 2009-02-09 11:34 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Yinghai Lu, tglx, hpa, linux-kernel


* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 9 Feb 2009 09:37:39 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > * Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > On Mon, 9 Feb 2009 09:11:24 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> > > 
> > > > 
> > > > * Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > 
> > > > > On Sat, 7 Feb 2009 01:01:03 -0800 Yinghai Lu <yinghai@kernel.org> wrote:
> > > > > 
> > > > > > 
> > > > > > add kzalloc_node_safe()?
> > > > > 
> > > > > I cannot find that function.
> > > > 
> > > > His suggestion is to provide that allocator variant.
> > > > 
> > > 
> > > Oh.
> > > 
> > > It isn't possible to write a kzalloc_node_safe(GFP_ATOMIC).  Or at
> > > least, we've never worked out a way.
> > > 
> > > Maybe I'm confused again.
> > 
> > Indeed - duh - more morning tea needed.
> > 
> > Yinghai, why are those allocations GFP_ATOMIC to begin with? These:
> > 
> > earth4:~/tip> grep GFP_ATOMIC kernel/irq/*.c
> > kernel/irq/handle.c:	ptr = kzalloc_node(nr * sizeof(*desc->kstat_irqs), GFP_ATOMIC, node);
> > kernel/irq/handle.c:	desc = kzalloc_node(sizeof(*desc), GFP_ATOMIC, node);
> > kernel/irq/manage.c:	action = kmalloc(sizeof(struct irqaction), GFP_ATOMIC);
> > 
> > Should all be GFP_KERNEL. Wherever they are within a spinlocked section the code 
> > should be restructured. All descriptor data structures should be preallocated at 
> > __setup_irq() time. If we ever need to allocate dynamically later on, in the middle 
> > of some difficult codepath that's a structure bug in the code.
> 
> yup, something along those lines.
> 
> > and this one:
> > 
> > kernel/irq/numa_migrate.c:	desc = kzalloc_node(sizeof(*desc), GFP_ATOMIC, node);
> > 
> > should fail the migration silently if GFP_ATOMIC returns NULL.
> 
> Silent failure sounds bad?
> 
> The allocation attempt will spew a page-allocation-failure backtrace
> anyway, so people will still get alarmed.
> 
> You might instead choose to suppress that warning with __GFP_NOWARN and
> instead add a more meaningful warning at the calling codesite.

Well this is a NUMA performance optimization that gets re-tried anyway, so
spewing anything because we are so low on RAM that we cannot fulfill
GFP_ATOMIC wont help things. Silence is better there i think. At most
a WARN_ONCE().

	Ingo

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

end of thread, other threads:[~2009-02-09 11:34 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-02-06 21:50 [PATCH] irq: clear kstat_irqs Yinghai Lu
2009-02-06 22:00 ` Andrew Morton
2009-02-06 22:08   ` [PATCH] irq: clear kstat_irqs v2 Yinghai Lu
2009-02-07  7:49     ` [PATCH] irq: optimize init_kstat_irqs/init_copy_kstat_irqs Yinghai Lu
2009-02-07  8:26       ` Andrew Morton
2009-02-07  9:01         ` Yinghai Lu
2009-02-07  9:06           ` Andrew Morton
2009-02-09  8:11             ` Ingo Molnar
2009-02-09  8:19               ` Andrew Morton
2009-02-09  8:37                 ` Ingo Molnar
2009-02-09  8:43                   ` Andrew Morton
2009-02-09 11:34                     ` Ingo Molnar
2009-02-09  8:44                   ` Yinghai Lu

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).