2009-01-11 19:46:45

by Mike Travis

[permalink] [raw]
Subject: [PATCH]: xen: fix too early kmalloc call

Subject: xen: fix too early kmalloc call

From: Christophe Saout <[email protected]>

Impact: fixes bootup of xen.

Christophe reported the following problem:

> (basically it seems that SLAB is not yet up, with earlyprintk it is
> giving me an Oops in __kmalloc before)

Replace call to kmalloc with alloc_bootmem.

Also from Christophe:
> (me)
> > Or I could copy the text and submit it as a new patch?
>
> Yes, I would prefer that. Also, my commit message was not really
> following Kernel standards and it's only a very simple change.

Signed-off-by: Mike Travis <[email protected]>
---
drivers/xen/events.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

--- linux-2.6-for-ingo.orig/drivers/xen/events.c
+++ linux-2.6-for-ingo/drivers/xen/events.c
@@ -26,6 +26,7 @@
#include <linux/irq.h>
#include <linux/module.h>
#include <linux/string.h>
+#include <linux/bootmem.h>

#include <asm/ptrace.h>
#include <asm/irq.h>
@@ -831,8 +832,8 @@ void __init xen_init_IRQ(void)
int i;
size_t size = nr_cpu_ids * sizeof(struct cpu_evtchn_s);

- cpu_evtchn_mask_p = kmalloc(size, GFP_KERNEL);
- BUG_ON(cpu_evtchn_mask == NULL);
+ cpu_evtchn_mask_p = alloc_bootmem(size);
+ BUG_ON(cpu_evtchn_mask_p == NULL);

init_evtchn_cpu_bindings();


2009-01-12 08:53:24

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH]: xen: fix too early kmalloc call


* Mike Travis <[email protected]> wrote:

> Subject: xen: fix too early kmalloc call
>
> From: Christophe Saout <[email protected]>
>
> Impact: fixes bootup of xen.
>
> Christophe reported the following problem:
>
> > (basically it seems that SLAB is not yet up, with earlyprintk it is
> > giving me an Oops in __kmalloc before)
>
> Replace call to kmalloc with alloc_bootmem.
>
> Also from Christophe:
> > (me)
> > > Or I could copy the text and submit it as a new patch?
> >
> > Yes, I would prefer that. Also, my commit message was not really
> > following Kernel standards and it's only a very simple change.
>
> Signed-off-by: Mike Travis <[email protected]>

i've applied the fix to tip/cpus4096, in the form below.

Please submit clean log messages. Your commit description was confused -
the patch is From: Christophe, but the message talks about Christophe in
the third person. Also, it included conversational bits that have no
value for the Git history.

Ingo

-------------------->
>From 28e08861b9afab4168b758fb7b95aa7a4da0f668 Mon Sep 17 00:00:00 2001
From: Christophe Saout <[email protected]>
Date: Sun, 11 Jan 2009 11:46:23 -0800
Subject: [PATCH] xen: fix too early kmalloc call

Impact: fix bootup crash on xen guests

SLAB is not yet up, with earlyprintk it is giving me an Oops in __kmalloc.

Replace call to kmalloc() with alloc_bootmem().

Reported-by: Christophe Saout <[email protected]>
Signed-off-by: Mike Travis <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
drivers/xen/events.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index ed7527b..3141e14 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -26,6 +26,7 @@
#include <linux/irq.h>
#include <linux/module.h>
#include <linux/string.h>
+#include <linux/bootmem.h>

#include <asm/ptrace.h>
#include <asm/irq.h>
@@ -831,8 +832,8 @@ void __init xen_init_IRQ(void)
int i;
size_t size = nr_cpu_ids * sizeof(struct cpu_evtchn_s);

- cpu_evtchn_mask_p = kmalloc(size, GFP_KERNEL);
- BUG_ON(cpu_evtchn_mask == NULL);
+ cpu_evtchn_mask_p = alloc_bootmem(size);
+ BUG_ON(cpu_evtchn_mask_p == NULL);

init_evtchn_cpu_bindings();

2009-01-12 14:42:55

by Johannes Weiner

[permalink] [raw]
Subject: Re: [PATCH]: xen: fix too early kmalloc call

