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
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 <[email protected]>
CC: Joonsoo Kim <[email protected]>
CC: David Rientjes <[email protected]>
CC: Pekka Enberg <[email protected]>
CC: Christoph Lameter <[email protected]>
---
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
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 <[email protected]>
CC: Joonsoo Kim <[email protected]>
CC: David Rientjes <[email protected]>
CC: Pekka Enberg <[email protected]>
CC: Christoph Lameter <[email protected]>
---
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
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?
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.
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
--
[email protected] -- Speaking for myself only.
2012/10/24 Glauber Costa <[email protected]>:
> 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.
On Wed, Oct 24, 2012 at 4:59 PM, Glauber Costa <[email protected]> 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 <[email protected]>
> CC: Joonsoo Kim <[email protected]>
> CC: David Rientjes <[email protected]>
> CC: Pekka Enberg <[email protected]>
> CC: Christoph Lameter <[email protected]>
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
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.
2012/10/25 Christoph Lameter <[email protected]>:
> 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?
On Fri, 26 Oct 2012, JoonSoo Kim wrote:
> 2012/10/25 Christoph Lameter <[email protected]>:
> > 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?
On 10/30/2012 07:31 PM, Christoph Lameter wrote:
> On Fri, 26 Oct 2012, JoonSoo Kim wrote:
>
>> 2012/10/25 Christoph Lameter <[email protected]>:
>>> 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.
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 <[email protected]>:
> > > 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
--
[email protected] -- Speaking for myself only.