[PATCH] slub: move kmem_cache_node into it's own cacheline

[PATCH] slub: move kmem_cache_node into it's own cacheline

[Alexander Duyck]

This patch is meant to improve the performance of SLUB by moving the local
kmem_cache_node lock into it's own cacheline separate from kmem_cache.
This is accomplished by simply removing the local_node when NUMA is enabled.

On my system with 2 nodes I saw around a 5% performance increase w/
hackbench times dropping from 6.2 seconds to 5.9 seconds on average.  I
suspect the performance gain would increase as the number of nodes
increases, but I do not have the data to currently back that up.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

 include/linux/slub_def.h |   11 ++++-------
 mm/slub.c                |   33 +++++++++++----------------------
 2 files changed, 15 insertions(+), 29 deletions(-)

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index 0249d41..e6217bb 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -52,7 +52,7 @@ struct kmem_cache_node {
 	atomic_long_t total_objects;
 	struct list_head full;
 #endif
-};
+} ____cacheline_internodealigned_in_smp;
 
 /*
  * Word size structure that can be atomically updated or read and that
@@ -75,12 +75,6 @@ struct kmem_cache {
 	int offset;		/* Free pointer offset. */
 	struct kmem_cache_order_objects oo;
 
-	/*
-	 * Avoid an extra cache line for UP, SMP and for the node local to
-	 * struct kmem_cache.
-	 */
-	struct kmem_cache_node local_node;
-
 	/* Allocation and freeing of slabs */
 	struct kmem_cache_order_objects max;
 	struct kmem_cache_order_objects min;
@@ -102,6 +96,9 @@ struct kmem_cache {
 	 */
 	int remote_node_defrag_ratio;
 	struct kmem_cache_node *node[MAX_NUMNODES];
+#else
+	/* Avoid an extra cache line for UP */
+	struct kmem_cache_node local_node;
 #endif
 };
 
diff --git a/mm/slub.c b/mm/slub.c
index 461314b..8af03de 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2141,7 +2141,7 @@ static void free_kmem_cache_nodes(struct kmem_cache *s)
 
 	for_each_node_state(node, N_NORMAL_MEMORY) {
 		struct kmem_cache_node *n = s->node[node];
-		if (n && n != &s->local_node)
+		if (n)
 			kmem_cache_free(kmalloc_caches, n);
 		s->node[node] = NULL;
 	}
