[PATCH -tip] mm: introduce __GFP_PANIC modifier

[PATCH -tip] mm: introduce __GFP_PANIC modifier

[Cyrill Gorcunov]

Hi,

here is an attempt to bring in __GFP_PANIC modifier.
The patch is made on top of -tip repo. I've been
trying to apply it in top of -mm tree but it
seems -tip is a bit newer, at least it already
has __GFP_BITS_SHIFT = 22 defined.

Mel, could you take a look?

For easier review -- here is what is done:
1) __GFP_PANIC introduced
2) __alloc_pages_internal now checks for this flag
   and panic if needed.

	-- Cyrill
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: [PATCH -tip] mm: introduce __GFP_PANIC modifier

Sometime we need that memory obtained via kmalloc
is always granted cause if there is not enough memory
we just can't go further.

For such a case we introduce __GFP_PANIC modificator
If memory can't be granted -- we just panic and halt.

Note 1: it trigger panic only if we reach out-of-memory
situation. MAX_ORDER and SLAB built-in constants are
not covered by intent.

Note 2: __GFP_PANIC implicitly turn off failslab
facility on such kind of calls.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 include/linux/gfp.h |    4 +++-
 mm/failslab.c       |    3 +++
 mm/page_alloc.c     |   10 ++++++++--
 3 files changed, 14 insertions(+), 3 deletions(-)

Index: linux-2.6.git/include/linux/gfp.h
=====================================================================
--- linux-2.6.git.orig/include/linux/gfp.h
+++ linux-2.6.git/include/linux/gfp.h
@@ -58,7 +58,9 @@ struct vm_area_struct;
 #define __GFP_NOTRACK	((__force gfp_t)0)
 #endif
 
-#define __GFP_BITS_SHIFT 22	/* Room for 22 __GFP_FOO bits */
+#define __GFP_PANIC	((__force gfp_t)0x400000u) /* Panic on page alloction failure */
+
+#define __GFP_BITS_SHIFT 23	/* Room for 23 __GFP_FOO bits */
 #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
 
 /* This equals 0, but use constants in case they ever change */
Index: linux-2.6.git/mm/failslab.c
=====================================================================
--- linux-2.6.git.orig/mm/failslab.c
+++ linux-2.6.git/mm/failslab.c
@@ -17,6 +17,9 @@ bool should_failslab(size_t size, gfp_t 
 	if (gfpflags & __GFP_NOFAIL)
 		return false;
 
+	if (gfpflags & __GFP_PANIC)
+		return false;
+
         if (failslab.ignore_gfp_wait && (gfpflags & __GFP_WAIT))
 		return false;
 
Index: linux-2.6.git/mm/page_alloc.c
=====================================================================
--- linux-2.6.git.orig/mm/page_alloc.c
+++ linux-2.6.git/mm/page_alloc.c
@@ -1185,6 +1185,8 @@ static int should_fail_alloc_page(gfp_t 
 		return 0;
 	if (gfp_mask & __GFP_NOFAIL)
 		return 0;
+	if (gfp_mask & __GFP_PANIC)
+		return 0;
 	if (fail_page_alloc.ignore_gfp_highmem && (gfp_mask & __GFP_HIGHMEM))
 		return 0;
 	if (fail_page_alloc.ignore_gfp_wait && (gfp_mask & __GFP_WAIT))
@@ -1506,7 +1508,7 @@ restart:
 		 * Happens if we have an empty zonelist as a result of
 		 * GFP_THISNODE being used on a memoryless node
 		 */
-		return NULL;
+		goto nopage;
 	}
 
 	page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, nodemask, order,
@@ -1685,7 +1687,11 @@ nopage:
 		dump_stack();
 		show_mem();
 	}
-	return page;
+	if (unlikely(gfp_mask & __GFP_PANIC))
+		panic("Out of memory: panic due to __GFP_PANIC."
+			" %s order:%d, gfp_mask:0x%x\n", p->comm,
+			order, gfp_mask);
+	return NULL;
 got_pg:
 	if (kmemcheck_enabled)
 		kmemcheck_pagealloc_alloc(page, order, gfp_mask);

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> Hi,
> 
> here is an attempt to bring in __GFP_PANIC modifier.
> The patch is made on top of -tip repo. I've been
> trying to apply it in top of -mm tree but it
> seems -tip is a bit newer, at least it already
> has __GFP_BITS_SHIFT = 22 defined.

Please create a patch against -mm instead. The one in -tip is bumped 
up by:

  8cd4001: kmemcheck: add hooks for the page allocator
  e6df103: kmemcheck: add mm functions

But we dont want to mix that into your patch.

Thanks,

	Ingo

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Cyrill Gorcunov]

