[PATCH v2 0/7] memcg-vs-slab related fixes, improvements, cleanups

[PATCH v2 0/7] memcg-vs-slab related fixes, improvements, cleanups

[Vladimir Davydov]

Hi,

This patch set mostly cleanups memcg slab caches creation/destruction
paths fixing a couple of bugs in the meanwhile. The only serious change
it introduces is a rework of the sysfs layout for memcg slub caches (see
patch 7).

Changes in v2:
 - do not remove cgroup name part from memcg cache names
 - do not export memcg cache id to userspace

Comments are appreciated.

Thanks.

Vladimir Davydov (7):
  memcg, slab: never try to merge memcg caches
  memcg, slab: cleanup memcg cache name creation
  memcg, slab: separate memcg vs root cache creation paths
  memcg, slab: unregister cache from memcg before starting to destroy
    it
  memcg, slab: do not destroy children caches if parent has aliases
  slub: adjust memcg caches when creating cache alias
  slub: rework sysfs layout for memcg caches

 include/linux/memcontrol.h |   16 ++--
 include/linux/slab.h       |    9 +--
 include/linux/slub_def.h   |    3 +
 mm/memcontrol.c            |  104 +++++++++++-------------
 mm/slab.h                  |   36 ++++-----
 mm/slab_common.c           |  192 ++++++++++++++++++++++++++++----------------
 mm/slub.c                  |  118 +++++++++++++++++++++------
 7 files changed, 296 insertions(+), 182 deletions(-)

-- 
1.7.10.4

[PATCH v2 0/7] memcg-vs-slab related fixes, improvements, cleanups

[Vladimir Davydov]

Hi,

This patch set mostly cleanups memcg slab caches creation/destruction
paths fixing a couple of bugs in the meanwhile. The only serious change
it introduces is a rework of the sysfs layout for memcg slub caches (see
patch 7).

Changes in v2:
 - do not remove cgroup name part from memcg cache names
 - do not export memcg cache id to userspace

Comments are appreciated.

Thanks.

Vladimir Davydov (7):
  memcg, slab: never try to merge memcg caches
  memcg, slab: cleanup memcg cache name creation
  memcg, slab: separate memcg vs root cache creation paths
  memcg, slab: unregister cache from memcg before starting to destroy
    it
  memcg, slab: do not destroy children caches if parent has aliases
  slub: adjust memcg caches when creating cache alias
  slub: rework sysfs layout for memcg caches

 include/linux/memcontrol.h |   16 ++--
 include/linux/slab.h       |    9 +--
 include/linux/slub_def.h   |    3 +
 mm/memcontrol.c            |  104 +++++++++++-------------
 mm/slab.h                  |   36 ++++-----
 mm/slab_common.c           |  192 ++++++++++++++++++++++++++++----------------
 mm/slub.c                  |  118 +++++++++++++++++++++------
 7 files changed, 296 insertions(+), 182 deletions(-)

-- 
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 1/7] memcg, slab: never try to merge memcg caches

[Vladimir Davydov]

Suppose we are creating memcg cache A that could be merged with cache B
of the same memcg. Since any memcg cache has the same parameters as its
parent cache, parent caches PA and PB of memcg caches A and B must be
mergeable too. That means PA was merged with PB on creation or vice
versa, i.e. PA = PB. From that it follows that A = B, and we couldn't
even try to create cache B, because it already exists - a contradiction.

So let's remove unused code responsible for merging memcg caches.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 mm/slab.h        |   21 ++++-----------------
 mm/slab_common.c |    8 +++++---
 mm/slub.c        |   19 +++++++++----------
 3 files changed, 18 insertions(+), 30 deletions(-)

diff --git a/mm/slab.h b/mm/slab.h
index 8184a7cde272..3045316b7c9d 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -55,12 +55,12 @@ extern void create_boot_cache(struct kmem_cache *, const char *name,
 struct mem_cgroup;
 #ifdef CONFIG_SLUB
 struct kmem_cache *
-__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
-		   size_t align, unsigned long flags, void (*ctor)(void *));
+__kmem_cache_alias(const char *name, size_t size, size_t align,
+		   unsigned long flags, void (*ctor)(void *));
 #else
 static inline struct kmem_cache *
-__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
-		   size_t align, unsigned long flags, void (*ctor)(void *))
+__kmem_cache_alias(const char *name, size_t size, size_t align,
+		   unsigned long flags, void (*ctor)(void *))
 { return NULL; }
 #endif
 
@@ -119,13 +119,6 @@ static inline bool is_root_cache(struct kmem_cache *s)
 	return !s->memcg_params || s->memcg_params->is_root_cache;
 }
 
-static inline bool cache_match_memcg(struct kmem_cache *cachep,
-				     struct mem_cgroup *memcg)
-{
-	return (is_root_cache(cachep) && !memcg) ||
-				(cachep->memcg_params->memcg == memcg);
-}
-
 static inline void memcg_bind_pages(struct kmem_cache *s, int order)
 {
 	if (!is_root_cache(s))
@@ -204,12 +197,6 @@ static inline bool is_root_cache(struct kmem_cache *s)
 	return true;
 }
 
-static inline bool cache_match_memcg(struct kmem_cache *cachep,
-				     struct mem_cgroup *memcg)
-{
-	return true;
-}
-
 static inline void memcg_bind_pages(struct kmem_cache *s, int order)
 {
 }
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 1ec3c619ba04..e77b51eb7347 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -200,9 +200,11 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	 */
 	flags &= CACHE_CREATE_MASK;
 
-	s = __kmem_cache_alias(memcg, name, size, align, flags, ctor);
-	if (s)
-		goto out_unlock;
+	if (!memcg) {
+		s = __kmem_cache_alias(name, size, align, flags, ctor);
+		if (s)
+			goto out_unlock;
+	}
 
 	err = -ENOMEM;
 	s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL);
diff --git a/mm/slub.c b/mm/slub.c
index 7e3e0458bce4..8659e7184338 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3692,9 +3692,8 @@ static int slab_unmergeable(struct kmem_cache *s)
 	return 0;
 }
 
-static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
-		size_t align, unsigned long flags, const char *name,
-		void (*ctor)(void *))
+static struct kmem_cache *find_mergeable(size_t size, size_t align,
+		unsigned long flags, const char *name, void (*ctor)(void *))
 {
 	struct kmem_cache *s;
 
@@ -3713,11 +3712,14 @@ static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
 		if (slab_unmergeable(s))
 			continue;
 
+		if (!is_root_cache(s))
+			continue;
+
 		if (size > s->size)
 			continue;
 
 		if ((flags & SLUB_MERGE_SAME) != (s->flags & SLUB_MERGE_SAME))
-				continue;
+			continue;
 		/*
 		 * Check if alignment is compatible.
 		 * Courtesy of Adrian Drzewiecki
@@ -3728,21 +3730,18 @@ static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
 		if (s->size - size >= sizeof(void *))
 			continue;
 
-		if (!cache_match_memcg(s, memcg))
-			continue;
-
 		return s;
 	}
 	return NULL;
 }
 
 struct kmem_cache *
-__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
-		   size_t align, unsigned long flags, void (*ctor)(void *))
+__kmem_cache_alias(const char *name, size_t size, size_t align,
+		   unsigned long flags, void (*ctor)(void *))
 {
 	struct kmem_cache *s;
 
-	s = find_mergeable(memcg, size, align, flags, name, ctor);
+	s = find_mergeable(size, align, flags, name, ctor);
 	if (s) {
 		s->refcount++;
 		/*
-- 
1.7.10.4

[PATCH v2 1/7] memcg, slab: never try to merge memcg caches

[Vladimir Davydov]

Suppose we are creating memcg cache A that could be merged with cache B
of the same memcg. Since any memcg cache has the same parameters as its
parent cache, parent caches PA and PB of memcg caches A and B must be
mergeable too. That means PA was merged with PB on creation or vice
versa, i.e. PA = PB. From that it follows that A = B, and we couldn't
even try to create cache B, because it already exists - a contradiction.

So let's remove unused code responsible for merging memcg caches.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 mm/slab.h        |   21 ++++-----------------
 mm/slab_common.c |    8 +++++---
 mm/slub.c        |   19 +++++++++----------
 3 files changed, 18 insertions(+), 30 deletions(-)

diff --git a/mm/slab.h b/mm/slab.h
index 8184a7cde272..3045316b7c9d 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -55,12 +55,12 @@ extern void create_boot_cache(struct kmem_cache *, const char *name,
 struct mem_cgroup;
 #ifdef CONFIG_SLUB
 struct kmem_cache *
-__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
-		   size_t align, unsigned long flags, void (*ctor)(void *));
+__kmem_cache_alias(const char *name, size_t size, size_t align,
+		   unsigned long flags, void (*ctor)(void *));
 #else
 static inline struct kmem_cache *
-__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
-		   size_t align, unsigned long flags, void (*ctor)(void *))
+__kmem_cache_alias(const char *name, size_t size, size_t align,
+		   unsigned long flags, void (*ctor)(void *))
 { return NULL; }
 #endif
 
@@ -119,13 +119,6 @@ static inline bool is_root_cache(struct kmem_cache *s)
 	return !s->memcg_params || s->memcg_params->is_root_cache;
 }
 
-static inline bool cache_match_memcg(struct kmem_cache *cachep,
-				     struct mem_cgroup *memcg)
-{
-	return (is_root_cache(cachep) && !memcg) ||
-				(cachep->memcg_params->memcg == memcg);
-}
-
 static inline void memcg_bind_pages(struct kmem_cache *s, int order)
 {
 	if (!is_root_cache(s))
@@ -204,12 +197,6 @@ static inline bool is_root_cache(struct kmem_cache *s)
 	return true;
 }
 
-static inline bool cache_match_memcg(struct kmem_cache *cachep,
-				     struct mem_cgroup *memcg)
-{
-	return true;
-}
-
 static inline void memcg_bind_pages(struct kmem_cache *s, int order)
 {
 }
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 1ec3c619ba04..e77b51eb7347 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -200,9 +200,11 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	 */
 	flags &= CACHE_CREATE_MASK;
 
-	s = __kmem_cache_alias(memcg, name, size, align, flags, ctor);
-	if (s)
-		goto out_unlock;
+	if (!memcg) {
+		s = __kmem_cache_alias(name, size, align, flags, ctor);
+		if (s)
+			goto out_unlock;
+	}
 
 	err = -ENOMEM;
 	s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL);
diff --git a/mm/slub.c b/mm/slub.c
index 7e3e0458bce4..8659e7184338 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3692,9 +3692,8 @@ static int slab_unmergeable(struct kmem_cache *s)
 	return 0;
 }
 
