Re: introducing __GFP_PANIC

Cyrill Gorcunov
2009-05-03T20:46:06Z

[Pekka Enberg - Sun, May 03, 2009 at 08:38:01PM +0300]
| On Sun, May 3, 2009 at 8:23 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > | > Hi Pekka,
| > | >
| > | > ufortunatelly __alloc_pages_internal is not the only place where
| > | > we do return NULL from kmalloc. As example - failslab facility
| > | > (in slab_alloc call). Anyway -- I'll take a closer look.
| > |
| > | Right. I think failslab needs some fixing _not_ to return NULL if
| > | __GFP_PANIC is set.
| > |
| >
| > Ok, as a first raw draft (_not_ covering all the cases) it could
| > be something like this. It touches only __alloc_pages_internal
| > and we have to bespread as well:
| >
| > 1) alloc_pages with order >= MAX_ORDER (gfp.h)
| > 2) the same for alloc_pages_node (both used by SLOB)
| > 3) all __kmalloc should be explored as well.
| > 4) ???
| >
| > Anyway -- take a look on __alloc_pages_internal part :)
| >
| >        -- Cyrill
| >
| > ---
| >  include/linux/gfp.h |    4 +++-
| >  mm/page_alloc.c     |    8 +++++---
| >  2 files changed, 8 insertions(+), 4 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/page_alloc.c
| > =====================================================================
| > --- linux-2.6.git.orig/mm/page_alloc.c
| > +++ linux-2.6.git/mm/page_alloc.c
| > @@ -1496,7 +1496,7 @@ __alloc_pages_internal(gfp_t gfp_mask, u
| >        might_sleep_if(wait);
| >
| >        if (should_fail_alloc_page(gfp_mask, order))
| > -               return NULL;
| > +               goto nopage;
|
| The point of fault injection is to increase coverage out-of-memory
| error handling code. So this doesn't make much sense to me. Why would
| you want to cause a __GFP_PANIC call-sites to panic()? It doesn't help
| testing one bit.
|
| So I still think you should just fix up should_fail_alloc_page() _not_
| to return true if __GFP_PANIC is set.
|

Just before I get some sleep. Here is an another attempt.

Not covered cases I know of:

1) SLAB with builtin constant as size and we don't find appropriate
cache for it do return NULL
2) alloc_pages_node and alloc_pages do check for MAX_ORDER which
could be exceeded as well and return NULL then.

ok, here is a preliminary patch just to hear some complains :)

-- Cyrill
---
include/linux/gfp.h | 4 +++-
mm/failslab.c | 3 +++
mm/page_alloc.c | 8 ++++++--
3 files changed, 12 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,9 @@ nopage:
dump_stack();
show_mem();
}
- return page;
+ if (unlikely(gfp_mask & __GFP_PANIC))
+ panic("Out of memory: panic due to __GFP_PANIC\n");
+ return NULL;
got_pg:
if (kmemcheck_enabled)
kmemcheck_pagealloc_alloc(page, order, gfp_mask);