[Ingo Molnar - Mon, May 04, 2009 at 03:13:11PM +0200]
| 
| * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
| 
| > Hi,
| > 
| > here is an attempt to bring in __GFP_PANIC modifier.
| > The patch is made on top of -tip repo. I've been
| > trying to apply it in top of -mm tree but it
| > seems -tip is a bit newer, at least it already
| > has __GFP_BITS_SHIFT = 22 defined.
| 
| Please create a patch against -mm instead. The one in -tip is bumped 
| up by:
| 
|   8cd4001: kmemcheck: add hooks for the page allocator
|   e6df103: kmemcheck: add mm functions
| 
| But we dont want to mix that into your patch.
| 
| Thanks,
| 
| 	Ingo
| 

ok, will do

	-- Cyrill

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Christoph Lameter]

Could you try to avoid consuming another GFP flag? __GFP_BITS_SHIFT is
used elsewhere to figure out where to put miscellanous flags into the gfp
mask. This is pretty limited right now and so the patch does work.

But maybe we can combine some flags __GFP_NOFAIL | __GFP_REPEAT or so?

__GFP_NOFAIL already has the semantics that the allocation cannot fail and
that the page allocator must never return NULL.

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Cyrill Gorcunov]

[Christoph Lameter - Mon, May 04, 2009 at 09:47:45AM -0400]
| 
| 
| Could you try to avoid consuming another GFP flag? __GFP_BITS_SHIFT is
| used elsewhere to figure out where to put miscellanous flags into the gfp
| mask. This is pretty limited right now and so the patch does work.
| 
| But maybe we can combine some flags __GFP_NOFAIL | __GFP_REPEAT or so?
| 
| __GFP_NOFAIL already has the semantics that the allocation cannot fail and
| that the page allocator must never return NULL.
| 
| 

As I see page allocator already quite modified in -mm tree.
Christoph will kmalloc with (__GFP_NOFAIL | __GFP_REPEAT)
inform us on exhausted memory? Or we just stuck with blank
screen instead (not blank actually but with previous messages)?

	-- Cyrill

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Christoph Lameter]

On Mon, 4 May 2009, Cyrill Gorcunov wrote:

> As I see page allocator already quite modified in -mm tree.
> Christoph will kmalloc with (__GFP_NOFAIL | __GFP_REPEAT)
> inform us on exhausted memory? Or we just stuck with blank
> screen instead (not blank actually but with previous messages)?

If you do not specify __GFP_NOWARN then it should give you messages,

Guess we need to very that it behaves in the right wya.

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Cyrill Gorcunov]

[Christoph Lameter - Mon, May 04, 2009 at 10:13:34AM -0400]
| On Mon, 4 May 2009, Cyrill Gorcunov wrote:
| 
| > As I see page allocator already quite modified in -mm tree.
| > Christoph will kmalloc with (__GFP_NOFAIL | __GFP_REPEAT)
| > inform us on exhausted memory? Or we just stuck with blank
| > screen instead (not blank actually but with previous messages)?
| 
| If you do not specify __GFP_NOWARN then it should give you messages,
| 
| Guess we need to very that it behaves in the right wya.
|

thanks Christoph, I need to read a modified version (which
is in -mm tree now). Maybe we could find a way indeed :)

	-- Cyrill

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Cyrill Gorcunov]

[Cyrill Gorcunov - Mon, May 04, 2009 at 06:29:03PM +0400]
| [Christoph Lameter - Mon, May 04, 2009 at 10:13:34AM -0400]
| | On Mon, 4 May 2009, Cyrill Gorcunov wrote:
| | 
| | > As I see page allocator already quite modified in -mm tree.
| | > Christoph will kmalloc with (__GFP_NOFAIL | __GFP_REPEAT)
| | > inform us on exhausted memory? Or we just stuck with blank
| | > screen instead (not blank actually but with previous messages)?
| | 
| | If you do not specify __GFP_NOWARN then it should give you messages,
| | 
| | Guess we need to very that it behaves in the right wya.
| |
| 
| thanks Christoph, I need to read a modified version (which
| is in -mm tree now). Maybe we could find a way indeed :)
| 
| 	-- Cyrill

You know I only found a message in case if page is already
not granted (ie NULL is going to be returned) and right
before that we print a warning about that (nopage: label).
Which means - no panic here and with (__GFP_NOFAIL | __GFP_REPEAT)
just spinning around in attepmt to allocate new memory. And how
to behave on atomic allocations. Almost give up... :)

	-- Cyrill

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Christoph Lameter]

On Mon, 4 May 2009, Cyrill Gorcunov wrote:

> You know I only found a message in case if page is already
> not granted (ie NULL is going to be returned) and right
> before that we print a warning about that (nopage: label).
> Which means - no panic here and with (__GFP_NOFAIL | __GFP_REPEAT)
> just spinning around in attepmt to allocate new memory. And how
> to behave on atomic allocations. Almost give up... :)

