[PATCH v2 0/2] common entry point for kmem_cache_free

[PATCH v2 0/2] common entry point for kmem_cache_free

[Glauber Costa]

The goal of this patchset is to provide a single entry point for
kmem_cache_free. Other functions, such as the allocation itself, and kmalloc
could easily follow.

The main problem here, is that if we keep the allocator-specific functions
in their .c file, we lose the ability to inline their fast paths. Being this
such a critical path, we would like to keep doing so.

During the last discussion around this (https://lkml.org/lkml/2012/10/22/639),
JoonSoo Kim suggested that we could achieve this by just including the
allocator-specific .c files in slab_common.c, a suggestion I considered but
quickly disregarding fearing a quite ugly end result.

Turns out it doesn't look so bad. So please let me know what you think.

Thanks

Glauber Costa (2):
  kmem_cache: include allocators code directly into slab_common
  slab: move kmem_cache_free to common code

 mm/Makefile      |  3 ---
 mm/slab.c        | 23 ++---------------------
 mm/slab_common.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 mm/slob.c        | 11 ++++-------
 mm/slub.c        |  5 +----
 5 files changed, 57 insertions(+), 35 deletions(-)

-- 
1.7.11.7

[PATCH v2 1/2] kmem_cache: include allocators code directly into slab_common

[Glauber Costa]

While the goal of slab_common.c is to have a common place for all
allocators, we face two different goals that are in opposition to each
other:

1) Have the different layouts be the business of each allocator, in
their .c
2) inline as much as we can for fast paths

Because of that, we either have to move all the entry points to the
mm/slab.h and rely heavily on the pre-processor, or include all .c files
in here.

The pre-processor solution has the disadvantage that some quite
non-trivial code gets even more non-trivial, and we end up leaving for
readers a non-pleasant indirection.

To keep this sane, we'll include the allocators .c files in here.  Which
means we will be able to inline any code they produced, but never the
other way around!

Doing this produced a name clash. This was resolved in this patch
itself.

Signed-off-by: Glauber Costa <glommer@parallels.com>
CC: Joonsoo Kim <js1304@gmail.com>
CC: David Rientjes <rientjes@google.com>
CC: Pekka Enberg <penberg@kernel.org>
CC: Christoph Lameter <cl@linux.com>
---
 mm/Makefile      |  3 ---
 mm/slab.c        |  9 ---------
 mm/slab_common.c | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/mm/Makefile b/mm/Makefile
index 6b025f8..796d893 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -36,12 +36,9 @@ obj-$(CONFIG_HUGETLBFS)	+= hugetlb.o
 obj-$(CONFIG_NUMA) 	+= mempolicy.o
 obj-$(CONFIG_SPARSEMEM)	+= sparse.o
 obj-$(CONFIG_SPARSEMEM_VMEMMAP) += sparse-vmemmap.o
-obj-$(CONFIG_SLOB) += slob.o
 obj-$(CONFIG_MMU_NOTIFIER) += mmu_notifier.o
 obj-$(CONFIG_KSM) += ksm.o
 obj-$(CONFIG_PAGE_POISONING) += debug-pagealloc.o
-obj-$(CONFIG_SLAB) += slab.o
-obj-$(CONFIG_SLUB) += slub.o
 obj-$(CONFIG_KMEMCHECK) += kmemcheck.o
 obj-$(CONFIG_FAILSLAB) += failslab.o
 obj-$(CONFIG_MEMORY_HOTPLUG) += memory_hotplug.o
diff --git a/mm/slab.c b/mm/slab.c
index 98b3460..72dadce 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -4583,15 +4583,6 @@ static const struct file_operations proc_slabstats_operations = {
 	.release	= seq_release_private,
 };
 #endif