-static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
-		size_t align, unsigned long flags, const char *name,
-		void (*ctor)(void *))
+static struct kmem_cache *find_mergeable(size_t size, size_t align,
+		unsigned long flags, const char *name, void (*ctor)(void *))
 {
 	struct kmem_cache *s;
 
@@ -3713,11 +3712,14 @@ static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
 		if (slab_unmergeable(s))
 			continue;
 
+		if (!is_root_cache(s))
+			continue;
+
 		if (size > s->size)
 			continue;
 
 		if ((flags & SLUB_MERGE_SAME) != (s->flags & SLUB_MERGE_SAME))
-				continue;
+			continue;
 		/*
 		 * Check if alignment is compatible.
 		 * Courtesy of Adrian Drzewiecki
@@ -3728,21 +3730,18 @@ static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
 		if (s->size - size >= sizeof(void *))
 			continue;
 
-		if (!cache_match_memcg(s, memcg))
-			continue;
-
 		return s;
 	}
 	return NULL;
 }
 
 struct kmem_cache *
-__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
-		   size_t align, unsigned long flags, void (*ctor)(void *))
+__kmem_cache_alias(const char *name, size_t size, size_t align,
+		   unsigned long flags, void (*ctor)(void *))
 {
 	struct kmem_cache *s;
 
-	s = find_mergeable(memcg, size, align, flags, name, ctor);
+	s = find_mergeable(size, align, flags, name, ctor);
 	if (s) {
 		s->refcount++;
 		/*
-- 
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 2/7] memcg, slab: cleanup memcg cache name creation

[Vladimir Davydov]

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.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 include/linux/memcontrol.h |    9 +++++
 mm/memcontrol.c            |   94 ++++++++++++++++++++++----------------------
 mm/slab_common.c           |    5 ++-
 3 files changed, 59 insertions(+), 49 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index abd0113b6620..84e4801fc36c 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -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)
 {
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 53385cd4e6f0..9e321650efb2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -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.
+	 */
+	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);
+
+	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);
 }
 
-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 +3474,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 +3491,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);
 }
 
diff --git a/mm/slab_common.c b/mm/slab_common.c
index e77b51eb7347..11857abf7057 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -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;
 
-- 
1.7.10.4

[PATCH v2 2/7] memcg, slab: cleanup memcg cache name creation

[Vladimir Davydov]

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.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 include/linux/memcontrol.h |    9 +++++
 mm/memcontrol.c            |   94 ++++++++++++++++++++++----------------------
 mm/slab_common.c           |    5 ++-
 3 files changed, 59 insertions(+), 49 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index abd0113b6620..84e4801fc36c 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -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)
 {
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 53385cd4e6f0..9e321650efb2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -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.
+	 */
+	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);
+
+	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);
 }
 
-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 +3474,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 +3491,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);
 }
 
diff --git a/mm/slab_common.c b/mm/slab_common.c
index e77b51eb7347..11857abf7057 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -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;
 
-- 
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[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.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 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(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 84e4801fc36c..de79a9617e09 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -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);
 void memcg_free_cache_params(struct kmem_cache *s);
 void memcg_register_cache(struct kmem_cache *s);
 void memcg_unregister_cache(struct kmem_cache *s);
@@ -644,14 +644,8 @@ 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)
+static inline int memcg_alloc_cache_params(struct kmem_cache *s,
+		struct mem_cgroup *memcg, struct kmem_cache *root_cache)
 {
 	return 0;
 }
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 9260abdd67df..e8c95d0bb879 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -113,11 +113,10 @@ void __init kmem_cache_init(void);
 int slab_is_available(void);
 
 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 *));
+#ifdef CONFIG_MEMCG_KMEM
+int kmem_cache_create_memcg(struct mem_cgroup *, struct kmem_cache *);
+#endif
 void kmem_cache_destroy(struct kmem_cache *);
 int kmem_cache_shrink(struct kmem_cache *);
 void kmem_cache_free(struct kmem_cache *, void *);
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 9e321650efb2..b94d9917707c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3224,8 +3224,8 @@ char *memcg_create_cache_name(struct mem_cgroup *memcg,
 	return name;
 }
 