Could you check for __GFP_FAIL|__GFP_REPEAT somewheres and panic? Not sure
if __GFP_REPEAT is the right second flag to use.

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Cyrill Gorcunov]

[Christoph Lameter - Mon, May 04, 2009 at 11:54:53AM -0400]
| On Mon, 4 May 2009, Cyrill Gorcunov wrote:
| 
| > You know I only found a message in case if page is already
| > not granted (ie NULL is going to be returned) and right
| > before that we print a warning about that (nopage: label).
| > Which means - no panic here and with (__GFP_NOFAIL | __GFP_REPEAT)
| > just spinning around in attepmt to allocate new memory. And how
| > to behave on atomic allocations. Almost give up... :)
| 
| Could you check for __GFP_FAIL|__GFP_REPEAT somewheres and panic? Not sure
| if __GFP_REPEAT is the right second flag to use.
| 

OK, I thought about various mods combinations too. Will recheck
if there a way more precise. Thanks!

	-- Cyrill

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Andrew Morton]

On Mon, 4 May 2009 09:47:45 -0400 (EDT)
Christoph Lameter <cl@linux.com> wrote:

> 
> 
> Could you try to avoid consuming another GFP flag? __GFP_BITS_SHIFT is
> used elsewhere to figure out where to put miscellanous flags into the gfp
> mask. This is pretty limited right now and so the patch does work.

hm, yes, there are seven bits left.

afaict bit 3 (0x08) is unused?

Is __GFP_PANIC very useful?  I expect it will permit a very small code
saving at a relatively small number of callsites, all of which are
__init anyway?

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Cyrill Gorcunov]

[Christoph Lameter - Mon, May 04, 2009 at 11:54:53AM -0400]
| On Mon, 4 May 2009, Cyrill Gorcunov wrote:
| 
| > You know I only found a message in case if page is already
| > not granted (ie NULL is going to be returned) and right
| > before that we print a warning about that (nopage: label).
| > Which means - no panic here and with (__GFP_NOFAIL | __GFP_REPEAT)
| > just spinning around in attepmt to allocate new memory. And how
| > to behave on atomic allocations. Almost give up... :)
| 
| Could you check for __GFP_FAIL|__GFP_REPEAT somewheres and panic? Not sure
| if __GFP_REPEAT is the right second flag to use.
| 

Well Christoph I've checked those flags (and though for some
max fail iterations counter as well which was then refused).
None of combination seems to satisfy this requirement (don't
return NULL on out-of-memory but panic instead) in clear way.
There is a question remains opened how to behave on a case if
allocation was atomic. Just dunno.

If it is critical to not bring new __GFP_PANIC then
I could be just using __GFP_NOFAIL and nofity a user via
printk that we're in stage of allocating memory (anyway
it looks strange that we have a number of callers with
__GFP_NOFAIL with BUG_ON/if-checks following. But I could
me missing).

	-- Cyrill

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Cyrill Gorcunov]

[Andrew Morton - Mon, May 04, 2009 at 11:53:35AM -0700]
| On Mon, 4 May 2009 09:47:45 -0400 (EDT)
| Christoph Lameter <cl@linux.com> wrote:
| 
| > 
| > 
| > Could you try to avoid consuming another GFP flag? __GFP_BITS_SHIFT is
| > used elsewhere to figure out where to put miscellanous flags into the gfp
| > mask. This is pretty limited right now and so the patch does work.
| 
| hm, yes, there are seven bits left.
| 
| afaict bit 3 (0x08) is unused?
| 
| Is __GFP_PANIC very useful?  I expect it will permit a very small code
| saving at a relatively small number of callsites, all of which are
| __init anyway?
| 

Actually the issue with possible NULL deref already fixed in -tip
tree commit 9a8709d. There was rather an idea on what will be more
convenient -- call for __GFP_NOFAIL or use BUG_ON on allocation
failure or call kmalloc with __GFP_PANIC and forget about if it could
fail :). Since Christoph said that it's discommended to introduce
new flag -- I'm fine with that.

	-- Cyrill

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Mel Gorman]

On Mon, May 04, 2009 at 04:27:40PM +0400, Cyrill Gorcunov wrote:
> Hi,
> 
> here is an attempt to bring in __GFP_PANIC modifier.
> The patch is made on top of -tip repo. I've been
> trying to apply it in top of -mm tree but it
> seems -tip is a bit newer, at least it already
> has __GFP_BITS_SHIFT = 22 defined.
> 
> Mel, could you take a look?
> 

I'm not seeing any users of the new flag so I'm not sure what your intended
use for the flag is.  Maybe it would have a small saving on call-sites that
panic but as they are boot-time functions, I would expect the memory is
getting freed anyway.