@@ -2150,33 +2150,22 @@ static void free_kmem_cache_nodes(struct kmem_cache *s)
 static int init_kmem_cache_nodes(struct kmem_cache *s, gfp_t gfpflags)
 {
 	int node;
-	int local_node;
-
-	if (slab_state >= UP && (s < kmalloc_caches ||
-			s >= kmalloc_caches + KMALLOC_CACHES))
-		local_node = page_to_nid(virt_to_page(s));
-	else
-		local_node = 0;
 
 	for_each_node_state(node, N_NORMAL_MEMORY) {
 		struct kmem_cache_node *n;
 
-		if (local_node == node)
-			n = &s->local_node;
-		else {
-			if (slab_state == DOWN) {
-				early_kmem_cache_node_alloc(gfpflags, node);
-				continue;
-			}
-			n = kmem_cache_alloc_node(kmalloc_caches,
-							gfpflags, node);
-
-			if (!n) {
-				free_kmem_cache_nodes(s);
-				return 0;
-			}
+		if (slab_state == DOWN) {
+			early_kmem_cache_node_alloc(gfpflags, node);
+			continue;
+		}
+		n = kmem_cache_alloc_node(kmalloc_caches,
+						gfpflags, node);
 
+		if (!n) {
+			free_kmem_cache_nodes(s);
+			return 0;
 		}
+
 		s->node[node] = n;
 		init_kmem_cache_node(n, s);
 	}

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] slub: move kmem_cache_node into it's own cacheline

[Pekka Enberg]

On Fri, May 21, 2010 at 2:47 AM, Alexander Duyck
<alexander.h.duyck@intel.com> wrote:
> This patch is meant to improve the performance of SLUB by moving the local
> kmem_cache_node lock into it's own cacheline separate from kmem_cache.
> This is accomplished by simply removing the local_node when NUMA is enabled.
>
> On my system with 2 nodes I saw around a 5% performance increase w/
> hackbench times dropping from 6.2 seconds to 5.9 seconds on average.  I
> suspect the performance gain would increase as the number of nodes
> increases, but I do not have the data to currently back that up.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>

Yanmin, does this fix the hackbench regression for you?

> ---
>
>  include/linux/slub_def.h |   11 ++++-------
>  mm/slub.c                |   33 +++++++++++----------------------
>  2 files changed, 15 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
> index 0249d41..e6217bb 100644
> --- a/include/linux/slub_def.h
> +++ b/include/linux/slub_def.h
> @@ -52,7 +52,7 @@ struct kmem_cache_node {
>        atomic_long_t total_objects;
>        struct list_head full;
>  #endif
> -};
> +} ____cacheline_internodealigned_in_smp;
>
>  /*
>  * Word size structure that can be atomically updated or read and that
> @@ -75,12 +75,6 @@ struct kmem_cache {
>        int offset;             /* Free pointer offset. */
>        struct kmem_cache_order_objects oo;
>
> -       /*
> -        * Avoid an extra cache line for UP, SMP and for the node local to
> -        * struct kmem_cache.
> -        */
> -       struct kmem_cache_node local_node;
> -
>        /* Allocation and freeing of slabs */
>        struct kmem_cache_order_objects max;
>        struct kmem_cache_order_objects min;
> @@ -102,6 +96,9 @@ struct kmem_cache {
>         */
>        int remote_node_defrag_ratio;
>        struct kmem_cache_node *node[MAX_NUMNODES];
> +#else
> +       /* Avoid an extra cache line for UP */
> +       struct kmem_cache_node local_node;
>  #endif
>  };
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 461314b..8af03de 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2141,7 +2141,7 @@ static void free_kmem_cache_nodes(struct kmem_cache *s)
>
>        for_each_node_state(node, N_NORMAL_MEMORY) {
>                struct kmem_cache_node *n = s->node[node];
> -               if (n && n != &s->local_node)
> +               if (n)
>                        kmem_cache_free(kmalloc_caches, n);
>                s->node[node] = NULL;
>        }
> @@ -2150,33 +2150,22 @@ static void free_kmem_cache_nodes(struct kmem_cache *s)
>  static int init_kmem_cache_nodes(struct kmem_cache *s, gfp_t gfpflags)
>  {
>        int node;
> -       int local_node;
> -
> -       if (slab_state >= UP && (s < kmalloc_caches ||
> -                       s >= kmalloc_caches + KMALLOC_CACHES))
> -               local_node = page_to_nid(virt_to_page(s));
> -       else
> -               local_node = 0;
>
>        for_each_node_state(node, N_NORMAL_MEMORY) {
>                struct kmem_cache_node *n;
>
> -               if (local_node == node)
> -                       n = &s->local_node;
> -               else {
> -                       if (slab_state == DOWN) {
> -                               early_kmem_cache_node_alloc(gfpflags, node);
> -                               continue;
> -                       }
> -                       n = kmem_cache_alloc_node(kmalloc_caches,
> -                                                       gfpflags, node);
> -
> -                       if (!n) {
> -                               free_kmem_cache_nodes(s);
> -                               return 0;
> -                       }
> +               if (slab_state == DOWN) {
> +                       early_kmem_cache_node_alloc(gfpflags, node);
> +                       continue;
> +               }
> +               n = kmem_cache_alloc_node(kmalloc_caches,
> +                                               gfpflags, node);
>
> +               if (!n) {
> +                       free_kmem_cache_nodes(s);
> +                       return 0;
>                }
> +
>                s->node[node] = n;
>                init_kmem_cache_node(n, s);
>        }
>
> --
> 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/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH] slub: move kmem_cache_node into it's own cacheline

[Shi, Alex]

I have tested this patch based latest Linus' kernel tree. It real works!
About 10% improvement happened for hackbench threads mode and 8%~13% improve for process mode on our 2 sockets Westmere machine and about 7% hackbench improvement on 2 sockets NHM. 

Alex 
>-----Original Message-----
>From: penberg@gmail.com [mailto:penberg@gmail.com] On Behalf Of Pekka Enberg
>Sent: Friday, May 21, 2010 1:00 PM
>To: Duyck, Alexander H
>Cc: cl@linux.com; linux-mm@kvack.org; Shi, Alex; Zhang Yanmin
>Subject: Re: [PATCH] slub: move kmem_cache_node into it's own cacheline
>
>On Fri, May 21, 2010 at 2:47 AM, Alexander Duyck
><alexander.h.duyck@intel.com> wrote:
>> This patch is meant to improve the performance of SLUB by moving the local
>> kmem_cache_node lock into it's own cacheline separate from kmem_cache.
>> This is accomplished by simply removing the local_node when NUMA is enabled.
>>
>> On my system with 2 nodes I saw around a 5% performance increase w/
>> hackbench times dropping from 6.2 seconds to 5.9 seconds on average.  I
>> suspect the performance gain would increase as the number of nodes
>> increases, but I do not have the data to currently back that up.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>
>Yanmin, does this fix the hackbench regression for you?
>
>> ---
>>
>>  include/linux/slub_def.h |   11 ++++-------
>>  mm/slub.c                |   33 +++++++++++----------------------
>>  2 files changed, 15 insertions(+), 29 deletions(-)
>>
>> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
>> index 0249d41..e6217bb 100644
>> --- a/include/linux/slub_def.h
>> +++ b/include/linux/slub_def.h
>> @@ -52,7 +52,7 @@ struct kmem_cache_node {
>>        atomic_long_t total_objects;
>>        struct list_head full;
>>  #endif
>> -};
>> +} ____cacheline_internodealigned_in_smp;
>>
>>  /*
>>  * Word size structure that can be atomically updated or read and that
>> @@ -75,12 +75,6 @@ struct kmem_cache {
>>        int offset;             /* Free pointer offset. */
>>        struct kmem_cache_order_objects oo;
>>
>> -       /*
>> -        * Avoid an extra cache line for UP, SMP and for the node local to
>> -        * struct kmem_cache.
>> -        */
>> -       struct kmem_cache_node local_node;
>> -
>>        /* Allocation and freeing of slabs */
>>        struct kmem_cache_order_objects max;
>>        struct kmem_cache_order_objects min;
>> @@ -102,6 +96,9 @@ struct kmem_cache {
>>         */
>>        int remote_node_defrag_ratio;
>>        struct kmem_cache_node *node[MAX_NUMNODES];
>> +#else
>> +       /* Avoid an extra cache line for UP */
>> +       struct kmem_cache_node local_node;
>>  #endif
>>  };
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 461314b..8af03de 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -2141,7 +2141,7 @@ static void free_kmem_cache_nodes(struct kmem_cache *s)
>>
>>        for_each_node_state(node, N_NORMAL_MEMORY) {
>>                struct kmem_cache_node *n = s->node[node];
>> -               if (n && n != &s->local_node)
>> +               if (n)
>>                        kmem_cache_free(kmalloc_caches, n);
>>                s->node[node] = NULL;
>>        }
>> @@ -2150,33 +2150,22 @@ static void free_kmem_cache_nodes(struct kmem_cache *s)
>>  static int init_kmem_cache_nodes(struct kmem_cache *s, gfp_t gfpflags)
>>  {
>>        int node;
>> -       int local_node;
>> -
>> -       if (slab_state >= UP && (s < kmalloc_caches ||
>> -                       s >= kmalloc_caches + KMALLOC_CACHES))
>> -               local_node = page_to_nid(virt_to_page(s));
>> -       else
>> -               local_node = 0;
>>
>>        for_each_node_state(node, N_NORMAL_MEMORY) {
>>                struct kmem_cache_node *n;
>>
>> -               if (local_node == node)
>> -                       n = &s->local_node;
>> -               else {
>> -                       if (slab_state == DOWN) {
>> -                               early_kmem_cache_node_alloc(gfpflags, node);
>> -                               continue;
>> -                       }
>> -                       n = kmem_cache_alloc_node(kmalloc_caches,
>> -                                                       gfpflags, node);
>> -
>> -                       if (!n) {
>> -                               free_kmem_cache_nodes(s);
>> -                               return 0;
>> -                       }
>> +               if (slab_state == DOWN) {
>> +                       early_kmem_cache_node_alloc(gfpflags, node);
>> +                       continue;
>> +               }
>> +               n = kmem_cache_alloc_node(kmalloc_caches,
>> +                                               gfpflags, node);
>>
>> +               if (!n) {
>> +                       free_kmem_cache_nodes(s);
>> +                       return 0;
>>                }
>> +
>>                s->node[node] = n;
>>                init_kmem_cache_node(n, s);
>>        }
>>
>> --
>> 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/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH] slub: move kmem_cache_node into it's own cacheline

[Christoph Lameter]

Yes right. The cacheline that also contains local_node is dirtied by the
locking in the SMP case and will evict the cacheline used to lookup the
per cpu vector and other important information. The per cpu patches
aggravated that problem by making more use of the fields that are evicted
with the cacheline.

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

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] slub: move kmem_cache_node into it's own cacheline