-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)
 {
 	size_t size;
 
@@ -3500,15 +3500,9 @@ struct create_work {
 static void memcg_create_cache_work_func(struct work_struct *w)
 {
 	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;
-
-	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);
+
+	kmem_cache_create_memcg(cw->memcg, cw->cachep);
+	css_put(&cw->memcg->css);
 	kfree(cw);
 }
 
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 11857abf7057..3314fb3ead7f 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -29,8 +29,7 @@ DEFINE_MUTEX(slab_mutex);
 struct kmem_cache *kmem_cache;
 
 #ifdef CONFIG_DEBUG_VM
-static int kmem_cache_sanity_check(struct mem_cgroup *memcg, const char *name,
-				   size_t size)
+static int kmem_cache_sanity_check(const char *name, size_t size)
 {
 	struct kmem_cache *s = NULL;
 
@@ -57,13 +56,7 @@ static int kmem_cache_sanity_check(struct mem_cgroup *memcg, const char *name,
 		}
 
 #if !defined(CONFIG_SLUB) || !defined(CONFIG_SLUB_DEBUG_ON)
-		/*
-		 * For simplicity, we won't check this in the list of memcg
-		 * caches. We have control over memcg naming, and if there
-		 * aren't duplicates in the global list, there won't be any
-		 * duplicates in the memcg lists as well.
-		 */
-		if (!memcg && !strcmp(s->name, name)) {
+		if (!strcmp(s->name, name)) {
 			pr_err("%s (%s): Cache name already exists.\n",
 			       __func__, name);
 			dump_stack();
@@ -77,8 +70,7 @@ static int kmem_cache_sanity_check(struct mem_cgroup *memcg, const char *name,
 	return 0;
 }
 #else
-static inline int kmem_cache_sanity_check(struct mem_cgroup *memcg,
-					  const char *name, size_t size)
+static inline int kmem_cache_sanity_check(const char *name, size_t size)
 {
 	return 0;
 }
@@ -164,11 +156,9 @@ unsigned long calculate_alignment(unsigned long flags,
  * cacheline.  This can be beneficial if you're counting cycles as closely
  * as davem.
  */
-
 struct kmem_cache *
-kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
-			size_t align, unsigned long flags, void (*ctor)(void *),
-			struct kmem_cache *parent_cache)
+kmem_cache_create(const char *name, size_t size, size_t align,
+		  unsigned long flags, void (*ctor)(void *))
 {
 	struct kmem_cache *s = NULL;
 	int err;
@@ -176,22 +166,10 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	get_online_cpus();
 	mutex_lock(&slab_mutex);
 
-	err = kmem_cache_sanity_check(memcg, name, size);
+	err = kmem_cache_sanity_check(name, size);
 	if (err)
 		goto out_unlock;
 
-	if (memcg) {
-		/*
-		 * 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. Therefore if we get here and see the cache has
-		 * already been created, we silently return NULL.
-		 */
-		if (cache_from_memcg_idx(parent_cache, memcg_cache_id(memcg)))
-			goto out_unlock;
-	}
-
 	/*
 	 * Some allocators will constraint the set of valid flags to a subset
 	 * of all flags. We expect them to define CACHE_CREATE_MASK in this
@@ -200,11 +178,9 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	 */
 	flags &= CACHE_CREATE_MASK;
 
-	if (!memcg) {
-		s = __kmem_cache_alias(name, size, align, flags, ctor);
-		if (s)
-			goto out_unlock;
-	}
+	s = __kmem_cache_alias(name, size, align, flags, ctor);
+	if (s)
+		goto out_unlock;
 
 	err = -ENOMEM;
 	s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL);
@@ -215,14 +191,11 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	s->align = calculate_alignment(flags, align, size);
 	s->ctor = ctor;
 
-	if (memcg)
-		s->name = memcg_create_cache_name(memcg, parent_cache);
-	else
-		s->name = kstrdup(name, GFP_KERNEL);
+	s->name = kstrdup(name, GFP_KERNEL);
 	if (!s->name)
 		goto out_free_cache;
 
-	err = memcg_alloc_cache_params(memcg, s, parent_cache);
+	err = memcg_alloc_cache_params(s, NULL, NULL);
 	if (err)
 		goto out_free_cache;
 
@@ -239,16 +212,6 @@ out_unlock:
 	put_online_cpus();
 
 	if (err) {
-		/*
-		 * There is no point in flooding logs with warnings or
-		 * especially crashing the system if we fail to create a cache
-		 * for a memcg. In this case we will be accounting the memcg
-		 * allocation to the root cgroup until we succeed to create its
-		 * own cache, but it isn't that critical.
-		 */
-		if (!memcg)
-			return NULL;
-
 		if (flags & SLAB_PANIC)
 			panic("kmem_cache_create: Failed to create slab '%s'. Error %d\n",
 				name, err);
@@ -267,14 +230,75 @@ out_free_cache:
 	kmem_cache_free(kmem_cache, s);
 	goto out_unlock;
 }
+EXPORT_SYMBOL(kmem_cache_create);
 
-struct kmem_cache *
-kmem_cache_create(const char *name, size_t size, size_t align,
-		  unsigned long flags, void (*ctor)(void *))
+#ifdef CONFIG_MEMCG_KMEM
+/*
+ * kmem_cache_create_memcg - Create a cache for a memory cgroup.
+ * @memcg: The memory cgroup the new cache is for.
+ * @cachep: The parent of the new cache.
+ *
+ * This function creates a kmem cache that will serve allocation requests going
+ * from @memcg to @cachep. The new cache inherits properties from its parent.
+ *
+ * Returns 0 on success, -errno on failure.
+ */
+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);
+
+	/*
+	 * 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;
+	if (cache_from_memcg_idx(cachep, memcg_cache_id(memcg)))
+		goto out_unlock;
+
+	err = -ENOMEM;
+	s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL);
+	if (!s)
+		goto out_unlock;
+
+	s->object_size = cachep->object_size;
+	s->size = cachep->size;
+	s->align = cachep->align;
+	s->ctor = cachep->ctor;
+
+	s->name = memcg_create_cache_name(memcg, cachep);
+	if (!s->name)
+		goto out_free_cache;
+
+	err = memcg_alloc_cache_params(s, memcg, cachep);
+	if (err)
+		goto out_free_cache;
+
+	err = __kmem_cache_create(s, cachep->flags & ~SLAB_PANIC);
+	if (err)
+		goto out_free_cache;
+
+	s->refcount = 1;
+	s->allocflags |= __GFP_KMEMCG;
+	list_add(&s->list, &slab_caches);
+	memcg_register_cache(s);
+
+out_unlock:
+	mutex_unlock(&slab_mutex);
+	put_online_cpus();
+	return err;
+
+out_free_cache:
+	memcg_free_cache_params(s);
+	kfree(s->name);
+	kmem_cache_free(kmem_cache, s);
+	goto out_unlock;
 }
-EXPORT_SYMBOL(kmem_cache_create);
+#endif /* CONFIG_MEMCG_KMEM */
 
 void kmem_cache_destroy(struct kmem_cache *s)
 {
-- 
1.7.10.4

[PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[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.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 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(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 84e4801fc36c..de79a9617e09 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -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);
 void memcg_free_cache_params(struct kmem_cache *s);
 void memcg_register_cache(struct kmem_cache *s);
 void memcg_unregister_cache(struct kmem_cache *s);
@@ -644,14 +644,8 @@ 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)
+static inline int memcg_alloc_cache_params(struct kmem_cache *s,
+		struct mem_cgroup *memcg, struct kmem_cache *root_cache)
 {
 	return 0;
 }
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 9260abdd67df..e8c95d0bb879 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -113,11 +113,10 @@ void __init kmem_cache_init(void);
 int slab_is_available(void);
 
 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 *));
+#ifdef CONFIG_MEMCG_KMEM
+int kmem_cache_create_memcg(struct mem_cgroup *, struct kmem_cache *);
+#endif
 void kmem_cache_destroy(struct kmem_cache *);
 int kmem_cache_shrink(struct kmem_cache *);
 void kmem_cache_free(struct kmem_cache *, void *);
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 9e321650efb2..b94d9917707c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3224,8 +3224,8 @@ char *memcg_create_cache_name(struct mem_cgroup *memcg,
 	return name;
 }
 
-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)
 {
 	size_t size;
 
@@ -3500,15 +3500,9 @@ struct create_work {
 static void memcg_create_cache_work_func(struct work_struct *w)
 {
 	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;
-
-	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);
+
+	kmem_cache_create_memcg(cw->memcg, cw->cachep);
+	css_put(&cw->memcg->css);
 	kfree(cw);
 }
 
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 11857abf7057..3314fb3ead7f 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -29,8 +29,7 @@ DEFINE_MUTEX(slab_mutex);
 struct kmem_cache *kmem_cache;
 
 #ifdef CONFIG_DEBUG_VM
-static int kmem_cache_sanity_check(struct mem_cgroup *memcg, const char *name,
-				   size_t size)
+static int kmem_cache_sanity_check(const char *name, size_t size)
 {
 	struct kmem_cache *s = NULL;
 
@@ -57,13 +56,7 @@ static int kmem_cache_sanity_check(struct mem_cgroup *memcg, const char *name,
 		}
 
 #if !defined(CONFIG_SLUB) || !defined(CONFIG_SLUB_DEBUG_ON)
-		/*
-		 * For simplicity, we won't check this in the list of memcg
-		 * caches. We have control over memcg naming, and if there
-		 * aren't duplicates in the global list, there won't be any
-		 * duplicates in the memcg lists as well.
-		 */
-		if (!memcg && !strcmp(s->name, name)) {
+		if (!strcmp(s->name, name)) {
 			pr_err("%s (%s): Cache name already exists.\n",
 			       __func__, name);
 			dump_stack();
@@ -77,8 +70,7 @@ static int kmem_cache_sanity_check(struct mem_cgroup *memcg, const char *name,
 	return 0;
 }
 #else
-static inline int kmem_cache_sanity_check(struct mem_cgroup *memcg,
-					  const char *name, size_t size)
+static inline int kmem_cache_sanity_check(const char *name, size_t size)
 {
 	return 0;
 }
@@ -164,11 +156,9 @@ unsigned long calculate_alignment(unsigned long flags,
  * cacheline.  This can be beneficial if you're counting cycles as closely
  * as davem.
  */
-
 struct kmem_cache *
-kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
-			size_t align, unsigned long flags, void (*ctor)(void *),
-			struct kmem_cache *parent_cache)
+kmem_cache_create(const char *name, size_t size, size_t align,
+		  unsigned long flags, void (*ctor)(void *))
 {
 	struct kmem_cache *s = NULL;
 	int err;
@@ -176,22 +166,10 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	get_online_cpus();
 	mutex_lock(&slab_mutex);
 
-	err = kmem_cache_sanity_check(memcg, name, size);
+	err = kmem_cache_sanity_check(name, size);
 	if (err)
 		goto out_unlock;
 
-	if (memcg) {
-		/*
-		 * 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. Therefore if we get here and see the cache has
-		 * already been created, we silently return NULL.
-		 */
-		if (cache_from_memcg_idx(parent_cache, memcg_cache_id(memcg)))
-			goto out_unlock;
-	}
-
 	/*
 	 * Some allocators will constraint the set of valid flags to a subset
 	 * of all flags. We expect them to define CACHE_CREATE_MASK in this
@@ -200,11 +178,9 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	 */
 	flags &= CACHE_CREATE_MASK;
 
-	if (!memcg) {
-		s = __kmem_cache_alias(name, size, align, flags, ctor);
-		if (s)
-			goto out_unlock;
-	}
+	s = __kmem_cache_alias(name, size, align, flags, ctor);
+	if (s)
+		goto out_unlock;
 
 	err = -ENOMEM;
 	s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL);
@@ -215,14 +191,11 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	s->align = calculate_alignment(flags, align, size);
 	s->ctor = ctor;
 
-	if (memcg)
-		s->name = memcg_create_cache_name(memcg, parent_cache);
-	else
-		s->name = kstrdup(name, GFP_KERNEL);
+	s->name = kstrdup(name, GFP_KERNEL);
 	if (!s->name)
 		goto out_free_cache;
 
-	err = memcg_alloc_cache_params(memcg, s, parent_cache);
+	err = memcg_alloc_cache_params(s, NULL, NULL);
 	if (err)
 		goto out_free_cache;
 
@@ -239,16 +212,6 @@ out_unlock:
 	put_online_cpus();
 
 	if (err) {
-		/*
-		 * There is no point in flooding logs with warnings or
-		 * especially crashing the system if we fail to create a cache
-		 * for a memcg. In this case we will be accounting the memcg
-		 * allocation to the root cgroup until we succeed to create its
-		 * own cache, but it isn't that critical.
-		 */
-		if (!memcg)
-			return NULL;
-
 		if (flags & SLAB_PANIC)
 			panic("kmem_cache_create: Failed to create slab '%s'. Error %d\n",
 				name, err);
@@ -267,14 +230,75 @@ out_free_cache:
 	kmem_cache_free(kmem_cache, s);
 	goto out_unlock;
 }
+EXPORT_SYMBOL(kmem_cache_create);
 
-struct kmem_cache *
-kmem_cache_create(const char *name, size_t size, size_t align,
-		  unsigned long flags, void (*ctor)(void *))
+#ifdef CONFIG_MEMCG_KMEM
+/*
+ * kmem_cache_create_memcg - Create a cache for a memory cgroup.
+ * @memcg: The memory cgroup the new cache is for.
+ * @cachep: The parent of the new cache.
+ *
+ * This function creates a kmem cache that will serve allocation requests going
+ * from @memcg to @cachep. The new cache inherits properties from its parent.
+ *
+ * Returns 0 on success, -errno on failure.
+ */
+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);
+
+	/*
+	 * 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;
+	if (cache_from_memcg_idx(cachep, memcg_cache_id(memcg)))
+		goto out_unlock;
+
+	err = -ENOMEM;
+	s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL);
+	if (!s)
+		goto out_unlock;
+
+	s->object_size = cachep->object_size;
+	s->size = cachep->size;
+	s->align = cachep->align;
+	s->ctor = cachep->ctor;
+
+	s->name = memcg_create_cache_name(memcg, cachep);
+	if (!s->name)
+		goto out_free_cache;
+
+	err = memcg_alloc_cache_params(s, memcg, cachep);
+	if (err)
+		goto out_free_cache;
+
+	err = __kmem_cache_create(s, cachep->flags & ~SLAB_PANIC);
+	if (err)
+		goto out_free_cache;
+
+	s->refcount = 1;
+	s->allocflags |= __GFP_KMEMCG;
+	list_add(&s->list, &slab_caches);
+	memcg_register_cache(s);
+
+out_unlock:
+	mutex_unlock(&slab_mutex);
+	put_online_cpus();
+	return err;
+
+out_free_cache:
+	memcg_free_cache_params(s);
+	kfree(s->name);
+	kmem_cache_free(kmem_cache, s);
+	goto out_unlock;
 }
-EXPORT_SYMBOL(kmem_cache_create);
+#endif /* CONFIG_MEMCG_KMEM */
 
 void kmem_cache_destroy(struct kmem_cache *s)
 {
-- 
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 4/7] memcg, slab: unregister cache from memcg before starting to destroy it

[Vladimir Davydov]

Currently, memcg_unregister_cache(), which deletes the cache being
destroyed from the memcg_slab_caches list, is called after
__kmem_cache_shutdown() (see kmem_cache_destroy()), which starts to
destroy the cache. As a result, one can access a partially destroyed
cache while traversing a memcg_slab_caches list, which can have deadly
consequences (for instance, cache_show() called for each cache on a
memcg_slab_caches list from mem_cgroup_slabinfo_read() will dereference
pointers to already freed data).

To fix this, let's move memcg_unregister_cache() before the cache
destruction process beginning, issuing memcg_register_cache() on
failure.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 mm/memcontrol.c  |   12 ++++++------
 mm/slab_common.c |    3 ++-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b94d9917707c..6a059e73212c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3247,6 +3247,7 @@ int memcg_alloc_cache_params(struct kmem_cache *s,
 		s->memcg_params->root_cache = root_cache;
 		INIT_WORK(&s->memcg_params->destroy,
 				kmem_cache_destroy_work_func);
+		css_get(&memcg->css);
 	} else
 		s->memcg_params->is_root_cache = true;
 
@@ -3255,6 +3256,10 @@ int memcg_alloc_cache_params(struct kmem_cache *s,
 
 void memcg_free_cache_params(struct kmem_cache *s)
 {
+	if (!s->memcg_params)
+		return;
+	if (!s->memcg_params->is_root_cache)
+		css_put(&s->memcg_params->memcg->css);
 	kfree(s->memcg_params);
 }
 
@@ -3277,9 +3282,6 @@ void memcg_register_cache(struct kmem_cache *s)
 	memcg = s->memcg_params->memcg;
 	id = memcg_cache_id(memcg);
 
-	css_get(&memcg->css);
-
-
 	/*
 	 * Since readers won't lock (see cache_from_memcg_idx()), we need a
 	 * barrier here to ensure nobody will see the kmem_cache partially
@@ -3328,10 +3330,8 @@ void memcg_unregister_cache(struct kmem_cache *s)
 	 * after removing it from the memcg_slab_caches list, otherwise we can
 	 * fail to convert memcg_params_to_cache() while traversing the list.
 	 */
-	VM_BUG_ON(!root->memcg_params->memcg_caches[id]);
+	VM_BUG_ON(root->memcg_params->memcg_caches[id] != s);
 	root->memcg_params->memcg_caches[id] = NULL;
-
-	css_put(&memcg->css);
 }
 
 /*
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 3314fb3ead7f..4dff4bb66f19 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -310,9 +310,9 @@ void kmem_cache_destroy(struct kmem_cache *s)
 	s->refcount--;
 	if (!s->refcount) {
 		list_del(&s->list);
+		memcg_unregister_cache(s);
 
 		if (!__kmem_cache_shutdown(s)) {
-			memcg_unregister_cache(s);
 			mutex_unlock(&slab_mutex);
 			if (s->flags & SLAB_DESTROY_BY_RCU)
 				rcu_barrier();
@@ -322,6 +322,7 @@ void kmem_cache_destroy(struct kmem_cache *s)
 			kmem_cache_free(kmem_cache, s);
 		} else {
 			list_add(&s->list, &slab_caches);
+			memcg_register_cache(s);
 			mutex_unlock(&slab_mutex);
 			printk(KERN_ERR "kmem_cache_destroy %s: Slab cache still has objects\n",
 				s->name);
-- 
1.7.10.4

[PATCH v2 4/7] memcg, slab: unregister cache from memcg before starting to destroy it

[Vladimir Davydov]

Currently, memcg_unregister_cache(), which deletes the cache being
destroyed from the memcg_slab_caches list, is called after
__kmem_cache_shutdown() (see kmem_cache_destroy()), which starts to
destroy the cache. As a result, one can access a partially destroyed
cache while traversing a memcg_slab_caches list, which can have deadly
consequences (for instance, cache_show() called for each cache on a
memcg_slab_caches list from mem_cgroup_slabinfo_read() will dereference
pointers to already freed data).

To fix this, let's move memcg_unregister_cache() before the cache
destruction process beginning, issuing memcg_register_cache() on
failure.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 mm/memcontrol.c  |   12 ++++++------
 mm/slab_common.c |    3 ++-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b94d9917707c..6a059e73212c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3247,6 +3247,7 @@ int memcg_alloc_cache_params(struct kmem_cache *s,
 		s->memcg_params->root_cache = root_cache;
 		INIT_WORK(&s->memcg_params->destroy,
 				kmem_cache_destroy_work_func);
+		css_get(&memcg->css);
 	} else
 		s->memcg_params->is_root_cache = true;
 
@@ -3255,6 +3256,10 @@ int memcg_alloc_cache_params(struct kmem_cache *s,
 
 void memcg_free_cache_params(struct kmem_cache *s)
 {
+	if (!s->memcg_params)
+		return;
+	if (!s->memcg_params->is_root_cache)
+		css_put(&s->memcg_params->memcg->css);
 	kfree(s->memcg_params);
 }
 
@@ -3277,9 +3282,6 @@ void memcg_register_cache(struct kmem_cache *s)
 	memcg = s->memcg_params->memcg;
 	id = memcg_cache_id(memcg);
 
-	css_get(&memcg->css);
-
-
 	/*
 	 * Since readers won't lock (see cache_from_memcg_idx()), we need a
 	 * barrier here to ensure nobody will see the kmem_cache partially
@@ -3328,10 +3330,8 @@ void memcg_unregister_cache(struct kmem_cache *s)
 	 * after removing it from the memcg_slab_caches list, otherwise we can
 	 * fail to convert memcg_params_to_cache() while traversing the list.
 	 */
-	VM_BUG_ON(!root->memcg_params->memcg_caches[id]);
+	VM_BUG_ON(root->memcg_params->memcg_caches[id] != s);
 	root->memcg_params->memcg_caches[id] = NULL;
-
-	css_put(&memcg->css);
 }
 
 /*
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 3314fb3ead7f..4dff4bb66f19 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -310,9 +310,9 @@ void kmem_cache_destroy(struct kmem_cache *s)
 	s->refcount--;
 	if (!s->refcount) {
 		list_del(&s->list);
+		memcg_unregister_cache(s);
 
 		if (!__kmem_cache_shutdown(s)) {
-			memcg_unregister_cache(s);
 			mutex_unlock(&slab_mutex);
 			if (s->flags & SLAB_DESTROY_BY_RCU)
 				rcu_barrier();
@@ -322,6 +322,7 @@ void kmem_cache_destroy(struct kmem_cache *s)
 			kmem_cache_free(kmem_cache, s);
 		} else {
 			list_add(&s->list, &slab_caches);
+			memcg_register_cache(s);
 			mutex_unlock(&slab_mutex);
 			printk(KERN_ERR "kmem_cache_destroy %s: Slab cache still has objects\n",
 				s->name);
-- 
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 5/7] memcg, slab: do not destroy children caches if parent has aliases

[Vladimir Davydov]

Currently we destroy children caches at the very beginning of
kmem_cache_destroy(). This is wrong, because the root cache will not
necessarily be destroyed in the end - if it has aliases (refcount > 0),
kmem_cache_destroy() will simply decrement its refcount and return. In
this case, at best we will get a bunch of warnings in dmesg, like this
one:

  kmem_cache_destroy kmalloc-32:0: Slab cache still has objects
  CPU: 1 PID: 7139 Comm: modprobe Tainted: G    B   W    3.13.0+ #117
  Hardware name:
   ffff88007d7a6368 ffff880039b07e48 ffffffff8168cc2e ffff88007d7a6d68
   ffff88007d7a6300 ffff880039b07e68 ffffffff81175e9f 0000000000000000
   ffff88007d7a6300 ffff880039b07e98 ffffffff811b67c7 ffff88003e803c00
  Call Trace:
   [<ffffffff8168cc2e>] dump_stack+0x49/0x5b
   [<ffffffff81175e9f>] kmem_cache_destroy+0xdf/0xf0
   [<ffffffff811b67c7>] kmem_cache_destroy_memcg_children+0x97/0xc0
   [<ffffffff81175dcf>] kmem_cache_destroy+0xf/0xf0
   [<ffffffffa0130b21>] xfs_mru_cache_uninit+0x21/0x30 [xfs]
   [<ffffffffa01893ea>] exit_xfs_fs+0x2e/0xc44 [xfs]
   [<ffffffff810eeb58>] SyS_delete_module+0x198/0x1f0
   [<ffffffff816994f9>] system_call_fastpath+0x16/0x1b

At worst - if kmem_cache_destroy() will race with an allocation from a
memcg cache - the kernel will panic.

This patch fixes this by moving children caches destruction after the
check if the cache has aliases. Plus, it forbids destroying a root cache
if it still has children caches, because each children cache keeps a
reference to its parent.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 include/linux/memcontrol.h |    5 ---
 mm/memcontrol.c            |    2 +-
 mm/slab.h                  |   17 ++++++++--
 mm/slab_common.c           |   74 +++++++++++++++++++++++++++++---------------
 4 files changed, 65 insertions(+), 33 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index de79a9617e09..1a7d9c6741b7 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -513,7 +513,6 @@ struct kmem_cache *
 __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp);
 
 void mem_cgroup_destroy_cache(struct kmem_cache *cachep);
-void kmem_cache_destroy_memcg_children(struct kmem_cache *s);
 
 /**
  * memcg_kmem_newpage_charge: verify if a new kmem allocation is allowed.
@@ -667,10 +666,6 @@ memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
 {
 	return cachep;
 }
-
-static inline void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
-{
-}
 #endif /* CONFIG_MEMCG_KMEM */
 #endif /* _LINUX_MEMCONTROL_H */
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 6a059e73212c..4b8dd7ef18bf 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3428,7 +3428,7 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 	schedule_work(&cachep->memcg_params->destroy);
 }
 
-void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
+void __kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 {
 	struct kmem_cache *c;
 	int i;
diff --git a/mm/slab.h b/mm/slab.h
index 3045316b7c9d..b5ad968020a3 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -191,7 +191,16 @@ static inline struct kmem_cache *memcg_root_cache(struct kmem_cache *s)
 		return s;
 	return s->memcg_params->root_cache;
 }
-#else
+
+extern void __kmem_cache_destroy_memcg_children(struct kmem_cache *s);
+
+static inline void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
+{
+	mutex_unlock(&slab_mutex);
+	__kmem_cache_destroy_memcg_children(s);
+	mutex_lock(&slab_mutex);
+}
+#else /* !CONFIG_MEMCG_KMEM */
 static inline bool is_root_cache(struct kmem_cache *s)
 {
 	return true;
@@ -226,7 +235,11 @@ static inline struct kmem_cache *memcg_root_cache(struct kmem_cache *s)
 {
 	return s;
 }
-#endif
+
+static inline void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
+{
+}
+#endif /* CONFIG_MEMCG_KMEM */
 
 static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
 {
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 4dff4bb66f19..f8de8e5a18fd 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -300,38 +300,62 @@ out_free_cache:
 }
 #endif /* CONFIG_MEMCG_KMEM */
 
-void kmem_cache_destroy(struct kmem_cache *s)
+static bool cache_has_children(struct kmem_cache *s)
 {
-	/* Destroy all the children caches if we aren't a memcg cache */
-	kmem_cache_destroy_memcg_children(s);
+	int i;
+
+	if (!is_root_cache(s))
+		return false;
+	for_each_memcg_cache_index(i) {
+		if (cache_from_memcg_idx(s, i))
+			return true;
+	}
+	return false;
+}
 
+void kmem_cache_destroy(struct kmem_cache *s)
+{
 	get_online_cpus();
 	mutex_lock(&slab_mutex);
+
 	s->refcount--;
-	if (!s->refcount) {
-		list_del(&s->list);
-		memcg_unregister_cache(s);
-
-		if (!__kmem_cache_shutdown(s)) {
-			mutex_unlock(&slab_mutex);
-			if (s->flags & SLAB_DESTROY_BY_RCU)
-				rcu_barrier();
-
-			memcg_free_cache_params(s);
-			kfree(s->name);
-			kmem_cache_free(kmem_cache, s);
-		} else {
-			list_add(&s->list, &slab_caches);
-			memcg_register_cache(s);
-			mutex_unlock(&slab_mutex);
-			printk(KERN_ERR "kmem_cache_destroy %s: Slab cache still has objects\n",
-				s->name);
-			dump_stack();
-		}
-	} else {
-		mutex_unlock(&slab_mutex);
+	if (s->refcount)
+		goto out_unlock;
+
+	list_del(&s->list);
+	memcg_unregister_cache(s);
+
+	/* Destroy all the children caches if we aren't a memcg cache */
+	kmem_cache_destroy_memcg_children(s);
+	if (cache_has_children(s))
+		goto out_undelete;
+
+	if (__kmem_cache_shutdown(s) != 0) {
+		printk(KERN_ERR "kmem_cache_destroy %s: "
+		       "Slab cache still has objects\n", s->name);
+		dump_stack();
+		goto out_undelete;
 	}
+
+	mutex_unlock(&slab_mutex);
+	if (s->flags & SLAB_DESTROY_BY_RCU)
+		rcu_barrier();
+
+	memcg_free_cache_params(s);
+	kfree(s->name);
+	kmem_cache_free(kmem_cache, s);
+	goto out_put_cpus; /* slab_mutex already unlocked */
+
+out_unlock:
+	mutex_unlock(&slab_mutex);
+out_put_cpus:
 	put_online_cpus();
+	return;
+
+out_undelete:
+	list_add(&s->list, &slab_caches);
+	memcg_register_cache(s);
+	goto out_unlock;
 }
 EXPORT_SYMBOL(kmem_cache_destroy);
 
-- 
1.7.10.4

[PATCH v2 5/7] memcg, slab: do not destroy children caches if parent has aliases

[Vladimir Davydov]

Currently we destroy children caches at the very beginning of
kmem_cache_destroy(). This is wrong, because the root cache will not
necessarily be destroyed in the end - if it has aliases (refcount > 0),
kmem_cache_destroy() will simply decrement its refcount and return. In
this case, at best we will get a bunch of warnings in dmesg, like this
one:

  kmem_cache_destroy kmalloc-32:0: Slab cache still has objects
  CPU: 1 PID: 7139 Comm: modprobe Tainted: G    B   W    3.13.0+ #117
  Hardware name:
   ffff88007d7a6368 ffff880039b07e48 ffffffff8168cc2e ffff88007d7a6d68
   ffff88007d7a6300 ffff880039b07e68 ffffffff81175e9f 0000000000000000
   ffff88007d7a6300 ffff880039b07e98 ffffffff811b67c7 ffff88003e803c00
  Call Trace:
   [<ffffffff8168cc2e>] dump_stack+0x49/0x5b
   [<ffffffff81175e9f>] kmem_cache_destroy+0xdf/0xf0
   [<ffffffff811b67c7>] kmem_cache_destroy_memcg_children+0x97/0xc0
   [<ffffffff81175dcf>] kmem_cache_destroy+0xf/0xf0
   [<ffffffffa0130b21>] xfs_mru_cache_uninit+0x21/0x30 [xfs]
   [<ffffffffa01893ea>] exit_xfs_fs+0x2e/0xc44 [xfs]
   [<ffffffff810eeb58>] SyS_delete_module+0x198/0x1f0
   [<ffffffff816994f9>] system_call_fastpath+0x16/0x1b

At worst - if kmem_cache_destroy() will race with an allocation from a
memcg cache - the kernel will panic.

This patch fixes this by moving children caches destruction after the
check if the cache has aliases. Plus, it forbids destroying a root cache
if it still has children caches, because each children cache keeps a
reference to its parent.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 include/linux/memcontrol.h |    5 ---
 mm/memcontrol.c            |    2 +-
 mm/slab.h                  |   17 ++++++++--
 mm/slab_common.c           |   74 +++++++++++++++++++++++++++++---------------
 4 files changed, 65 insertions(+), 33 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index de79a9617e09..1a7d9c6741b7 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -513,7 +513,6 @@ struct kmem_cache *
 __memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp);
 
 void mem_cgroup_destroy_cache(struct kmem_cache *cachep);
-void kmem_cache_destroy_memcg_children(struct kmem_cache *s);
 
 /**
  * memcg_kmem_newpage_charge: verify if a new kmem allocation is allowed.
@@ -667,10 +666,6 @@ memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
 {
 	return cachep;
 }
-
-static inline void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
-{
-}
 #endif /* CONFIG_MEMCG_KMEM */
 #endif /* _LINUX_MEMCONTROL_H */
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 6a059e73212c..4b8dd7ef18bf 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3428,7 +3428,7 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 	schedule_work(&cachep->memcg_params->destroy);
 }
 
-void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
+void __kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 {
 	struct kmem_cache *c;
 	int i;
diff --git a/mm/slab.h b/mm/slab.h
index 3045316b7c9d..b5ad968020a3 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -191,7 +191,16 @@ static inline struct kmem_cache *memcg_root_cache(struct kmem_cache *s)
 		return s;
 	return s->memcg_params->root_cache;
 }
-#else
+
+extern void __kmem_cache_destroy_memcg_children(struct kmem_cache *s);
+
+static inline void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
+{
+	mutex_unlock(&slab_mutex);
+	__kmem_cache_destroy_memcg_children(s);
+	mutex_lock(&slab_mutex);
+}
+#else /* !CONFIG_MEMCG_KMEM */
 static inline bool is_root_cache(struct kmem_cache *s)
 {
 	return true;
@@ -226,7 +235,11 @@ static inline struct kmem_cache *memcg_root_cache(struct kmem_cache *s)
 {
 	return s;
 }
-#endif
+
+static inline void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
+{
+}
+#endif /* CONFIG_MEMCG_KMEM */
 
 static inline struct kmem_cache *cache_from_obj(struct kmem_cache *s, void *x)
 {
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 4dff4bb66f19..f8de8e5a18fd 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -300,38 +300,62 @@ out_free_cache:
 }
 #endif /* CONFIG_MEMCG_KMEM */
 
-void kmem_cache_destroy(struct kmem_cache *s)
+static bool cache_has_children(struct kmem_cache *s)
 {
-	/* Destroy all the children caches if we aren't a memcg cache */
-	kmem_cache_destroy_memcg_children(s);
+	int i;
+
+	if (!is_root_cache(s))
+		return false;
+	for_each_memcg_cache_index(i) {
+		if (cache_from_memcg_idx(s, i))
+			return true;
+	}
+	return false;
+}
 
+void kmem_cache_destroy(struct kmem_cache *s)
+{
 	get_online_cpus();
 	mutex_lock(&slab_mutex);
+
 	s->refcount--;
-	if (!s->refcount) {
-		list_del(&s->list);
-		memcg_unregister_cache(s);
-
-		if (!__kmem_cache_shutdown(s)) {
-			mutex_unlock(&slab_mutex);
-			if (s->flags & SLAB_DESTROY_BY_RCU)
-				rcu_barrier();
-
-			memcg_free_cache_params(s);
-			kfree(s->name);
-			kmem_cache_free(kmem_cache, s);
-		} else {
-			list_add(&s->list, &slab_caches);
-			memcg_register_cache(s);
-			mutex_unlock(&slab_mutex);
-			printk(KERN_ERR "kmem_cache_destroy %s: Slab cache still has objects\n",
-				s->name);
-			dump_stack();
-		}
-	} else {
-		mutex_unlock(&slab_mutex);
+	if (s->refcount)
+		goto out_unlock;
+
+	list_del(&s->list);
+	memcg_unregister_cache(s);
+
+	/* Destroy all the children caches if we aren't a memcg cache */
+	kmem_cache_destroy_memcg_children(s);
+	if (cache_has_children(s))
+		goto out_undelete;
+
+	if (__kmem_cache_shutdown(s) != 0) {
+		printk(KERN_ERR "kmem_cache_destroy %s: "
+		       "Slab cache still has objects\n", s->name);
+		dump_stack();
+		goto out_undelete;
 	}
+
+	mutex_unlock(&slab_mutex);
+	if (s->flags & SLAB_DESTROY_BY_RCU)
+		rcu_barrier();
+
+	memcg_free_cache_params(s);
+	kfree(s->name);
+	kmem_cache_free(kmem_cache, s);
+	goto out_put_cpus; /* slab_mutex already unlocked */
+
+out_unlock:
+	mutex_unlock(&slab_mutex);
+out_put_cpus:
 	put_online_cpus();
+	return;
+
+out_undelete:
+	list_add(&s->list, &slab_caches);
+	memcg_register_cache(s);
+	goto out_unlock;
 }
 EXPORT_SYMBOL(kmem_cache_destroy);
 
-- 
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 6/7] slub: adjust memcg caches when creating cache alias

[Vladimir Davydov]

Otherwise, kzalloc() called from a memcg won't clear the whole object.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 mm/slub.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/mm/slub.c b/mm/slub.c
index 8659e7184338..f3d2ef725ed6 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3743,7 +3743,11 @@ __kmem_cache_alias(const char *name, size_t size, size_t align,
 
 	s = find_mergeable(size, align, flags, name, ctor);
 	if (s) {
+		int i;
+		struct kmem_cache *c;
+
 		s->refcount++;
+
 		/*
 		 * Adjust the object sizes so that we clear
 		 * the complete object on kzalloc.
@@ -3751,6 +3755,16 @@ __kmem_cache_alias(const char *name, size_t size, size_t align,
 		s->object_size = max(s->object_size, (int)size);
 		s->inuse = max_t(int, s->inuse, ALIGN(size, sizeof(void *)));
 
+		BUG_ON(!is_root_cache(s));
+		for_each_memcg_cache_index(i) {
+			c = cache_from_memcg_idx(s, i);
+			if (!c)
+				continue;
+			c->object_size = s->object_size;
+			c->inuse = max_t(int, c->inuse,
+					 ALIGN(size, sizeof(void *)));
+		}
+
 		if (sysfs_slab_alias(s, name)) {
 			s->refcount--;
 			s = NULL;
-- 
1.7.10.4

[PATCH v2 6/7] slub: adjust memcg caches when creating cache alias

[Vladimir Davydov]

Otherwise, kzalloc() called from a memcg won't clear the whole object.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 mm/slub.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/mm/slub.c b/mm/slub.c
index 8659e7184338..f3d2ef725ed6 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3743,7 +3743,11 @@ __kmem_cache_alias(const char *name, size_t size, size_t align,
 
 	s = find_mergeable(size, align, flags, name, ctor);
 	if (s) {
+		int i;
+		struct kmem_cache *c;
+
 		s->refcount++;
+
 		/*
 		 * Adjust the object sizes so that we clear
 		 * the complete object on kzalloc.
@@ -3751,6 +3755,16 @@ __kmem_cache_alias(const char *name, size_t size, size_t align,
 		s->object_size = max(s->object_size, (int)size);
 		s->inuse = max_t(int, s->inuse, ALIGN(size, sizeof(void *)));
 
+		BUG_ON(!is_root_cache(s));
+		for_each_memcg_cache_index(i) {
+			c = cache_from_memcg_idx(s, i);
+			if (!c)
+				continue;
+			c->object_size = s->object_size;
+			c->inuse = max_t(int, c->inuse,
+					 ALIGN(size, sizeof(void *)));
+		}
+
 		if (sysfs_slab_alias(s, name)) {
 			s->refcount--;
 			s = NULL;
-- 
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 7/7] slub: rework sysfs layout for memcg caches

[Vladimir Davydov]

Currently, we try to arrange sysfs entries for memcg caches in the same
manner as for global caches. Apart from turning /sys/kernel/slab into a
mess when there are a lot of kmem-active memcgs created, it actually
does not work properly - we won't create more than one link to a memcg
cache in case its parent is merged with another cache. For instance, if
A is a root cache merged with another root cache B, we will have the
following sysfs setup:

  X
  A -> X
  B -> X

where X is some unique id (see create_unique_id()). Now if memcgs M and
N start to allocate from cache A (or B, which is the same), we will get:

  X
  X:M
  X:N
  A -> X
  B -> X
  A:M -> X:M
  A:N -> X:N

Since B is an alias for A, we won't get entries B:M and B:N, which is
confusing.

It is more logical to have entries for memcg caches under the
corresponding root cache's sysfs directory. This would allow us to keep
sysfs layout clean, and avoid such inconsistencies like one described
above.

This patch does the trick. It creates a "cgroup" kset in each root cache
kobject to keep its children caches there.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 include/linux/slub_def.h |    3 ++
 mm/slub.c                |   85 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 73 insertions(+), 15 deletions(-)

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index f56bfa9e4526..f2f7398848cf 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -87,6 +87,9 @@ struct kmem_cache {
 #ifdef CONFIG_MEMCG_KMEM
 	struct memcg_cache_params *memcg_params;
 	int max_attr_size; /* for propagation, maximum size of a stored attr */
+#ifdef CONFIG_SYSFS
+	struct kset *memcg_kset;
+#endif
 #endif
 
 #ifdef CONFIG_NUMA
diff --git a/mm/slub.c b/mm/slub.c
index f3d2ef725ed6..d5d1ecc5ace9 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5137,6 +5137,44 @@ static const struct kset_uevent_ops slab_uevent_ops = {
 
 static struct kset *slab_kset;
 
+#ifdef CONFIG_MEMCG_KMEM
+static inline struct kset *cache_kset(struct kmem_cache *s)
+{
+	if (is_root_cache(s))
+		return slab_kset;
+	return s->memcg_params->root_cache->memcg_kset;
+}
+
+static int create_cache_memcg_kset(struct kmem_cache *s)
+{
+	if (!is_root_cache(s))
+		return 0;
+	s->memcg_kset = kset_create_and_add("cgroup", NULL, &s->kobj);
+	if (!s->memcg_kset)
+		return -ENOMEM;
+	return 0;
+}
+
+static void destroy_cache_memcg_kset(struct kmem_cache *s)
+{
+	kset_unregister(s->memcg_kset);
+}
+#else
+static inline struct kset *cache_kset(struct kmem_cache *s)
+{
+	return slab_kset;
+}
+
+static inline int create_cache_memcg_kset(struct kmem_cache *s)
+{
+	return 0;
+}
+
+static inline void destroy_cache_memcg_kset(struct kmem_cache *s)
+{
+}
+#endif /* CONFIG_MEMCG_KMEM */
+
 #define ID_STR_LENGTH 64
 
 /* Create a unique string id for a slab cache:
@@ -5148,7 +5186,8 @@ static char *create_unique_id(struct kmem_cache *s)
 	char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
 	char *p = name;
 
-	BUG_ON(!name);
+	if (!name)
+		return NULL;
 
 	*p++ = ':';
 	/*
@@ -5192,7 +5231,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
 		 * This is typically the case for debug situations. In that
 		 * case we can catch duplicate names easily.
 		 */
-		sysfs_remove_link(&slab_kset->kobj, s->name);
+		if (is_root_cache(s))
+			sysfs_remove_link(&slab_kset->kobj, s->name);
 		name = s->name;
 	} else {
 		/*
@@ -5200,28 +5240,40 @@ static int sysfs_slab_add(struct kmem_cache *s)
 		 * for the symlinks.
 		 */
 		name = create_unique_id(s);
+		if (!name)
+			return -ENOMEM;
 	}
 
-	s->kobj.kset = slab_kset;
+	s->kobj.kset = cache_kset(s);
 	err = kobject_init_and_add(&s->kobj, &slab_ktype, NULL, "%s", name);
-	if (err) {
-		kobject_put(&s->kobj);
-		return err;
-	}
+	if (err)
+		goto out_put_kobj;
+
+	err = create_cache_memcg_kset(s);
+	if (err)
+		goto out_del_kobj;
 
 	err = sysfs_create_group(&s->kobj, &slab_attr_group);
-	if (err) {
-		kobject_del(&s->kobj);
-		kobject_put(&s->kobj);
-		return err;
-	}
+	if (err)
+		goto out_del_memcg_kset;
+
 	kobject_uevent(&s->kobj, KOBJ_ADD);
-	if (!unmergeable) {
+	if (!unmergeable && is_root_cache(s)) {
 		/* Setup first alias */
 		sysfs_slab_alias(s, s->name);
-		kfree(name);
 	}
-	return 0;
+out:
+	if (!unmergeable)
+		kfree(name);
+	return err;
+
+out_del_memcg_kset:
+	destroy_cache_memcg_kset(s);
+out_del_kobj:
+	kobject_del(&s->kobj);
+out_put_kobj:
+	kobject_put(&s->kobj);
+	goto out;
 }
 
 static void sysfs_slab_remove(struct kmem_cache *s)
@@ -5233,6 +5285,7 @@ static void sysfs_slab_remove(struct kmem_cache *s)
 		 */
 		return;
 
+	destroy_cache_memcg_kset(s);
 	kobject_uevent(&s->kobj, KOBJ_REMOVE);
 	kobject_del(&s->kobj);
 	kobject_put(&s->kobj);
@@ -5254,6 +5307,8 @@ static int sysfs_slab_alias(struct kmem_cache *s, const char *name)
 {
 	struct saved_alias *al;
 
+	BUG_ON(!is_root_cache(s));
+
 	if (slab_state == FULL) {
 		/*
 		 * If we have a leftover link then remove it.
-- 
1.7.10.4

[PATCH v2 7/7] slub: rework sysfs layout for memcg caches

[Vladimir Davydov]

Currently, we try to arrange sysfs entries for memcg caches in the same
manner as for global caches. Apart from turning /sys/kernel/slab into a
mess when there are a lot of kmem-active memcgs created, it actually
does not work properly - we won't create more than one link to a memcg
cache in case its parent is merged with another cache. For instance, if
A is a root cache merged with another root cache B, we will have the
following sysfs setup:

  X
  A -> X
  B -> X

where X is some unique id (see create_unique_id()). Now if memcgs M and
N start to allocate from cache A (or B, which is the same), we will get:

  X
  X:M
  X:N
  A -> X
  B -> X
  A:M -> X:M
  A:N -> X:N

Since B is an alias for A, we won't get entries B:M and B:N, which is
confusing.

It is more logical to have entries for memcg caches under the
corresponding root cache's sysfs directory. This would allow us to keep
sysfs layout clean, and avoid such inconsistencies like one described
above.

This patch does the trick. It creates a "cgroup" kset in each root cache
kobject to keep its children caches there.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 include/linux/slub_def.h |    3 ++
 mm/slub.c                |   85 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 73 insertions(+), 15 deletions(-)

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index f56bfa9e4526..f2f7398848cf 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -87,6 +87,9 @@ struct kmem_cache {
 #ifdef CONFIG_MEMCG_KMEM
 	struct memcg_cache_params *memcg_params;
 	int max_attr_size; /* for propagation, maximum size of a stored attr */
+#ifdef CONFIG_SYSFS
+	struct kset *memcg_kset;
+#endif
 #endif
 
 #ifdef CONFIG_NUMA
diff --git a/mm/slub.c b/mm/slub.c
index f3d2ef725ed6..d5d1ecc5ace9 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5137,6 +5137,44 @@ static const struct kset_uevent_ops slab_uevent_ops = {
 
 static struct kset *slab_kset;
 
+#ifdef CONFIG_MEMCG_KMEM
+static inline struct kset *cache_kset(struct kmem_cache *s)
+{
+	if (is_root_cache(s))
+		return slab_kset;
+	return s->memcg_params->root_cache->memcg_kset;
+}
+
+static int create_cache_memcg_kset(struct kmem_cache *s)
+{
+	if (!is_root_cache(s))
+		return 0;
+	s->memcg_kset = kset_create_and_add("cgroup", NULL, &s->kobj);
+	if (!s->memcg_kset)
+		return -ENOMEM;
+	return 0;
+}
+
+static void destroy_cache_memcg_kset(struct kmem_cache *s)
+{
+	kset_unregister(s->memcg_kset);
+}
+#else
+static inline struct kset *cache_kset(struct kmem_cache *s)
+{
+	return slab_kset;
+}
+
+static inline int create_cache_memcg_kset(struct kmem_cache *s)
+{
+	return 0;
+}
+
+static inline void destroy_cache_memcg_kset(struct kmem_cache *s)
+{
+}
+#endif /* CONFIG_MEMCG_KMEM */
+
 #define ID_STR_LENGTH 64
 
 /* Create a unique string id for a slab cache:
@@ -5148,7 +5186,8 @@ static char *create_unique_id(struct kmem_cache *s)
 	char *name = kmalloc(ID_STR_LENGTH, GFP_KERNEL);
 	char *p = name;
 
-	BUG_ON(!name);
+	if (!name)
+		return NULL;
 
 	*p++ = ':';
 	/*
@@ -5192,7 +5231,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
 		 * This is typically the case for debug situations. In that
 		 * case we can catch duplicate names easily.
 		 */
-		sysfs_remove_link(&slab_kset->kobj, s->name);
+		if (is_root_cache(s))
+			sysfs_remove_link(&slab_kset->kobj, s->name);
 		name = s->name;
 	} else {
 		/*
@@ -5200,28 +5240,40 @@ static int sysfs_slab_add(struct kmem_cache *s)
 		 * for the symlinks.
 		 */
 		name = create_unique_id(s);
+		if (!name)
+			return -ENOMEM;
 	}
 
-	s->kobj.kset = slab_kset;
+	s->kobj.kset = cache_kset(s);
 	err = kobject_init_and_add(&s->kobj, &slab_ktype, NULL, "%s", name);
-	if (err) {
-		kobject_put(&s->kobj);
-		return err;
-	}
+	if (err)
+		goto out_put_kobj;
+
+	err = create_cache_memcg_kset(s);
+	if (err)
+		goto out_del_kobj;
 
 	err = sysfs_create_group(&s->kobj, &slab_attr_group);
-	if (err) {
-		kobject_del(&s->kobj);
-		kobject_put(&s->kobj);
-		return err;
-	}
+	if (err)
+		goto out_del_memcg_kset;
+
 	kobject_uevent(&s->kobj, KOBJ_ADD);
-	if (!unmergeable) {
+	if (!unmergeable && is_root_cache(s)) {
 		/* Setup first alias */
 		sysfs_slab_alias(s, s->name);
-		kfree(name);
 	}
-	return 0;
+out:
+	if (!unmergeable)
+		kfree(name);
+	return err;
+
+out_del_memcg_kset:
+	destroy_cache_memcg_kset(s);
+out_del_kobj:
+	kobject_del(&s->kobj);
+out_put_kobj:
+	kobject_put(&s->kobj);
+	goto out;
 }
 
 static void sysfs_slab_remove(struct kmem_cache *s)
@@ -5233,6 +5285,7 @@ static void sysfs_slab_remove(struct kmem_cache *s)
 		 */
 		return;
 
+	destroy_cache_memcg_kset(s);
 	kobject_uevent(&s->kobj, KOBJ_REMOVE);
 	kobject_del(&s->kobj);
 	kobject_put(&s->kobj);
@@ -5254,6 +5307,8 @@ static int sysfs_slab_alias(struct kmem_cache *s, const char *name)
 {
 	struct saved_alias *al;
 
+	BUG_ON(!is_root_cache(s));
+
 	if (slab_state == FULL) {
 		/*
 		 * If we have a leftover link then remove it.
-- 
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/7] memcg, slab: cleanup memcg cache name creation

[Andrew Morton]

On Mon, 3 Feb 2014 19:54:37 +0400 Vladimir Davydov <vdavydov@parallels.com> wrote:

> 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.
> 
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -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.
> +	 */
> +	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.

> +	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);
>  }
>  

Re: [PATCH v2 2/7] memcg, slab: cleanup memcg cache name creation

[Andrew Morton]

On Mon, 3 Feb 2014 19:54:37 +0400 Vladimir Davydov <vdavydov@parallels.com> wrote:

> 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.
> 
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -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.
> +	 */
> +	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.

> +	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);
>  }
>  

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/7] memcg, slab: cleanup memcg cache name creation

[Vladimir Davydov]

On 02/04/2014 02:08 AM, Andrew Morton wrote:
> On Mon, 3 Feb 2014 19:54:37 +0400 Vladimir Davydov <vdavydov@parallels.com> wrote:
>
>> 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.
>>
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -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.
>> +	 */
>> +	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.

Oops, I missed that. Thank you for pointing me out. It seems the usage
of the temporary buffer is inevitable. However, a dedicated mutex
protecting it can be removed, because we already hold the slab_mutex
while calling this function. Will rework.

Thanks.

>
>> +	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);
>>  }
>>  

Re: [PATCH v2 2/7] memcg, slab: cleanup memcg cache name creation

[Vladimir Davydov]

On 02/04/2014 02:08 AM, Andrew Morton wrote:
> On Mon, 3 Feb 2014 19:54:37 +0400 Vladimir Davydov <vdavydov@parallels.com> wrote:
>
>> 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.
>>
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -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.
>> +	 */
>> +	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.

Oops, I missed that. Thank you for pointing me out. It seems the usage
of the temporary buffer is inevitable. However, a dedicated mutex
protecting it can be removed, because we already hold the slab_mutex
while calling this function. Will rework.

Thanks.

>
>> +	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);
>>  }
>>  

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] memcg, slab: cleanup memcg cache creation

