memcg-vs-slab related fixes, improvements, cleanups

The way memcg_create_kmem_cache() creates the name for a memcg cache looks rather strange: it first formats the name in the static buffer tmp_name protected by a mutex, then passes the pointer to the buffer to kmem_cache_create_memcg(), which finally duplicates it to the cache name.

Let's clean this up by moving memcg cache name creation to a separate function to be called by kmem_cache_create_memcg(), and estimating the length of the name string before copying anything to it so that we won't need a temporary buffer.

mm/memcontrol.c +17 -0

@@ -3193,6 +3193,37 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
 	return 0;
 }
 
+static int memcg_print_cache_name(char *buf, size_t size,
+		struct mem_cgroup *memcg, struct kmem_cache *root_cache)
+{
+	int ret;
+
+	rcu_read_lock();
+	ret = snprintf(buf, size, "%s(%d:%s)", root_cache->name,
+		       memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
+	rcu_read_unlock();
+	return ret;
+}
+
+char *memcg_create_cache_name(struct mem_cgroup *memcg,
+			      struct kmem_cache *root_cache)
+{
+	int len;
+	char *name;

Quoted 39 lines · On Mon, 3 Feb 2014 19:54:37 +0400 Vladimir Davydov <[email protected]>…

The way memcg_create_kmem_cache() creates the name for a memcg cache looks rather strange: it first formats the name in the static buffer tmp_name protected by a mutex, then passes the pointer to the buffer to kmem_cache_create_memcg(), which finally duplicates it to the cache name.

Let's clean this up by moving memcg cache name creation to a separate function to be called by kmem_cache_create_memcg(), and estimating the length of the name string before copying anything to it so that we won't need a temporary buffer.

mm/memcontrol.c +22 -0

@@ -3193,6 +3193,37 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
 	return 0;
 }
 
+static int memcg_print_cache_name(char *buf, size_t size,
+		struct mem_cgroup *memcg, struct kmem_cache *root_cache)
+{
+	int ret;
+
+	rcu_read_lock();
+	ret = snprintf(buf, size, "%s(%d:%s)", root_cache->name,
+		       memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
+	rcu_read_unlock();
+	return ret;
+}
+
+char *memcg_create_cache_name(struct mem_cgroup *memcg,
+			      struct kmem_cache *root_cache)
+{
+	int len;
+	char *name;
+
+	/*
+	 * We cannot use kasprintf() here, because cgroup_name() must be called
+	 * under RCU protection.
+	 */

mm/memcontrol.c int memcg_update_cache_size(struct kmem_cache *s, int num_groups) · line 3222

Quoted 5 lines

mm/memcontrol.c +5 -0

+	len = memcg_print_cache_name(NULL, 0, memcg, root_cache);
+
+	name = kmalloc(len + 1, GFP_KERNEL);
+	if (name)
+		memcg_print_cache_name(name, len + 1, memcg, root_cache);

but but but this assumes that cgroup_name(memcg->css.cgroup) did not change between the two calls to memcg_print_cache_name(). If that is the case then the locking was unneeded anyway.

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

Quoted 11 lines

mm/memcontrol.c +3 -0

+	return name;
+}
+
 int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
 			     struct kmem_cache *root_cache)
 {

@@ -3397,44 +3428,6 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 	schedule_work(&cachep->memcg_params->destroy);
 }
 

This patch cleanups the memcg cache creation path as follows:
- Move memcg cache name creation to a separate function to be called from kmem_cache_create_memcg(). This allows us to get rid of the mutex protecting the temporary buffer used for the name formatting, because the whole cache creation path is protected by the slab_mutex.
- Get rid of memcg_create_kmem_cache(). This function serves as a proxy to kmem_cache_create_memcg(). After separating the cache name creation path, it would be reduced to a function call, so let's inline it.

Signed-off-by Vladimir Davydov <[email protected]>

---
 include/linux/memcontrol.h |    9 +++++
 mm/memcontrol.c            |   89 ++++++++++++++++++++------------------------
 mm/slab_common.c           |    5 ++-
 3 files changed, 54 insertions(+), 49 deletions(-)

include/linux/memcontrol.h +9 -0

index abd0113b6620..84e4801fc36c 100644
@@ -497,6 +497,9 @@ void __memcg_kmem_commit_charge(struct page *page,
 void __memcg_kmem_uncharge_pages(struct page *page, int order);
 
 int memcg_cache_id(struct mem_cgroup *memcg);
+
+char *memcg_create_cache_name(struct mem_cgroup *memcg,
+			      struct kmem_cache *root_cache);
 int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
 			     struct kmem_cache *root_cache);
 void memcg_free_cache_params(struct kmem_cache *s);

@@ -641,6 +644,12 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
 	return -1;
 }
 