[Christoph Lameter]

On Thu, 20 May 2010, Alexander Duyck wrote:

> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
> index 0249d41..e6217bb 100644
> --- a/include/linux/slub_def.h
> +++ b/include/linux/slub_def.h
> @@ -52,7 +52,7 @@ struct kmem_cache_node {
>  	atomic_long_t total_objects;
>  	struct list_head full;
>  #endif
> -};
> +} ____cacheline_internodealigned_in_smp;

What does this do? Leftovers?

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH] slub: move kmem_cache_node into it's own cacheline

[Duyck, Alexander H]

Christoph Lameter wrote:
> On Thu, 20 May 2010, Alexander Duyck wrote:
> 
>> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
>> index 0249d41..e6217bb 100644 --- a/include/linux/slub_def.h
>> +++ b/include/linux/slub_def.h
>> @@ -52,7 +52,7 @@ struct kmem_cache_node {
>>  	atomic_long_t total_objects;
>>  	struct list_head full;
>>  #endif
>> -};
>> +} ____cacheline_internodealigned_in_smp;
> 
> What does this do? Leftovers?

It aligns it to the correct size so that no two instances can occupy a shared cacheline.  I put that in place to avoid any false sharing of the objects should they fit into a shared cacheline on a NUMA system.

Thanks,

