memcg: fix memcg_cache_name() to use cgroup_name()

As cgroup supports rename, it's unsafe to dereference dentry->d_name without proper vfs locks. Fix this by using cgroup_name().

Signed-off-by Li Zefan <[email protected]>

---

This patch depends on "cgroup: fix cgroup_path() vs rename() race", which has been queued for 3.10.

---
 mm/memcontrol.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

mm/memcontrol.c +0 -0

index 53b8201..72be5c9 100644

mm/memcontrol.c +5 -7

+	if (!name)
+		return NULL;
 
 	rcu_read_lock();
-	dentry = rcu_dereference(memcg->css.cgroup->dentry);
+	snprintf(name, PAGE_SIZE, "%s(%d:%s)", s->name, memcg_cache_id(memcg),
+		 cgroup_name(memcg->css.cgroup));
 	rcu_read_unlock();
 
-	BUG_ON(dentry == NULL);
-
-	name = kasprintf(GFP_KERNEL, "%s(%d:%s)", s->name,
-			 memcg_cache_id(memcg), dentry->d_name.name);
-
 	return name;
 }
 

@@ -3247,7 +3246,7 @@ static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
 	if (new)
 		new->allocflags |= __GFP_KMEMCG;
 
-	kfree(name);
+	free_page((unsigned long)name);
 	return new;
 }
 

Signature · 6 lines

1.8.0.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 18 lines · Li Zefan

As cgroup supports rename, it's unsafe to dereference dentry->d_name without proper vfs locks. Fix this by using cgroup_name().

Signed-off-by Li Zefan <[email protected]>

---

This patch depends on "cgroup: fix cgroup_path() vs rename() race", which has been queued for 3.10.

---
 mm/memcontrol.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

mm/memcontrol.c +0 -0

index 53b8201..72be5c9 100644

mm/memcontrol.c void mem_cgroup_destroy_cache(struct kmem_cache *cachep) · line 3221

Quoted 6 lines

mm/memcontrol.c +2 -1