[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.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 include/linux/memcontrol.h |    9 +++++
 mm/memcontrol.c            |   89 ++++++++++++++++++++------------------------
 mm/slab_common.c           |    5 ++-
 3 files changed, 54 insertions(+), 49 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index abd0113b6620..84e4801fc36c 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -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)
 {
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 53385cd4e6f0..43e08b7bb365 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -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);
 }
 
diff --git a/mm/slab_common.c b/mm/slab_common.c
index e77b51eb7347..11857abf7057 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -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;
 
-- 
1.7.10.4

[PATCH] memcg, slab: cleanup memcg cache creation

[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.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 include/linux/memcontrol.h |    9 +++++
 mm/memcontrol.c            |   89 ++++++++++++++++++++------------------------
 mm/slab_common.c           |    5 ++-
 3 files changed, 54 insertions(+), 49 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index abd0113b6620..84e4801fc36c 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -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)
 {
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 53385cd4e6f0..43e08b7bb365 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -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);
 }
 
diff --git a/mm/slab_common.c b/mm/slab_common.c
index e77b51eb7347..11857abf7057 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -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;
 
-- 
1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] memcg, slab: cleanup memcg cache creation

[Michal Hocko]

On Tue 04-02-14 11:39:07, Vladimir Davydov wrote:
> 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?

> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> ---
>  include/linux/memcontrol.h |    9 +++++
>  mm/memcontrol.c            |   89 ++++++++++++++++++++------------------------
>  mm/slab_common.c           |    5 ++-
>  3 files changed, 54 insertions(+), 49 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index abd0113b6620..84e4801fc36c 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -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)
>  {
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 53385cd4e6f0..43e08b7bb365 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -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);
>  }
>  
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index e77b51eb7347..11857abf7057 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -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;
>  
> -- 
> 1.7.10.4
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] memcg, slab: cleanup memcg cache creation

[Michal Hocko]

On Tue 04-02-14 11:39:07, Vladimir Davydov wrote:
> 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?

> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> ---
>  include/linux/memcontrol.h |    9 +++++
>  mm/memcontrol.c            |   89 ++++++++++++++++++++------------------------
>  mm/slab_common.c           |    5 ++-
>  3 files changed, 54 insertions(+), 49 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index abd0113b6620..84e4801fc36c 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -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)
>  {
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 53385cd4e6f0..43e08b7bb365 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -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);
>  }
>  
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index e77b51eb7347..11857abf7057 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -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;
>  
> -- 
> 1.7.10.4
> 

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[Michal Hocko]

On Mon 03-02-14 19:54:38, Vladimir Davydov wrote:
> 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?
 
> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> ---
>  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(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 84e4801fc36c..de79a9617e09 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -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. Also what does `s' stand for and can we use a more
descriptive name, please?

[...]
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[Michal Hocko]

On Mon 03-02-14 19:54:38, Vladimir Davydov wrote:
> 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?
 
> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> ---
>  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(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 84e4801fc36c..de79a9617e09 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -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. Also what does `s' stand for and can we use a more
descriptive name, please?

[...]
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] memcg, slab: cleanup memcg cache creation

[Vladimir Davydov]

On 02/04/2014 07:33 PM, Michal Hocko wrote:
> On Tue 04-02-14 11:39:07, Vladimir Davydov wrote:
>> 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?

The new cgroup_name() has not been committed neither to master, nor
for-next branches of

git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup

so I guess it will take a while.

> Maybe we can get rid of the static buffer altogether?

It seems we can't. The point is that we don't know in advance how much
memory we would like to allocate for slab cache name, because we don't
know the length of the cgroup name. Note, we can't estimate the length
(as I tried in the initial version of this patch), because cgroup name
can change any time. So we will have to copy the cgroup name to a buffer
large enough to accommodate it first, and only then copy it to slab
cache name.  The alternative way would be allocation >= NAME_MAX bytes
for any memcg cache name, but it will be waste of memory.