+static inline char *memcg_create_cache_name(struct mem_cgroup *memcg,
+					    struct kmem_cache *root_cache)
+{
+	return NULL;
+}
+
 static inline int memcg_alloc_cache_params(struct mem_cgroup *memcg,
 		struct kmem_cache *s, struct kmem_cache *root_cache)
 {

mm/memcontrol.c +41 -48

index 53385cd4e6f0..43e08b7bb365 100644
@@ -3193,6 +3193,32 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
 	return 0;
 }
 
+char *memcg_create_cache_name(struct mem_cgroup *memcg,
+			      struct kmem_cache *root_cache)
+{
+	static char *buf = NULL;
+
+	/*
+	 * We need a mutex here to protect the shared buffer. Since this is
+	 * expected to be called only on cache creation, we can employ the
+	 * slab_mutex for that purpose.
+	 */
+	lockdep_assert_held(&slab_mutex);
+
+	if (!buf) {
+		buf = kmalloc(PATH_MAX, GFP_KERNEL);
+		if (!buf)
+			return NULL;
+	}
+
+	rcu_read_lock();
+	snprintf(buf, PATH_MAX, "%s(%d:%s)", root_cache->name,
+		 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
+	rcu_read_unlock();
+
+	return kstrdup(buf, GFP_KERNEL);
+}
+
 int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
 			     struct kmem_cache *root_cache)
 {

@@ -3397,44 +3423,6 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 	schedule_work(&cachep->memcg_params->destroy);
 }
 
-static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
-						  struct kmem_cache *s)
-{
-	struct kmem_cache *new = NULL;
-	static char *tmp_name = NULL;
-	static DEFINE_MUTEX(mutex);	/* protects tmp_name */
-
-	BUG_ON(!memcg_can_account_kmem(memcg));
-
-	mutex_lock(&mutex);
-	/*
-	 * 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.
-	 */
-	if (!tmp_name) {
-		tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
-		if (!tmp_name)
-			goto out;
-	}
-
-	rcu_read_lock();
-	snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name,
-			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
-	rcu_read_unlock();
-
-	new = kmem_cache_create_memcg(memcg, tmp_name, s->object_size, s->align,
-				      (s->flags & ~SLAB_PANIC), s->ctor, s);
-	if (new)
-		new->allocflags |= __GFP_KMEMCG;
-	else
-		new = s;
-out:
-	mutex_unlock(&mutex);
-	return new;
-}
-
 void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 {
 	struct kmem_cache *c;

@@ -3481,12 +3469,6 @@ void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 	mutex_unlock(&activate_kmem_mutex);
 }
 
-struct create_work {
-	struct mem_cgroup *memcg;
-	struct kmem_cache *cachep;
-	struct work_struct work;
-};
-
 static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg)
 {
 	struct kmem_cache *cachep;

@@ -3504,13 +3486,24 @@ static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg)
 	mutex_unlock(&memcg->slab_caches_mutex);
 }
 
