[PATCH 1/2] mm: Introduce GFP_PANIC for early-boot allocations

[PATCH 1/2] mm: Introduce GFP_PANIC for early-boot allocations

[Pekka Enberg]

From: Pekka Enberg <penberg@cs.helsinki.fi>

This patch introduces a GFP_PANIC flag that can be used as an annotation
that an early-boot call to kmalloc() or alloc_pages() is expected to
never fail.

To save one GFP flag bit, use a combination of __GFP_NOFAIL and
__GFP_NOREPEAT to make sure we always end up in the "nopage" path of the
page allocator if an allocation fails.

[ gorcunov@openvz.org: initial version of the patch ]
Cc: Christoph Lameter <cl@linux-foundation.org>
Cc: David Rientjes <rientjes@google.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Mel Gorman <mel@csn.ul.ie>
Cc: Rik van Riel <riel@redhat.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
Andrew, this one is rebased on top of -mm.

 include/linux/gfp.h |    1 +
 mm/page_alloc.c     |   12 ++++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index c7429b8..2e76e7b 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -71,6 +71,7 @@ struct vm_area_struct;
 #define GFP_HIGHUSER_MOVABLE	(__GFP_WAIT | __GFP_IO | __GFP_FS | \
 				 __GFP_HARDWALL | __GFP_HIGHMEM | \
 				 __GFP_MOVABLE)