Thanks.

>
>> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
>> ---
>>  include/linux/memcontrol.h |    9 +++++
>>  mm/memcontrol.c            |   89 ++++++++++++++++++++------------------------
>>  mm/slab_common.c           |    5 ++-
>>  3 files changed, 54 insertions(+), 49 deletions(-)
>>
>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> index abd0113b6620..84e4801fc36c 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -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)
>>  {
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 53385cd4e6f0..43e08b7bb365 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -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);
>>  }
>>  
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index e77b51eb7347..11857abf7057 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
>> @@ -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;
>>  
>> -- 
>> 1.7.10.4
>>

Re: [PATCH] memcg, slab: cleanup memcg cache creation

[Vladimir Davydov]

On 02/04/2014 07:33 PM, Michal Hocko wrote:
> On Tue 04-02-14 11:39:07, Vladimir Davydov wrote:
>> 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?

The new cgroup_name() has not been committed neither to master, nor
for-next branches of

git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup

so I guess it will take a while.

> Maybe we can get rid of the static buffer altogether?

It seems we can't. The point is that we don't know in advance how much
memory we would like to allocate for slab cache name, because we don't
know the length of the cgroup name. Note, we can't estimate the length
(as I tried in the initial version of this patch), because cgroup name
can change any time. So we will have to copy the cgroup name to a buffer
large enough to accommodate it first, and only then copy it to slab
cache name.  The alternative way would be allocation >= NAME_MAX bytes
for any memcg cache name, but it will be waste of memory.

Thanks.

>
>> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
>> ---
>>  include/linux/memcontrol.h |    9 +++++
>>  mm/memcontrol.c            |   89 ++++++++++++++++++++------------------------
>>  mm/slab_common.c           |    5 ++-
>>  3 files changed, 54 insertions(+), 49 deletions(-)
>>
>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> index abd0113b6620..84e4801fc36c 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -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)
>>  {
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 53385cd4e6f0..43e08b7bb365 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -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);
>>  }
>>  
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index e77b51eb7347..11857abf7057 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
>> @@ -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;
>>  
>> -- 
>> 1.7.10.4
>>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[Vladimir Davydov]

[-- Attachment #1: Type: text/plain, Size: 2440 bytes --]

On 02/04/2014 08:03 PM, Michal Hocko wrote:
> On Mon 03-02-14 19:54:38, Vladimir Davydov wrote:
>> 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?

May be, something like the patch attached?

>  
>> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
>> ---
>>  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(-)
>>
>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> index 84e4801fc36c..de79a9617e09 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -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.

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.

> 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.

Thanks.

[-- Attachment #2: 0001-memcg-slab-separate-memcg-vs-root-cache-creation-pat.patch --]
[-- Type: text/x-patch, Size: 9587 bytes --]

>From 55f0916c794ad25a8bf45566f6d333bea956e0d4 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov@parallels.com>
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 <vdavydov@parallels.com>
---
 include/linux/slab.h |    9 ++-
 mm/memcontrol.c      |   12 +---
 mm/slab_common.c     |  174 +++++++++++++++++++++++++++-----------------------
 3 files changed, 101 insertions(+), 94 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 9260abdd67df..e8c95d0bb879 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -113,11 +113,10 @@ void __init kmem_cache_init(void);
 int slab_is_available(void);
 
 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 *));
+#ifdef CONFIG_MEMCG_KMEM
+int kmem_cache_create_memcg(struct mem_cgroup *, struct kmem_cache *);
+#endif
 void kmem_cache_destroy(struct kmem_cache *);
 int kmem_cache_shrink(struct kmem_cache *);
 void kmem_cache_free(struct kmem_cache *, void *);
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 43e08b7bb365..3857033c2718 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3495,15 +3495,9 @@ struct create_work {
 static void memcg_create_cache_work_func(struct work_struct *w)
 {
 	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;
-
-	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);
+
+	kmem_cache_create_memcg(cw->memcg, cw->cachep);
+	css_put(&cw->memcg->css);
 	kfree(cw);
 }
 
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 11857abf7057..6bee919ece80 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -29,8 +29,7 @@ DEFINE_MUTEX(slab_mutex);
 struct kmem_cache *kmem_cache;
 
 #ifdef CONFIG_DEBUG_VM
-static int kmem_cache_sanity_check(struct mem_cgroup *memcg, const char *name,
-				   size_t size)
+static int kmem_cache_sanity_check(const char *name, size_t size)
 {
 	struct kmem_cache *s = NULL;
 
@@ -57,13 +56,7 @@ static int kmem_cache_sanity_check(struct mem_cgroup *memcg, const char *name,
 		}
 
 #if !defined(CONFIG_SLUB) || !defined(CONFIG_SLUB_DEBUG_ON)
-		/*
-		 * For simplicity, we won't check this in the list of memcg
-		 * caches. We have control over memcg naming, and if there
-		 * aren't duplicates in the global list, there won't be any
-		 * duplicates in the memcg lists as well.
-		 */
-		if (!memcg && !strcmp(s->name, name)) {
+		if (!strcmp(s->name, name)) {
 			pr_err("%s (%s): Cache name already exists.\n",
 			       __func__, name);
 			dump_stack();
@@ -77,8 +70,7 @@ static int kmem_cache_sanity_check(struct mem_cgroup *memcg, const char *name,
 	return 0;
 }
 #else
-static inline int kmem_cache_sanity_check(struct mem_cgroup *memcg,
-					  const char *name, size_t size)
+static inline int kmem_cache_sanity_check(const char *name, size_t size)
 {
 	return 0;
 }
@@ -139,6 +131,47 @@ unsigned long calculate_alignment(unsigned long flags,
 	return ALIGN(align, sizeof(void *));
 }
 
+static struct kmem_cache *
+do_kmem_cache_create(char *name, size_t object_size, size_t size, size_t align,
+		     unsigned long flags, void (*ctor)(void *),
+		     struct mem_cgroup *memcg, struct kmem_cache *cachep)
+{
+	struct kmem_cache *s = NULL;
+	int err = -ENOMEM;
+
+	if (!name)
+		goto out;
+
+	s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL);
+	if (!s)
+		goto out;
+
+	s->name = name;
+	s->object_size = object_size;
+	s->size = size;
+	s->align = align;
+	s->ctor = ctor;
+
+	err = memcg_alloc_cache_params(memcg, s, cachep);
+	if (err)
+		goto out;
+
+	err = __kmem_cache_create(s, flags);
+	if (err)
+		goto out;
+
+	s->refcount = 1;
+	list_add(&s->list, &slab_caches);
+	memcg_register_cache(s);
+out:
+	if (err) {
+		memcg_free_cache_params(s);
+		kfree(name);
+		kfree(s);
+		s = ERR_PTR(err);
+	}
+	return s;
+}
 
 /*
  * kmem_cache_create - Create a cache.
@@ -164,11 +197,9 @@ unsigned long calculate_alignment(unsigned long flags,
  * cacheline.  This can be beneficial if you're counting cycles as closely
  * as davem.
  */
-
 struct kmem_cache *
-kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
-			size_t align, unsigned long flags, void (*ctor)(void *),
-			struct kmem_cache *parent_cache)
+kmem_cache_create(const char *name, size_t size, size_t align,
+		  unsigned long flags, void (*ctor)(void *))
 {
 	struct kmem_cache *s = NULL;
 	int err;
@@ -176,22 +207,10 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	get_online_cpus();
 	mutex_lock(&slab_mutex);
 
-	err = kmem_cache_sanity_check(memcg, name, size);
+	err = kmem_cache_sanity_check(name, size);
 	if (err)
 		goto out_unlock;
 
-	if (memcg) {
-		/*
-		 * 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. Therefore if we get here and see the cache has
-		 * already been created, we silently return NULL.
-		 */
-		if (cache_from_memcg_idx(parent_cache, memcg_cache_id(memcg)))
-			goto out_unlock;
-	}
-
 	/*
 	 * Some allocators will constraint the set of valid flags to a subset
 	 * of all flags. We expect them to define CACHE_CREATE_MASK in this
@@ -200,55 +219,20 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
 	 */
 	flags &= CACHE_CREATE_MASK;
 
-	if (!memcg) {
-		s = __kmem_cache_alias(name, size, align, flags, ctor);
-		if (s)
-			goto out_unlock;
-	}
-
-	err = -ENOMEM;
-	s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL);
-	if (!s)
+	s = __kmem_cache_alias(name, size, align, flags, ctor);
+	if (s)
 		goto out_unlock;
 
-	s->object_size = s->size = size;
-	s->align = calculate_alignment(flags, align, size);
-	s->ctor = ctor;
-
-	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;
-
-	err = memcg_alloc_cache_params(memcg, s, parent_cache);
-	if (err)
-		goto out_free_cache;
-
-	err = __kmem_cache_create(s, flags);
-	if (err)
-		goto out_free_cache;
-
-	s->refcount = 1;
-	list_add(&s->list, &slab_caches);
-	memcg_register_cache(s);
+	s = do_kmem_cache_create(kstrdup(name, GFP_KERNEL),
+			size, size, calculate_alignment(flags, align, size),
+			flags, ctor, NULL, NULL);
+	err = IS_ERR(s) ? PTR_ERR(s) : 0;
 
 out_unlock:
 	mutex_unlock(&slab_mutex);
 	put_online_cpus();
 
 	if (err) {
-		/*
-		 * There is no point in flooding logs with warnings or
-		 * especially crashing the system if we fail to create a cache
-		 * for a memcg. In this case we will be accounting the memcg
-		 * allocation to the root cgroup until we succeed to create its
-		 * own cache, but it isn't that critical.
-		 */
-		if (!memcg)
-			return NULL;
-
 		if (flags & SLAB_PANIC)
 			panic("kmem_cache_create: Failed to create slab '%s'. Error %d\n",
 				name, err);
@@ -260,21 +244,51 @@ out_unlock:
 		return NULL;
 	}
 	return s;
-
-out_free_cache:
-	memcg_free_cache_params(s);
-	kfree(s->name);
-	kmem_cache_free(kmem_cache, s);
-	goto out_unlock;
 }