+struct create_work {
+	struct mem_cgroup *memcg;
+	struct kmem_cache *cachep;
+	struct work_struct work;
+};
+
 static void memcg_create_cache_work_func(struct work_struct *w)
 {
-	struct create_work *cw;
+	struct create_work *cw = container_of(w, struct create_work, work);
+	struct mem_cgroup *memcg = cw->memcg;
+	struct kmem_cache *s = cw->cachep;
+	struct kmem_cache *new;
 
-	cw = container_of(w, struct create_work, work);
-	memcg_create_kmem_cache(cw->memcg, cw->cachep);
-	css_put(&cw->memcg->css);
+	new = kmem_cache_create_memcg(memcg, s->name, s->object_size, s->align,
+				      (s->flags & ~SLAB_PANIC), s->ctor, s);
+	if (new)
+		new->allocflags |= __GFP_KMEMCG;
+	css_put(&memcg->css);
 	kfree(cw);
 }
 

mm/slab_common.c +4 -1

index e77b51eb7347..11857abf7057 100644
@@ -215,7 +215,10 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	s->align = calculate_alignment(flags, align, size);
 	s->ctor = ctor;
 
-	s->name = kstrdup(name, GFP_KERNEL);
+	if (memcg)
+		s->name = memcg_create_cache_name(memcg, parent_cache);
+	else
+		s->name = kstrdup(name, GFP_KERNEL);
 	if (!s->name)
 		goto out_free_cache;
 

Signature · 1 line

1.7.10.4

Quoted 9 lines · Vladimir Davydov

This patch cleanups the memcg cache creation path as follows:
- Move memcg cache name creation to a separate function to be called from kmem_cache_create_memcg(). This allows us to get rid of the mutex protecting the temporary buffer used for the name formatting, because the whole cache creation path is protected by the slab_mutex.
- Get rid of memcg_create_kmem_cache(). This function serves as a proxy to kmem_cache_create_memcg(). After separating the cache name creation path, it would be reduced to a function call, so let's inline it.

OK, this looks better but it will still conflict with Tejun's cleanup now? Can we wait until mmotm settles down a bit and sees those changes?

Maybe we can get rid of the static buffer altogether?

Quoted 175 lines

Signed-off-by Vladimir Davydov <[email protected]>

---
 include/linux/memcontrol.h |    9 +++++
 mm/memcontrol.c            |   89 ++++++++++++++++++++------------------------
 mm/slab_common.c           |    5 ++-
 3 files changed, 54 insertions(+), 49 deletions(-)

include/linux/memcontrol.h +9 -0

index abd0113b6620..84e4801fc36c 100644
@@ -497,6 +497,9 @@ void __memcg_kmem_commit_charge(struct page *page,
 void __memcg_kmem_uncharge_pages(struct page *page, int order);
 
 int memcg_cache_id(struct mem_cgroup *memcg);
+
+char *memcg_create_cache_name(struct mem_cgroup *memcg,
+			      struct kmem_cache *root_cache);
 int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
 			     struct kmem_cache *root_cache);
 void memcg_free_cache_params(struct kmem_cache *s);

@@ -641,6 +644,12 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
 	return -1;
 }
 
+static inline char *memcg_create_cache_name(struct mem_cgroup *memcg,
+					    struct kmem_cache *root_cache)
+{
+	return NULL;
+}
+
 static inline int memcg_alloc_cache_params(struct mem_cgroup *memcg,
 		struct kmem_cache *s, struct kmem_cache *root_cache)
 {

mm/memcontrol.c +41 -48

index 53385cd4e6f0..43e08b7bb365 100644
@@ -3193,6 +3193,32 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
 	return 0;
 }
 
+char *memcg_create_cache_name(struct mem_cgroup *memcg,
+			      struct kmem_cache *root_cache)
+{
+	static char *buf = NULL;
+
+	/*
+	 * We need a mutex here to protect the shared buffer. Since this is
+	 * expected to be called only on cache creation, we can employ the
+	 * slab_mutex for that purpose.
+	 */
+	lockdep_assert_held(&slab_mutex);
+
+	if (!buf) {
+		buf = kmalloc(PATH_MAX, GFP_KERNEL);
+		if (!buf)
+			return NULL;
+	}
+
+	rcu_read_lock();
+	snprintf(buf, PATH_MAX, "%s(%d:%s)", root_cache->name,
+		 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
+	rcu_read_unlock();
+
+	return kstrdup(buf, GFP_KERNEL);
+}
+
 int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
 			     struct kmem_cache *root_cache)
 {

@@ -3397,44 +3423,6 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 	schedule_work(&cachep->memcg_params->destroy);
 }
 