If this is about should_failslab(), can it be determined that we should not
fail another way? For example, never randomly fail the allocation if the
system is booting and __GFP_NOFAIL is specified.

> For easier review -- here is what is done:
> 1) __GFP_PANIC introduced
> 2) __alloc_pages_internal now checks for this flag
>    and panic if needed.
> 
> 	-- Cyrill
> ---
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Subject: [PATCH -tip] mm: introduce __GFP_PANIC modifier
> 
> Sometime we need that memory obtained via kmalloc
> is always granted cause if there is not enough memory
> we just can't go further.
> 
> For such a case we introduce __GFP_PANIC modificator
> If memory can't be granted -- we just panic and halt.
> 
> Note 1: it trigger panic only if we reach out-of-memory
> situation. MAX_ORDER and SLAB built-in constants are
> not covered by intent.
> 
> Note 2: __GFP_PANIC implicitly turn off failslab
> facility on such kind of calls.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>
> ---
>  include/linux/gfp.h |    4 +++-
>  mm/failslab.c       |    3 +++
>  mm/page_alloc.c     |   10 ++++++++--
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6.git/include/linux/gfp.h
> =====================================================================
> --- linux-2.6.git.orig/include/linux/gfp.h
> +++ linux-2.6.git/include/linux/gfp.h
> @@ -58,7 +58,9 @@ struct vm_area_struct;
>  #define __GFP_NOTRACK	((__force gfp_t)0)
>  #endif
>  
> -#define __GFP_BITS_SHIFT 22	/* Room for 22 __GFP_FOO bits */
> +#define __GFP_PANIC	((__force gfp_t)0x400000u) /* Panic on page alloction failure */
> +
> +#define __GFP_BITS_SHIFT 23	/* Room for 23 __GFP_FOO bits */
>  #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
>  
>  /* This equals 0, but use constants in case they ever change */
> Index: linux-2.6.git/mm/failslab.c
> =====================================================================
> --- linux-2.6.git.orig/mm/failslab.c
> +++ linux-2.6.git/mm/failslab.c
> @@ -17,6 +17,9 @@ bool should_failslab(size_t size, gfp_t 
>  	if (gfpflags & __GFP_NOFAIL)
>  		return false;
>  
> +	if (gfpflags & __GFP_PANIC)
> +		return false;
> +
>          if (failslab.ignore_gfp_wait && (gfpflags & __GFP_WAIT))
>  		return false;
>  
> Index: linux-2.6.git/mm/page_alloc.c
> =====================================================================
> --- linux-2.6.git.orig/mm/page_alloc.c
> +++ linux-2.6.git/mm/page_alloc.c
> @@ -1185,6 +1185,8 @@ static int should_fail_alloc_page(gfp_t 
>  		return 0;
>  	if (gfp_mask & __GFP_NOFAIL)
>  		return 0;
> +	if (gfp_mask & __GFP_PANIC)
> +		return 0;
>  	if (fail_page_alloc.ignore_gfp_highmem && (gfp_mask & __GFP_HIGHMEM))
>  		return 0;
>  	if (fail_page_alloc.ignore_gfp_wait && (gfp_mask & __GFP_WAIT))
> @@ -1506,7 +1508,7 @@ restart:
>  		 * Happens if we have an empty zonelist as a result of
>  		 * GFP_THISNODE being used on a memoryless node
>  		 */
> -		return NULL;
> +		goto nopage;
>  	}
>  
>  	page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, nodemask, order,
> @@ -1685,7 +1687,11 @@ nopage:
>  		dump_stack();
>  		show_mem();
>  	}
> -	return page;
> +	if (unlikely(gfp_mask & __GFP_PANIC))
> +		panic("Out of memory: panic due to __GFP_PANIC."
> +			" %s order:%d, gfp_mask:0x%x\n", p->comm,
> +			order, gfp_mask);
> +	return NULL;
>  got_pg:
>  	if (kmemcheck_enabled)
>  		kmemcheck_pagealloc_alloc(page, order, gfp_mask);
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Andrew Morton]

On Mon, 4 May 2009 23:21:17 +0400
Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> [Andrew Morton - Mon, May 04, 2009 at 11:53:35AM -0700]
> | On Mon, 4 May 2009 09:47:45 -0400 (EDT)
> | Christoph Lameter <cl@linux.com> wrote:
> | 
> | > 
> | > 
> | > Could you try to avoid consuming another GFP flag? __GFP_BITS_SHIFT is
> | > used elsewhere to figure out where to put miscellanous flags into the gfp
> | > mask. This is pretty limited right now and so the patch does work.
> | 
> | hm, yes, there are seven bits left.
> | 
> | afaict bit 3 (0x08) is unused?
> | 
> | Is __GFP_PANIC very useful?  I expect it will permit a very small code
> | saving at a relatively small number of callsites, all of which are
> | __init anyway?
> | 
> 
> Actually the issue with possible NULL deref already fixed in -tip
> tree commit 9a8709d. There was rather an idea on what will be more
> convenient -- call for __GFP_NOFAIL or use BUG_ON on allocation
> failure or call kmalloc with __GFP_PANIC and forget about if it could
> fail :). Since Christoph said that it's discommended to introduce
> new flag -- I'm fine with that.
> 