-
-static int __init slab_proc_init(void)
-{
-#ifdef CONFIG_DEBUG_SLAB_LEAK
-	proc_create("slab_allocators", 0, NULL, &proc_slabstats_operations);
-#endif
-	return 0;
-}
-module_init(slab_proc_init);
 #endif
 
 /**
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 06f87db..66416ee 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -21,6 +21,36 @@
 
 #include "slab.h"
 
+/*
+ * While the goal of this file is to have a common place for all allocators,
+ * we face two different goals that are in opposition to each other:
+ *
+ * 1) Have the different layouts be the business of each allocator, in their .c
+ * 2) inline as much as we can for fast paths
+ *
+ * Because of that, we either have to move all the entry points to the mm/slab.h
+ * and rely heavily on the pre-processor, or include all .c files in here.
+ *
+ * The pre-processor solution has the disadvantage that some quite non-trivial
+ * code gets even more non-trivial, and we end up leaving for readers a
+ * non-pleasant indirection.
+ *
+ * To keep this sane, we'll include the allocators .c files in here. Which
+ * means we will be able to inline any code they produced, but never the other
+ * way around!
+ *
+ * Please also always make sure that the functions being called have the same
+ * name and signature in all alocators to avoid cumbersome conditionals in
+ * here.
+ */
+#ifdef CONFIG_SLUB
+#include "slub.c"
+#elif defined(CONFIG_SLOB)
+#include "slob.c"
+#else
+#include "slab.c"
+#endif
+
 enum slab_state slab_state;
 LIST_HEAD(slab_caches);
 DEFINE_MUTEX(slab_mutex);