-static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
-						  struct kmem_cache *s)
-{
-	struct kmem_cache *new = NULL;
-	static char *tmp_name = NULL;
-	static DEFINE_MUTEX(mutex);	/* protects tmp_name */
-
-	BUG_ON(!memcg_can_account_kmem(memcg));
-
-	mutex_lock(&mutex);
-	/*
-	 * 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.
-	 */
-	if (!tmp_name) {
-		tmp_name = kmalloc(PATH_MAX, GFP_KERNEL);
-		if (!tmp_name)
-			goto out;
-	}
-
-	rcu_read_lock();
-	snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name,
-			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
-	rcu_read_unlock();
-
-	new = kmem_cache_create_memcg(memcg, tmp_name, s->object_size, s->align,
-				      (s->flags & ~SLAB_PANIC), s->ctor, s);
-	if (new)
-		new->allocflags |= __GFP_KMEMCG;
-	else
-		new = s;
-out:
-	mutex_unlock(&mutex);
-	return new;
-}
-
 void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 {
 	struct kmem_cache *c;

@@ -3481,12 +3469,6 @@ void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 	mutex_unlock(&activate_kmem_mutex);
 }
 
-struct create_work {
-	struct mem_cgroup *memcg;
-	struct kmem_cache *cachep;
-	struct work_struct work;
-};
-
 static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg)
 {
 	struct kmem_cache *cachep;

@@ -3504,13 +3486,24 @@ static void mem_cgroup_destroy_all_caches(struct mem_cgroup *memcg)
 	mutex_unlock(&memcg->slab_caches_mutex);
 }
 
+struct create_work {
+	struct mem_cgroup *memcg;
+	struct kmem_cache *cachep;
+	struct work_struct work;
+};
+
 static void memcg_create_cache_work_func(struct work_struct *w)
 {
-	struct create_work *cw;
+	struct create_work *cw = container_of(w, struct create_work, work);
+	struct mem_cgroup *memcg = cw->memcg;
+	struct kmem_cache *s = cw->cachep;
+	struct kmem_cache *new;
 
-	cw = container_of(w, struct create_work, work);
-	memcg_create_kmem_cache(cw->memcg, cw->cachep);
-	css_put(&cw->memcg->css);
+	new = kmem_cache_create_memcg(memcg, s->name, s->object_size, s->align,
+				      (s->flags & ~SLAB_PANIC), s->ctor, s);
+	if (new)
+		new->allocflags |= __GFP_KMEMCG;
+	css_put(&memcg->css);
 	kfree(cw);
 }
 

mm/slab_common.c +4 -1

index e77b51eb7347..11857abf7057 100644
@@ -215,7 +215,10 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	s->align = calculate_alignment(flags, align, size);
 	s->ctor = ctor;
 
-	s->name = kstrdup(name, GFP_KERNEL);
+	if (memcg)
+		s->name = memcg_create_cache_name(memcg, parent_cache);
+	else
+		s->name = kstrdup(name, GFP_KERNEL);
 	if (!s->name)
 		goto out_free_cache;
 

Quoted 3 lines

Signature · 1 line

1.7.10.4

Memcg-awareness turned kmem_cache_create() into a dirty interweaving of memcg-only and except-for-memcg calls. To clean this up, let's create a separate function handling memcg caches creation. Although this will result in the two functions having several hunks of practically the same code, I guess this is the case when readability fully covers the cost of code duplication.

Signed-off-by Vladimir Davydov <[email protected]>

---
 include/linux/memcontrol.h |   14 ++---
 include/linux/slab.h       |    9 ++-
 mm/memcontrol.c            |   16 ++----
 mm/slab_common.c           |  130 ++++++++++++++++++++++++++------------------
 4 files changed, 90 insertions(+), 79 deletions(-)

include/linux/memcontrol.h +0 -0