Alex
--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH] slub: move kmem_cache_node into it's own cacheline

[Christoph Lameter]

On Fri, 21 May 2010, Duyck, Alexander H wrote:

> Christoph Lameter wrote:
> > On Thu, 20 May 2010, Alexander Duyck wrote:
> >
> >> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
> >> index 0249d41..e6217bb 100644 --- a/include/linux/slub_def.h
> >> +++ b/include/linux/slub_def.h
> >> @@ -52,7 +52,7 @@ struct kmem_cache_node {
> >>  	atomic_long_t total_objects;
> >>  	struct list_head full;
> >>  #endif
> >> -};
> >> +} ____cacheline_internodealigned_in_smp;
> >
> > What does this do? Leftovers?
>
> It aligns it to the correct size so that no two instances can occupy a shared cacheline.  I put that in place to avoid any false sharing of the objects should they fit into a shared cacheline on a NUMA system.

It has no effect in the NUMA case since the slab allocator is used to
allocate the object. Alignments would have to be specified at slab creation.

Maybe in the SMP case? But then struct kmem_cache_node is part of the
struct kmem_cache.

internode aligned? This creates > 4k kmem_cache structures on some
platforms.

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH] slub: move kmem_cache_node into it's own cacheline

[Christoph Lameter]

struct kmem_cache is allocated without any alignment so the alignment
spec does not work.

If you want this then you also need to align struct kmem_cache. internode
aligned would require the kmem_cache to be page aligned. So lets drop the
hunk from this patch for now. A separate patch may convince us to merge
aligning kmem_cache_node within kmem_cache.

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH] slub: move kmem_cache_node into it's own cacheline

[Duyck, Alexander H]

Christoph Lameter wrote:
> struct kmem_cache is allocated without any alignment so the alignment
> spec does not work.
> 
> If you want this then you also need to align struct kmem_cache.
> internode aligned would require the kmem_cache to be page aligned. So
> lets drop the hunk from this patch for now. A separate patch may
> convince us to merge aligning kmem_cache_node within kmem_cache.

I will pull that hunk out, test it, and resubmit within the next hour or so if everything looks good.

Thanks,

Alex
--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH] slub: move kmem_cache_node into it's own cacheline

[Christoph Lameter]

On Fri, 21 May 2010, Duyck, Alexander H wrote:

> Christoph Lameter wrote:
> > struct kmem_cache is allocated without any alignment so the alignment
> > spec does not work.
> >
> > If you want this then you also need to align struct kmem_cache.
> > internode aligned would require the kmem_cache to be page aligned. So
> > lets drop the hunk from this patch for now. A separate patch may
> > convince us to merge aligning kmem_cache_node within kmem_cache.
>
> I will pull that hunk out, test it, and resubmit within the next hour or so if everything looks good.

Again internode aligned may need page alignment. You may be getting into
messy issues. The architectures requiring internode alignment are NUMA
anyways so it may not matter because you only have local_node for the SMP
case.

Cacheline alignment therefore may be sufficient. But the variables at the
tail of the kmem_cache structure are mostly read only. Therefore may be
just forget about the alignment. It likely makes no difference.

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] slub: move kmem_cache_node into it's own cacheline

[Pekka Enberg]

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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH] slub: move kmem_cache_node into it's own cacheline

[Shi, Alex]

Tim reminder me that I need clearly add the following line to confirm my agreement for this patch. Sorry for miss this. 

Tested-by: Alex Shi <alex.shi@intel.com> 

>-----Original Message-----
>From: Pekka Enberg [mailto:penberg@cs.helsinki.fi]
>Sent: Tuesday, May 25, 2010 2:14 AM
>To: Shi, Alex
>Cc: Duyck, Alexander H; cl@linux.com; linux-mm@kvack.org; Zhang Yanmin; Chen, Tim C
>Subject: Re: [PATCH] slub: move kmem_cache_node into it's own cacheline
>
>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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>