On Mon, Jan 12, 2009 at 09:53:02AM +0100, Ingo Molnar wrote:
>
> * Mike Travis <[email protected]> wrote:
>
> > Subject: xen: fix too early kmalloc call
> >
> > From: Christophe Saout <[email protected]>
> >
> > Impact: fixes bootup of xen.
> >
> > Christophe reported the following problem:
> >
> > > (basically it seems that SLAB is not yet up, with earlyprintk it is
> > > giving me an Oops in __kmalloc before)
> >
> > Replace call to kmalloc with alloc_bootmem.
> >
> > Also from Christophe:
> > > (me)
> > > > Or I could copy the text and submit it as a new patch?
> > >
> > > Yes, I would prefer that. Also, my commit message was not really
> > > following Kernel standards and it's only a very simple change.
> >
> > Signed-off-by: Mike Travis <[email protected]>
>
> i've applied the fix to tip/cpus4096, in the form below.
>
> Please submit clean log messages. Your commit description was confused -
> the patch is From: Christophe, but the message talks about Christophe in
> the third person. Also, it included conversational bits that have no
> value for the Git history.
>
> Ingo
>
> -------------------->
> >>From 28e08861b9afab4168b758fb7b95aa7a4da0f668 Mon Sep 17 00:00:00 2001
> From: Christophe Saout <[email protected]>
> Date: Sun, 11 Jan 2009 11:46:23 -0800
> Subject: [PATCH] xen: fix too early kmalloc call
>
> Impact: fix bootup crash on xen guests
>
> SLAB is not yet up, with earlyprintk it is giving me an Oops in __kmalloc.
>
> Replace call to kmalloc() with alloc_bootmem().
>
> Reported-by: Christophe Saout <[email protected]>
> Signed-off-by: Mike Travis <[email protected]>
> Signed-off-by: Ingo Molnar <[email protected]>
> ---
> drivers/xen/events.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index ed7527b..3141e14 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -26,6 +26,7 @@
> #include <linux/irq.h>
> #include <linux/module.h>
> #include <linux/string.h>
> +#include <linux/bootmem.h>
>
> #include <asm/ptrace.h>
> #include <asm/irq.h>
> @@ -831,8 +832,8 @@ void __init xen_init_IRQ(void)
> int i;
> size_t size = nr_cpu_ids * sizeof(struct cpu_evtchn_s);
>
> - cpu_evtchn_mask_p = kmalloc(size, GFP_KERNEL);
> - BUG_ON(cpu_evtchn_mask == NULL);
> + cpu_evtchn_mask_p = alloc_bootmem(size);
> + BUG_ON(cpu_evtchn_mask_p == NULL);

Impossible condition :-) alloc_bootmem() crashes before you could
catch it here.

Hannes

2009-01-12 14:47:21

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH]: xen: fix too early kmalloc call


* Johannes Weiner <[email protected]> wrote:

> On Mon, Jan 12, 2009 at 09:53:02AM +0100, Ingo Molnar wrote:
> >
> > * Mike Travis <[email protected]> wrote:
> >
> > > Subject: xen: fix too early kmalloc call
> > >
> > > From: Christophe Saout <[email protected]>
> > >
> > > Impact: fixes bootup of xen.
> > >
> > > Christophe reported the following problem:
> > >
> > > > (basically it seems that SLAB is not yet up, with earlyprintk it is
> > > > giving me an Oops in __kmalloc before)
> > >
> > > Replace call to kmalloc with alloc_bootmem.
> > >
> > > Also from Christophe:
> > > > (me)
> > > > > Or I could copy the text and submit it as a new patch?
> > > >
> > > > Yes, I would prefer that. Also, my commit message was not really
> > > > following Kernel standards and it's only a very simple change.
> > >
> > > Signed-off-by: Mike Travis <[email protected]>
> >
> > i've applied the fix to tip/cpus4096, in the form below.
> >
> > Please submit clean log messages. Your commit description was confused -
> > the patch is From: Christophe, but the message talks about Christophe in
> > the third person. Also, it included conversational bits that have no
> > value for the Git history.
> >
> > Ingo
> >
> > -------------------->
> > >>From 28e08861b9afab4168b758fb7b95aa7a4da0f668 Mon Sep 17 00:00:00 2001
> > From: Christophe Saout <[email protected]>
> > Date: Sun, 11 Jan 2009 11:46:23 -0800
> > Subject: [PATCH] xen: fix too early kmalloc call
> >
> > Impact: fix bootup crash on xen guests
> >
> > SLAB is not yet up, with earlyprintk it is giving me an Oops in __kmalloc.
> >
> > Replace call to kmalloc() with alloc_bootmem().
> >
> > Reported-by: Christophe Saout <[email protected]>
> > Signed-off-by: Mike Travis <[email protected]>
> > Signed-off-by: Ingo Molnar <[email protected]>
> > ---
> > drivers/xen/events.c | 5 +++--
> > 1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > index ed7527b..3141e14 100644
> > --- a/drivers/xen/events.c
> > +++ b/drivers/xen/events.c
> > @@ -26,6 +26,7 @@
> > #include <linux/irq.h>
> > #include <linux/module.h>
> > #include <linux/string.h>
> > +#include <linux/bootmem.h>
> >
> > #include <asm/ptrace.h>
> > #include <asm/irq.h>
> > @@ -831,8 +832,8 @@ void __init xen_init_IRQ(void)
> > int i;
> > size_t size = nr_cpu_ids * sizeof(struct cpu_evtchn_s);
> >
> > - cpu_evtchn_mask_p = kmalloc(size, GFP_KERNEL);
> > - BUG_ON(cpu_evtchn_mask == NULL);
> > + cpu_evtchn_mask_p = alloc_bootmem(size);
> > + BUG_ON(cpu_evtchn_mask_p == NULL);
>
> Impossible condition :-) alloc_bootmem() crashes before you could catch
> it here.