index 84e4801fc36c..de79a9617e09 100644

Quoted 6 lines · Vladimir Davydov

Memcg-awareness turned kmem_cache_create() into a dirty interweaving of memcg-only and except-for-memcg calls. To clean this up, let's create a separate function handling memcg caches creation. Although this will result in the two functions having several hunks of practically the same code, I guess this is the case when readability fully covers the cost of code duplication.

I don't know. The code is apparently cleaner because calling a function with NULL memcg just to go via several if (memcg) branches is ugly as hell. But having a duplicated function like this calls for a problem later.

Would it be possible to split kmem_cache_create into memcg independant part and do the rest in a single memcg branch?

Quoted 15 lines

Signed-off-by Vladimir Davydov <[email protected]>

---
 include/linux/memcontrol.h |   14 ++---
 include/linux/slab.h       |    9 ++-
 mm/memcontrol.c            |   16 ++----
 mm/slab_common.c           |  130 ++++++++++++++++++++++++++------------------
 4 files changed, 90 insertions(+), 79 deletions(-)

include/linux/memcontrol.h +0 -0

index 84e4801fc36c..de79a9617e09 100644
@@ -500,8 +500,8 @@ int memcg_cache_id(struct mem_cgroup *memcg);
 

include/linux/memcontrol.h int memcg_cache_id(struct mem_cgroup *memcg); · line 504

Quoted 6 lines

include/linux/memcontrol.h +2 -2

 char *memcg_create_cache_name(struct mem_cgroup *memcg,
 			      struct kmem_cache *root_cache);
-int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
-			     struct kmem_cache *root_cache);
+int memcg_alloc_cache_params(struct kmem_cache *s,
+		struct mem_cgroup *memcg, struct kmem_cache *root_cache);

Why is the parameters ordering changed? It really doesn't help review the patch.

Also what does `s' stand for and can we use a more descriptive name, please?

Quoted 7 lines · Michal Hocko

Quoted 6 lines · Vladimir Davydov

Memcg-awareness turned kmem_cache_create() into a dirty interweaving of memcg-only and except-for-memcg calls. To clean this up, let's create a separate function handling memcg caches creation. Although this will result in the two functions having several hunks of practically the same code, I guess this is the case when readability fully covers the cost of code duplication.

Quoted 7 lines

I don't know. The code is apparently cleaner because calling a function with NULL memcg just to go via several if (memcg) branches is ugly as hell. But having a duplicated function like this calls for a problem later.

Would it be possible to split kmem_cache_create into memcg independant part and do the rest in a single memcg branch?

May be, something like the patch attached?

Quoted 15 lines

Quoted 15 lines

Signed-off-by Vladimir Davydov <[email protected]>

---
 include/linux/memcontrol.h |   14 ++---
 include/linux/slab.h       |    9 ++-
 mm/memcontrol.c            |   16 ++----
 mm/slab_common.c           |  130 ++++++++++++++++++++++++++------------------
 4 files changed, 90 insertions(+), 79 deletions(-)

include/linux/memcontrol.h +0 -0

index 84e4801fc36c..de79a9617e09 100644
@@ -500,8 +500,8 @@ int memcg_cache_id(struct mem_cgroup *memcg);
 

Quoted 8 lines

include/linux/memcontrol.h int memcg_cache_id(struct mem_cgroup *memcg); · line 504

Quoted 6 lines

include/linux/memcontrol.h +2 -2

 char *memcg_create_cache_name(struct mem_cgroup *memcg,
 			      struct kmem_cache *root_cache);
-int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
-			     struct kmem_cache *root_cache);
+int memcg_alloc_cache_params(struct kmem_cache *s,
+		struct mem_cgroup *memcg, struct kmem_cache *root_cache);

Why is the parameters ordering changed? It really doesn't help review the patch.

Oh, this is because seeing something like

memcg_alloc_cache_params(NULL, s, NULL);

hurts my brain :-) I prefer to have NULLs in the end.

Quoted 2 lines

Also what does `s' stand for and can we use a more descriptive name, please?

