This is some initial work to make kmem_cache_free at least callable from
a common entry point. This will be useful in future work, like kmemcg-slab,
that needs to further change those callers in both slab and slub.
Patch1 is not really a dependency for 2, but it will be for the work I am doing
in kmemcg-slab, so I'm sending both patches for your appreciation.
Glauber Costa (2):
slab: commonize slab_cache field in struct page
slab: move kmem_cache_free to common code
include/linux/mm_types.h | 7 ++-----
mm/slab.c | 13 +------------
mm/slab.h | 1 +
mm/slab_common.c | 17 +++++++++++++++++
mm/slob.c | 11 ++++-------
mm/slub.c | 29 +++++++++++++----------------
6 files changed, 38 insertions(+), 40 deletions(-)
--
1.7.11.7
Right now, slab and slub have fields in struct page to derive which
cache a page belongs to, but they do it slightly differently.
slab uses a field called slab_cache, that lives in the third double
word. slub, uses a field called "slab", living outside of the
doublewords area.
Ideally, we could use the same field for this. Since slub heavily makes
use of the doubleword region, there isn't really much room to move
slub's slab_cache field around. Since slab does not have such strict
placement restrictions, we can move it outside the doubleword area.
The naming used by slab, "slab_cache", is less confusing, and it is
preferred over slub's generic "slab".
Signed-off-by: Glauber Costa <[email protected]>
CC: Christoph Lameter <[email protected]>
CC: Pekka Enberg <[email protected]>
CC: David Rientjes <[email protected]>
---
include/linux/mm_types.h | 7 ++-----
mm/slub.c | 24 ++++++++++++------------
2 files changed, 14 insertions(+), 17 deletions(-)
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 31f8a3a..2fef4e7 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -128,10 +128,7 @@ struct page {
};
struct list_head list; /* slobs list of pages */
- struct { /* slab fields */
- struct kmem_cache *slab_cache;
- struct slab *slab_page;
- };
+ struct slab *slab_page; /* slab fields */
};
/* Remainder is not double word aligned */
@@ -146,7 +143,7 @@ struct page {
#if USE_SPLIT_PTLOCKS
spinlock_t ptl;
#endif
- struct kmem_cache *slab; /* SLUB: Pointer to slab */
+ struct kmem_cache *slab_cache; /* SL[AU]B: Pointer to slab */
struct page *first_page; /* Compound tail pages */
};
diff --git a/mm/slub.c b/mm/slub.c
index a34548e..259bc2c 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1087,11 +1087,11 @@ static noinline struct kmem_cache_node *free_debug_processing(
if (!check_object(s, page, object, SLUB_RED_ACTIVE))
goto out;
- if (unlikely(s != page->slab)) {
+ if (unlikely(s != page->slab_cache)) {
if (!PageSlab(page)) {
slab_err(s, page, "Attempt to free object(0x%p) "
"outside of slab", object);
- } else if (!page->slab) {
+ } else if (!page->slab_cache) {
printk(KERN_ERR
"SLUB <none>: no slab for object 0x%p.\n",
object);
@@ -1352,7 +1352,7 @@ static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
goto out;
inc_slabs_node(s, page_to_nid(page), page->objects);
- page->slab = s;
+ page->slab_cache = s;
__SetPageSlab(page);
if (page->pfmemalloc)
SetPageSlabPfmemalloc(page);
@@ -1419,7 +1419,7 @@ static void rcu_free_slab(struct rcu_head *h)
else
page = container_of((struct list_head *)h, struct page, lru);
- __free_slab(page->slab, page);
+ __free_slab(page->slab_cache, page);
}
static void free_slab(struct kmem_cache *s, struct page *page)
@@ -2612,9 +2612,9 @@ void kmem_cache_free(struct kmem_cache *s, void *x)
page = virt_to_head_page(x);
- if (kmem_cache_debug(s) && page->slab != s) {
+ if (kmem_cache_debug(s) && page->slab_cache != s) {
pr_err("kmem_cache_free: Wrong slab cache. %s but object"
- " is from %s\n", page->slab->name, s->name);
+ " is from %s\n", page->slab_cache->name, s->name);
WARN_ON_ONCE(1);
return;
}
@@ -3413,7 +3413,7 @@ size_t ksize(const void *object)
return PAGE_SIZE << compound_order(page);
}
- return slab_ksize(page->slab);
+ return slab_ksize(page->slab_cache);
}
EXPORT_SYMBOL(ksize);
@@ -3438,8 +3438,8 @@ bool verify_mem_not_deleted(const void *x)
}
slab_lock(page);
- if (on_freelist(page->slab, page, object)) {
- object_err(page->slab, page, object, "Object is on free-list");
+ if (on_freelist(page->slab_cache, page, object)) {
+ object_err(page->slab_cache, page, object, "Object is on free-list");
rv = false;
} else {
rv = true;
@@ -3470,7 +3470,7 @@ void kfree(const void *x)
__free_pages(page, compound_order(page));
return;
}
- slab_free(page->slab, page, object, _RET_IP_);
+ slab_free(page->slab_cache, page, object, _RET_IP_);
}
EXPORT_SYMBOL(kfree);
@@ -3681,11 +3681,11 @@ static void __init kmem_cache_bootstrap_fixup(struct kmem_cache *s)
if (n) {
list_for_each_entry(p, &n->partial, lru)
- p->slab = s;
+ p->slab_cache = s;
#ifdef CONFIG_SLUB_DEBUG
list_for_each_entry(p, &n->full, lru)
- p->slab = s;
+ p->slab_cache = s;
#endif
}
}
--
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: Christoph Lameter <[email protected]>
CC: Pekka Enberg <[email protected]>
CC: David Rientjes <[email protected]>
---
mm/slab.c | 13 +------------
mm/slab.h | 1 +
mm/slab_common.c | 17 +++++++++++++++++
mm/slob.c | 11 ++++-------
mm/slub.c | 5 +----
5 files changed, 24 insertions(+), 23 deletions(-)
diff --git a/mm/slab.c b/mm/slab.c
index 98b3460..b8171ab 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3903,15 +3903,7 @@ 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)
+void __kmem_cache_free(struct kmem_cache *cachep, void *objp)
{
unsigned long flags;
@@ -3921,10 +3913,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.h b/mm/slab.h
index 66a62d3..dc1024f 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -34,6 +34,7 @@ extern struct kmem_cache *kmem_cache;
/* Functions provided by the slab allocators */
extern int __kmem_cache_create(struct kmem_cache *, unsigned long flags);
+extern void __kmem_cache_free(struct kmem_cache *s, void *x);
#ifdef CONFIG_SLUB
struct kmem_cache *__kmem_cache_alias(const char *name, size_t size,
diff --git a/mm/slab_common.c b/mm/slab_common.c
index bf4b4f1..3b9d5c5 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"
enum slab_state slab_state;
@@ -168,6 +170,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..d131f75 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)
+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..0c512de 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2606,7 +2606,7 @@ redo:
}
-void kmem_cache_free(struct kmem_cache *s, void *x)
+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
On Mon, 22 Oct 2012, Glauber Costa wrote:
> The naming used by slab, "slab_cache", is less confusing, and it is
> preferred over slub's generic "slab".
Acked-by: Christoph Lameter <[email protected]>
On Mon, 22 Oct 2012, Glauber Costa wrote:
> + * 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);
> +
This results in an additional indirection if tracing is off. Wonder if
there is a performance impact?
On 10/22/2012 06:45 PM, Christoph Lameter wrote:
> On Mon, 22 Oct 2012, Glauber Costa wrote:
>
>> + * 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);
>> +
>
> This results in an additional indirection if tracing is off. Wonder if
> there is a performance impact?
>
if tracing is on, you mean?
Tracing already incurs overhead, not sure how much a function call would
add to the tracing overhead.
I would not be concerned with this, but I can measure, if you have any
specific workload in mind.
Hello, Glauber.
2012/10/23 Glauber Costa <[email protected]>:
> On 10/22/2012 06:45 PM, Christoph Lameter wrote:
>> On Mon, 22 Oct 2012, Glauber Costa wrote:
>>
>>> + * 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);
>>> +
>>
>> This results in an additional indirection if tracing is off. Wonder if
>> there is a performance impact?
>>
> if tracing is on, you mean?
>
> Tracing already incurs overhead, not sure how much a function call would
> add to the tracing overhead.
>
> I would not be concerned with this, but I can measure, if you have any
> specific workload in mind.
With this patch, kmem_cache_free() invokes __kmem_cache_free(),
that is, it add one more "call instruction" than before.
I think that Christoph's comment means above fact.
Thanks.
On 10/23/2012 04:48 AM, JoonSoo Kim wrote:
> Hello, Glauber.
>
> 2012/10/23 Glauber Costa <[email protected]>:
>> On 10/22/2012 06:45 PM, Christoph Lameter wrote:
>>> On Mon, 22 Oct 2012, Glauber Costa wrote:
>>>
>>>> + * 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);
>>>> +
>>>
>>> This results in an additional indirection if tracing is off. Wonder if
>>> there is a performance impact?
>>>
>> if tracing is on, you mean?
>>
>> Tracing already incurs overhead, not sure how much a function call would
>> add to the tracing overhead.
>>
>> I would not be concerned with this, but I can measure, if you have any
>> specific workload in mind.
>
> With this patch, kmem_cache_free() invokes __kmem_cache_free(),
> that is, it add one more "call instruction" than before.
>
> I think that Christoph's comment means above fact.
Ah, this. Ok, I got fooled by his mention to tracing.
I do agree, but since freeing is ultimately dependent on the allocator
layout, I don't see a clean way of doing this without dropping tears of
sorrow around. The calls in slub/slab/slob would have to be somehow
inlined. Hum... maybe it is possible to do it from
include/linux/sl*b_def.h...
Let me give it a try and see what I can come up with.
On 10/23/2012 12:07 PM, Glauber Costa wrote:
> On 10/23/2012 04:48 AM, JoonSoo Kim wrote:
>> Hello, Glauber.
>>
>> 2012/10/23 Glauber Costa <[email protected]>:
>>> On 10/22/2012 06:45 PM, Christoph Lameter wrote:
>>>> On Mon, 22 Oct 2012, Glauber Costa wrote:
>>>>
>>>>> + * 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);
>>>>> +
>>>>
>>>> This results in an additional indirection if tracing is off. Wonder if
>>>> there is a performance impact?
>>>>
>>> if tracing is on, you mean?
>>>
>>> Tracing already incurs overhead, not sure how much a function call would
>>> add to the tracing overhead.
>>>
>>> I would not be concerned with this, but I can measure, if you have any
>>> specific workload in mind.
>>
>> With this patch, kmem_cache_free() invokes __kmem_cache_free(),
>> that is, it add one more "call instruction" than before.
>>
>> I think that Christoph's comment means above fact.
>
> Ah, this. Ok, I got fooled by his mention to tracing.
>
> I do agree, but since freeing is ultimately dependent on the allocator
> layout, I don't see a clean way of doing this without dropping tears of
> sorrow around. The calls in slub/slab/slob would have to be somehow
> inlined. Hum... maybe it is possible to do it from
> include/linux/sl*b_def.h...
>
> Let me give it a try and see what I can come up with.
>
Ok.
I am attaching a PoC for this for your appreciation. This gets quite
ugly, but it's the way I found without including sl{a,u,o}b.c directly -
which would be even worse.
But I guess if we really want to avoid the cost of a function call,
there has to be a tradeoff...
For the record, the proposed usage for this would be:
1) Given a (inline) function, defined in mm/slab.h that translates the
cache from its object address (and then sanity checks it against the
cache parameter), translate_cache():
#define KMEM_CACHE_FREE(allocator_fn) \
void kmem_cache_free(struct kmem_cache *s, void *x) \
{ \
struct kmem_cache *cachep; \
cachep = translate_cache(s, x); \
if (!cachep) \
return; \
allocator_fn(cachep, x); \
trace_kmem_cache_free(_RET_IP_, x); \
} \
EXPORT_SYMBOL(kmem_cache_free)
On Tue, 23 Oct 2012, Glauber Costa wrote:
> I do agree, but since freeing is ultimately dependent on the allocator
> layout, I don't see a clean way of doing this without dropping tears of
> sorrow around. The calls in slub/slab/slob would have to be somehow
> inlined. Hum... maybe it is possible to do it from
> include/linux/sl*b_def.h...
>
> Let me give it a try and see what I can come up with.
The best solution would be something that would have a consolidated
kmem_cache_free() in include/linux/slab.h.
On 10/23/2012 06:12 PM, Christoph Lameter wrote:
> On Tue, 23 Oct 2012, Glauber Costa wrote:
>
>> I do agree, but since freeing is ultimately dependent on the allocator
>> layout, I don't see a clean way of doing this without dropping tears of
>> sorrow around. The calls in slub/slab/slob would have to be somehow
>> inlined. Hum... maybe it is possible to do it from
>> include/linux/sl*b_def.h...
>>
>> Let me give it a try and see what I can come up with.
>
> The best solution would be something that would have a consolidated
> kmem_cache_free() in include/linux/slab.h.
>
I don't know what exactly do you have in mind, but since the cache
layouts are very different, this is quite hard to do without incurring
without function calls anyway.
Do take a look at what I sent in response to that, and tell me what do
you think.
On Tue, 23 Oct 2012, Glauber Costa wrote:
> I don't know what exactly do you have in mind, but since the cache
> layouts are very different, this is quite hard to do without incurring
> without function calls anyway.
True. Sigh.
2012/10/23 Glauber Costa <[email protected]>:
> On 10/23/2012 12:07 PM, Glauber Costa wrote:
>> On 10/23/2012 04:48 AM, JoonSoo Kim wrote:
>>> Hello, Glauber.
>>>
>>> 2012/10/23 Glauber Costa <[email protected]>:
>>>> On 10/22/2012 06:45 PM, Christoph Lameter wrote:
>>>>> On Mon, 22 Oct 2012, Glauber Costa wrote:
>>>>>
>>>>>> + * 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);
>>>>>> +
>>>>>
>>>>> This results in an additional indirection if tracing is off. Wonder if
>>>>> there is a performance impact?
>>>>>
>>>> if tracing is on, you mean?
>>>>
>>>> Tracing already incurs overhead, not sure how much a function call would
>>>> add to the tracing overhead.
>>>>
>>>> I would not be concerned with this, but I can measure, if you have any
>>>> specific workload in mind.
>>>
>>> With this patch, kmem_cache_free() invokes __kmem_cache_free(),
>>> that is, it add one more "call instruction" than before.
>>>
>>> I think that Christoph's comment means above fact.
>>
>> Ah, this. Ok, I got fooled by his mention to tracing.
>>
>> I do agree, but since freeing is ultimately dependent on the allocator
>> layout, I don't see a clean way of doing this without dropping tears of
>> sorrow around. The calls in slub/slab/slob would have to be somehow
>> inlined. Hum... maybe it is possible to do it from
>> include/linux/sl*b_def.h...
>>
>> Let me give it a try and see what I can come up with.
>>
>
> Ok.
>
> I am attaching a PoC for this for your appreciation. This gets quite
> ugly, but it's the way I found without including sl{a,u,o}b.c directly -
> which would be even worse.
Hmm...
This is important issue for sl[aou]b common allocators.
Because there are similar functions like as kmem_cache_alloc, ksize, kfree, ...
So it is good time to resolve this issue.
As far as I know, now, we have 3 solutions.
1. include/linux/slab.h
__always_inline kmem_cache_free()
{
__kmem_cache_free();
blablabla...
}
2. define macro like as Glauber's solution
3. include sl[aou]b.c directly.
Is there other good solution?
Among them, I prefer "solution 3", because future developing cost may
be minimum among them.
"Solution 2" may be error-prone for future developing.
"Solution 1" may make compile-time longer and larger code.
Is my understanding right?
Is "Solution 3" really ugly?
Thanks.
On Mon, 22 Oct 2012, Glauber Costa wrote:
> > This results in an additional indirection if tracing is off. Wonder if
> > there is a performance impact?
> >
> if tracing is on, you mean?
Sorry I meant *even* if tracing is off.
On 10/23/2012 07:43 PM, JoonSoo Kim wrote:
> 2012/10/23 Glauber Costa <[email protected]>:
>> On 10/23/2012 12:07 PM, Glauber Costa wrote:
>>> On 10/23/2012 04:48 AM, JoonSoo Kim wrote:
>>>> Hello, Glauber.
>>>>
>>>> 2012/10/23 Glauber Costa <[email protected]>:
>>>>> On 10/22/2012 06:45 PM, Christoph Lameter wrote:
>>>>>> On Mon, 22 Oct 2012, Glauber Costa wrote:
>>>>>>
>>>>>>> + * 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);
>>>>>>> +
>>>>>>
>>>>>> This results in an additional indirection if tracing is off. Wonder if
>>>>>> there is a performance impact?
>>>>>>
>>>>> if tracing is on, you mean?
>>>>>
>>>>> Tracing already incurs overhead, not sure how much a function call would
>>>>> add to the tracing overhead.
>>>>>
>>>>> I would not be concerned with this, but I can measure, if you have any
>>>>> specific workload in mind.
>>>>
>>>> With this patch, kmem_cache_free() invokes __kmem_cache_free(),
>>>> that is, it add one more "call instruction" than before.
>>>>
>>>> I think that Christoph's comment means above fact.
>>>
>>> Ah, this. Ok, I got fooled by his mention to tracing.
>>>
>>> I do agree, but since freeing is ultimately dependent on the allocator
>>> layout, I don't see a clean way of doing this without dropping tears of
>>> sorrow around. The calls in slub/slab/slob would have to be somehow
>>> inlined. Hum... maybe it is possible to do it from
>>> include/linux/sl*b_def.h...
>>>
>>> Let me give it a try and see what I can come up with.
>>>
>>
>> Ok.
>>
>> I am attaching a PoC for this for your appreciation. This gets quite
>> ugly, but it's the way I found without including sl{a,u,o}b.c directly -
>> which would be even worse.
>
> Hmm...
> This is important issue for sl[aou]b common allocators.
> Because there are similar functions like as kmem_cache_alloc, ksize, kfree, ...
> So it is good time to resolve this issue.
>
> As far as I know, now, we have 3 solutions.
>
> 1. include/linux/slab.h
> __always_inline kmem_cache_free()
> {
> __kmem_cache_free();
> blablabla...
> }
>
> 2. define macro like as Glauber's solution
> 3. include sl[aou]b.c directly.
>
> Is there other good solution?
> Among them, I prefer "solution 3", because future developing cost may
> be minimum among them.
>
> "Solution 2" may be error-prone for future developing.
> "Solution 1" may make compile-time longer and larger code.
>
> Is my understanding right?
> Is "Solution 3" really ugly?
>
As much as I agree my proposed solution is ugly (I wouldn't necessarily
call it error-prone, it's just that that doesn't belong in there, and it
is hard for people to figure out what is going on at first look), I
don't like "Solution 3" either.
The main reason is that over time, .c files tend to grow with more code.
You are not necessarily seeing what are in the other files, and the
opportunities for name clashes become abundant. Among others...
So what I coded, at least has the advantage of restricting this to only
a selected subset of user-visible functions.
On Mon, Oct 22, 2012 at 5:05 PM, Glauber Costa <[email protected]> wrote:
> +/**
> + * 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);
As Christoph mentioned, this is going to hurt performance. The proper
way to do this is to implement the *hook* in mm/slab_common.c and call
that from all the allocator specific kmem_cache_free() functions.
Pekka
On Mon, Oct 22, 2012 at 5:05 PM, Glauber Costa <[email protected]> wrote:
> +/**
> + * 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);
As Christoph mentioned, this is going to hurt performance. The proper
way to do this is to implement the *hook* in mm/slab_common.c and call
that from all the allocator specific kmem_cache_free() functions.
Pekka
On Mon, 22 Oct 2012, Glauber Costa wrote:
> Right now, slab and slub have fields in struct page to derive which
> cache a page belongs to, but they do it slightly differently.
>
> slab uses a field called slab_cache, that lives in the third double
> word. slub, uses a field called "slab", living outside of the
> doublewords area.
>
> Ideally, we could use the same field for this. Since slub heavily makes
> use of the doubleword region, there isn't really much room to move
> slub's slab_cache field around. Since slab does not have such strict
> placement restrictions, we can move it outside the doubleword area.
>
> The naming used by slab, "slab_cache", is less confusing, and it is
> preferred over slub's generic "slab".
>
> Signed-off-by: Glauber Costa <[email protected]>
> CC: Christoph Lameter <[email protected]>
> CC: Pekka Enberg <[email protected]>
> CC: David Rientjes <[email protected]>
Applied, thanks!
On 10/24/2012 12:56 PM, Pekka Enberg wrote:
> On Mon, Oct 22, 2012 at 5:05 PM, Glauber Costa <[email protected]> wrote:
>> +/**
>> + * 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);
>
> As Christoph mentioned, this is going to hurt performance. The proper
> way to do this is to implement the *hook* in mm/slab_common.c and call
> that from all the allocator specific kmem_cache_free() functions.
>
> Pekka
>
We would ideally like the hooks to be inlined as well. Specially for the
memcg-disabled case, this will only get us a function call for no reason.
On 10/23/2012 07:43 PM, JoonSoo Kim wrote:
> 2012/10/23 Glauber Costa <[email protected]>:
>> On 10/23/2012 12:07 PM, Glauber Costa wrote:
>>> On 10/23/2012 04:48 AM, JoonSoo Kim wrote:
>>>> Hello, Glauber.
>>>>
>>>> 2012/10/23 Glauber Costa <[email protected]>:
>>>>> On 10/22/2012 06:45 PM, Christoph Lameter wrote:
>>>>>> On Mon, 22 Oct 2012, Glauber Costa wrote:
>>>>>>
>>>>>>> + * 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);
>>>>>>> +
>>>>>>
>>>>>> This results in an additional indirection if tracing is off. Wonder if
>>>>>> there is a performance impact?
>>>>>>
>>>>> if tracing is on, you mean?
>>>>>
>>>>> Tracing already incurs overhead, not sure how much a function call would
>>>>> add to the tracing overhead.
>>>>>
>>>>> I would not be concerned with this, but I can measure, if you have any
>>>>> specific workload in mind.
>>>>
>>>> With this patch, kmem_cache_free() invokes __kmem_cache_free(),
>>>> that is, it add one more "call instruction" than before.
>>>>
>>>> I think that Christoph's comment means above fact.
>>>
>>> Ah, this. Ok, I got fooled by his mention to tracing.
>>>
>>> I do agree, but since freeing is ultimately dependent on the allocator
>>> layout, I don't see a clean way of doing this without dropping tears of
>>> sorrow around. The calls in slub/slab/slob would have to be somehow
>>> inlined. Hum... maybe it is possible to do it from
>>> include/linux/sl*b_def.h...
>>>
>>> Let me give it a try and see what I can come up with.
>>>
>>
>> Ok.
>>
>> I am attaching a PoC for this for your appreciation. This gets quite
>> ugly, but it's the way I found without including sl{a,u,o}b.c directly -
>> which would be even worse.
>
> Hmm...
> This is important issue for sl[aou]b common allocators.
> Because there are similar functions like as kmem_cache_alloc, ksize, kfree, ...
> So it is good time to resolve this issue.
>
> As far as I know, now, we have 3 solutions.
>
> 1. include/linux/slab.h
> __always_inline kmem_cache_free()
> {
> __kmem_cache_free();
> blablabla...
> }
>
> 2. define macro like as Glauber's solution
> 3. include sl[aou]b.c directly.
>
> Is there other good solution?
> Among them, I prefer "solution 3", because future developing cost may
> be minimum among them.
>
> "Solution 2" may be error-prone for future developing.
> "Solution 1" may make compile-time longer and larger code.
>
> Is my understanding right?
> Is "Solution 3" really ugly?
>
I just gave it a try. Turns out the result is not *that* bad for my eyes.
I'll post soon.