yeah ... but that's just a current behavior of bootmem. It's always
prudent to check for allocation errors.

Ingo

2009-01-12 18:20:50

by Mike Travis

[permalink] [raw]
Subject: Re: [PATCH]: xen: fix too early kmalloc call

Ingo Molnar wrote:
> * Mike Travis <[email protected]> wrote:
>
>> Subject: xen: fix too early kmalloc call
>>
>> From: Christophe Saout <[email protected]>
>>
>> Impact: fixes bootup of xen.
>>
>> Christophe reported the following problem:
>>
>>> (basically it seems that SLAB is not yet up, with earlyprintk it is
>>> giving me an Oops in __kmalloc before)
>> Replace call to kmalloc with alloc_bootmem.
>>
>> Also from Christophe:
>>> (me)
>>>> Or I could copy the text and submit it as a new patch?
>>> Yes, I would prefer that. Also, my commit message was not really
>>> following Kernel standards and it's only a very simple change.
>> Signed-off-by: Mike Travis <[email protected]>
>
> i've applied the fix to tip/cpus4096, in the form below.
>
> Please submit clean log messages. Your commit description was confused -
> the patch is From: Christophe, but the message talks about Christophe in
> the third person. Also, it included conversational bits that have no
> value for the Git history.
>
> Ingo

Ahh, ok, will do. I wasn't sure how many "liberties" we can take in
submitting/changing something from someone else.

THanks,
Mike

>
> -------------------->
>>From 28e08861b9afab4168b758fb7b95aa7a4da0f668 Mon Sep 17 00:00:00 2001
> From: Christophe Saout <[email protected]>
> Date: Sun, 11 Jan 2009 11:46:23 -0800
> Subject: [PATCH] xen: fix too early kmalloc call
>
> Impact: fix bootup crash on xen guests
>
> SLAB is not yet up, with earlyprintk it is giving me an Oops in __kmalloc.
>
> Replace call to kmalloc() with alloc_bootmem().
>
> Reported-by: Christophe Saout <[email protected]>
> Signed-off-by: Mike Travis <[email protected]>
> Signed-off-by: Ingo Molnar <[email protected]>
> ---
> drivers/xen/events.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index ed7527b..3141e14 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -26,6 +26,7 @@
> #include <linux/irq.h>
> #include <linux/module.h>
> #include <linux/string.h>
> +#include <linux/bootmem.h>
>
> #include <asm/ptrace.h>
> #include <asm/irq.h>
> @@ -831,8 +832,8 @@ void __init xen_init_IRQ(void)
> int i;
> size_t size = nr_cpu_ids * sizeof(struct cpu_evtchn_s);
>
> - cpu_evtchn_mask_p = kmalloc(size, GFP_KERNEL);
> - BUG_ON(cpu_evtchn_mask == NULL);
> + cpu_evtchn_mask_p = alloc_bootmem(size);
> + BUG_ON(cpu_evtchn_mask_p == NULL);
>
> init_evtchn_cpu_bindings();
>

2009-01-12 18:23:01

by Mike Travis

[permalink] [raw]
Subject: Re: [PATCH]: xen: fix too early kmalloc call

