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