@@ -302,6 +332,9 @@ static const struct file_operations proc_slabinfo_operations = {
 static int __init slab_proc_init(void)
 {
 	proc_create("slabinfo", S_IRUSR, NULL, &proc_slabinfo_operations);
+#ifdef CONFIG_DEBUG_SLAB_LEAK
+	proc_create("slab_allocators", 0, NULL, &proc_slabstats_operations);
+#endif
 	return 0;
 }
 module_init(slab_proc_init);
-- 
1.7.11.7

[PATCH v2 2/2] slab: move kmem_cache_free to common code

[Glauber Costa]

In the effort of commonizing the slab allocators, it would be better if
we had a single entry-point for kmem_cache_free. The low-level freeing
is still left to the allocators, But at least the tracing can be done in
slab_common.c

Signed-off-by: Glauber Costa <glommer@parallels.com>
CC: Joonsoo Kim <js1304@gmail.com>
CC: David Rientjes <rientjes@google.com>
CC: Pekka Enberg <penberg@kernel.org>
CC: Christoph Lameter <cl@linux.com>

---
 mm/slab.c        | 14 ++------------
 mm/slab_common.c | 17 +++++++++++++++++
 mm/slob.c        | 11 ++++-------
 mm/slub.c        |  5 +----
 4 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 72dadce..11e5f3b 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3903,15 +3903,8 @@ void *__kmalloc(size_t size, gfp_t flags)
 EXPORT_SYMBOL(__kmalloc);
 #endif
 
-/**
- * kmem_cache_free - Deallocate an object
- * @cachep: The cache the allocation was from.
- * @objp: The previously allocated object.
- *
- * Free an object which was previously allocated from this
- * cache.
- */
-void kmem_cache_free(struct kmem_cache *cachep, void *objp)
+static __always_inline
+void __kmem_cache_free(struct kmem_cache *cachep, void *objp)
 {
 	unsigned long flags;
 
@@ -3921,10 +3914,7 @@ void kmem_cache_free(struct kmem_cache *cachep, void *objp)
 		debug_check_no_obj_freed(objp, cachep->object_size);
 	__cache_free(cachep, objp, __builtin_return_address(0));
 	local_irq_restore(flags);
-
-	trace_kmem_cache_free(_RET_IP_, objp);
 }
-EXPORT_SYMBOL(kmem_cache_free);
 
 /**
  * kfree - free previously allocated memory
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 66416ee..c6c6e52 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -19,6 +19,8 @@
 #include <asm/tlbflush.h>
 #include <asm/page.h>
 
+#include <trace/events/kmem.h>
+
 #include "slab.h"
 
 /*
@@ -198,6 +200,21 @@ out_locked:
 }
 EXPORT_SYMBOL(kmem_cache_create);
 
+/**
+ * kmem_cache_free - Deallocate an object
+ * @cachep: The cache the allocation was from.
+ * @objp: The previously allocated object.
+ *
+ * Free an object which was previously allocated from this
+ * cache.
+ */
+void kmem_cache_free(struct kmem_cache *s, void *x)
+{
+	__kmem_cache_free(s, x);
+	trace_kmem_cache_free(_RET_IP_, x);
+}
+EXPORT_SYMBOL(kmem_cache_free);
+
 void kmem_cache_destroy(struct kmem_cache *s)
 {
 	get_online_cpus();
diff --git a/mm/slob.c b/mm/slob.c
index 3edfeaa..4033ce2 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -555,7 +555,7 @@ void *kmem_cache_alloc_node(struct kmem_cache *c, gfp_t flags, int node)
 }
 EXPORT_SYMBOL(kmem_cache_alloc_node);
 
-static void __kmem_cache_free(void *b, int size)
+static void do_kmem_cache_free(void *b, int size)
 {
 	if (size < PAGE_SIZE)
 		slob_free(b, size);
@@ -568,10 +568,10 @@ static void kmem_rcu_free(struct rcu_head *head)
 	struct slob_rcu *slob_rcu = (struct slob_rcu *)head;
 	void *b = (void *)slob_rcu - (slob_rcu->size - sizeof(struct slob_rcu));
 
-	__kmem_cache_free(b, slob_rcu->size);
+	do_kmem_cache_free(b, slob_rcu->size);
 }
 
-void kmem_cache_free(struct kmem_cache *c, void *b)
+static __always_inline void __kmem_cache_free(struct kmem_cache *c, void *b)
 {
 	kmemleak_free_recursive(b, c->flags);
 	if (unlikely(c->flags & SLAB_DESTROY_BY_RCU)) {
@@ -580,12 +580,9 @@ void kmem_cache_free(struct kmem_cache *c, void *b)
 		slob_rcu->size = c->size;
 		call_rcu(&slob_rcu->head, kmem_rcu_free);
 	} else {
-		__kmem_cache_free(b, c->size);
+		do_kmem_cache_free(b, c->size);
 	}
-
-	trace_kmem_cache_free(_RET_IP_, b);
 }
-EXPORT_SYMBOL(kmem_cache_free);
 
 unsigned int kmem_cache_size(struct kmem_cache *c)
 {
diff --git a/mm/slub.c b/mm/slub.c
index 259bc2c..3430564 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2606,7 +2606,7 @@ redo:
 
 }
 
-void kmem_cache_free(struct kmem_cache *s, void *x)
+static __always_inline void __kmem_cache_free(struct kmem_cache *s, void *x)
 {
 	struct page *page;
 
@@ -2620,10 +2620,7 @@ void kmem_cache_free(struct kmem_cache *s, void *x)
 	}
 
 	slab_free(s, page, x, _RET_IP_);
-
-	trace_kmem_cache_free(_RET_IP_, x);
 }