@@ -3217,17 +3217,16 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
 {
 	char *name;
-	struct dentry *dentry;
+
+	name = (char *)__get_free_page(GFP_TEMPORARY);

Ouch. Can we use a static temporary buffer instead?

This is called from workqueue context so we do not have to be afraid of the deep call chain.

It is also not a hot path AFAICS.

Even GFP_ATOMIC for kasprintf would be an improvement IMO.

Quoted 27 lines · Michal Hocko

Quoted 18 lines · Li Zefan

As cgroup supports rename, it's unsafe to dereference dentry->d_name without proper vfs locks. Fix this by using cgroup_name().

Signed-off-by Li Zefan <[email protected]>

---

This patch depends on "cgroup: fix cgroup_path() vs rename() race", which has been queued for 3.10.

---
 mm/memcontrol.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

mm/memcontrol.c +0 -0

index 53b8201..72be5c9 100644

mm/memcontrol.c void mem_cgroup_destroy_cache(struct kmem_cache *cachep) · line 3221

Quoted 6 lines

mm/memcontrol.c +2 -1

@@ -3217,17 +3217,16 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
 {
 	char *name;
-	struct dentry *dentry;
+
+	name = (char *)__get_free_page(GFP_TEMPORARY);

Ouch. Can we use a static temporary buffer instead?

Quoted 2 lines

This is called from workqueue context so we do not have to be afraid of the deep call chain.

Bahh, I was thinking about two things at the same time and that is how it ends... I meant a temporary buffer on the stack. But a separate allocation sounds even easier.

Quoted 3 lines

It is also not a hot path AFAICS.

Even GFP_ATOMIC for kasprintf would be an improvement IMO.

What about the following (not even compile tested because I do not have cgroup_name in my tree yet):

---

mm/memcontrol.c +3 -5

index f608546..ede0382 100644
@@ -3370,13 +3370,18 @@ static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
 	struct dentry *dentry;
 
 	rcu_read_lock();
-	dentry = rcu_dereference(memcg->css.cgroup->dentry);
+	name = kasprintf(GFP_ATOMIC, "%s(%d:%s)", s->name,
+			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
 	rcu_read_unlock();
 
-	BUG_ON(dentry == NULL);
-
-	name = kasprintf(GFP_KERNEL, "%s(%d:%s)", s->name,
-			 memcg_cache_id(memcg), dentry->d_name.name);
+	if (!name) {

Quoted 29 lines · Michal Hocko

Quoted 27 lines · Michal Hocko

Quoted 18 lines · Li Zefan

As cgroup supports rename, it's unsafe to dereference dentry->d_name without proper vfs locks. Fix this by using cgroup_name().

Signed-off-by Li Zefan <[email protected]>

---

This patch depends on "cgroup: fix cgroup_path() vs rename() race", which has been queued for 3.10.

---
 mm/memcontrol.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

mm/memcontrol.c +0 -0

index 53b8201..72be5c9 100644

mm/memcontrol.c void mem_cgroup_destroy_cache(struct kmem_cache *cachep) · line 3221

Quoted 6 lines

mm/memcontrol.c +2 -1

@@ -3217,17 +3217,16 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
 {
 	char *name;
-	struct dentry *dentry;
+
+	name = (char *)__get_free_page(GFP_TEMPORARY);

Ouch. Can we use a static temporary buffer instead?

Quoted 7 lines

Quoted 2 lines

This is called from workqueue context so we do not have to be afraid of the deep call chain.

Bahh, I was thinking about two things at the same time and that is how it ends... I meant a temporary buffer on the stack. But a separate allocation sounds even easier.

Actually I don't care much about which way to take. Use on-stack buffer (if stack usage is not a concern) or local static buffer (caller already held memcg_cache_mutex) is simplest.

But why it's bad to allocate a page for temp use?

Quoted 6 lines

Quoted 3 lines

It is also not a hot path AFAICS.

Even GFP_ATOMIC for kasprintf would be an improvement IMO.

What about the following (not even compile tested because I do not have cgroup_name in my tree yet):

No, it won't compile. ;)

Quoted 18 lines

Quoted 17 lines

Quoted 10 lines

Review void mem_cgroup_destroy_cache(struct kmem_cache *cachep) · line 3221

Quoted 7 lines

(quoted diff) +2 -1

@@ -3217,17 +3217,16 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
 {
 	char *name;
-	struct dentry *dentry;
+
+	name = (char *)__get_free_page(GFP_TEMPORARY);

Ouch. Can we use a static temporary buffer instead?

Quoted 2 lines

This is called from workqueue context so we do not have to be afraid of the deep call chain.

Bahh, I was thinking about two things at the same time and that is how it ends... I meant a temporary buffer on the stack. But a separate allocation sounds even easier.

Quoted 5 lines

Actually I don't care much about which way to take. Use on-stack buffer (if stack usage is not a concern) or local static buffer (caller already held memcg_cache_mutex) is simplest.

But why it's bad to allocate a page for temp use?

GFP_TEMPORARY groups short lived allocations but the mem cache is not an ideal candidate of this type of allocations..

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

It is also not a hot path AFAICS.

Even GFP_ATOMIC for kasprintf would be an improvement IMO.

What about the following (not even compile tested because I do not have cgroup_name in my tree yet):

No, it won't compile. ;)

Somehow expected so as this was just a quick hack to show what I meant. The full patch is bellow (compile time tested on top of for-3.10 branch this time :P)
---

Quoted 1 line

From 7e1f6f0e266a230ced238a9bf2398b4069a6a764 Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Fri, 22 Mar 2013 09:04:58 +0100
Subject: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

As cgroup supports rename, it's unsafe to dereference dentry->d_name without proper vfs locks. Fix this by using cgroup_name().

Signed-off-by Li Zefan <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

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

mm/memcontrol.c +9 -4

index 53b8201..5741bf5 100644
@@ -3220,13 +3220,18 @@ static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
 	struct dentry *dentry;
 
 	rcu_read_lock();
-	dentry = rcu_dereference(memcg->css.cgroup->dentry);
+	name = kasprintf(GFP_ATOMIC, "%s(%d:%s)", s->name,
+			 memcg_cache_id(memcg), dcgroup_name(memcg->css.cgroup));
 	rcu_read_unlock();
 
-	BUG_ON(dentry == NULL);
+	if (!name) {
+		name = kmalloc(PAGE_SIZE, GFP_KERNEL);
+		rcu_read_lock();
+		name = snprintf(name, PAGE_SIZE, "%s(%d:%s)", s->name,
+				 memcg_cache_id(memcg), dcgroup_name(memcg->css.cgroup));
+		rcu_read_unlock();
 
-	name = kasprintf(GFP_KERNEL, "%s(%d:%s)", s->name,
-			 memcg_cache_id(memcg), dentry->d_name.name);
+	}
 
 	return name;
 }

Quoted 3 lines

GFP_TEMPORARY groups short lived allocations but the mem cache is not

Quoted 2 lines

an ideal candidate of this type of allocations..

I'm not sure I'm following you...

char *memcg_cache_name()

{
	char *name = alloc();
	return name;
}

kmem_cache_dup()

{
	name = memcg_cache_name();
	kmem_cache_create_memcg(name);
	free(name);
}

Isn't this a short lived allocation?

Quoted 11 lines · Li Zefan

Quoted 3 lines

GFP_TEMPORARY groups short lived allocations but the mem cache is not

Quoted 2 lines

an ideal candidate of this type of allocations..

I'm not sure I'm following you...

char *memcg_cache_name()

{
	char *name = alloc();
	return name;
}

Quoted 9 lines

kmem_cache_dup()

{
	name = memcg_cache_name();
	kmem_cache_create_memcg(name);
	free(name);
}

Isn't this a short lived allocation?

Hi,

Thanks for identifying and fixing this.

Li is right. The cache name will live long, but this is because the slab/slub caches will strdup it internally. So the actual memcg allocation is short lived.

Quoted 26 lines · Glauber Costa

Quoted 11 lines · Li Zefan

Quoted 3 lines

GFP_TEMPORARY groups short lived allocations but the mem cache is not

Quoted 2 lines

an ideal candidate of this type of allocations..

I'm not sure I'm following you...

char *memcg_cache_name()

{
	char *name = alloc();
	return name;
}

Quoted 9 lines

kmem_cache_dup()

{
	name = memcg_cache_name();
	kmem_cache_create_memcg(name);
	free(name);
}

Isn't this a short lived allocation?

Hi,

Thanks for identifying and fixing this.

Quoted 3 lines

Li is right. The cache name will live long, but this is because the slab/slub caches will strdup it internally. So the actual memcg allocation is short lived.

OK, I have totally missed that. Sorry about the confusion. Then all the churn around the allocation is pointless, no?
What about:

Quoted 31 lines · Michal Hocko

Quoted 29 lines · Glauber Costa

Quoted 11 lines · Li Zefan

Quoted 3 lines

GFP_TEMPORARY groups short lived allocations but the mem cache is not

Quoted 2 lines

an ideal candidate of this type of allocations..

I'm not sure I'm following you...

char *memcg_cache_name()

{
	char *name = alloc();
	return name;
}

Quoted 9 lines

kmem_cache_dup()

{
	name = memcg_cache_name();
	kmem_cache_create_memcg(name);
	free(name);
}

Isn't this a short lived allocation?

Hi,

Thanks for identifying and fixing this.

Li is right. The cache name will live long, but this is because the slab/slub caches will strdup it internally. So the actual memcg allocation is short lived.

Quoted 3 lines

OK, I have totally missed that. Sorry about the confusion. Then all the churn around the allocation is pointless, no?
What about:

If we're really not concerned about stack, then yes. Even if always running from workqueues, a PAGE_SIZEd stack variable seems risky to me.

Quoted 32 lines · Glauber Costa

Quoted 31 lines · Michal Hocko

Quoted 29 lines · Glauber Costa

Quoted 11 lines · Li Zefan

Quoted 3 lines

GFP_TEMPORARY groups short lived allocations but the mem cache is not

Quoted 2 lines

an ideal candidate of this type of allocations..

I'm not sure I'm following you...

char *memcg_cache_name()

{
	char *name = alloc();
	return name;
}

Quoted 9 lines

kmem_cache_dup()

{
	name = memcg_cache_name();
	kmem_cache_create_memcg(name);
	free(name);
}

Isn't this a short lived allocation?

Hi,

Thanks for identifying and fixing this.

Li is right. The cache name will live long, but this is because the slab/slub caches will strdup it internally. So the actual memcg allocation is short lived.

Quoted 6 lines

Quoted 3 lines

OK, I have totally missed that. Sorry about the confusion. Then all the churn around the allocation is pointless, no?
What about:

If we're really not concerned about stack, then yes. Even if always running from workqueues, a PAGE_SIZEd stack variable seems risky to me.

This is not on stack. It is static

Quoted 40 lines · Michal Hocko

Quoted 38 lines · Glauber Costa

Quoted 27 lines · Michal Hocko

Quoted 26 lines · Glauber Costa

Quoted 11 lines · Li Zefan

Quoted 3 lines

GFP_TEMPORARY groups short lived allocations but the mem cache is not

Quoted 2 lines

an ideal candidate of this type of allocations..

I'm not sure I'm following you...

char *memcg_cache_name()

{
	char *name = alloc();
	return name;
}

Quoted 9 lines

kmem_cache_dup()

{
	name = memcg_cache_name();
	kmem_cache_create_memcg(name);
	free(name);
}

Isn't this a short lived allocation?

Hi,

Thanks for identifying and fixing this.

Quoted 7 lines

Quoted 3 lines

Li is right. The cache name will live long, but this is because the slab/slub caches will strdup it internally. So the actual memcg allocation is short lived.

OK, I have totally missed that. Sorry about the confusion. Then all the churn around the allocation is pointless, no?
What about:

If we're really not concerned about stack, then yes. Even if always running from workqueues, a PAGE_SIZEd stack variable seems risky to me.

Quoted 2 lines

This is not on stack. It is static

Ah, right, I totally missed that. And then you're taking the mutex.

But actually, you don't need to take the mutex. All calls to kmem_cache_dup are protected by the memcg_cache_mutex.

So you should be able to just use the buffer without further problems.

Quoted 43 lines · Glauber Costa

Quoted 42 lines · Michal Hocko

Quoted 32 lines · Glauber Costa

Quoted 31 lines · Michal Hocko

Quoted 29 lines · Glauber Costa

Quoted 11 lines · Li Zefan

Quoted 3 lines

GFP_TEMPORARY groups short lived allocations but the mem cache is not

Quoted 2 lines

an ideal candidate of this type of allocations..

I'm not sure I'm following you...

char *memcg_cache_name()

{
	char *name = alloc();
	return name;
}

Quoted 9 lines

kmem_cache_dup()

{
	name = memcg_cache_name();
	kmem_cache_create_memcg(name);
	free(name);
}

Isn't this a short lived allocation?

Hi,

Thanks for identifying and fixing this.

Li is right. The cache name will live long, but this is because the slab/slub caches will strdup it internally. So the actual memcg allocation is short lived.

Quoted 6 lines

Quoted 3 lines

OK, I have totally missed that. Sorry about the confusion. Then all the churn around the allocation is pointless, no?
What about:

If we're really not concerned about stack, then yes. Even if always running from workqueues, a PAGE_SIZEd stack variable seems risky to me.

This is not on stack. It is static

Quoted 4 lines

Ah, right, I totally missed that. And then you're taking the mutex.

But actually, you don't need to take the mutex. All calls to kmem_cache_dup are protected by the memcg_cache_mutex.

Yes and I am not taking that mutex. I've just added lockdep assert to make sure that this still holds true.

Quoted 46 lines · Michal Hocko

Quoted 45 lines · Glauber Costa

Quoted 33 lines · Michal Hocko

Quoted 32 lines · Glauber Costa

Quoted 31 lines · Michal Hocko

Quoted 29 lines · Glauber Costa

Quoted 11 lines · Li Zefan

Quoted 3 lines

GFP_TEMPORARY groups short lived allocations but the mem cache is not

Quoted 2 lines

an ideal candidate of this type of allocations..

I'm not sure I'm following you...

char *memcg_cache_name()

{
	char *name = alloc();
	return name;
}

Quoted 9 lines

kmem_cache_dup()

{
	name = memcg_cache_name();
	kmem_cache_create_memcg(name);
	free(name);
}

Isn't this a short lived allocation?

Hi,

Thanks for identifying and fixing this.

Li is right. The cache name will live long, but this is because the slab/slub caches will strdup it internally. So the actual memcg allocation is short lived.

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

OK, I have totally missed that. Sorry about the confusion. Then all the churn around the allocation is pointless, no?
What about:

If we're really not concerned about stack, then yes. Even if always running from workqueues, a PAGE_SIZEd stack variable seems risky to me.

This is not on stack. It is static

Ah, right, I totally missed that. And then you're taking the mutex.

Quoted 6 lines

Quoted 2 lines

But actually, you don't need to take the mutex. All calls to kmem_cache_dup are protected by the memcg_cache_mutex.

Yes and I am not taking that mutex. I've just added lockdep assert to make sure that this still holds true.

It is impressive what a busy week does to our brains...

I read the code as lockdep_assert(memcg_cache_mutex), and then later on mutex_lock(&memcg_mutex). But reading again, that was a just an rcu_read_lock(). Good thing it is Friday

You guys can add my Acked-by, and thanks again

Quoted 5 lines

I read the code as lockdep_assert(memcg_cache_mutex), and then later on mutex_lock(&memcg_mutex). But reading again, that was a just an rcu_read_lock(). Good thing it is Friday

You guys can add my Acked-by, and thanks again

Li, are you ok to take the page via your tree?

Quoted 8 lines

Quoted 5 lines

I read the code as lockdep_assert(memcg_cache_mutex), and then later on mutex_lock(&memcg_mutex). But reading again, that was a just an rcu_read_lock(). Good thing it is Friday

You guys can add my Acked-by, and thanks again

Li, are you ok to take the page via your tree?

I don't have a git tree in kernel.org. It's Tejun that picks up cgroup patches.

Quoted 5 lines

Thanks for identifying and fixing this.

Li is right. The cache name will live long, but this is because the slab/slub caches will strdup it internally. So the actual memcg allocation is short lived.

OK, I have totally missed that. Sorry about the confusion. Then all the churn around the allocation is pointless, no?
What about:
---

Quoted 1 line

From 7ed7f53bb597e8cb40d9ac91ce16142fb60f1e93 Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Fri, 22 Mar 2013 10:22:54 +0100
Subject: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

As cgroup supports rename, it's unsafe to dereference dentry->d_name without proper vfs locks. Fix this by using cgroup_name() rather than dentry directly.

Also open code memcg_cache_name because it is called only from kmem_cache_dup which frees the returned name right after kmem_cache_create_memcg makes a copy of it. Such a short-lived allocation doesn't make too much sense. So replace it by a static buffer as kmem_cache_dup is called with memcg_cache_mutex.

Signed-off-by Li Zefan <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 mm/memcontrol.c |   33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,

mm/memcontrol.c +0 -0

 					 struct kmem_cache *s)
 {
 	char *name;
 	struct kmem_cache *new;

mm/memcontrol.c +1 -0

+	rcu_read_unlock();
 

Quoted 19 lines

Quoted 5 lines

Thanks for identifying and fixing this.

Li is right. The cache name will live long, but this is because the slab/slub caches will strdup it internally. So the actual memcg allocation is short lived.

OK, I have totally missed that. Sorry about the confusion. Then all the churn around the allocation is pointless, no?
What about:
---

Quoted 1 line

From 7ed7f53bb597e8cb40d9ac91ce16142fb60f1e93 Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Fri, 22 Mar 2013 10:22:54 +0100
Subject: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

As cgroup supports rename, it's unsafe to dereference dentry->d_name without proper vfs locks. Fix this by using cgroup_name() rather than dentry directly.

Quoted 6 lines

Also open code memcg_cache_name because it is called only from kmem_cache_dup which frees the returned name right after kmem_cache_create_memcg makes a copy of it. Such a short-lived allocation doesn't make too much sense. So replace it by a static buffer as kmem_cache_dup is called with memcg_cache_mutex.

I doubt it's a win to add 4K to kernel text size instead of adding a few extra lines of code... but it's up to you.

Quoted 5 lines

Signed-off-by Li Zefan <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 mm/memcontrol.c |   33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

...

Quoted 7 lines

static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,

(quoted diff) +1 -0

 					 struct kmem_cache *s)
 {
 	char *name;
 	struct kmem_cache *new;
+	static char tmp_name[PAGE_SIZE];

Review

Quoted 8 lines

(quoted diff) +5 -3

-	name = memcg_cache_name(memcg, s);
-	if (!name)
-		return NULL;
+	lockdep_assert_held(&memcg_cache_mutex);
+
+	rcu_read_lock();
+	tmp_name = snprintf(tmp_name, sizeof(tmp_name), "%s(%d:%s)", s->name,
+			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));

I guess you didn't turn on CONFIG_MEMCG_KMEM?

snprintf() returns a int value.

Quoted 16 lines

Quoted 15 lines

Quoted 1 line

From 7ed7f53bb597e8cb40d9ac91ce16142fb60f1e93 Mon Sep 17 00:00:00 2001

From: Michal Hocko <[email protected]>
Date: Fri, 22 Mar 2013 10:22:54 +0100
Subject: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

As cgroup supports rename, it's unsafe to dereference dentry->d_name without proper vfs locks. Fix this by using cgroup_name() rather than dentry directly.

Also open code memcg_cache_name because it is called only from kmem_cache_dup which frees the returned name right after kmem_cache_create_memcg makes a copy of it. Such a short-lived allocation doesn't make too much sense. So replace it by a static buffer as kmem_cache_dup is called with memcg_cache_mutex.

Quoted 2 lines

I doubt it's a win to add 4K to kernel text size instead of adding a few extra lines of code... but it's up to you.

I will leave the decision to Glauber. The updated version which uses kmalloc for the static buffer is bellow.

...

mm/memcontrol.c void mem_cgroup_destroy_cache(struct kmem_cache *cachep) · line 3234

Quoted 6 lines

static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,

mm/memcontrol.c +1 -1

 					 struct kmem_cache *s)
 {
-	char *name;
 	struct kmem_cache *new;
+	static char *tmp_name = NULL;

(minor nitpick) why not preserve the name "name"

Quoted 3 lines

I doubt it's a win to add 4K to kernel text size instead of adding a few extra lines of code... but it's up to you.

I will leave the decision to Glauber. The updated version which uses kmalloc for the static buffer is bellow.

Quoted 7 lines

Quoted 3 lines

I doubt it's a win to add 4K to kernel text size instead of adding a few extra lines of code... but it's up to you.

I will leave the decision to Glauber. The updated version which uses kmalloc for the static buffer is bellow.

I prefer to allocate dynamically here. But although I understand why we need to call cgroup_name, I don't understand what is wrong with kasprintf if we're going to allocate anyway. It will allocate a string just big enough. A PAGE_SIZE'd allocation is a lot more likely to fail.

Now, if we really want to be smart here, we can do something like what I've done for the slub attribute buffers, that can actually have very long values.

allocate a small buffer that will hold 80 % > of the allocations (256 bytes should be enough for most cache names), and if the string is bigger than this, we allocate. Once we allocate, we save it in a static pointer and leave it there. The hope here is that we may be able to live without ever allocating in many systems.

Review

Quoted 7 lines

(quoted diff) +7 -0

+
+	/*
+	 * kmem_cache_create_memcg duplicates the given name and
+	 * cgroup_name for this name requires RCU context.
+	 * This static temporary buffer is used to prevent from
+	 * pointless shortliving allocation.
+	 */

The comment is also no longer true if you don't resort to a static buffer.

The following (untested) patch implements the idea I outlined above.

What do you guys think ?

(quoted diff) +5 -2

+static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
+					 struct kmem_cache *s)
 {
-	char *name;
-	struct dentry *dentry;
+	const char *cgname; /* actual cache name */
+	char *name = NULL; /* actual cache name */
+	char buf[256]; /* stack buffer for small allocations */

Quoted 22 lines · Glauber Costa

Quoted 7 lines

Quoted 3 lines

I doubt it's a win to add 4K to kernel text size instead of adding a few extra lines of code... but it's up to you.

I will leave the decision to Glauber. The updated version which uses kmalloc for the static buffer is bellow.

I prefer to allocate dynamically here. But although I understand why we need to call cgroup_name, I don't understand what is wrong with kasprintf if we're going to allocate anyway. It will allocate a string just big enough. A PAGE_SIZE'd allocation is a lot more likely to fail.

Now, if we really want to be smart here, we can do something like what I've done for the slub attribute buffers, that can actually have very long values.

allocate a small buffer that will hold 80 % > of the allocations (256 bytes should be enough for most cache names), and if the string is bigger than this, we allocate. Once we allocate, we save it in a static pointer and leave it there. The hope here is that we may be able to live without ever allocating in many systems.

Quoted 8 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+
+	/*
+	 * kmem_cache_create_memcg duplicates the given name and
+	 * cgroup_name for this name requires RCU context.
+	 * This static temporary buffer is used to prevent from
+	 * pointless shortliving allocation.
+	 */

The comment is also no longer true if you don't resort to a static buffer.

The buffer _is_ static (read global variable hidden with the function scope).

Although correct, it is a bit misleading. It is static in the sense it is held by a static variable. But it is acquired by kmalloc...

In any way, this is a tiny detail.

FWIW, I am fine with the patch you provided:

Acked-by Glauber Costa <[email protected]>

Quoted 5 lines

Although correct, it is a bit misleading. It is static in the sense it is held by a static variable. But it is acquired by kmalloc...

In any way, this is a tiny detail.

Quoted 4 lines

FWIW, I am fine with the patch you provided:

Acked-by Glauber Costa <[email protected]>

Michal, could you resend your final patch to Tejun in a new mail thread? There are quite a few different patches inlined in this thread.