Well.  Allowing the caller to dereference the NULL pointer is a good
solution.  It gives us the runtime behaviour we want, and requires no
new code at all.  Usages of this technique should be commented so that
people don't "fix" it.


An alternative to __GFP_PANIC might be:

-	foo = kmalloc(size, GFP_KERNEL);
+	foo = panic_if_null(kmalloc(size, GFP_KERNEL));

void *panic_if_null(void *p)
{
	if (unlikely(p == NULL))
		panic("NULL pointer panic");
	return p;
}

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Cyrill Gorcunov]

[Mel Gorman - Mon, May 04, 2009 at 08:23:43PM +0100]
| On Mon, May 04, 2009 at 04:27:40PM +0400, Cyrill Gorcunov wrote:
| > Hi,
| > 
| > here is an attempt to bring in __GFP_PANIC modifier.
| > The patch is made on top of -tip repo. I've been
| > trying to apply it in top of -mm tree but it
| > seems -tip is a bit newer, at least it already
| > has __GFP_BITS_SHIFT = 22 defined.
| > 
| > Mel, could you take a look?
| > 
| 
| I'm not seeing any users of the new flag so I'm not sure what your intended
| use for the flag is.  Maybe it would have a small saving on call-sites that
| panic but as they are boot-time functions, I would expect the memory is
| getting freed anyway.

Yes they are boot-time, UV apic init at moment (already fixed via BUG_ON).
But I already noticed by Christoph that it's not a good idea to bring
this flag in. Sorry for bothering.

| 
| If this is about should_failslab(), can it be determined that we should not
| fail another way? For example, never randomly fail the allocation if the
| system is booting and __GFP_NOFAIL is specified.
| 
| > For easier review -- here is what is done:
| > 1) __GFP_PANIC introduced
| > 2) __alloc_pages_internal now checks for this flag
| >    and panic if needed.
| > 
| > 	-- Cyrill

Re: [PATCH -tip] mm: introduce __GFP_PANIC modifier

[Cyrill Gorcunov]

[Andrew Morton - Mon, May 04, 2009 at 12:34:53PM -0700]
| On Mon, 4 May 2009 23:21:17 +0400
| Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > [Andrew Morton - Mon, May 04, 2009 at 11:53:35AM -0700]
| > | On Mon, 4 May 2009 09:47:45 -0400 (EDT)
| > | Christoph Lameter <cl@linux.com> wrote:
| > | 
| > | > 
| > | > 
| > | > Could you try to avoid consuming another GFP flag? __GFP_BITS_SHIFT is
| > | > used elsewhere to figure out where to put miscellanous flags into the gfp
| > | > mask. This is pretty limited right now and so the patch does work.
| > | 
| > | hm, yes, there are seven bits left.
| > | 
| > | afaict bit 3 (0x08) is unused?
| > | 
| > | Is __GFP_PANIC very useful?  I expect it will permit a very small code
| > | saving at a relatively small number of callsites, all of which are
| > | __init anyway?
| > | 
| > 
| > Actually the issue with possible NULL deref already fixed in -tip
| > tree commit 9a8709d. There was rather an idea on what will be more
| > convenient -- call for __GFP_NOFAIL or use BUG_ON on allocation
| > failure or call kmalloc with __GFP_PANIC and forget about if it could
| > fail :). Since Christoph said that it's discommended to introduce
| > new flag -- I'm fine with that.
| > 
| 
| Well.  Allowing the caller to dereference the NULL pointer is a good
| solution.  It gives us the runtime behaviour we want, and requires no
| new code at all.  Usages of this technique should be commented so that
| people don't "fix" it.
| 
| 
| An alternative to __GFP_PANIC might be:
| 
| -	foo = kmalloc(size, GFP_KERNEL);
| +	foo = panic_if_null(kmalloc(size, GFP_KERNEL));
| 
| void *panic_if_null(void *p)
| {
| 	if (unlikely(p == NULL))
| 		panic("NULL pointer panic");
| 	return p;
| }
| 

Yes, it's simple and elegant but at this point I'm not
going to bring this one -- I could be just shot :)

	-- Cyrill

[RFC/PATCH v2] mm: Introduce GFP_PANIC for non-failing allocations

[Pekka Enberg]