Yes, we can call it `cachep', but it would be too long :-/

`s' is the common name for a kmem_cache throughout mm/sl[au]b.c so I guess it fits here. However, this function certainly needs a comment - I guess I'll do it along with swapping the function parameters in a separate patch.

From 55f0916c794ad25a8bf45566f6d333bea956e0d4 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <[email protected]>
Date: Mon, 3 Feb 2014 19:18:22 +0400
Subject: [PATCH] memcg, slab: separate memcg vs root cache creation paths

Memcg-awareness turned kmem_cache_create() into a dirty interweaving of memcg-only and except-for-memcg calls. To clean this up, let's create a separate function handling memcg caches creation. Although this will result in the two functions having several hunks of practically the same code, I guess this is the case when readability fully covers the cost of code duplication.

Signed-off-by Vladimir Davydov <[email protected]>

---
 include/linux/slab.h |    9 ++-
 mm/memcontrol.c      |   12 +---
 mm/slab_common.c     |  174 +++++++++++++++++++++++++++-----------------------
 3 files changed, 101 insertions(+), 94 deletions(-)

include/linux/slab.h +0 -0

index 9260abdd67df..e8c95d0bb879 100644

mm/slab_common.c +0 -0

index 11857abf7057..6bee919ece80 100644

mm/slab_common.c +3 -1

+int kmem_cache_create_memcg(struct mem_cgroup *memcg, struct kmem_cache *cachep)
 {
-	return kmem_cache_create_memcg(NULL, name, size, align, flags, ctor, NULL);
+	struct kmem_cache *s;
+	int err;

mm/slab_common.c +16 -1

+	if (cache_from_memcg_idx(cachep, memcg_cache_id(memcg)))
+		goto out_unlock;
+
+	s = do_kmem_cache_create(memcg_create_cache_name(memcg, cachep),
+			cachep->object_size, cachep->size, cachep->align,
+			cachep->flags & ~SLAB_PANIC, cachep->ctor,
+			memcg, cachep);
+	err = IS_ERR(s) ? PTR_ERR(s) : 0;
+	if (!err)
+		s->allocflags |= __GFP_KMEMCG;
+
+out_unlock:
+	mutex_unlock(&slab_mutex);
+	put_online_cpus();
+	return err;
 }
-EXPORT_SYMBOL(kmem_cache_create);
+#endif /* CONFIG_MEMCG_KMEM */
 
 void kmem_cache_destroy(struct kmem_cache *s)
 {

Signature · 1 line

1.7.10.4

Quoted 40 lines · Vladimir Davydov

Quoted 7 lines · Michal Hocko

Quoted 6 lines · Vladimir Davydov

Memcg-awareness turned kmem_cache_create() into a dirty interweaving of memcg-only and except-for-memcg calls. To clean this up, let's create a separate function handling memcg caches creation. Although this will result in the two functions having several hunks of practically the same code, I guess this is the case when readability fully covers the cost of code duplication.

Quoted 7 lines

I don't know. The code is apparently cleaner because calling a function with NULL memcg just to go via several if (memcg) branches is ugly as hell. But having a duplicated function like this calls for a problem later.

Would it be possible to split kmem_cache_create into memcg independant part and do the rest in a single memcg branch?

May be, something like the patch attached?

Quoted 23 lines

Quoted 14 lines

Signed-off-by Vladimir Davydov <[email protected]>

---
 include/linux/memcontrol.h |   14 ++---
 include/linux/slab.h       |    9 ++-
 mm/memcontrol.c            |   16 ++----
 mm/slab_common.c           |  130 ++++++++++++++++++++++++++------------------
 4 files changed, 90 insertions(+), 79 deletions(-)

include/linux/memcontrol.h +0 -0

index 84e4801fc36c..de79a9617e09 100644

include/linux/memcontrol.h int memcg_cache_id(struct mem_cgroup *memcg); · line 504

Quoted 7 lines

include/linux/memcontrol.h +2 -2

@@ -500,8 +500,8 @@ int memcg_cache_id(struct mem_cgroup *memcg);
 
 char *memcg_create_cache_name(struct mem_cgroup *memcg,
 			      struct kmem_cache *root_cache);
-int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
-			     struct kmem_cache *root_cache);
+int memcg_alloc_cache_params(struct kmem_cache *s,
+		struct mem_cgroup *memcg, struct kmem_cache *root_cache);