-EXPORT_SYMBOL(kmem_cache_free);
 
 /*
  * Object placement in a slab is made very easy because we always start at
-- 
1.7.11.7

Re: [PATCH v2 1/2] kmem_cache: include allocators code directly into slab_common

[Christoph Lameter]

On Wed, 24 Oct 2012, Glauber Costa wrote:

> Because of that, we either have to move all the entry points to the
> mm/slab.h and rely heavily on the pre-processor, or include all .c files
> in here.

Hmm... That is a bit of a radical solution. The global optimizations now
possible with the new gcc compiler include the ability to fold functions
across different linkable objects. Andi, is that usable for kernel builds?

Re: [PATCH v2 1/2] kmem_cache: include allocators code directly into slab_common

[Glauber Costa]

On 10/24/2012 06:29 PM, Christoph Lameter wrote:
> On Wed, 24 Oct 2012, Glauber Costa wrote:
> 
>> Because of that, we either have to move all the entry points to the
>> mm/slab.h and rely heavily on the pre-processor, or include all .c files
>> in here.
> 
> Hmm... That is a bit of a radical solution. The global optimizations now
> possible with the new gcc compiler include the ability to fold functions
> across different linkable objects. Andi, is that usable for kernel builds?
> 

In general, it takes quite a lot of time to take all those optimizations
for granted. We still live a lot of time with multiple compiler versions
building distros, etc, for quite some time.

I would expect the end result for anyone not using such a compiler to be
a sudden performance drop when using a new kernel. Not really pleasant.

Re: [PATCH v2 1/2] kmem_cache: include allocators code directly into slab_common

[Andi Kleen]

On Wed, Oct 24, 2012 at 02:29:09PM +0000, Christoph Lameter wrote:
> On Wed, 24 Oct 2012, Glauber Costa wrote:
> 
> > Because of that, we either have to move all the entry points to the
> > mm/slab.h and rely heavily on the pre-processor, or include all .c files
> > in here.
> 
> Hmm... That is a bit of a radical solution. The global optimizations now
> possible with the new gcc compiler include the ability to fold functions
> across different linkable objects. Andi, is that usable for kernel builds?

Yes, but you need a fairly large patchkit which is not mainline yet.

https://github.com/andikleen/linux-misc/tree/lto

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH v2 1/2] kmem_cache: include allocators code directly into slab_common

[JoonSoo Kim]

2012/10/24 Glauber Costa <glommer@parallels.com>:
> On 10/24/2012 06:29 PM, Christoph Lameter wrote:
>> On Wed, 24 Oct 2012, Glauber Costa wrote:
>>
>>> Because of that, we either have to move all the entry points to the
>>> mm/slab.h and rely heavily on the pre-processor, or include all .c files
>>> in here.
>>
>> Hmm... That is a bit of a radical solution. The global optimizations now
>> possible with the new gcc compiler include the ability to fold functions
>> across different linkable objects. Andi, is that usable for kernel builds?
>>
>
> In general, it takes quite a lot of time to take all those optimizations
> for granted. We still live a lot of time with multiple compiler versions
> building distros, etc, for quite some time.
>
> I would expect the end result for anyone not using such a compiler to be
> a sudden performance drop when using a new kernel. Not really pleasant.

I agree with Glauber's opinion.
And patch looks fine to me.

Re: [PATCH v2 1/2] kmem_cache: include allocators code directly into slab_common

[Pekka Enberg]

On Wed, Oct 24, 2012 at 4:59 PM, Glauber Costa <glommer@parallels.com> wrote:
> While the goal of slab_common.c is to have a common place for all
> allocators, we face two different goals that are in opposition to each
> other:
>
> 1) Have the different layouts be the business of each allocator, in
> their .c
> 2) inline as much as we can for fast paths
>
> Because of that, we either have to move all the entry points to the
> mm/slab.h and rely heavily on the pre-processor, or include all .c files
> in here.
>
> The pre-processor solution has the disadvantage that some quite
> non-trivial code gets even more non-trivial, and we end up leaving for
> readers a non-pleasant indirection.
>
> To keep this sane, we'll include the allocators .c files in here.  Which
> means we will be able to inline any code they produced, but never the
> other way around!
>
> Doing this produced a name clash. This was resolved in this patch
> itself.
>
> Signed-off-by: Glauber Costa <glommer@parallels.com>
> CC: Joonsoo Kim <js1304@gmail.com>
> CC: David Rientjes <rientjes@google.com>
> CC: Pekka Enberg <penberg@kernel.org>
> CC: Christoph Lameter <cl@linux.com>

So I hate this patch with a passion. We don't have any fastpaths in
mm/slab_common.c nor should we. Those should be allocator specific.

                         Pekka

Re: [PATCH v2 1/2] kmem_cache: include allocators code directly into slab_common

[Christoph Lameter]

On Wed, 24 Oct 2012, Pekka Enberg wrote:

> So I hate this patch with a passion. We don't have any fastpaths in
> mm/slab_common.c nor should we. Those should be allocator specific.

I have similar thoughts on the issue. Lets keep the fast paths allocator
specific until we find a better way to handle this issue.

Re: [PATCH v2 1/2] kmem_cache: include allocators code directly into slab_common

[JoonSoo Kim]

2012/10/25 Christoph Lameter <cl@linux.com>:
> On Wed, 24 Oct 2012, Pekka Enberg wrote:
>
>> So I hate this patch with a passion. We don't have any fastpaths in
>> mm/slab_common.c nor should we. Those should be allocator specific.
>
> I have similar thoughts on the issue. Lets keep the fast paths allocator
> specific until we find a better way to handle this issue.

Okay. I see.
How about applying LTO not to the whole kernel code, but just to
slab_common.o + sl[aou]b.o?
I think that it may be possible, isn't it?

Re: [PATCH v2 1/2] kmem_cache: include allocators code directly into slab_common

[Christoph Lameter]

On Fri, 26 Oct 2012, JoonSoo Kim wrote:

> 2012/10/25 Christoph Lameter <cl@linux.com>:
> > On Wed, 24 Oct 2012, Pekka Enberg wrote:
> >
> >> So I hate this patch with a passion. We don't have any fastpaths in
> >> mm/slab_common.c nor should we. Those should be allocator specific.
> >
> > I have similar thoughts on the issue. Lets keep the fast paths allocator
> > specific until we find a better way to handle this issue.
>
> Okay. I see.
> How about applying LTO not to the whole kernel code, but just to
> slab_common.o + sl[aou]b.o?
> I think that it may be possible, isn't it?

Well.... Andi: Is that possible?

Re: [PATCH v2 1/2] kmem_cache: include allocators code directly into slab_common

[Glauber Costa]

On 10/30/2012 07:31 PM, Christoph Lameter wrote:
> On Fri, 26 Oct 2012, JoonSoo Kim wrote:
> 
>> 2012/10/25 Christoph Lameter <cl@linux.com>:
>>> On Wed, 24 Oct 2012, Pekka Enberg wrote:
>>>
>>>> So I hate this patch with a passion. We don't have any fastpaths in
>>>> mm/slab_common.c nor should we. Those should be allocator specific.
>>>
>>> I have similar thoughts on the issue. Lets keep the fast paths allocator
>>> specific until we find a better way to handle this issue.
>>
>> Okay. I see.
>> How about applying LTO not to the whole kernel code, but just to
>> slab_common.o + sl[aou]b.o?
>> I think that it may be possible, isn't it?
> 
> Well.... Andi: Is that possible?
> 

FYI: In the next version of my series, there is a patch that puts
the common code in an inline function in mm/slab.h. Then the allocators
just call that function.

I think it is the best we can do for now, given all the constraints.

Re: [PATCH v2 1/2] kmem_cache: include allocators code directly into slab_common

[Andi Kleen]

On Tue, Oct 30, 2012 at 03:31:51PM +0000, Christoph Lameter wrote:
> On Fri, 26 Oct 2012, JoonSoo Kim wrote:
> 
> > 2012/10/25 Christoph Lameter <cl@linux.com>:
> > > On Wed, 24 Oct 2012, Pekka Enberg wrote:
> > >
> > >> So I hate this patch with a passion. We don't have any fastpaths in
> > >> mm/slab_common.c nor should we. Those should be allocator specific.
> > >
> > > I have similar thoughts on the issue. Lets keep the fast paths allocator
> > > specific until we find a better way to handle this issue.
> >
> > Okay. I see.
> > How about applying LTO not to the whole kernel code, but just to
> > slab_common.o + sl[aou]b.o?
> > I think that it may be possible, isn't it?
> 
> Well.... Andi: Is that possible?

In principle yes, but would still need a toolchain with LTO 
support and a few changes.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.