Johannes Weiner wrote:
> On Mon, Jan 12, 2009 at 09:53:02AM +0100, Ingo Molnar wrote:
>> * Mike Travis <[email protected]> wrote:
>>
>>> Subject: xen: fix too early kmalloc call
>>>
>>> From: Christophe Saout <[email protected]>
>>>
>>> Impact: fixes bootup of xen.
>>>
>>> Christophe reported the following problem:
>>>
>>>> (basically it seems that SLAB is not yet up, with earlyprintk it is
>>>> giving me an Oops in __kmalloc before)
>>> Replace call to kmalloc with alloc_bootmem.
>>>
>>> Also from Christophe:
>>>> (me)
>>>>> Or I could copy the text and submit it as a new patch?
>>>> Yes, I would prefer that. Also, my commit message was not really
>>>> following Kernel standards and it's only a very simple change.
>>> Signed-off-by: Mike Travis <[email protected]>
>> i've applied the fix to tip/cpus4096, in the form below.
>>
>> Please submit clean log messages. Your commit description was confused -
>> the patch is From: Christophe, but the message talks about Christophe in
>> the third person. Also, it included conversational bits that have no
>> value for the Git history.
>>
>> Ingo
>>
>> -------------------->
>>> >From 28e08861b9afab4168b758fb7b95aa7a4da0f668 Mon Sep 17 00:00:00 2001
>> From: Christophe Saout <[email protected]>
>> Date: Sun, 11 Jan 2009 11:46:23 -0800
>> Subject: [PATCH] xen: fix too early kmalloc call
>>
>> Impact: fix bootup crash on xen guests
>>
>> SLAB is not yet up, with earlyprintk it is giving me an Oops in __kmalloc.
>>
>> Replace call to kmalloc() with alloc_bootmem().
>>
>> Reported-by: Christophe Saout <[email protected]>
>> Signed-off-by: Mike Travis <[email protected]>
>> Signed-off-by: Ingo Molnar <[email protected]>
>> ---
>> drivers/xen/events.c | 5 +++--
>> 1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
>> index ed7527b..3141e14 100644
>> --- a/drivers/xen/events.c
>> +++ b/drivers/xen/events.c
>> @@ -26,6 +26,7 @@
>> #include <linux/irq.h>
>> #include <linux/module.h>
>> #include <linux/string.h>
>> +#include <linux/bootmem.h>
>>
>> #include <asm/ptrace.h>
>> #include <asm/irq.h>
>> @@ -831,8 +832,8 @@ void __init xen_init_IRQ(void)
>> int i;
>> size_t size = nr_cpu_ids * sizeof(struct cpu_evtchn_s);
>>
>> - cpu_evtchn_mask_p = kmalloc(size, GFP_KERNEL);
>> - BUG_ON(cpu_evtchn_mask == NULL);
>> + cpu_evtchn_mask_p = alloc_bootmem(size);
>> + BUG_ON(cpu_evtchn_mask_p == NULL);
>
> Impossible condition :-) alloc_bootmem() crashes before you could
> catch it here.
>
> Hannes

Yes, that's picked up in a subsequent patch (I didn't catch it the
first time I reviewed it as well.) There appears to be a queue of
patches for Xen coming, so this last "memory reduction patch" will
follow (or be included with) those.

Thanks,
Mike

2009-01-12 19:13:56

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH]: xen: fix too early kmalloc call


* Mike Travis <[email protected]> wrote:

> > i've applied the fix to tip/cpus4096, in the form below.
> >
> > Please submit clean log messages. Your commit description was confused
> > - the patch is From: Christophe, but the message talks about
> > Christophe in the third person. Also, it included conversational bits
> > that have no value for the Git history.
> >
> > Ingo
>
> Ahh, ok, will do. I wasn't sure how many "liberties" we can take in
> submitting/changing something from someone else.

You are free to perfect anyone's commit log in essence - i do it for 90%
of all commits that go via -tip. I fix tyops and grammar mistaiks in every
second commit, and have to fix basic formatting, structure, explanations
and reported-by credits and tags in most of them. Submitting patches with
a well-structured commit log gives you extra credit :-)

[ Obviously you are not free to make a commit log worse and then make it
appear as if the guy who submitted it did it ;-) ]

Ingo

2009-01-12 19:30:57

by Christophe Saout

[permalink] [raw]
Subject: Re: [PATCH]: xen: fix too early kmalloc call

Hi Mike,

> Yes, that's picked up in a subsequent patch (I didn't catch it the
> first time I reviewed it as well.) There appears to be a queue of
> patches for Xen coming, so this last "memory reduction patch" will
> follow (or be included with) those.

It appears Jeremy is on vacation and I also don't know which of the
patches are meant for upstream (there seem still to be some bugs that
might need fixing), and it doesn't really matter if the XEN patches are
updated to your patch or the other way around. The patches in the XEN
mercurial tree already do not apply the way they were before Jeremy went
on vacation, so in my opinion you can as well just go ahead.

Cheers,
Christophe