[PATCH] fix BUG_ON(!PageSlab) from fallback_alloc

[PATCH] fix BUG_ON(!PageSlab) from fallback_alloc

[Hugh Dickins]

pdflush hit the BUG_ON(!PageSlab(page)) in kmem_freepages called from
fallback_alloc: cache_grow already freed those pages when alloc_slabmgmt
failed.  But it wouldn't have freed them if __GFP_NO_GROW, so make sure
fallback_alloc doesn't waste its time on that case.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
___
Fixes a CONFIG_NUMA regression introduced in 2.6.20-rc1.  Or you may
feel it's cleaner for cache_grow to skip its kmem_freepages when objp
is input: patch below is slightly simpler, but I've no strong feeling.

 mm/slab.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- 2.6.20-rc3/mm/slab.c	2007-01-01 10:30:46.000000000 +0000
+++ linux/mm/slab.c	2007-01-04 17:30:11.000000000 +0000
@@ -3281,7 +3281,7 @@ retry:
 					flags | GFP_THISNODE, nid);
 	}
 
-	if (!obj) {
+	if (!obj && !(flags & __GFP_NO_GROW)) {
 		/*
 		 * This allocation will be performed within the constraints
 		 * of the current cpuset / memory policy requirements.
@@ -3310,7 +3310,7 @@ retry:
 					 */
 					goto retry;
 			} else {
-				kmem_freepages(cache, obj);
+				/* cache_grow already freed obj */
 				obj = NULL;
 			}
 		}

Re: [PATCH] fix BUG_ON(!PageSlab) from fallback_alloc

[Pekka Enberg]

Hi Hugh,