From: Cyrill Gorcunov <gorcunov@openvz.org>

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

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

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
Yes, this is totally untested and needs to be rebased on top of -mm.

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

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 0bbc15f..c7c8a63 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -70,6 +70,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 | __GFP_NOMEMALLOC)
 
 #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 e2f2699..4b062e5 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1670,6 +1670,9 @@ nopage:
 		dump_stack();
 		show_mem();
 	}
+	if (unlikely(gfp_mask & GFP_PANIC))
+		panic("Out of memory: %s order: %d, gfp_mask:0x%x\n",
+			p->comm, order, gfp_mask);
 got_pg:
 	return page;
 }
-- 
1.5.6.3

Re: [RFC/PATCH v2] mm: Introduce GFP_PANIC for non-failing allocations

[Cyrill Gorcunov]

[Pekka Enberg - Fri, May 08, 2009 at 03:57:28PM +0300]
| From: Cyrill Gorcunov <gorcunov@openvz.org>
| 
| This patch introduces a GFP_PANIC flag that can be used as an annotation
| that a call to kmalloc() or alloc_pages() is expected to never fail.
| This is useful in early boot code, for example.
| 
| To save one GFP flag bit, use a combination of __GFP_NOFAIL,
| __GFP_NOREPEAT, and __GFP_NOMEMALLOC to make sure we always end up in
| the "nopage" path of the page allocator if an allocation fails.
| 
| Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
| Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
| ---
| Yes, this is totally untested and needs to be rebased on top of -mm.
| 
|  include/linux/gfp.h |    1 +
|  mm/page_alloc.c     |    3 +++
|  2 files changed, 4 insertions(+), 0 deletions(-)
| 
| diff --git a/include/linux/gfp.h b/include/linux/gfp.h
| index 0bbc15f..c7c8a63 100644
| --- a/include/linux/gfp.h
| +++ b/include/linux/gfp.h
| @@ -70,6 +70,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 | __GFP_NOMEMALLOC)
|  
|  #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 e2f2699..4b062e5 100644
| --- a/mm/page_alloc.c
| +++ b/mm/page_alloc.c
| @@ -1670,6 +1670,9 @@ nopage:
|  		dump_stack();
|  		show_mem();
|  	}
| +	if (unlikely(gfp_mask & GFP_PANIC))
| +		panic("Out of memory: %s order: %d, gfp_mask:0x%x\n",
| +			p->comm, order, gfp_mask);
|  got_pg:
|  	return page;
|  }
| -- 
| 1.5.6.3
|

Thanks Pekka! At moment it's usefull for x86 uv_system_init (3 times)
for early boot code. Plain panic_on_null (suggested by Andrew) could
be usefull as well but would lead to additional wrapper around kmalloc
which is already inlined. Which means -- at moment we have those cases
handled by BUG_ON but having one common option would be better I guess
(ie have an ability to say page allocator that just do panic on fail).

Anyway, if there is still objections on such an approach (even having
gpf bits saved by elegant way Pekka suggested) -- I'm fine with having
those BUG_ONs instead of short and gracious GFP_PANIC :)

	-- Cyrill

Re: [RFC/PATCH v2] mm: Introduce GFP_PANIC for non-failing allocations

[Pekka Enberg]

On Fri, 2009-05-08 at 17:37 +0400, Cyrill Gorcunov wrote:
> Thanks Pekka! At moment it's usefull for x86 uv_system_init (3 times)
> for early boot code. Plain panic_on_null (suggested by Andrew) could
> be usefull as well but would lead to additional wrapper around kmalloc
> which is already inlined. Which means -- at moment we have those cases
> handled by BUG_ON but having one common option would be better I guess
> (ie have an ability to say page allocator that just do panic on fail).
> 
> Anyway, if there is still objections on such an approach (even having
> gpf bits saved by elegant way Pekka suggested) -- I'm fine with having
> those BUG_ONs instead of short and gracious GFP_PANIC :)

There are such BUG_ONs in SLUB, for example, and probably elsewhere in
early-boot code.

			Pekka

Re: [RFC/PATCH v2] mm: Introduce GFP_PANIC for non-failing allocations

[Christoph Lameter]

On Fri, 8 May 2009, Pekka Enberg wrote:

> There are such BUG_ONs in SLUB, for example, and probably elsewhere in
> early-boot code.

Thus Pekka's patch would allow us to reduce code in all these cases.

Re: [RFC/PATCH v2] mm: Introduce GFP_PANIC for non-failing allocations

[Christoph Lameter]

On Fri, 8 May 2009, Pekka Enberg wrote:

> +#define GFP_PANIC	(__GFP_NOFAIL | __GFP_NORETRY | __GFP_NOMEMALLOC)

So this means not retrying the allocation a couple of times? Not delving
into reserve pools? Such behavior is good for a allocation that causes a
panic if it fails?

Re: [RFC/PATCH v2] mm: Introduce GFP_PANIC for non-failing allocations

