[Slub cleanup6 0/5] SLUB: Cleanups V6

[Slub cleanup6 0/5] SLUB: Cleanups V6

[Christoph Lameter]

Cleanups that resulted from the lockless work

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>

[Slub cleanup6 1/5] slub: Use NUMA_NO_NODE in get_partial

[Christoph Lameter]

[-- Attachment #1: use_numa_nonode --]
[-- Type: text/plain, Size: 988 bytes --]

A -1 was leftover during the conversion.

Signed-off-by: Christoph Lameter <cl@linux.com>

---
 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-04-15 14:27:58.000000000 -0500
+++ linux-2.6/mm/slub.c	2011-04-15 14:28:18.000000000 -0500
@@ -1487,7 +1487,7 @@ static struct page *get_partial(struct k
 	int searchnode = (node == NUMA_NO_NODE) ? numa_node_id() : node;
 
 	page = get_partial_node(get_node(s, searchnode));
-	if (page || node != -1)
+	if (page || node != NUMA_NO_NODE)
 		return page;
 
 	return get_any_partial(s, flags);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>

[Slub cleanup6 2/5] slub: get_map() function to establish map of free objects in a slab

[Christoph Lameter]

[-- Attachment #1: slub_slowpath_get_map --]
[-- Type: text/plain, Size: 2992 bytes --]

The bit map of free objects in a slab page is determined in various functions
if debugging is enabled.

Provide a common function for that purpose.

Signed-off-by: Christoph Lameter <cl@linux.com>

---
 mm/slub.c |   34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c	2011-03-30 14:09:27.000000000 -0500
+++ linux-2.6/mm/slub.c	2011-03-30 14:30:24.000000000 -0500
@@ -271,10 +271,6 @@ static inline void set_freepointer(struc
 	for (__p = (__addr); __p < (__addr) + (__objects) * (__s)->size;\
 			__p += (__s)->size)
 
-/* Scan freelist */
-#define for_each_free_object(__p, __s, __free) \
-	for (__p = (__free); __p; __p = get_freepointer((__s), __p))
-
 /* Determine object index from a given position */
 static inline int slab_index(void *p, struct kmem_cache *s, void *addr)
 {
@@ -330,6 +326,21 @@ static inline int oo_objects(struct kmem
 	return x.x & OO_MASK;
 }
 
+/*
+ * Determine a map of object in use on a page.
+ *
+ * Slab lock or node listlock must be held to guarantee that the page does
+ * not vanish from under us.
+ */
+static void get_map(struct kmem_cache *s, struct page *page, unsigned long *map)
+{
+	void *p;
+	void *addr = page_address(page);
+
+	for (p = page->freelist; p; p = get_freepointer(s, p))
+		set_bit(slab_index(p, s, addr), map);
+}
+
 #ifdef CONFIG_SLUB_DEBUG
 /*
  * Debug settings:
@@ -2673,9 +2684,8 @@ static void list_slab_objects(struct kme
 		return;
 	slab_err(s, page, "%s", text);
 	slab_lock(page);
-	for_each_free_object(p, s, page->freelist)
-		set_bit(slab_index(p, s, addr), map);
 
+	get_map(s, page, map);
 	for_each_object(p, s, addr, page->objects) {
 
 		if (!test_bit(slab_index(p, s, addr), map)) {
@@ -3610,10 +3620,11 @@ static int validate_slab(struct kmem_cac
 	/* Now we know that a valid freelist exists */
 	bitmap_zero(map, page->objects);
 
-	for_each_free_object(p, s, page->freelist) {
-		set_bit(slab_index(p, s, addr), map);
-		if (!check_object(s, page, p, SLUB_RED_INACTIVE))
-			return 0;
+	get_map(s, page, map);
+	for_each_object(p, s, addr, page->objects) {
+		if (test_bit(slab_index(p, s, addr), map))
+			if (!check_object(s, page, p, SLUB_RED_INACTIVE))
+				return 0;
 	}
 
 	for_each_object(p, s, addr, page->objects)
@@ -3821,8 +3832,7 @@ static void process_slab(struct loc_trac
 	void *p;
 
 	bitmap_zero(map, page->objects);
-	for_each_free_object(p, s, page->freelist)
-		set_bit(slab_index(p, s, addr), map);
+	get_map(s, page, map);
 
 	for_each_object(p, s, addr, page->objects)
 		if (!test_bit(slab_index(p, s, addr), map))

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>

[Slub cleanup6 3/5] slub: Eliminate repeated use of c->page through a new page variable

[Christoph Lameter]

[-- Attachment #1: avoid_c_page --]
[-- Type: text/plain, Size: 3180 bytes --]

__slab_alloc is full of "c->page" repeats. Lets just use one local variable
named "page" for this. Also avoids the need to a have another variable
called "new".

Signed-off-by: Christoph Lameter <cl@linux.com>

---
 mm/slub.c |   41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c	2011-03-30 14:30:24.000000000 -0500
+++ linux-2.6/mm/slub.c	2011-03-30 14:30:51.000000000 -0500
@@ -1790,7 +1790,7 @@ static void *__slab_alloc(struct kmem_ca
 			  unsigned long addr, struct kmem_cache_cpu *c)
 {
 	void **object;
-	struct page *new;
+	struct page *page;
 #ifdef CONFIG_CMPXCHG_LOCAL
 	unsigned long flags;
 
@@ -1808,28 +1808,30 @@ static void *__slab_alloc(struct kmem_ca
 	/* We handle __GFP_ZERO in the caller */
 	gfpflags &= ~__GFP_ZERO;
 
-	if (!c->page)
+	page = c->page;
+	if (!page)
 		goto new_slab;
 
-	slab_lock(c->page);
+	slab_lock(page);
 	if (unlikely(!node_match(c, node)))
 		goto another_slab;
 
 	stat(s, ALLOC_REFILL);
 
 load_freelist:
-	object = c->page->freelist;
+	object = page->freelist;
 	if (unlikely(!object))
 		goto another_slab;
 	if (kmem_cache_debug(s))
 		goto debug;
 
 	c->freelist = get_freepointer(s, object);
-	c->page->inuse = c->page->objects;
-	c->page->freelist = NULL;
-	c->node = page_to_nid(c->page);
+	page->inuse = page->objects;
+	page->freelist = NULL;
+	c->node = page_to_nid(page);
+
 unlock_out:
-	slab_unlock(c->page);
+	slab_unlock(page);
 #ifdef CONFIG_CMPXCHG_LOCAL
 	c->tid = next_tid(c->tid);
 	local_irq_restore(flags);
@@ -1841,9 +1843,9 @@ another_slab:
 	deactivate_slab(s, c);
 
 new_slab:
-	new = get_partial(s, gfpflags, node);
-	if (new) {
-		c->page = new;
+	page = get_partial(s, gfpflags, node);
+	if (page) {
+		c->page = page;
 		stat(s, ALLOC_FROM_PARTIAL);
 		goto load_freelist;
 	}
@@ -1852,19 +1854,20 @@ new_slab:
 	if (gfpflags & __GFP_WAIT)
 		local_irq_enable();
 
-	new = new_slab(s, gfpflags, node);
+	page = new_slab(s, gfpflags, node);
 
 	if (gfpflags & __GFP_WAIT)
 		local_irq_disable();
 
-	if (new) {
+	if (page) {
 		c = __this_cpu_ptr(s->cpu_slab);
 		stat(s, ALLOC_SLAB);
 		if (c->page)
 			flush_slab(s, c);
-		slab_lock(new);
-		__SetPageSlubFrozen(new);
-		c->page = new;
+
+		slab_lock(page);
+		__SetPageSlubFrozen(page);
+		c->page = page;
 		goto load_freelist;
 	}
 	if (!(gfpflags & __GFP_NOWARN) && printk_ratelimit())
@@ -1874,11 +1877,11 @@ new_slab:
 #endif
 	return NULL;
 debug:
-	if (!alloc_debug_processing(s, c->page, object, addr))
+	if (!alloc_debug_processing(s, page, object, addr))
 		goto another_slab;
 
-	c->page->inuse++;
-	c->page->freelist = get_freepointer(s, object);
+	page->inuse++;
+	page->freelist = get_freepointer(s, object);
 	c->node = NUMA_NO_NODE;
 	goto unlock_out;
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>

[Slub cleanup6 4/5] slub: Move node determination out of hotpath

[Christoph Lameter]

[-- Attachment #1: move_slab_node --]
[-- Type: text/plain, Size: 1542 bytes --]

If the node does not change then there is no need to recalculate
the node from the page struct. So move the node determination
into the places where we acquire a new slab page.

Signed-off-by: Christoph Lameter <cl@linux.com>

---
 mm/slub.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c	2011-04-15 12:52:17.000000000 -0500
+++ linux-2.6/mm/slub.c	2011-04-15 12:54:15.000000000 -0500
@@ -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);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>

[Slub cleanup6 5/5] slub: Move debug handlign in __slab_free

[Christoph Lameter]

[-- Attachment #1: move_debug --]
[-- Type: text/plain, Size: 1273 bytes --]

Its easier to read if its with the check for debugging flags.

Signed-off-by: Christoph Lameter <cl@linux.com>

---
 mm/slub.c |   11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c	2011-04-15 12:54:21.000000000 -0500
+++ linux-2.6/mm/slub.c	2011-04-15 12:54:21.000000000 -0500
@@ -2057,10 +2057,9 @@ static void __slab_free(struct kmem_cach
 	slab_lock(page);
 	stat(s, FREE_SLOWPATH);
 
-	if (kmem_cache_debug(s))
-		goto debug;
+	if (kmem_cache_debug(s) && !free_debug_processing(s, page, x, addr))
+		goto out_unlock;
 
-checks_ok:
 	prior = page->freelist;
 	set_freepointer(s, object, prior);
 	page->freelist = object;
@@ -2104,12 +2103,6 @@ slab_empty:
 #endif
 	stat(s, FREE_SLAB);
 	discard_slab(s, page);
-	return;
-
-debug:
-	if (!free_debug_processing(s, page, x, addr))
-		goto out_unlock;
-	goto checks_ok;
 }
 
 /*

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>

Re: [Slub cleanup6 0/5] SLUB: Cleanups V6

[Pekka Enberg]

On Fri, 2011-04-15 at 14:48 -0500, Christoph Lameter wrote:
> Cleanups that resulted from the lockless work

Applied, thanks!


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>

Re: [Slub cleanup6 1/5] slub: Use NUMA_NO_NODE in get_partial

[David Rientjes]

On Fri, 15 Apr 2011, Christoph Lameter wrote:

> A -1 was leftover during the conversion.
> 
> Signed-off-by: Christoph Lameter <cl@linux.com>

Acked-by: David Rientjes <rientjes@google.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>

Re: [Slub cleanup6 2/5] slub: get_map() function to establish map of free objects in a slab

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1869 bytes --]

On Fri, 15 Apr 2011, Christoph Lameter wrote:

> The bit map of free objects in a slab page is determined in various functions
> if debugging is enabled.
> 
> Provide a common function for that purpose.
> 

Although it makes writing to /sys/kernel/slab/cache/validate slower 
because of the double iteration in validate_slab().

> Signed-off-by: Christoph Lameter <cl@linux.com>
> 
> ---
>  mm/slub.c |   34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> Index: linux-2.6/mm/slub.c
> ===================================================================
> --- linux-2.6.orig/mm/slub.c	2011-03-30 14:09:27.000000000 -0500
> +++ linux-2.6/mm/slub.c	2011-03-30 14:30:24.000000000 -0500
> @@ -271,10 +271,6 @@ static inline void set_freepointer(struc
>  	for (__p = (__addr); __p < (__addr) + (__objects) * (__s)->size;\
>  			__p += (__s)->size)
>  
> -/* Scan freelist */
> -#define for_each_free_object(__p, __s, __free) \
> -	for (__p = (__free); __p; __p = get_freepointer((__s), __p))
> -
>  /* Determine object index from a given position */
>  static inline int slab_index(void *p, struct kmem_cache *s, void *addr)
>  {
> @@ -330,6 +326,21 @@ static inline int oo_objects(struct kmem
>  	return x.x & OO_MASK;
>  }
>  
> +/*
> + * Determine a map of object in use on a page.
> + *
> + * Slab lock or node listlock must be held to guarantee that the page does
> + * not vanish from under us.
> + */
> +static void get_map(struct kmem_cache *s, struct page *page, unsigned long *map)
> +{
> +	void *p;
> +	void *addr = page_address(page);
> +
> +	for (p = page->freelist; p; p = get_freepointer(s, p))
> +		set_bit(slab_index(p, s, addr), map);
> +}
> +
>  #ifdef CONFIG_SLUB_DEBUG
>  /*
>   * Debug settings:

This generates a warning without CONFIG_SLUB_DEBUG:

mm/slub.c:335: warning: a??get_mapa?? defined but not used

Re: [Slub cleanup6 3/5] slub: Eliminate repeated use of c->page through a new page variable

[David Rientjes]

On Fri, 15 Apr 2011, Christoph Lameter wrote:

> __slab_alloc is full of "c->page" repeats. Lets just use one local variable
> named "page" for this. Also avoids the need to a have another variable
> called "new".
> 
> Signed-off-by: Christoph Lameter <cl@linux.com>

Acked-by: David Rientjes <rientjes@google.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>

Re: [Slub cleanup6 4/5] slub: Move node determination out of hotpath

[David Rientjes]

On Fri, 15 Apr 2011, Christoph Lameter wrote:

> Index: linux-2.6/mm/slub.c
> ===================================================================
> --- linux-2.6.orig/mm/slub.c	2011-04-15 12:52:17.000000000 -0500
> +++ linux-2.6/mm/slub.c	2011-04-15 12:54:15.000000000 -0500
> @@ -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;
>  	}
>  

I don't like this because it's using a label within a conditional in an 
already very cluttered __slab_alloc().  This function could benefit from 
some serious cleanup instead of adding even more code that resembles 
spaghetti code from BASIC just to avoid two lines of duplicate code.

> @@ -1867,8 +1868,8 @@ new_slab:
>  
>  		slab_lock(page);
>  		__SetPageSlubFrozen(page);
> -		c->page = page;
> -		goto load_freelist;
> +
> +		goto load_from_page;

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);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>

Re: [Slub cleanup6 5/5] slub: Move debug handlign in __slab_free

[David Rientjes]

On Fri, 15 Apr 2011, Christoph Lameter wrote:

> Its easier to read if its with the check for debugging flags.
> 
> Signed-off-by: Christoph Lameter <cl@linux.com>

Acked-by: David Rientjes <rientjes@google.com>

Nice reduction in an unnecessary label and goto :)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>

Re: [Slub cleanup6 2/5] slub: get_map() function to establish map of free objects in a slab

[Christoph Lameter]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1005 bytes --]

On Wed, 11 May 2011, David Rientjes wrote:

> This generates a warning without CONFIG_SLUB_DEBUG:
>
> mm/slub.c:335: warning: ‘get_map’ defined but not used

Subject: slub: Avoid warning for !CONFIG_SLUB_DEBUG

Move the #ifdef so that get_map is only defined if CONFIG_SLUB_DEBUG is defined.

Signed-off-by: Christoph Lameter <cl@linux.com>

---
 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:38:42.000000000 -0500
+++ linux-2.6/mm/slub.c	2011-05-12 11:39:40.000000000 -0500
@@ -326,6 +326,7 @@ static inline int oo_objects(struct kmem
 	return x.x & OO_MASK;
 }

+#ifdef CONFIG_SLUB_DEBUG
 /*
  * Determine a map of object in use on a page.
  *
@@ -341,7 +342,6 @@ static void get_map(struct kmem_cache *s
 		set_bit(slab_index(p, s, addr), map);
 }

-#ifdef CONFIG_SLUB_DEBUG
 /*
  * Debug settings:
  */

Re: [Slub cleanup6 4/5] slub: Move node determination out of hotpath

[Christoph Lameter]

On Wed, 11 May 2011, David Rientjes 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.


Subject: slub: Fix control flow in slab_alloc

Signed-off-by: Christoph Lameter <cl@linux.com>

---
 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;
 	}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>

Re: [Slub cleanup6 2/5] slub: get_map() function to establish map of free objects in a slab

[David Rientjes]

On Thu, 12 May 2011, Christoph Lameter wrote:

> Subject: slub: Avoid warning for !CONFIG_SLUB_DEBUG
> 
> Move the #ifdef so that get_map is only defined if CONFIG_SLUB_DEBUG is defined.
> 
> Signed-off-by: Christoph Lameter <cl@linux.com>

Acked-by: David Rientjes <rientjes@google.com>

> 
> ---
>  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:38:42.000000000 -0500
> +++ linux-2.6/mm/slub.c	2011-05-12 11:39:40.000000000 -0500
> @@ -326,6 +326,7 @@ static inline int oo_objects(struct kmem
>  	return x.x & OO_MASK;
>  }
> 
> +#ifdef CONFIG_SLUB_DEBUG
>  /*
>   * Determine a map of object in use on a page.
>   *
> @@ -341,7 +342,6 @@ static void get_map(struct kmem_cache *s
>  		set_bit(slab_index(p, s, addr), map);
>  }
> 
> -#ifdef CONFIG_SLUB_DEBUG
>  /*
>   * Debug settings:
>   */

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>

[patch] slub: avoid label inside conditional

[David Rientjes]

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@linux.com>
> 
> ---
>  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@google.com>
---
 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@kvack.org.  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@kvack.org"> email@kvack.org </a>

Re: [patch] slub: avoid label inside conditional

[Christoph Lameter]

On Thu, 12 May 2011, David Rientjes wrote:

> slub: avoid label inside conditional

Acked-by: Christoph Lameter <cl@linux.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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@kvack.org"> email@kvack.org </a>