Re: introducing __GFP_PANIC

[Cyrill Gorcunov]

[Pekka Enberg - Sun, May 03, 2009 at 08:38:01PM +0300]
| On Sun, May 3, 2009 at 8:23 PM, Cyrill Gorcunov <gorcunov@xxxxxxxxx> 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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/