[patch] slub: avoid label inside conditional

[David Rientjes <rientjes@xxxxxxxxxx>]

On Thu, 12 May 2011, Christoph Lameter wrote:

> > I'd much prefer to just add a
> >
> > 	c->node = page_to_nid(page);
> >
> > rather than the new label and goto into a conditional.
> >
> > >  	}
> > >  	if (!(gfpflags & __GFP_NOWARN) && printk_ratelimit())
> > >  		slab_out_of_memory(s, gfpflags, node);
> >
> 
> Hmmm... Looks like we also missed to use the label.
> 

It was used in the same patch it was introduced:

@@ -1828,7 +1828,6 @@ load_freelist:
 	c->freelist = get_freepointer(s, object);
 	page->inuse = page->objects;
 	page->freelist = NULL;
-	c->node = page_to_nid(page);
 
 unlock_out:
 	slab_unlock(page);
@@ -1845,8 +1844,10 @@ another_slab:
 new_slab:
 	page = get_partial(s, gfpflags, node);
 	if (page) {
-		c->page = page;
 		stat(s, ALLOC_FROM_PARTIAL);
+load_from_page:
+		c->node = page_to_nid(page);
+		c->page = page;
 		goto load_freelist;
 	}
 
@@ -1867,8 +1868,8 @@ new_slab:
 
 		slab_lock(page);
 		__SetPageSlubFrozen(page);
-		c->page = page;
-		goto load_freelist;
+
+		goto load_from_page;
 	}
 	if (!(gfpflags & __GFP_NOWARN) && printk_ratelimit())
 		slab_out_of_memory(s, gfpflags, node);

> 
> Subject: slub: Fix control flow in slab_alloc
> 
> Signed-off-by: Christoph Lameter <cl@xxxxxxxxx>
> 
> ---
>  mm/slub.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6/mm/slub.c
> ===================================================================
> --- linux-2.6.orig/mm/slub.c	2011-05-12 11:41:44.000000000 -0500
> +++ linux-2.6/mm/slub.c	2011-05-12 11:42:25.000000000 -0500
> @@ -1833,7 +1833,6 @@ new_slab:
>  	page = get_partial(s, gfpflags, node);
>  	if (page) {
>  		stat(s, ALLOC_FROM_PARTIAL);
> -load_from_page:
>  		c->node = page_to_nid(page);
>  		c->page = page;
>  		goto load_freelist;
> @@ -1856,6 +1855,7 @@ load_from_page:
> 
>  		slab_lock(page);
>  		__SetPageSlubFrozen(page);
> +		c->node = page_to_nid(page);
>  		c->page = page;
>  		goto load_freelist;
>  	}
> 

So this doesn't apply on top of the stack.


slub: avoid label inside conditional

Jumping to a label inside a conditional is considered poor style, 
especially considering the current organization of __slab_alloc().

This removes the 'load_from_page' label and just duplicates the three 
lines of code that it uses:

	c->node = page_to_nid(page);
	c->page = page;
	goto load_freelist;

since it's probably not worth making this a separate helper function.

Signed-off-by: David Rientjes <rientjes@xxxxxxxxxx>
---
 mm/slub.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1845,7 +1845,6 @@ new_slab:
 	page = get_partial(s, gfpflags, node);
 	if (page) {
 		stat(s, ALLOC_FROM_PARTIAL);
-load_from_page:
 		c->node = page_to_nid(page);
 		c->page = page;
 		goto load_freelist;
@@ -1868,8 +1867,9 @@ load_from_page:
 
 		slab_lock(page);
 		__SetPageSlubFrozen(page);
-
-		goto load_from_page;
+		c->node = page_to_nid(page);
+		c->page = page;
+		goto load_freelist;
 	}
 	if (!(gfpflags & __GFP_NOWARN) && printk_ratelimit())
 		slab_out_of_memory(s, gfpflags, node);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>