+EXPORT_SYMBOL(kmem_cache_create);
 
-struct kmem_cache *
-kmem_cache_create(const char *name, size_t size, size_t align,
-		  unsigned long flags, void (*ctor)(void *))
+#ifdef CONFIG_MEMCG_KMEM
+/*
+ * kmem_cache_create_memcg - Create a cache for a memory cgroup.
+ * @memcg: The memory cgroup the new cache is for.
+ * @cachep: The parent of the new cache.
+ *
+ * This function creates a kmem cache that will serve allocation requests going
+ * from @memcg to @cachep. The new cache inherits properties from its parent.
+ *
+ * Returns 0 on success, -errno on failure.
+ */
+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);
+
+	/*
+	 * 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;
+	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)
 {
-- 
1.7.10.4

Re: [PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[Vladimir Davydov]

[-- Attachment #1: Type: text/plain, Size: 2440 bytes --]

On 02/04/2014 08:03 PM, Michal Hocko wrote:
> On Mon 03-02-14 19:54:38, Vladimir Davydov wrote:
>> 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?

May be, something like the patch attached?

>  
>> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
>> ---
>>  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(-)
>>
>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> index 84e4801fc36c..de79a9617e09 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -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.

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.

> 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.

Thanks.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-memcg-slab-separate-memcg-vs-root-cache-creation-pat.patch --]
[-- Type: text/x-patch; name="0001-memcg-slab-separate-memcg-vs-root-cache-creation-pat.patch", Size: 0 bytes --]

Re: [PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[Michal Hocko]

On Tue 04-02-14 23:19:24, Vladimir Davydov wrote:
> On 02/04/2014 08:03 PM, Michal Hocko wrote:
> > On Mon 03-02-14 19:54:38, Vladimir Davydov wrote:
> >> 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?
> 
> May be, something like the patch attached?
> 
> >  
> >> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> >> ---
> >>  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(-)
> >>
> >> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> >> index 84e4801fc36c..de79a9617e09 100644
> >> --- a/include/linux/memcontrol.h
> >> +++ b/include/linux/memcontrol.h
> >> @@ -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.
> 
> 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.
 
> > 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.

> From 55f0916c794ad25a8bf45566f6d333bea956e0d4 Mon Sep 17 00:00:00 2001
> From: Vladimir Davydov <vdavydov@parallels.com>
> 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 <vdavydov@parallels.com>

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

> ---
>  include/linux/slab.h |    9 ++-
>  mm/memcontrol.c      |   12 +---
>  mm/slab_common.c     |  174 +++++++++++++++++++++++++++-----------------------
>  3 files changed, 101 insertions(+), 94 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 9260abdd67df..e8c95d0bb879 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -113,11 +113,10 @@ void __init kmem_cache_init(void);
>  int slab_is_available(void);
>  
>  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.

[...]
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 11857abf7057..6bee919ece80 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
[...]
> +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);
> +
> +	/*
> +	 * 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.

> +	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)
>  {
> -- 
> 1.7.10.4
> 


-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[Michal Hocko]

On Tue 04-02-14 23:19:24, Vladimir Davydov wrote:
> On 02/04/2014 08:03 PM, Michal Hocko wrote:
> > On Mon 03-02-14 19:54:38, Vladimir Davydov wrote:
> >> 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?
> 
> May be, something like the patch attached?
> 
> >  
> >> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> >> ---
> >>  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(-)
> >>
> >> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> >> index 84e4801fc36c..de79a9617e09 100644
> >> --- a/include/linux/memcontrol.h
> >> +++ b/include/linux/memcontrol.h
> >> @@ -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.
> 
> 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.
 
> > 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.

> From 55f0916c794ad25a8bf45566f6d333bea956e0d4 Mon Sep 17 00:00:00 2001
> From: Vladimir Davydov <vdavydov@parallels.com>
> 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 <vdavydov@parallels.com>

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

> ---
>  include/linux/slab.h |    9 ++-
>  mm/memcontrol.c      |   12 +---
>  mm/slab_common.c     |  174 +++++++++++++++++++++++++++-----------------------
>  3 files changed, 101 insertions(+), 94 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 9260abdd67df..e8c95d0bb879 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -113,11 +113,10 @@ void __init kmem_cache_init(void);
>  int slab_is_available(void);
>  
>  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.

[...]
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 11857abf7057..6bee919ece80 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
[...]
> +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);
> +
> +	/*
> +	 * 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.

> +	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)
>  {
> -- 
> 1.7.10.4
> 


-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[Vladimir Davydov]

On 02/06/2014 08:41 PM, Michal Hocko wrote:
> On Tue 04-02-14 23:19:24, Vladimir Davydov wrote:
>> On 02/04/2014 08:03 PM, Michal Hocko wrote:
>>> On Mon 03-02-14 19:54:38, Vladimir Davydov wrote:
>>>> 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?
>> May be, something like the patch attached?
>>
>>>  
>>>> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
>>>> ---
>>>>  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(-)
>>>>
>>>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>>>> index 84e4801fc36c..de79a9617e09 100644
>>>> --- a/include/linux/memcontrol.h
>>>> +++ b/include/linux/memcontrol.h
>>>> @@ -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.
>> 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.

All right, I'll leave it as is then - anyway, in this patch this hunk is
absent.

>  
>>> 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.
>
>> From 55f0916c794ad25a8bf45566f6d333bea956e0d4 Mon Sep 17 00:00:00 2001
>> From: Vladimir Davydov <vdavydov@parallels.com>
>> 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 <vdavydov@parallels.com>
> 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
>
>> ---
>>  include/linux/slab.h |    9 ++-
>>  mm/memcontrol.c      |   12 +---
>>  mm/slab_common.c     |  174 +++++++++++++++++++++++++++-----------------------
>>  3 files changed, 101 insertions(+), 94 deletions(-)
>>
>> diff --git a/include/linux/slab.h b/include/linux/slab.h
>> index 9260abdd67df..e8c95d0bb879 100644
>> --- a/include/linux/slab.h
>> +++ b/include/linux/slab.h
>> @@ -113,11 +113,10 @@ void __init kmem_cache_init(void);
>>  int slab_is_available(void);
>>  
>>  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.

Oh, just could not pass this by. I guess I'll do it in a separate patch
then.

>
> [...]
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index 11857abf7057..6bee919ece80 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
> [...]
>> +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);
>> +
>> +	/*
>> +	 * 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.

> 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.

Thanks.

>
>> +	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)
>>  {
>> -- 
>> 1.7.10.4
>>
>

Re: [PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[Vladimir Davydov]

On 02/06/2014 08:41 PM, Michal Hocko wrote:
> On Tue 04-02-14 23:19:24, Vladimir Davydov wrote:
>> On 02/04/2014 08:03 PM, Michal Hocko wrote:
>>> On Mon 03-02-14 19:54:38, Vladimir Davydov wrote:
>>>> 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?
>> May be, something like the patch attached?
>>
>>>  
>>>> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
>>>> ---
>>>>  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(-)
>>>>
>>>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>>>> index 84e4801fc36c..de79a9617e09 100644
>>>> --- a/include/linux/memcontrol.h
>>>> +++ b/include/linux/memcontrol.h
>>>> @@ -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.
>> 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.

All right, I'll leave it as is then - anyway, in this patch this hunk is
absent.

>  
>>> 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.
>
>> From 55f0916c794ad25a8bf45566f6d333bea956e0d4 Mon Sep 17 00:00:00 2001
>> From: Vladimir Davydov <vdavydov@parallels.com>
>> 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 <vdavydov@parallels.com>
> 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
>
>> ---
>>  include/linux/slab.h |    9 ++-
>>  mm/memcontrol.c      |   12 +---
>>  mm/slab_common.c     |  174 +++++++++++++++++++++++++++-----------------------
>>  3 files changed, 101 insertions(+), 94 deletions(-)
>>
>> diff --git a/include/linux/slab.h b/include/linux/slab.h
>> index 9260abdd67df..e8c95d0bb879 100644
>> --- a/include/linux/slab.h
>> +++ b/include/linux/slab.h
>> @@ -113,11 +113,10 @@ void __init kmem_cache_init(void);
>>  int slab_is_available(void);
>>  
>>  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.

Oh, just could not pass this by. I guess I'll do it in a separate patch
then.

>
> [...]
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index 11857abf7057..6bee919ece80 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
> [...]
>> +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);
>> +
>> +	/*
>> +	 * 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.

> 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.

Thanks.

>
>> +	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)
>>  {
>> -- 
>> 1.7.10.4
>>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[Michal Hocko]

On Thu 06-02-14 21:12:51, Vladimir Davydov wrote:
> On 02/06/2014 08:41 PM, Michal Hocko wrote:
[...]
> >> +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);
> >> +
> >> +	/*
> >> +	 * 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.

Why not void?

> > 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.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[Michal Hocko]

On Thu 06-02-14 21:12:51, Vladimir Davydov wrote:
> On 02/06/2014 08:41 PM, Michal Hocko wrote:
[...]
> >> +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);
> >> +
> >> +	/*
> >> +	 * 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.

Why not void?

> > 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.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[Vladimir Davydov]

On 02/06/2014 10:17 PM, Michal Hocko wrote:
> On Thu 06-02-14 21:12:51, Vladimir Davydov wrote:
>> On 02/06/2014 08:41 PM, Michal Hocko wrote:
> [...]
>>>> +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);
>>>> +
>>>> +	/*
>>>> +	 * 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.
> Why not void?

Yeah, better to make it void for now, just to keep it clean. I guess if
one day we need an error code there (for accounting of error reporting),
we'll add it then, but currently there is no point in that.

>
>>> 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.

Hmm, an alert in dmesg first time kmem_cache_create_memcg() fails for a
particular memcg, just to draw attention, plus accounting of total
number of failures for each memcg so that admin could check if it's a
real problem... Sounds reasonable to me. I guess I'll handle it in a
separate patch a bit later.

Thanks.

Re: [PATCH v2 3/7] memcg, slab: separate memcg vs root cache creation paths

[Vladimir Davydov]

On 02/06/2014 10:17 PM, Michal Hocko wrote:
> On Thu 06-02-14 21:12:51, Vladimir Davydov wrote:
>> On 02/06/2014 08:41 PM, Michal Hocko wrote:
> [...]
>>>> +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);
>>>> +
>>>> +	/*
>>>> +	 * 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.
> Why not void?

Yeah, better to make it void for now, just to keep it clean. I guess if
one day we need an error code there (for accounting of error reporting),
we'll add it then, but currently there is no point in that.

>
>>> 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.

Hmm, an alert in dmesg first time kmem_cache_create_memcg() fails for a
particular memcg, just to draw attention, plus accounting of total
number of failures for each memcg so that admin could check if it's a
real problem... Sounds reasonable to me. I guess I'll handle it in a
separate patch a bit later.

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>