Why is the parameters ordering changed? It really doesn't help review the patch.

Quoted 5 lines

Oh, this is because seeing something like

memcg_alloc_cache_params(NULL, s, NULL);

hurts my brain :-) I prefer to have NULLs in the end.

the function still allocates parameters for the given memcg and cache and needs a reference to root cache so the ordering kind of makes sense to me.

Quoted 9 lines

Quoted 3 lines

Also what does `s' stand for and can we use a more descriptive name, please?

Yes, we can call it `cachep', but it would be too long :-/

`s' is the common name for a kmem_cache throughout mm/sl[au]b.c so I guess it fits here. However, this function certainly needs a comment - I guess I'll do it along with swapping the function parameters in a separate patch.

Yes, it seems that self explaining `s' is spread all over the place.

Quoted 5 lines

From 55f0916c794ad25a8bf45566f6d333bea956e0d4 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <[email protected]>
Date: Mon, 3 Feb 2014 19:18:22 +0400
Subject: [PATCH] memcg, slab: separate memcg vs root cache creation paths

Quoted 8 lines

Memcg-awareness turned kmem_cache_create() into a dirty interweaving of memcg-only and except-for-memcg calls. To clean this up, let's create a separate function handling memcg caches creation. Although this will result in the two functions having several hunks of practically the same code, I guess this is the case when readability fully covers the cost of code duplication.

Signed-off-by Vladimir Davydov <[email protected]>

This looks better. The naming could still be little bit better because do_kmem_cache_create suggests that no memcg is involved but it at least reduced all the code duplication and nasty if(memcg) parts.

Few minor comments bellow

Quoted 13 lines

---
 include/linux/slab.h |    9 ++-
 mm/memcontrol.c      |   12 +---
 mm/slab_common.c     |  174 +++++++++++++++++++++++++++-----------------------
 3 files changed, 101 insertions(+), 94 deletions(-)

include/linux/slab.h +0 -0

index 9260abdd67df..e8c95d0bb879 100644
@@ -113,11 +113,10 @@ void __init kmem_cache_init(void);
 int slab_is_available(void);
 

include/linux/slab.h void __init kmem_cache_init(void); · line 116

Quoted 7 lines

include/linux/slab.h +1 -5

 struct kmem_cache *kmem_cache_create(const char *, size_t, size_t,
-			unsigned long,
-			void (*)(void *));
-struct kmem_cache *
-kmem_cache_create_memcg(struct mem_cgroup *, const char *, size_t, size_t,
-			unsigned long, void (*)(void *), struct kmem_cache *);
+				     unsigned long, void (*)(void *));

It is quite confusing when you mix formatting changes with other ones.

[...]

Quoted 4 lines

mm/slab_common.c +0 -0

index 11857abf7057..6bee919ece80 100644

[...]

Quoted 9 lines

mm/slab_common.c +7 -1

+int kmem_cache_create_memcg(struct mem_cgroup *memcg, struct kmem_cache *cachep)
 {
-	return kmem_cache_create_memcg(NULL, name, size, align, flags, ctor, NULL);
+	struct kmem_cache *s;
+	int err;
+
+	get_online_cpus();
+	mutex_lock(&slab_mutex);
+

mm/slab_common.c

Quoted 6 lines

mm/slab_common.c +6 -0

+	/*
+	 * Since per-memcg caches are created asynchronously on first
+	 * allocation (see memcg_kmem_get_cache()), several threads can try to
+	 * create the same cache, but only one of them may succeed.
+	 */
+	err = -EEXIST;

Does it make any sense to report the error here? If we are racing then at least on part wins and the work is done.

We should probably warn about errors which prevent from accounting but I do not think there is much more we can do so returning an error code from this function seems pointless. memcg_create_cache_work_func ignores the return value anyway.

Quoted 25 lines

mm/slab_common.c +16 -1

+	if (cache_from_memcg_idx(cachep, memcg_cache_id(memcg)))
+		goto out_unlock;
+
+	s = do_kmem_cache_create(memcg_create_cache_name(memcg, cachep),
+			cachep->object_size, cachep->size, cachep->align,
+			cachep->flags & ~SLAB_PANIC, cachep->ctor,
+			memcg, cachep);
+	err = IS_ERR(s) ? PTR_ERR(s) : 0;
+	if (!err)
+		s->allocflags |= __GFP_KMEMCG;
+
+out_unlock:
+	mutex_unlock(&slab_mutex);
+	put_online_cpus();
+	return err;
 }