+#define GFP_PANIC	(__GFP_NOFAIL | __GFP_NORETRY)
 
 #ifdef CONFIG_NUMA
 #define GFP_THISNODE	(__GFP_THISNODE | __GFP_NOWARN | __GFP_NORETRY)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index be0ed90..eb8d953 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1516,8 +1516,11 @@ static inline int
 should_alloc_retry(gfp_t gfp_mask, unsigned int order,
 				unsigned long pages_reclaimed)
 {
-	/* Do not loop if specifically requested */
-	if (gfp_mask & __GFP_NORETRY)
+	/*
+	 * Do not loop if specifically requested. Note: GFP_PANIC sets both
+	 * flags.
+	 */
+	if ((gfp_mask & __GFP_NORETRY) && !(gfp_mask & __GFP_NOFAIL))
 		return 0;
 
 	/*
@@ -1813,6 +1816,11 @@ rebalance:
 	}
 
 nopage:
+	if (unlikely((gfp_mask & GFP_PANIC) == GFP_PANIC)) {
+		show_mem();
+		panic("Out of memory: %s order: %d, gfp_mask:0x%x\n",
+			p->comm, order, gfp_mask);
+	}
 	if (!(gfp_mask & __GFP_NOWARN) && printk_ratelimit()) {
 		printk(KERN_WARNING "%s: page allocation failure."
 			" order:%d, mode:0x%x\n",
-- 
1.5.6.3

Re: [PATCH 1/2] mm: Introduce GFP_PANIC for early-boot allocations

[Christoph Lameter]

On Fri, 8 May 2009, Pekka Enberg wrote:

> From: Pekka Enberg <penberg@cs.helsinki.fi>
>
> This patch introduces a GFP_PANIC flag that can be used as an annotation
> that an early-boot call to kmalloc() or alloc_pages() is expected to
> never fail.

I think the early-boot requirement needs to be dropped. Make this safe to
be used at anytime.

Re: [PATCH 1/2] mm: Introduce GFP_PANIC for early-boot allocations

[Pekka Enberg]

On Fri, 2009-05-08 at 11:29 -0400, Christoph Lameter wrote:
> On Fri, 8 May 2009, Pekka Enberg wrote:
> 
> > From: Pekka Enberg <penberg@cs.helsinki.fi>
> >
> > This patch introduces a GFP_PANIC flag that can be used as an annotation
> > that an early-boot call to kmalloc() or alloc_pages() is expected to
> > never fail.
> 
> I think the early-boot requirement needs to be dropped. Make this safe to
> be used at anytime.

OK, here's an updated patch description:

Subject: [PATCH 1/2] mm: Introduce GFP_PANIC
From: Pekka Enberg <penberg@cs.helsinki.fi>

This patch introduces a GFP_PANIC flag that can be used as an annotation
that a kmalloc() or alloc_pages() call is expected to never fail which is
useful for early-boot code, for example.

To save one GFP flag bit, use a combination of __GFP_NOFAIL and
__GFP_NOREPEAT to make sure we always end up in the "nopage" path of the
page allocator if an allocation fails.

[ gorcunov@openvz.org: initial version of the patch ]
Cc: Christoph Lameter <cl@linux-foundation.org>
Cc: David Rientjes <rientjes@google.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Mel Gorman <mel@csn.ul.ie>
Cc: Rik van Riel <riel@redhat.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
Andrew, this one is rebased on top of -mm.

 include/linux/gfp.h |    1 +
 mm/page_alloc.c     |   12 ++++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index c7429b8..2e76e7b 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -71,6 +71,7 @@ struct vm_area_struct;
 #define GFP_HIGHUSER_MOVABLE	(__GFP_WAIT | __GFP_IO | __GFP_FS | \
 				 __GFP_HARDWALL | __GFP_HIGHMEM | \
 				 __GFP_MOVABLE)
+#define GFP_PANIC	(__GFP_NOFAIL | __GFP_NORETRY)
 
 #ifdef CONFIG_NUMA
 #define GFP_THISNODE	(__GFP_THISNODE | __GFP_NOWARN | __GFP_NORETRY)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index be0ed90..eb8d953 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1516,8 +1516,11 @@ static inline int
 should_alloc_retry(gfp_t gfp_mask, unsigned int order,
 				unsigned long pages_reclaimed)
 {
-	/* Do not loop if specifically requested */
-	if (gfp_mask & __GFP_NORETRY)
+	/*
+	 * Do not loop if specifically requested. Note: GFP_PANIC sets both
+	 * flags.
+	 */
+	if ((gfp_mask & __GFP_NORETRY) && !(gfp_mask & __GFP_NOFAIL))
 		return 0;
 
 	/*
@@ -1813,6 +1816,11 @@ rebalance:
 	}
 
 nopage:
+	if (unlikely((gfp_mask & GFP_PANIC) == GFP_PANIC)) {
+		show_mem();
+		panic("Out of memory: %s order: %d, gfp_mask:0x%x\n",
+			p->comm, order, gfp_mask);
+	}
 	if (!(gfp_mask & __GFP_NOWARN) && printk_ratelimit()) {
 		printk(KERN_WARNING "%s: page allocation failure."
 			" order:%d, mode:0x%x\n",
-- 
1.5.6.3


 

Re: [PATCH 1/2] mm: Introduce GFP_PANIC for early-boot allocations

[Ingo Molnar]

* Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> On Fri, 2009-05-08 at 11:29 -0400, Christoph Lameter wrote:
> > On Fri, 8 May 2009, Pekka Enberg wrote:
> > 
> > > From: Pekka Enberg <penberg@cs.helsinki.fi>
> > >
> > > This patch introduces a GFP_PANIC flag that can be used as an annotation
> > > that an early-boot call to kmalloc() or alloc_pages() is expected to
> > > never fail.
> > 
> > I think the early-boot requirement needs to be dropped. Make this safe to
> > be used at anytime.
> 
> OK, here's an updated patch description:
> 
> Subject: [PATCH 1/2] mm: Introduce GFP_PANIC
> From: Pekka Enberg <penberg@cs.helsinki.fi>
> 
> This patch introduces a GFP_PANIC flag that can be used as an annotation
> that a kmalloc() or alloc_pages() call is expected to never fail which is
> useful for early-boot code, for example.
> 
> To save one GFP flag bit, use a combination of __GFP_NOFAIL and
> __GFP_NOREPEAT to make sure we always end up in the "nopage" path of the
> page allocator if an allocation fails.
> 
> [ gorcunov@openvz.org: initial version of the patch ]
> Cc: Christoph Lameter <cl@linux-foundation.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: Rik van Riel <riel@redhat.com>
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>

Thanks Pekka for taking care of this,

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [PATCH 1/2] mm: Introduce GFP_PANIC for early-boot allocations

[Andrew Morton]

On Fri, 08 May 2009 18:10:28 +0300
Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> +#define GFP_PANIC	(__GFP_NOFAIL | __GFP_NORETRY)

urgh, you have to be kidding me.  This significantly worsens complexity
and risk in core MM and it's just yuk.

I think we can justify pulling such dopey party tricks to save
pageframe space, or bits in page.flags and such.  But just to save a
scrap of memory which would have been released during boot anwyay? 
Don't think so.

I'd suggest panic_if_null().  Or just leave everything alone - it's
hardly a pressing problem.

Re: [PATCH 1/2] mm: Introduce GFP_PANIC for early-boot allocations

[Pekka Enberg]

Hi Andrew,

Andrew Morton wrote:
> On Fri, 08 May 2009 18:10:28 +0300
> Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> 
>> +#define GFP_PANIC	(__GFP_NOFAIL | __GFP_NORETRY)
> 
> urgh, you have to be kidding me.  This significantly worsens complexity
> and risk in core MM and it's just yuk.
> 
> I think we can justify pulling such dopey party tricks to save
> pageframe space, or bits in page.flags and such.  But just to save a
> scrap of memory which would have been released during boot anwyay? 
> Don't think so.

No, I wasn't kidding and I don't agree that it "significantly worsens 
complexity". The point is not to save memory but to clearly annotate 
those special call-sites that really don't need to check for out-of-memory.

But anyway, if you don't want it, then I guess it stays out of the kernel.

Andrew Morton wrote:
> I'd suggest panic_if_null().  Or just leave everything alone - it's
> hardly a pressing problem.

It has no advantage over the current BUG_ON() pattern.

			Pekka

Re: [PATCH 1/2] mm: Introduce GFP_PANIC for early-boot allocations

[Ingo Molnar]

* Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> Hi Andrew,
>
> Andrew Morton wrote:
>> On Fri, 08 May 2009 18:10:28 +0300
>> Pekka Enberg <penberg@cs.helsinki.fi> wrote:
>>
>>> +#define GFP_PANIC	(__GFP_NOFAIL | __GFP_NORETRY)
>>
>> urgh, you have to be kidding me.  This significantly worsens complexity
>> and risk in core MM and it's just yuk.
>>
>> I think we can justify pulling such dopey party tricks to save 
>> pageframe space, or bits in page.flags and such.  But just to 
>> save a scrap of memory which would have been released during boot 
>> anwyay?  Don't think so.
>
> No, I wasn't kidding and I don't agree that it "significantly 
> worsens complexity". The point is not to save memory but to 
> clearly annotate those special call-sites that really don't need 
> to check for out-of-memory.

Frankly, i cannot believe that so many smart people dont see the 
simple, universal, un-arguable truism in the following statement:

 it is shorter, tidier, more maintainable, more reviewable to write:

	ptr = kmalloc(GFP_BOOT, size);

 than to write:

	ptr = kmalloc(GFP_KERNEL, size);
	BUG_ON(!ptr);

We have a lot of such patterns in platform code. Dozens and dozens 
of them.

There _might_ be some more nuanced second-level arguments: "yes, I 
agree in principle, but complexity elsewhere or other side-effects 
make this a net negative change."

Alas, those arguments would be wrong too:

 - we have a lot of such patterns and GFP_BOOT is unambigious 

 - post-bootup mis-use of GFP_BOOT could be warned about 
   unconditionally if used after free_initmem(), if we care enough.

 - Pekka's patch is dead simple. There's no "complexity" anywhere.

Agreeing to this and introducing this should have been a matter of 
30 seconds of thinking. Why are we still arguing about this? Dont we 
have enough bugs to worry about?

	Ingo

Re: [PATCH 1/2] mm: Introduce GFP_PANIC for early-boot allocations

[Pekka Enberg]

Ingo Molnar wrote:
> * Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> 
>> Hi Andrew,
>>
>> Andrew Morton wrote:
>>> On Fri, 08 May 2009 18:10:28 +0300
>>> Pekka Enberg <penberg@cs.helsinki.fi> wrote:
>>>
>>>> +#define GFP_PANIC	(__GFP_NOFAIL | __GFP_NORETRY)
>>> urgh, you have to be kidding me.  This significantly worsens complexity
>>> and risk in core MM and it's just yuk.
>>>
>>> I think we can justify pulling such dopey party tricks to save 
>>> pageframe space, or bits in page.flags and such.  But just to 
>>> save a scrap of memory which would have been released during boot 
>>> anwyay?  Don't think so.
>> No, I wasn't kidding and I don't agree that it "significantly 
>> worsens complexity". The point is not to save memory but to 
>> clearly annotate those special call-sites that really don't need 
>> to check for out-of-memory.
> 
> Frankly, i cannot believe that so many smart people dont see the 
> simple, universal, un-arguable truism in the following statement:
> 
>  it is shorter, tidier, more maintainable, more reviewable to write:
> 
> 	ptr = kmalloc(GFP_BOOT, size);
> 
>  than to write:
> 
> 	ptr = kmalloc(GFP_KERNEL, size);
> 	BUG_ON(!ptr);

Hey, that's a much better name! I guess we don't need to support 
GFP_ATOMIC? I'll repost the series with Peter's system_state != BOOTING 
warning. Lets see if that makes the patch more palatable to Andrew.

			Pekka

Re: [PATCH 1/2] mm: Introduce GFP_PANIC for early-boot allocations

[Ingo Molnar]

* Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> Ingo Molnar wrote:
>> * Pekka Enberg <penberg@cs.helsinki.fi> wrote:
>>
>>> Hi Andrew,
>>>
>>> Andrew Morton wrote:
>>>> On Fri, 08 May 2009 18:10:28 +0300
>>>> Pekka Enberg <penberg@cs.helsinki.fi> wrote:
>>>>
>>>>> +#define GFP_PANIC	(__GFP_NOFAIL | __GFP_NORETRY)
>>>> urgh, you have to be kidding me.  This significantly worsens complexity
>>>> and risk in core MM and it's just yuk.
>>>>
>>>> I think we can justify pulling such dopey party tricks to save  
>>>> pageframe space, or bits in page.flags and such.  But just to save 
>>>> a scrap of memory which would have been released during boot  
>>>> anwyay?  Don't think so.
>>> No, I wasn't kidding and I don't agree that it "significantly  
>>> worsens complexity". The point is not to save memory but to clearly 
>>> annotate those special call-sites that really don't need to check for 
>>> out-of-memory.
>>
>> Frankly, i cannot believe that so many smart people dont see the  
>> simple, universal, un-arguable truism in the following statement:
>>
>>  it is shorter, tidier, more maintainable, more reviewable to write:
>>
>> 	ptr = kmalloc(GFP_BOOT, size);
>>
>>  than to write:
>>
>> 	ptr = kmalloc(GFP_KERNEL, size);
>> 	BUG_ON(!ptr);
>
> Hey, that's a much better name! I guess we don't need to support 
> GFP_ATOMIC? I'll repost the series with Peter's system_state != 
> BOOTING warning. Lets see if that makes the patch more palatable 
> to Andrew.

Hey, and as we all know, names do matter! We are much better off 
hacking Linux not Freax, after all.

Successful projects need good names, and anyone who doesnt know that 
must be a true git. Hm, wait a minute ...

	Ingo

Re: [PATCH 1/2] mm: Introduce GFP_PANIC for early-boot allocations

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

>  it is shorter, tidier, more maintainable, more reviewable to write:
> 
> 	ptr = kmalloc(GFP_BOOT, size);
> 
>  than to write:
> 
> 	ptr = kmalloc(GFP_KERNEL, size);
> 	BUG_ON(!ptr);

[ Oh, and everyone saw the kernel bug i hid, and did notice it in 
  the first variant sooner than in the second, right? :) ]

	Ingo

Re: [PATCH 1/2] mm: Introduce GFP_PANIC for early-boot allocations

[Andrew Morton]

On Sat, 9 May 2009 11:19:11 +0200 Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> 
> > Hi Andrew,
> >
> > Andrew Morton wrote:
> >> On Fri, 08 May 2009 18:10:28 +0300
> >> Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> >>
> >>> +#define GFP_PANIC	(__GFP_NOFAIL | __GFP_NORETRY)
> >>
> >> urgh, you have to be kidding me.  This significantly worsens complexity
> >> and risk in core MM and it's just yuk.
> >>
> >> I think we can justify pulling such dopey party tricks to save 
> >> pageframe space, or bits in page.flags and such.  But just to 
> >> save a scrap of memory which would have been released during boot 
> >> anwyay?  Don't think so.
> >
> > No, I wasn't kidding and I don't agree that it "significantly 
> > worsens complexity". The point is not to save memory but to 
> > clearly annotate those special call-sites that really don't need 
> > to check for out-of-memory.
> 
> Frankly, i cannot believe that so many smart people dont see the 
> simple, universal, un-arguable truism in the following statement:
> 
>  it is shorter, tidier, more maintainable, more reviewable to write:
> 
> 	ptr = kmalloc(GFP_BOOT, size);
> 
>  than to write:
> 
> 	ptr = kmalloc(GFP_KERNEL, size);
> 	BUG_ON(!ptr);
> 
> We have a lot of such patterns in platform code. Dozens and dozens 
> of them.
> 
> There _might_ be some more nuanced second-level arguments: "yes, I 
> agree in principle, but complexity elsewhere or other side-effects 
> make this a net negative change."
> 
> Alas, those arguments would be wrong too:
> 
>  - we have a lot of such patterns and GFP_BOOT is unambigious 
> 
>  - post-bootup mis-use of GFP_BOOT could be warned about 
>    unconditionally if used after free_initmem(), if we care enough.
> 
>  - Pekka's patch is dead simple. There's no "complexity" anywhere.
> 
> Agreeing to this and introducing this should have been a matter of 
> 30 seconds of thinking. Why are we still arguing about this? Dont we 
> have enough bugs to worry about?
> 

Your reply has nothing at all to do with the email to which you
replied.  How strange.


Here's an example:

void *some_library_function(..., gfp_t gfpflags)
{
	...
	p = kmalloc(..., gfpflags | __GFP_NOFAIL);
	...
}

Now here we have a nice little hand-grenade.  If someone accidentally
does

	some_library_function(..., GFP_KERNEL | __GFP_NORETRY);

their code will happily pass testing.  Except one day someone's kernel
will run out of memory and instead of handling it properly, their kernel
will panic.

Another issue is that now all memory allocation functions which inspect
__GFP_NOFAIL or __GFP_NORETRY need to remember to check for the other
flag to filter out GFP_PANIC.  


Those two flags were well-chosen and it's a good bet that they will
never be used together.  But it's not certain, and the _results_ of
that mistake are really bad: a very small number of machines will crash
very rarely.


We have plenty of bits free there - six, I think.  And we could get
four more if needed by overlaying the `enum mapping_flags' on top of
certain __GFP_* flags and adding appropriate masking in
mapping_gfp_mask().

So I suggest that we add a new __GFP_foo.