[Pekka Enberg]

On Fri, 8 May 2009, Pekka Enberg wrote:
> > +#define GFP_PANIC	(__GFP_NOFAIL | __GFP_NORETRY | __GFP_NOMEMALLOC)

On Fri, 2009-05-08 at 10:20 -0400, Christoph Lameter wrote:
> So this means not retrying the allocation a couple of times? Not delving
> into reserve pools? Such behavior is good for a allocation that causes a
> panic if it fails?

If you do GFP_KERNEL|GFP_PANIC, we will cond_resched() and retry if we
made some progress. So yes, I think the behavior is good for early-boot
call-sites that can't really fail anyway.

			Pekka

Re: [RFC/PATCH v2] mm: Introduce GFP_PANIC for non-failing allocations

[Christoph Lameter]

On Fri, 8 May 2009, Pekka Enberg wrote:

> On Fri, 8 May 2009, Pekka Enberg wrote:
> > > +#define GFP_PANIC	(__GFP_NOFAIL | __GFP_NORETRY | __GFP_NOMEMALLOC)
>
> On Fri, 2009-05-08 at 10:20 -0400, Christoph Lameter wrote:
> > So this means not retrying the allocation a couple of times? Not delving
> > into reserve pools? Such behavior is good for a allocation that causes a
> > panic if it fails?
>
> If you do GFP_KERNEL|GFP_PANIC, we will cond_resched() and retry if we
> made some progress. So yes, I think the behavior is good for early-boot
> call-sites that can't really fail anyway.

Better make sure that GFP_PANIC is only used during early boot then.

If memory is low on boot (due to node hotplug or some such thing, powerpc
may do evil tricks here) then the panic may trigger after the patch.
We would have just delved into the reserves a bit before.

Re: [RFC/PATCH v2] mm: Introduce GFP_PANIC for non-failing allocations

[Pekka Enberg]

On Fri, 2009-05-08 at 10:29 -0400, Christoph Lameter wrote:
> On Fri, 8 May 2009, Pekka Enberg wrote:
> 
> > On Fri, 8 May 2009, Pekka Enberg wrote:
> > > > +#define GFP_PANIC	(__GFP_NOFAIL | __GFP_NORETRY | __GFP_NOMEMALLOC)
> >
> > On Fri, 2009-05-08 at 10:20 -0400, Christoph Lameter wrote:
> > > So this means not retrying the allocation a couple of times? Not delving
> > > into reserve pools? Such behavior is good for a allocation that causes a
> > > panic if it fails?
> >
> > If you do GFP_KERNEL|GFP_PANIC, we will cond_resched() and retry if we
> > made some progress. So yes, I think the behavior is good for early-boot
> > call-sites that can't really fail anyway.
> 
> Better make sure that GFP_PANIC is only used during early boot then.
> 
> If memory is low on boot (due to node hotplug or some such thing, powerpc
> may do evil tricks here) then the panic may trigger after the patch.
> We would have just delved into the reserves a bit before.

Nah, it's probably better to drop __GFP_NOMEMALLOC instead. Does this
look better?

			Pekka

>From c91c70265545f2fcc727b4a0d37162b5aa0f5ecf Mon Sep 17 00:00:00 2001
From: Cyrill Gorcunov <gorcunov@openvz.org>
Date: Fri, 8 May 2009 15:44:50 +0300
Subject: [PATCH] mm: Introduce GFP_PANIC for non-failing allocations

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

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

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 include/linux/gfp.h |    1 +
 mm/page_alloc.c     |    6 +++++-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 0bbc15f..b34e6e5 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -70,6 +70,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 e2f2699..de7f666 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1556,7 +1556,8 @@ nofail_alloc:
 				zonelist, high_zoneidx, ALLOC_NO_WATERMARKS);
 			if (page)
 				goto got_pg;
-			if (gfp_mask & __GFP_NOFAIL) {
+			/* GFP_PANIC sets both flags */
+			if ((gfp_mask & __GFP_NOFAIL) && !(gfp_mask & __GFP_NORETRY)) {
 				congestion_wait(WRITE, HZ/50);
 				goto nofail_alloc;
 			}
@@ -1670,6 +1671,9 @@ nopage:
 		dump_stack();
 		show_mem();
 	}
+	if (unlikely(gfp_mask & GFP_PANIC))
+		panic("Out of memory: %s order: %d, gfp_mask:0x%x\n",
+			p->comm, order, gfp_mask);
 got_pg:
 	return page;
 }
-- 
1.5.6.3

Re: [RFC/PATCH v2] mm: Introduce GFP_PANIC for non-failing allocations

[Rik van Riel]