-EXPORT_SYMBOL(kmem_cache_create);
+#endif /* CONFIG_MEMCG_KMEM */
 
 void kmem_cache_destroy(struct kmem_cache *s)
 {

Signature · 1 line

1.7.10.4

On 02/06/2014 08:41 PM, Michal Hocko wrote:

Quoted 9 lines

Quoted 9 lines

(quoted diff) +7 -1

+int kmem_cache_create_memcg(struct mem_cgroup *memcg, struct kmem_cache *cachep)
 {
-	return kmem_cache_create_memcg(NULL, name, size, align, flags, ctor, NULL);
+	struct kmem_cache *s;
+	int err;
+
+	get_online_cpus();
+	mutex_lock(&slab_mutex);
+

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+	/*
+	 * Since per-memcg caches are created asynchronously on first
+	 * allocation (see memcg_kmem_get_cache()), several threads can try to
+	 * create the same cache, but only one of them may succeed.
+	 */
+	err = -EEXIST;

Does it make any sense to report the error here? If we are racing then at least on part wins and the work is done.

Yeah, you're perfectly right. It's better to return 0 here.

Quoted 4 lines

We should probably warn about errors which prevent from accounting but I do not think there is much more we can do so returning an error code from this function seems pointless. memcg_create_cache_work_func ignores the return value anyway.

I do not think warnings are appropriate here, because it is not actually an error if we are short on memory and can't do proper memcg accounting due to this. Perhaps, we'd better add fail counters for memcg cache creations and/or accounting to the root cache instead of memcg's one. That would be useful for debugging. I'm not sure though.

Quoted 1 line · Vladimir Davydov

On 02/06/2014 08:41 PM, Michal Hocko wrote:

[...]

Quoted 17 lines

Quoted 17 lines

Quoted 5 lines

(quoted diff) +3 -1

+int kmem_cache_create_memcg(struct mem_cgroup *memcg, struct kmem_cache *cachep)
 {
-	return kmem_cache_create_memcg(NULL, name, size, align, flags, ctor, NULL);
+	struct kmem_cache *s;
+	int err;

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+	get_online_cpus();
+	mutex_lock(&slab_mutex);
+
+	/*
+	 * Since per-memcg caches are created asynchronously on first
+	 * allocation (see memcg_kmem_get_cache()), several threads can try to
+	 * create the same cache, but only one of them may succeed.
+	 */
+	err = -EEXIST;

Does it make any sense to report the error here? If we are racing then at least on part wins and the work is done.

Quoted 1 line

Yeah, you're perfectly right. It's better to return 0 here.

Why not void?

Quoted 10 lines

Quoted 5 lines

We should probably warn about errors which prevent from accounting but I do not think there is much more we can do so returning an error code from this function seems pointless. memcg_create_cache_work_func ignores the return value anyway.

I do not think warnings are appropriate here, because it is not actually an error if we are short on memory and can't do proper memcg accounting due to this. Perhaps, we'd better add fail counters for memcg cache creations and/or accounting to the root cache instead of memcg's one. That would be useful for debugging. I'm not sure though.

warn on once per memcg would be probably sufficient but it would still be great if an admin could see that a memcg is not accounted although it is supposed to be. Scanning all the memcgs might be really impractical. We do not fail allocations needed for those object in the real life now but we shouldn't rely on that.