[Sorry, no access to kernel tree right now, so can't send a patch.]

On 1/4/07, Hugh Dickins <hugh@veritas.com> wrote:
> @@ -3310,7 +3310,7 @@ retry:
>                                          */
>                                         goto retry;
>                         } else {
> -                               kmem_freepages(cache, obj);
> +                               /* cache_grow already freed obj */
>                                 obj = NULL;

So, how about we rename the current cache_grow() to __cache_grow() and
move the kmem_freepages() to a higher level function like this:

static int cache_grow(struct kmem_cache *cache,
                                gfp_t flags, int nodeid)
{
        void *objp;
        int ret;

        if (flags & __GFP_NO_GROW)
                return 0;

        objp = kmem_getpages(cachep, flags, nodeid);
        if (!objp)
                return 0;

        ret = __cache_grow(cache, flags, nodeid, objp);
        if (!ret)
                kmem_freepages(cachep, objp);

        return ret;
}

And use the non-allocating __cache_grow version() in fallback_alloc() instead?

Re: [PATCH] fix BUG_ON(!PageSlab) from fallback_alloc

[Christoph Lameter]

On Thu, 4 Jan 2007, Hugh Dickins wrote:

> Fixes a CONFIG_NUMA regression introduced in 2.6.20-rc1.  Or you may
> feel it's cleaner for cache_grow to skip its kmem_freepages when objp
> is input: patch below is slightly simpler, but I've no strong feeling.

Acked-by: Christoph Lameter <clameter@sgi.com>

Re: [PATCH] fix BUG_ON(!PageSlab) from fallback_alloc

[Christoph Lameter]

On Thu, 4 Jan 2007, Pekka Enberg wrote:

> So, how about we rename the current cache_grow() to __cache_grow() and
> move the kmem_freepages() to a higher level function like this:
> 
> static int cache_grow(struct kmem_cache *cache,
>                                gfp_t flags, int nodeid)
> {
>        void *objp;
>        int ret;
> 
>        if (flags & __GFP_NO_GROW)
>                return 0;
> 
>        objp = kmem_getpages(cachep, flags, nodeid);
>        if (!objp)
>                return 0;
> 
>        ret = __cache_grow(cache, flags, nodeid, objp);
>        if (!ret)
>                kmem_freepages(cachep, objp);
> 
>        return ret;
> }
> 
> And use the non-allocating __cache_grow version() in fallback_alloc() instead?

Good idea if you can make it so that it is clean. There is some 
additional processing in cache_grow() that would have to be taken into 
account.

Re: [PATCH] fix BUG_ON(!PageSlab) from fallback_alloc

[Hugh Dickins]

On Thu, 4 Jan 2007, Pekka Enberg wrote:

> Hi Hugh,
> 
> [Sorry, no access to kernel tree right now, so can't send a patch.]
> 
> On 1/4/07, Hugh Dickins <hugh@veritas.com> wrote:
> > @@ -3310,7 +3310,7 @@ retry:
> >                                         */
> >                                         goto retry;
> >                         } else {
> > -                               kmem_freepages(cache, obj);
> > +                               /* cache_grow already freed obj */
> >                                 obj = NULL;
> 
> So, how about we rename the current cache_grow() to __cache_grow() and
> move the kmem_freepages() to a higher level function like this:
> 
> static int cache_grow(struct kmem_cache *cache,
>                                gfp_t flags, int nodeid)
> {
>        void *objp;
>        int ret;
> 
>        if (flags & __GFP_NO_GROW)
>                return 0;
> 
>        objp = kmem_getpages(cachep, flags, nodeid);
>        if (!objp)
>                return 0;
> 
>        ret = __cache_grow(cache, flags, nodeid, objp);
>        if (!ret)
>                kmem_freepages(cachep, objp);
> 
>        return ret;
> }
> 
> And use the non-allocating __cache_grow version() in fallback_alloc() instead?

That does indeed look more tasteful.  But there appears to be a fair
bit more refactoring one would want to do, if aiming for good taste
there: the kmem_flagcheck, the conditional local_irq_dis/enable...
I think I'll leave that to you and Christoph to fight over later!

Hugh

Re: [PATCH] fix BUG_ON(!PageSlab) from fallback_alloc

[Pekka Enberg]

On 1/4/07, Hugh Dickins <hugh@veritas.com> wrote:
> That does indeed look more tasteful.  But there appears to be a fair
> bit more refactoring one would want to do, if aiming for good taste
> there: the kmem_flagcheck, the conditional local_irq_dis/enable...
> I think I'll leave that to you and Christoph to fight over later!

Fair enough :-)

Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>

Re: [PATCH] fix BUG_ON(!PageSlab) from fallback_alloc

[Pekka J Enberg]

Hi Christoph,

On Thu, 4 Jan 2007, Christoph Lameter wrote:
> Good idea if you can make it so that it is clean. There is some 
> additional processing in cache_grow() that would have to be taken into 
> account.

Something like this (totally untested) patch?

diff --git a/mm/slab.c b/mm/slab.c
index 0d4e574..48fa397 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1608,8 +1608,19 @@ static void *kmem_getpages(struct kmem_cache *cachep, gfp_t flags, int nodeid)
 {
 	struct page *page;
 	int nr_pages;
+	void *ret;
 	int i;
 
+	/*
+	 * The test for missing atomic flag is performed here, rather than
+	 * the more obvious place, simply to reduce the critical path length
+	 * in kmem_cache_alloc(). If a caller is seriously mis-behaving they
+	 * will eventually be caught here (where it matters).
+	 */
+	kmem_flagcheck(cachep, flags);
+	if (flags & __GFP_WAIT)
+		local_irq_enable();
+
 #ifndef CONFIG_MMU
 	/*
 	 * Nommu uses slab's for process anonymous memory allocations, and thus
@@ -1621,8 +1632,10 @@ static void *kmem_getpages(struct kmem_cache *cachep, gfp_t flags, int nodeid)
 	flags |= cachep->gfpflags;
 
 	page = alloc_pages_node(nodeid, flags, cachep->gfporder);
-	if (!page)
-		return NULL;
+	if (!page) {
+		ret = NULL;
+		goto out;
+	}
 
 	nr_pages = (1 << cachep->gfporder);
 	if (cachep->flags & SLAB_RECLAIM_ACCOUNT)
@@ -1633,7 +1646,12 @@ static void *kmem_getpages(struct kmem_cache *cachep, gfp_t flags, int nodeid)
 			NR_SLAB_UNRECLAIMABLE, nr_pages);
 	for (i = 0; i < nr_pages; i++)
 		__SetPageSlab(page + i);
-	return page_address(page);
+
+	ret = page_address(page);
+  out:
+	if (flags & __GFP_WAIT)
+		local_irq_disable();
+	return ret;
 }
 
 /*
@@ -2714,7 +2732,7 @@ static void slab_map_pages(struct kmem_cache *cache, struct slab *slab,
  * Grow (by 1) the number of slabs within a cache.  This is called by
  * kmem_cache_alloc() when there are no active objs left in a cache.
  */
-static int cache_grow(struct kmem_cache *cachep,
+static int __cache_grow(struct kmem_cache *cachep,
 		gfp_t flags, int nodeid, void *objp)
 {
 	struct slab *slabp;
@@ -2754,39 +2772,17 @@ static int cache_grow(struct kmem_cache *cachep,
 
 	offset *= cachep->colour_off;
 
-	if (local_flags & __GFP_WAIT)
-		local_irq_enable();
-
-	/*
-	 * The test for missing atomic flag is performed here, rather than
-	 * the more obvious place, simply to reduce the critical path length
-	 * in kmem_cache_alloc(). If a caller is seriously mis-behaving they
-	 * will eventually be caught here (where it matters).
-	 */
-	kmem_flagcheck(cachep, flags);
-
-	/*
-	 * Get mem for the objs.  Attempt to allocate a physical page from
-	 * 'nodeid'.
-	 */
-	if (!objp)
-		objp = kmem_getpages(cachep, flags, nodeid);
-	if (!objp)
-		goto failed;
-
 	/* Get slab management. */
 	slabp = alloc_slabmgmt(cachep, objp, offset,
 			local_flags & ~GFP_THISNODE, nodeid);
 	if (!slabp)
-		goto opps1;
+		return 0;
 
 	slabp->nodeid = nodeid;
 	slab_map_pages(cachep, slabp, objp);
 
 	cache_init_objs(cachep, slabp, ctor_flags);
 
-	if (local_flags & __GFP_WAIT)
-		local_irq_disable();
 	check_irq_off();
 	spin_lock(&l3->list_lock);
 
@@ -2796,12 +2792,28 @@ static int cache_grow(struct kmem_cache *cachep,
 	l3->free_objects += cachep->num;
 	spin_unlock(&l3->list_lock);
 	return 1;
-opps1:
-	kmem_freepages(cachep, objp);
-failed:
-	if (local_flags & __GFP_WAIT)
-		local_irq_disable();
-	return 0;
+}
+
+static int cache_grow(struct kmem_cache *cachep, gfp_t flags, int nodeid)
+{
+	void *objp;
+	int ret;
+
+	if (flags & __GFP_NO_GROW)
+		return 0;
+
+	/*
+	 * Get mem for the objs.  Attempt to allocate a physical page from
+	 * 'nodeid'.
+	 */
+	objp = kmem_getpages(cachep, flags, nodeid);
+	if (!objp)
+		return 0;
+	
+	ret = __cache_grow(cachep, flags, nodeid, obj);
+	if (!ret)
+		kmem_freepages(cachep, objp);
+	return ret;
 }
 
 #if DEBUG
@@ -3014,7 +3026,7 @@ alloc_done:
 
 	if (unlikely(!ac->avail)) {
 		int x;
-		x = cache_grow(cachep, flags | GFP_THISNODE, node, NULL);
+		x = cache_grow(cachep, flags | GFP_THISNODE, node);
 
 		/* cache_grow can reenable interrupts, then ac could change. */
 		ac = cpu_cache_get(cachep);
@@ -3288,18 +3300,13 @@ retry:
 		 * We may trigger various forms of reclaim on the allowed
 		 * set and go into memory reserves if necessary.
 		 */
-		if (local_flags & __GFP_WAIT)
-			local_irq_enable();
-		kmem_flagcheck(cache, flags);
 		obj = kmem_getpages(cache, flags, -1);
-		if (local_flags & __GFP_WAIT)
-			local_irq_disable();
 		if (obj) {
 			/*
 			 * Insert into the appropriate per node queues
 			 */
 			nid = page_to_nid(virt_to_page(obj));
-			if (cache_grow(cache, flags, nid, obj)) {
+			if (__cache_grow(cache, flags, nid, obj)) {
 				obj = ____cache_alloc_node(cache,
 					flags | GFP_THISNODE, nid);
 				if (!obj)
@@ -3370,7 +3377,7 @@ retry:
 
 must_grow:
 	spin_unlock(&l3->list_lock);
-	x = cache_grow(cachep, flags | GFP_THISNODE, nodeid, NULL);
+	x = cache_grow(cachep, flags | GFP_THISNODE, nodeid);
 	if (x)
 		goto retry;
 

Re: [PATCH] fix BUG_ON(!PageSlab) from fallback_alloc

[Christoph Lameter]

On Thu, 4 Jan 2007, Pekka J Enberg wrote:

> Something like this (totally untested) patch?

Yup. Moving the GFP_WAIT processing into kmem_getpages() will clean up a 
lot.