Pekka Enberg wrote:
> On Fri, 2009-05-08 at 10:29 -0400, Christoph Lameter wrote:
>> On Fri, 8 May 2009, Pekka Enberg wrote:
>>
>>> On Fri, 8 May 2009, Pekka Enberg wrote:
>>>>> +#define GFP_PANIC	(__GFP_NOFAIL | __GFP_NORETRY | __GFP_NOMEMALLOC)
>>> On Fri, 2009-05-08 at 10:20 -0400, Christoph Lameter wrote:
>>>> So this means not retrying the allocation a couple of times? Not delving
>>>> into reserve pools? Such behavior is good for a allocation that causes a
>>>> panic if it fails?
>>> If you do GFP_KERNEL|GFP_PANIC, we will cond_resched() and retry if we
>>> made some progress. So yes, I think the behavior is good for early-boot
>>> call-sites that can't really fail anyway.
>> Better make sure that GFP_PANIC is only used during early boot then.
>>
>> If memory is low on boot (due to node hotplug or some such thing, powerpc
>> may do evil tricks here) then the panic may trigger after the patch.
>> We would have just delved into the reserves a bit before.
> 
> Nah, it's probably better to drop __GFP_NOMEMALLOC instead. Does this
> look better?
> 
> 			Pekka
> 
>>From c91c70265545f2fcc727b4a0d37162b5aa0f5ecf Mon Sep 17 00:00:00 2001
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Date: Fri, 8 May 2009 15:44:50 +0300
> Subject: [PATCH] mm: Introduce GFP_PANIC for non-failing allocations
> 
> This patch introduces a GFP_PANIC flag that can be used as an annotation
> that a call to kmalloc() or alloc_pages() is expected to never fail.
> This is useful in early boot code, for example.
> 
> To save one GFP flag bit, use a combination of __GFP_NOFAIL,
> __GFP_NOREPEAT, and __GFP_NOMEMALLOC to make sure we always end up in
> the "nopage" path of the page allocator if an allocation fails.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
> ---
>  include/linux/gfp.h |    1 +
>  mm/page_alloc.c     |    6 +++++-
>  2 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 0bbc15f..b34e6e5 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -70,6 +70,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 e2f2699..de7f666 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1556,7 +1556,8 @@ nofail_alloc:
>  				zonelist, high_zoneidx, ALLOC_NO_WATERMARKS);
>  			if (page)
>  				goto got_pg;
> -			if (gfp_mask & __GFP_NOFAIL) {
> +			/* GFP_PANIC sets both flags */
> +			if ((gfp_mask & __GFP_NOFAIL) && !(gfp_mask & __GFP_NORETRY)) {
>  				congestion_wait(WRITE, HZ/50);

Do you mean ((gfp_mask & GFP_PANIC) == GFP_PANIC))  ?

> @@ -1670,6 +1671,9 @@ nopage:
>  		dump_stack();
>  		show_mem();
>  	}
> +	if (unlikely(gfp_mask & GFP_PANIC))
> +		panic("Out of memory: %s order: %d, gfp_mask:0x%x\n",
> +			p->comm, order, gfp_mask);

Ditto here.

-- 
All rights reversed.

Re: [RFC/PATCH v2] mm: Introduce GFP_PANIC for non-failing allocations

[Pekka Enberg]

Hi Rik,

On Fri, 2009-05-08 at 10:37 -0400, Rik van Riel wrote:
> > @@ -1556,7 +1556,8 @@ nofail_alloc:
> >  				zonelist, high_zoneidx, ALLOC_NO_WATERMARKS);
> >  			if (page)
> >  				goto got_pg;
> > -			if (gfp_mask & __GFP_NOFAIL) {
> > +			/* GFP_PANIC sets both flags */
> > +			if ((gfp_mask & __GFP_NOFAIL) && !(gfp_mask & __GFP_NORETRY)) {
> >  				congestion_wait(WRITE, HZ/50);
> 
> Do you mean ((gfp_mask & GFP_PANIC) == GFP_PANIC))  ?

No, we only want to loop if __GFP_NOFAIL is set but __GFP_NORETRY is not
set.

> 
> > @@ -1670,6 +1671,9 @@ nopage:
> >  		dump_stack();
> >  		show_mem();
> >  	}
> > +	if (unlikely(gfp_mask & GFP_PANIC))
> > +		panic("Out of memory: %s order: %d, gfp_mask:0x%x\n",
> > +			p->comm, order, gfp_mask);
> 
> Ditto here.

Yes, indeed. Fixed up. Thanks!

Re: [RFC/PATCH v2] mm: Introduce GFP_PANIC for non-failing allocations

[Christoph Lameter]

On Fri, 8 May 2009, Pekka Enberg wrote:

>  		show_mem();
>  	}
> +	if (unlikely(gfp_mask & GFP_PANIC))

(gfp_mask & GFP_PANIC) == GFP_PANIC please. Otherwise any one of the bits
alone will cause a panic.