[PATCH 1/2] memcg: move memcg_{alloc,free}_cache_params to slab_common.c

[PATCH 1/2] memcg: move memcg_{alloc,free}_cache_params to slab_common.c

[Vladimir Davydov]

The only reason why they live in memcontrol.c is that we get/put css
reference to the owner memory cgroup in them. However, we can do that in
memcg_{un,}register_cache.

So let's move them to slab_common.c and make them static.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Christoph Lameter <cl@linux.com>
---
 include/linux/memcontrol.h |   14 --------------
 mm/memcontrol.c            |   41 ++++-------------------------------------
 mm/slab_common.c           |   44 +++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 47 insertions(+), 52 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index e0752d204d9e..4d17242eeff7 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -440,10 +440,6 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order);
 
 int memcg_cache_id(struct mem_cgroup *memcg);
 
-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);
-
 int memcg_update_cache_size(struct kmem_cache *s, int num_groups);
 void memcg_update_array_size(int num_groups);
 
@@ -574,16 +570,6 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
 	return -1;
 }
 
-static inline int memcg_alloc_cache_params(struct mem_cgroup *memcg,
-		struct kmem_cache *s, struct kmem_cache *root_cache)
-{
-	return 0;
-}
-
-static inline void memcg_free_cache_params(struct kmem_cache *s)
-{
-}
-
 static inline struct kmem_cache *
 memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
 {
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 085dc6d2f876..b6bbb1e3e2ab 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2970,43 +2970,6 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
 	return 0;
 }
 
-int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
-			     struct kmem_cache *root_cache)
-{
-	size_t size;
-
-	if (!memcg_kmem_enabled())
-		return 0;
-
-	if (!memcg) {
-		size = offsetof(struct memcg_cache_params, memcg_caches);
-		size += memcg_limited_groups_array_size * sizeof(void *);
-	} else
-		size = sizeof(struct memcg_cache_params);
-
-	s->memcg_params = kzalloc(size, GFP_KERNEL);
-	if (!s->memcg_params)
-		return -ENOMEM;
-
-	if (memcg) {
-		s->memcg_params->memcg = memcg;
-		s->memcg_params->root_cache = root_cache;
-		css_get(&memcg->css);
-	} else
-		s->memcg_params->is_root_cache = true;
-
-	return 0;
-}
-
-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);
-}
-
 static void memcg_register_cache(struct mem_cgroup *memcg,
 				 struct kmem_cache *root_cache)
 {
@@ -3037,6 +3000,7 @@ static void memcg_register_cache(struct mem_cgroup *memcg,
 	if (!cachep)
 		return;
 
+	css_get(&memcg->css);
 	list_add(&cachep->memcg_params->list, &memcg->memcg_slab_caches);
 
 	/*
@@ -3070,6 +3034,9 @@ static void memcg_unregister_cache(struct kmem_cache *cachep)
 	list_del(&cachep->memcg_params->list);
 
 	kmem_cache_destroy(cachep);
+
+	/* drop the reference taken in memcg_register_cache */
+	css_put(&memcg->css);
 }
 
 /*
diff --git a/mm/slab_common.c b/mm/slab_common.c
index d7d8ffd0c306..9c29ba792368 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -88,6 +88,38 @@ static inline int kmem_cache_sanity_check(const char *name, size_t size)
 #endif
 
 #ifdef CONFIG_MEMCG_KMEM
+static int memcg_alloc_cache_params(struct mem_cgroup *memcg,
+		struct kmem_cache *s, struct kmem_cache *root_cache)
+{
+	size_t size;
+
+	if (!memcg_kmem_enabled())
+		return 0;
+
+	if (!memcg) {
+		size = offsetof(struct memcg_cache_params, memcg_caches);
+		size += memcg_limited_groups_array_size * sizeof(void *);
+	} else
+		size = sizeof(struct memcg_cache_params);
+
+	s->memcg_params = kzalloc(size, GFP_KERNEL);
+	if (!s->memcg_params)
+		return -ENOMEM;
+
+	if (memcg) {
+		s->memcg_params->memcg = memcg;
+		s->memcg_params->root_cache = root_cache;
+	} else
+		s->memcg_params->is_root_cache = true;
+
+	return 0;
+}
+
+static void memcg_free_cache_params(struct kmem_cache *s)
+{
+	kfree(s->memcg_params);
+}
+
 int memcg_update_all_caches(int num_memcgs)
 {
 	struct kmem_cache *s;
@@ -113,7 +145,17 @@ out:
 	mutex_unlock(&slab_mutex);
 	return ret;
 }
-#endif
+#else
+static inline int memcg_alloc_cache_params(struct mem_cgroup *memcg,
+		struct kmem_cache *s, struct kmem_cache *root_cache)
+{
+	return 0;
+}
+
+static inline void memcg_free_cache_params(struct kmem_cache *s)
+{
+}
+#endif /* CONFIG_MEMCG_KMEM */
 
 /*
  * Figure out what the alignment of the objects will be given a set of
-- 
1.7.10.4

[PATCH 1/2] memcg: move memcg_{alloc,free}_cache_params to slab_common.c

[Vladimir Davydov]

The only reason why they live in memcontrol.c is that we get/put css
reference to the owner memory cgroup in them. However, we can do that in
memcg_{un,}register_cache.

So let's move them to slab_common.c and make them static.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Christoph Lameter <cl@linux.com>
---
 include/linux/memcontrol.h |   14 --------------
 mm/memcontrol.c            |   41 ++++-------------------------------------
 mm/slab_common.c           |   44 +++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 47 insertions(+), 52 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index e0752d204d9e..4d17242eeff7 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -440,10 +440,6 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order);
 
 int memcg_cache_id(struct mem_cgroup *memcg);
 
-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);
-
 int memcg_update_cache_size(struct kmem_cache *s, int num_groups);
 void memcg_update_array_size(int num_groups);
 
@@ -574,16 +570,6 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
 	return -1;
 }
 
-static inline int memcg_alloc_cache_params(struct mem_cgroup *memcg,
-		struct kmem_cache *s, struct kmem_cache *root_cache)
-{
-	return 0;
-}
-
-static inline void memcg_free_cache_params(struct kmem_cache *s)
-{
-}
-
 static inline struct kmem_cache *
 memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
 {
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 085dc6d2f876..b6bbb1e3e2ab 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2970,43 +2970,6 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
 	return 0;
 }
 
-int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
-			     struct kmem_cache *root_cache)
-{
-	size_t size;
-
-	if (!memcg_kmem_enabled())
-		return 0;
-
-	if (!memcg) {
-		size = offsetof(struct memcg_cache_params, memcg_caches);
-		size += memcg_limited_groups_array_size * sizeof(void *);
-	} else
-		size = sizeof(struct memcg_cache_params);
-
-	s->memcg_params = kzalloc(size, GFP_KERNEL);
-	if (!s->memcg_params)
-		return -ENOMEM;
-
-	if (memcg) {
-		s->memcg_params->memcg = memcg;
-		s->memcg_params->root_cache = root_cache;
-		css_get(&memcg->css);
-	} else
-		s->memcg_params->is_root_cache = true;
-
-	return 0;
-}
-
-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);
-}
-
 static void memcg_register_cache(struct mem_cgroup *memcg,
 				 struct kmem_cache *root_cache)
 {
@@ -3037,6 +3000,7 @@ static void memcg_register_cache(struct mem_cgroup *memcg,
 	if (!cachep)
 		return;
 
+	css_get(&memcg->css);
 	list_add(&cachep->memcg_params->list, &memcg->memcg_slab_caches);
 
 	/*
@@ -3070,6 +3034,9 @@ static void memcg_unregister_cache(struct kmem_cache *cachep)
 	list_del(&cachep->memcg_params->list);
 
 	kmem_cache_destroy(cachep);
+
+	/* drop the reference taken in memcg_register_cache */
+	css_put(&memcg->css);
 }
 
 /*
diff --git a/mm/slab_common.c b/mm/slab_common.c
index d7d8ffd0c306..9c29ba792368 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -88,6 +88,38 @@ static inline int kmem_cache_sanity_check(const char *name, size_t size)
 #endif
 
 #ifdef CONFIG_MEMCG_KMEM
+static int memcg_alloc_cache_params(struct mem_cgroup *memcg,
+		struct kmem_cache *s, struct kmem_cache *root_cache)
+{
+	size_t size;
+
+	if (!memcg_kmem_enabled())
+		return 0;
+
+	if (!memcg) {
+		size = offsetof(struct memcg_cache_params, memcg_caches);
+		size += memcg_limited_groups_array_size * sizeof(void *);
+	} else
+		size = sizeof(struct memcg_cache_params);
+
+	s->memcg_params = kzalloc(size, GFP_KERNEL);
+	if (!s->memcg_params)
+		return -ENOMEM;
+
+	if (memcg) {
+		s->memcg_params->memcg = memcg;
+		s->memcg_params->root_cache = root_cache;
+	} else
+		s->memcg_params->is_root_cache = true;
+
+	return 0;
+}
+
+static void memcg_free_cache_params(struct kmem_cache *s)
+{
+	kfree(s->memcg_params);
+}
+
 int memcg_update_all_caches(int num_memcgs)
 {
 	struct kmem_cache *s;
@@ -113,7 +145,17 @@ out:
 	mutex_unlock(&slab_mutex);
 	return ret;
 }
-#endif
+#else
+static inline int memcg_alloc_cache_params(struct mem_cgroup *memcg,
+		struct kmem_cache *s, struct kmem_cache *root_cache)
+{
+	return 0;
+}
+
+static inline void memcg_free_cache_params(struct kmem_cache *s)
+{
+}
+#endif /* CONFIG_MEMCG_KMEM */
 
 /*
  * Figure out what the alignment of the objects will be given a set of
-- 
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 2/2] memcg: move memcg_update_cache_size to slab_common.c

[Vladimir Davydov]

The only reason why this function lives in memcontrol.c is that it
depends on memcg_caches_array_size. However, we can pass the new array
size immediately to it instead of new_id+1 so that it will be free of
any memcontrol.c dependencies.

So let's move this function to slab_common.c and make it static.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Christoph Lameter <cl@linux.com>
---
 include/linux/memcontrol.h |    1 -
 mm/memcontrol.c            |  114 ++++++++++++++------------------------------
 mm/slab_common.c           |   30 +++++++++++-
 3 files changed, 65 insertions(+), 80 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 4d17242eeff7..19df5d857411 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -440,7 +440,6 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order);
 
 int memcg_cache_id(struct mem_cgroup *memcg);
 
-int memcg_update_cache_size(struct kmem_cache *s, int num_groups);
 void memcg_update_array_size(int num_groups);
 
 struct kmem_cache *
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b6bbb1e3e2ab..9431024e490c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -646,11 +646,13 @@ int memcg_limited_groups_array_size;
 struct static_key memcg_kmem_enabled_key;
 EXPORT_SYMBOL(memcg_kmem_enabled_key);
 
+static void memcg_free_cache_id(int id);
+
 static void disarm_kmem_keys(struct mem_cgroup *memcg)
 {
 	if (memcg_kmem_is_active(memcg)) {
 		static_key_slow_dec(&memcg_kmem_enabled_key);
-		ida_simple_remove(&kmem_limited_groups, memcg->kmemcg_id);
+		memcg_free_cache_id(memcg->kmemcg_id);
 	}
 	/*
 	 * This check can't live in kmem destruction function,
@@ -2892,19 +2894,45 @@ int memcg_cache_id(struct mem_cgroup *memcg)
 	return memcg ? memcg->kmemcg_id : -1;
 }
 
-static size_t memcg_caches_array_size(int num_groups)
+static int memcg_alloc_cache_id(void)
 {
-	ssize_t size;
-	if (num_groups <= 0)
-		return 0;
+	int id, size;
+	int err;
+
+	id = ida_simple_get(&kmem_limited_groups,
+			    0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL);
+	if (id < 0)
+		return id;
 
-	size = 2 * num_groups;
+	if (id < memcg_limited_groups_array_size)
+		return id;
+
+	/*
+	 * There's no space for the new id in memcg_caches arrays,
+	 * so we have to grow them.
+	 */
+
+	size = 2 * (id + 1);
 	if (size < MEMCG_CACHES_MIN_SIZE)
 		size = MEMCG_CACHES_MIN_SIZE;
 	else if (size > MEMCG_CACHES_MAX_SIZE)
 		size = MEMCG_CACHES_MAX_SIZE;
 
-	return size;
+	mutex_lock(&memcg_slab_mutex);
+	err = memcg_update_all_caches(size);
+	mutex_unlock(&memcg_slab_mutex);
+
+	if (err) {
+		ida_simple_remove(&kmem_limited_groups, id);
+		return err;
+	}
+	return id;
+
+}
+
+static void memcg_free_cache_id(int id)
+{
+	ida_simple_remove(&kmem_limited_groups, id);
 }
 
 /*
@@ -2914,60 +2942,7 @@ static size_t memcg_caches_array_size(int num_groups)
  */
 void memcg_update_array_size(int num)
 {
-	if (num > memcg_limited_groups_array_size)
-		memcg_limited_groups_array_size = memcg_caches_array_size(num);
-}
-
-int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
-{
-	struct memcg_cache_params *cur_params = s->memcg_params;
-
-	VM_BUG_ON(!is_root_cache(s));
-
-	if (num_groups > memcg_limited_groups_array_size) {
-		int i;
-		struct memcg_cache_params *new_params;
-		ssize_t size = memcg_caches_array_size(num_groups);
-
-		size *= sizeof(void *);
-		size += offsetof(struct memcg_cache_params, memcg_caches);
-
-		new_params = kzalloc(size, GFP_KERNEL);
-		if (!new_params)
-			return -ENOMEM;
-
-		new_params->is_root_cache = true;
-
-		/*
-		 * There is the chance it will be bigger than
-		 * memcg_limited_groups_array_size, if we failed an allocation
-		 * in a cache, in which case all caches updated before it, will
-		 * have a bigger array.
-		 *
-		 * But if that is the case, the data after
-		 * memcg_limited_groups_array_size is certainly unused
-		 */
-		for (i = 0; i < memcg_limited_groups_array_size; i++) {
-			if (!cur_params->memcg_caches[i])
-				continue;
-			new_params->memcg_caches[i] =
-						cur_params->memcg_caches[i];
-		}
-
-		/*
-		 * Ideally, we would wait until all caches succeed, and only
-		 * then free the old one. But this is not worth the extra
-		 * pointer per-cache we'd have to have for this.
-		 *
-		 * It is not a big deal if some caches are left with a size
-		 * bigger than the others. And all updates will reset this
-		 * anyway.
-		 */
-		rcu_assign_pointer(s->memcg_params, new_params);
-		if (cur_params)
-			kfree_rcu(cur_params, rcu_head);
-	}
-	return 0;
+	memcg_limited_groups_array_size = num;
 }
 
 static void memcg_register_cache(struct mem_cgroup *memcg,
@@ -4167,23 +4142,12 @@ static int __memcg_activate_kmem(struct mem_cgroup *memcg,
 	if (err)
 		goto out;
 
-	memcg_id = ida_simple_get(&kmem_limited_groups,
-				  0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL);
+	memcg_id = memcg_alloc_cache_id();
 	if (memcg_id < 0) {
 		err = memcg_id;
 		goto out;
 	}
 
-	/*
-	 * Make sure we have enough space for this cgroup in each root cache's
-	 * memcg_params.
-	 */
-	mutex_lock(&memcg_slab_mutex);
-	err = memcg_update_all_caches(memcg_id + 1);
-	mutex_unlock(&memcg_slab_mutex);
-	if (err)
-		goto out_rmid;
-
 	memcg->kmemcg_id = memcg_id;
 	INIT_LIST_HEAD(&memcg->memcg_slab_caches);
 
@@ -4204,10 +4168,6 @@ static int __memcg_activate_kmem(struct mem_cgroup *memcg,
 out:
 	memcg_resume_kmem_account();
 	return err;
-
-out_rmid:
-	ida_simple_remove(&kmem_limited_groups, memcg_id);
-	goto out;
 }
 
 static int memcg_activate_kmem(struct mem_cgroup *memcg,
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 9c29ba792368..800314e2a075 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -120,6 +120,33 @@ static void memcg_free_cache_params(struct kmem_cache *s)
 	kfree(s->memcg_params);
 }
 
+static int memcg_update_cache_params(struct kmem_cache *s, int num_memcgs)
+{
+	int size;
+	struct memcg_cache_params *new_params, *cur_params;
+
+	BUG_ON(!is_root_cache(s));
+
+	size = offsetof(struct memcg_cache_params, memcg_caches);
+	size += num_memcgs * sizeof(void *);
+
+	new_params = kzalloc(size, GFP_KERNEL);
+	if (!new_params)
+		return -ENOMEM;
+
+	cur_params = s->memcg_params;
+	memcpy(new_params->memcg_caches, cur_params->memcg_caches,
+	       memcg_limited_groups_array_size * sizeof(void *));
+
+	new_params->is_root_cache = true;
+
+	rcu_assign_pointer(s->memcg_params, new_params);
+	if (cur_params)
+		kfree_rcu(cur_params, rcu_head);
+
+	return 0;
+}
+
 int memcg_update_all_caches(int num_memcgs)
 {
 	struct kmem_cache *s;
@@ -130,9 +157,8 @@ int memcg_update_all_caches(int num_memcgs)
 		if (!is_root_cache(s))
 			continue;
 
-		ret = memcg_update_cache_size(s, num_memcgs);
+		ret = memcg_update_cache_params(s, num_memcgs);
 		/*
-		 * See comment in memcontrol.c, memcg_update_cache_size:
 		 * Instead of freeing the memory, we'll just leave the caches
 		 * up to this point in an updated state.
 		 */
-- 
1.7.10.4

[PATCH 2/2] memcg: move memcg_update_cache_size to slab_common.c

[Vladimir Davydov]

The only reason why this function lives in memcontrol.c is that it
depends on memcg_caches_array_size. However, we can pass the new array
size immediately to it instead of new_id+1 so that it will be free of
any memcontrol.c dependencies.

So let's move this function to slab_common.c and make it static.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Christoph Lameter <cl@linux.com>
---
 include/linux/memcontrol.h |    1 -
 mm/memcontrol.c            |  114 ++++++++++++++------------------------------
 mm/slab_common.c           |   30 +++++++++++-
 3 files changed, 65 insertions(+), 80 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 4d17242eeff7..19df5d857411 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -440,7 +440,6 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order);
 
 int memcg_cache_id(struct mem_cgroup *memcg);
 
-int memcg_update_cache_size(struct kmem_cache *s, int num_groups);
 void memcg_update_array_size(int num_groups);
 
 struct kmem_cache *
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b6bbb1e3e2ab..9431024e490c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -646,11 +646,13 @@ int memcg_limited_groups_array_size;
 struct static_key memcg_kmem_enabled_key;
 EXPORT_SYMBOL(memcg_kmem_enabled_key);
 
+static void memcg_free_cache_id(int id);
+
 static void disarm_kmem_keys(struct mem_cgroup *memcg)
 {
 	if (memcg_kmem_is_active(memcg)) {
 		static_key_slow_dec(&memcg_kmem_enabled_key);
-		ida_simple_remove(&kmem_limited_groups, memcg->kmemcg_id);
+		memcg_free_cache_id(memcg->kmemcg_id);
 	}
 	/*
 	 * This check can't live in kmem destruction function,
@@ -2892,19 +2894,45 @@ int memcg_cache_id(struct mem_cgroup *memcg)
 	return memcg ? memcg->kmemcg_id : -1;
 }
 
-static size_t memcg_caches_array_size(int num_groups)
+static int memcg_alloc_cache_id(void)
 {
-	ssize_t size;
-	if (num_groups <= 0)
-		return 0;
+	int id, size;
+	int err;
+
+	id = ida_simple_get(&kmem_limited_groups,
+			    0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL);
+	if (id < 0)
+		return id;
 
-	size = 2 * num_groups;
+	if (id < memcg_limited_groups_array_size)
+		return id;
+
+	/*
+	 * There's no space for the new id in memcg_caches arrays,
+	 * so we have to grow them.
+	 */
+
+	size = 2 * (id + 1);
 	if (size < MEMCG_CACHES_MIN_SIZE)
 		size = MEMCG_CACHES_MIN_SIZE;
 	else if (size > MEMCG_CACHES_MAX_SIZE)
 		size = MEMCG_CACHES_MAX_SIZE;
 
-	return size;
+	mutex_lock(&memcg_slab_mutex);
+	err = memcg_update_all_caches(size);
+	mutex_unlock(&memcg_slab_mutex);
+
+	if (err) {
+		ida_simple_remove(&kmem_limited_groups, id);
+		return err;
+	}
+	return id;
+
+}
+
+static void memcg_free_cache_id(int id)
+{
+	ida_simple_remove(&kmem_limited_groups, id);
 }
 
 /*
@@ -2914,60 +2942,7 @@ static size_t memcg_caches_array_size(int num_groups)
  */
 void memcg_update_array_size(int num)
 {
-	if (num > memcg_limited_groups_array_size)
-		memcg_limited_groups_array_size = memcg_caches_array_size(num);
-}
-
-int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
-{
-	struct memcg_cache_params *cur_params = s->memcg_params;
-
-	VM_BUG_ON(!is_root_cache(s));
-
-	if (num_groups > memcg_limited_groups_array_size) {
-		int i;
-		struct memcg_cache_params *new_params;
-		ssize_t size = memcg_caches_array_size(num_groups);
-
-		size *= sizeof(void *);
-		size += offsetof(struct memcg_cache_params, memcg_caches);
-
-		new_params = kzalloc(size, GFP_KERNEL);
-		if (!new_params)
-			return -ENOMEM;
-
-		new_params->is_root_cache = true;
-
-		/*
-		 * There is the chance it will be bigger than
-		 * memcg_limited_groups_array_size, if we failed an allocation
-		 * in a cache, in which case all caches updated before it, will
-		 * have a bigger array.
-		 *
-		 * But if that is the case, the data after
-		 * memcg_limited_groups_array_size is certainly unused
-		 */
-		for (i = 0; i < memcg_limited_groups_array_size; i++) {
-			if (!cur_params->memcg_caches[i])
-				continue;
-			new_params->memcg_caches[i] =
-						cur_params->memcg_caches[i];
-		}
-
-		/*
-		 * Ideally, we would wait until all caches succeed, and only
-		 * then free the old one. But this is not worth the extra
-		 * pointer per-cache we'd have to have for this.
-		 *
-		 * It is not a big deal if some caches are left with a size
-		 * bigger than the others. And all updates will reset this
-		 * anyway.
-		 */
-		rcu_assign_pointer(s->memcg_params, new_params);
-		if (cur_params)
-			kfree_rcu(cur_params, rcu_head);
-	}
-	return 0;
+	memcg_limited_groups_array_size = num;
 }
 
 static void memcg_register_cache(struct mem_cgroup *memcg,
@@ -4167,23 +4142,12 @@ static int __memcg_activate_kmem(struct mem_cgroup *memcg,
 	if (err)
 		goto out;
 
-	memcg_id = ida_simple_get(&kmem_limited_groups,
-				  0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL);
+	memcg_id = memcg_alloc_cache_id();
 	if (memcg_id < 0) {
 		err = memcg_id;
 		goto out;
 	}
 
-	/*
-	 * Make sure we have enough space for this cgroup in each root cache's
-	 * memcg_params.
-	 */
-	mutex_lock(&memcg_slab_mutex);
-	err = memcg_update_all_caches(memcg_id + 1);
-	mutex_unlock(&memcg_slab_mutex);
-	if (err)
-		goto out_rmid;
-
 	memcg->kmemcg_id = memcg_id;
 	INIT_LIST_HEAD(&memcg->memcg_slab_caches);
 
@@ -4204,10 +4168,6 @@ static int __memcg_activate_kmem(struct mem_cgroup *memcg,
 out:
 	memcg_resume_kmem_account();
 	return err;
-
-out_rmid:
-	ida_simple_remove(&kmem_limited_groups, memcg_id);
-	goto out;
 }
 
 static int memcg_activate_kmem(struct mem_cgroup *memcg,
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 9c29ba792368..800314e2a075 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -120,6 +120,33 @@ static void memcg_free_cache_params(struct kmem_cache *s)
 	kfree(s->memcg_params);
 }
 
+static int memcg_update_cache_params(struct kmem_cache *s, int num_memcgs)
+{
+	int size;
+	struct memcg_cache_params *new_params, *cur_params;
+
+	BUG_ON(!is_root_cache(s));
+
+	size = offsetof(struct memcg_cache_params, memcg_caches);
+	size += num_memcgs * sizeof(void *);
+
+	new_params = kzalloc(size, GFP_KERNEL);
+	if (!new_params)
+		return -ENOMEM;
+
+	cur_params = s->memcg_params;
+	memcpy(new_params->memcg_caches, cur_params->memcg_caches,
+	       memcg_limited_groups_array_size * sizeof(void *));
+
+	new_params->is_root_cache = true;
+
+	rcu_assign_pointer(s->memcg_params, new_params);
+	if (cur_params)
+		kfree_rcu(cur_params, rcu_head);
+
+	return 0;
+}
+
 int memcg_update_all_caches(int num_memcgs)
 {
 	struct kmem_cache *s;
@@ -130,9 +157,8 @@ int memcg_update_all_caches(int num_memcgs)
 		if (!is_root_cache(s))
 			continue;
 
-		ret = memcg_update_cache_size(s, num_memcgs);
+		ret = memcg_update_cache_params(s, num_memcgs);
 		/*
-		 * See comment in memcontrol.c, memcg_update_cache_size:
 		 * Instead of freeing the memory, we'll just leave the caches
 		 * up to this point in an updated state.
 		 */
-- 
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 1/2] memcg: move memcg_{alloc,free}_cache_params to slab_common.c

[Michal Hocko]

On Thu 18-09-14 19:50:19, Vladimir Davydov wrote:
> The only reason why they live in memcontrol.c is that we get/put css
> reference to the owner memory cgroup in them. However, we can do that in
> memcg_{un,}register_cache.
> 
> So let's move them to slab_common.c and make them static.

Why is it better?

> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.cz>
> Cc: Christoph Lameter <cl@linux.com>
> ---
>  include/linux/memcontrol.h |   14 --------------
>  mm/memcontrol.c            |   41 ++++-------------------------------------
>  mm/slab_common.c           |   44 +++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 47 insertions(+), 52 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index e0752d204d9e..4d17242eeff7 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -440,10 +440,6 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order);
>  
>  int memcg_cache_id(struct mem_cgroup *memcg);
>  
> -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);
> -
>  int memcg_update_cache_size(struct kmem_cache *s, int num_groups);
>  void memcg_update_array_size(int num_groups);
>  
> @@ -574,16 +570,6 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
>  	return -1;
>  }
>  
> -static inline int memcg_alloc_cache_params(struct mem_cgroup *memcg,
> -		struct kmem_cache *s, struct kmem_cache *root_cache)
> -{
> -	return 0;
> -}
> -
> -static inline void memcg_free_cache_params(struct kmem_cache *s)
> -{
> -}
> -
>  static inline struct kmem_cache *
>  memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
>  {
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 085dc6d2f876..b6bbb1e3e2ab 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2970,43 +2970,6 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
>  	return 0;
>  }
>  
> -int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
> -			     struct kmem_cache *root_cache)
> -{
> -	size_t size;
> -
> -	if (!memcg_kmem_enabled())
> -		return 0;
> -
> -	if (!memcg) {
> -		size = offsetof(struct memcg_cache_params, memcg_caches);
> -		size += memcg_limited_groups_array_size * sizeof(void *);
> -	} else
> -		size = sizeof(struct memcg_cache_params);
> -
> -	s->memcg_params = kzalloc(size, GFP_KERNEL);
> -	if (!s->memcg_params)
> -		return -ENOMEM;
> -
> -	if (memcg) {
> -		s->memcg_params->memcg = memcg;
> -		s->memcg_params->root_cache = root_cache;
> -		css_get(&memcg->css);
> -	} else
> -		s->memcg_params->is_root_cache = true;
> -
> -	return 0;
> -}
> -
> -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);
> -}
> -
>  static void memcg_register_cache(struct mem_cgroup *memcg,
>  				 struct kmem_cache *root_cache)
>  {
> @@ -3037,6 +3000,7 @@ static void memcg_register_cache(struct mem_cgroup *memcg,
>  	if (!cachep)
>  		return;
>  
> +	css_get(&memcg->css);
>  	list_add(&cachep->memcg_params->list, &memcg->memcg_slab_caches);
>  
>  	/*
> @@ -3070,6 +3034,9 @@ static void memcg_unregister_cache(struct kmem_cache *cachep)
>  	list_del(&cachep->memcg_params->list);
>  
>  	kmem_cache_destroy(cachep);
> +
> +	/* drop the reference taken in memcg_register_cache */
> +	css_put(&memcg->css);
>  }
>  
>  /*
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index d7d8ffd0c306..9c29ba792368 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -88,6 +88,38 @@ static inline int kmem_cache_sanity_check(const char *name, size_t size)
>  #endif
>  
>  #ifdef CONFIG_MEMCG_KMEM
> +static int memcg_alloc_cache_params(struct mem_cgroup *memcg,
> +		struct kmem_cache *s, struct kmem_cache *root_cache)
> +{
> +	size_t size;
> +
> +	if (!memcg_kmem_enabled())
> +		return 0;
> +
> +	if (!memcg) {
> +		size = offsetof(struct memcg_cache_params, memcg_caches);
> +		size += memcg_limited_groups_array_size * sizeof(void *);
> +	} else
> +		size = sizeof(struct memcg_cache_params);
> +
> +	s->memcg_params = kzalloc(size, GFP_KERNEL);
> +	if (!s->memcg_params)
> +		return -ENOMEM;
> +
> +	if (memcg) {
> +		s->memcg_params->memcg = memcg;
> +		s->memcg_params->root_cache = root_cache;
> +	} else
> +		s->memcg_params->is_root_cache = true;
> +
> +	return 0;
> +}
> +
> +static void memcg_free_cache_params(struct kmem_cache *s)
> +{
> +	kfree(s->memcg_params);
> +}
> +
>  int memcg_update_all_caches(int num_memcgs)
>  {
>  	struct kmem_cache *s;
> @@ -113,7 +145,17 @@ out:
>  	mutex_unlock(&slab_mutex);
>  	return ret;
>  }
> -#endif
> +#else
> +static inline int memcg_alloc_cache_params(struct mem_cgroup *memcg,
> +		struct kmem_cache *s, struct kmem_cache *root_cache)
> +{
> +	return 0;
> +}
> +
> +static inline void memcg_free_cache_params(struct kmem_cache *s)
> +{
> +}
> +#endif /* CONFIG_MEMCG_KMEM */
>  
>  /*
>   * Figure out what the alignment of the objects will be given a set of
> -- 
> 1.7.10.4
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] memcg: move memcg_{alloc,free}_cache_params to slab_common.c

[Michal Hocko]

On Thu 18-09-14 19:50:19, Vladimir Davydov wrote:
> The only reason why they live in memcontrol.c is that we get/put css
> reference to the owner memory cgroup in them. However, we can do that in
> memcg_{un,}register_cache.
> 
> So let's move them to slab_common.c and make them static.

Why is it better?

> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.cz>
> Cc: Christoph Lameter <cl@linux.com>
> ---
>  include/linux/memcontrol.h |   14 --------------
>  mm/memcontrol.c            |   41 ++++-------------------------------------
>  mm/slab_common.c           |   44 +++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 47 insertions(+), 52 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index e0752d204d9e..4d17242eeff7 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -440,10 +440,6 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order);
>  
>  int memcg_cache_id(struct mem_cgroup *memcg);
>  
> -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);
> -
>  int memcg_update_cache_size(struct kmem_cache *s, int num_groups);
>  void memcg_update_array_size(int num_groups);
>  
> @@ -574,16 +570,6 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
>  	return -1;
>  }
>  
> -static inline int memcg_alloc_cache_params(struct mem_cgroup *memcg,
> -		struct kmem_cache *s, struct kmem_cache *root_cache)
> -{
> -	return 0;
> -}
> -
> -static inline void memcg_free_cache_params(struct kmem_cache *s)
> -{
> -}
> -
>  static inline struct kmem_cache *
>  memcg_kmem_get_cache(struct kmem_cache *cachep, gfp_t gfp)
>  {
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 085dc6d2f876..b6bbb1e3e2ab 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2970,43 +2970,6 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
>  	return 0;
>  }
>  
> -int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
> -			     struct kmem_cache *root_cache)
> -{
> -	size_t size;
> -
> -	if (!memcg_kmem_enabled())
> -		return 0;
> -
> -	if (!memcg) {
> -		size = offsetof(struct memcg_cache_params, memcg_caches);
> -		size += memcg_limited_groups_array_size * sizeof(void *);
> -	} else
> -		size = sizeof(struct memcg_cache_params);
> -
> -	s->memcg_params = kzalloc(size, GFP_KERNEL);
> -	if (!s->memcg_params)
> -		return -ENOMEM;
> -
> -	if (memcg) {
> -		s->memcg_params->memcg = memcg;
> -		s->memcg_params->root_cache = root_cache;
> -		css_get(&memcg->css);
> -	} else
> -		s->memcg_params->is_root_cache = true;
> -
> -	return 0;
> -}
> -
> -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);
> -}
> -
>  static void memcg_register_cache(struct mem_cgroup *memcg,
>  				 struct kmem_cache *root_cache)
>  {
> @@ -3037,6 +3000,7 @@ static void memcg_register_cache(struct mem_cgroup *memcg,
>  	if (!cachep)
>  		return;
>  
> +	css_get(&memcg->css);
>  	list_add(&cachep->memcg_params->list, &memcg->memcg_slab_caches);
>  
>  	/*
> @@ -3070,6 +3034,9 @@ static void memcg_unregister_cache(struct kmem_cache *cachep)
>  	list_del(&cachep->memcg_params->list);
>  
>  	kmem_cache_destroy(cachep);
> +
> +	/* drop the reference taken in memcg_register_cache */
> +	css_put(&memcg->css);
>  }
>  
>  /*
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index d7d8ffd0c306..9c29ba792368 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -88,6 +88,38 @@ static inline int kmem_cache_sanity_check(const char *name, size_t size)
>  #endif
>  
>  #ifdef CONFIG_MEMCG_KMEM
> +static int memcg_alloc_cache_params(struct mem_cgroup *memcg,
> +		struct kmem_cache *s, struct kmem_cache *root_cache)
> +{
> +	size_t size;
> +
> +	if (!memcg_kmem_enabled())
> +		return 0;
> +
> +	if (!memcg) {
> +		size = offsetof(struct memcg_cache_params, memcg_caches);
> +		size += memcg_limited_groups_array_size * sizeof(void *);
> +	} else
> +		size = sizeof(struct memcg_cache_params);
> +
> +	s->memcg_params = kzalloc(size, GFP_KERNEL);
> +	if (!s->memcg_params)
> +		return -ENOMEM;
> +
> +	if (memcg) {
> +		s->memcg_params->memcg = memcg;
> +		s->memcg_params->root_cache = root_cache;
> +	} else
> +		s->memcg_params->is_root_cache = true;
> +
> +	return 0;
> +}
> +
> +static void memcg_free_cache_params(struct kmem_cache *s)
> +{
> +	kfree(s->memcg_params);
> +}
> +
>  int memcg_update_all_caches(int num_memcgs)
>  {
>  	struct kmem_cache *s;
> @@ -113,7 +145,17 @@ out:
>  	mutex_unlock(&slab_mutex);
>  	return ret;
>  }
> -#endif
> +#else
> +static inline int memcg_alloc_cache_params(struct mem_cgroup *memcg,
> +		struct kmem_cache *s, struct kmem_cache *root_cache)
> +{
> +	return 0;
> +}
> +
> +static inline void memcg_free_cache_params(struct kmem_cache *s)
> +{
> +}
> +#endif /* CONFIG_MEMCG_KMEM */
>  
>  /*
>   * Figure out what the alignment of the objects will be given a set of
> -- 
> 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 2/2] memcg: move memcg_update_cache_size to slab_common.c

[Michal Hocko]

On Thu 18-09-14 19:50:20, Vladimir Davydov wrote:
> The only reason why this function lives in memcontrol.c is that it
> depends on memcg_caches_array_size. However, we can pass the new array
> size immediately to it instead of new_id+1 so that it will be free of
> any memcontrol.c dependencies.
> 
> So let's move this function to slab_common.c and make it static.

Why?

besides that the patch does more code reshuffling which should be
documented. I have got lost a bit to be honest.

> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.cz>
> Cc: Christoph Lameter <cl@linux.com>
> ---
>  include/linux/memcontrol.h |    1 -
>  mm/memcontrol.c            |  114 ++++++++++++++------------------------------
>  mm/slab_common.c           |   30 +++++++++++-
>  3 files changed, 65 insertions(+), 80 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 4d17242eeff7..19df5d857411 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -440,7 +440,6 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order);
>  
>  int memcg_cache_id(struct mem_cgroup *memcg);
>  
> -int memcg_update_cache_size(struct kmem_cache *s, int num_groups);
>  void memcg_update_array_size(int num_groups);
>  
>  struct kmem_cache *
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b6bbb1e3e2ab..9431024e490c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -646,11 +646,13 @@ int memcg_limited_groups_array_size;
>  struct static_key memcg_kmem_enabled_key;
>  EXPORT_SYMBOL(memcg_kmem_enabled_key);
>  
> +static void memcg_free_cache_id(int id);
> +
>  static void disarm_kmem_keys(struct mem_cgroup *memcg)
>  {
>  	if (memcg_kmem_is_active(memcg)) {
>  		static_key_slow_dec(&memcg_kmem_enabled_key);
> -		ida_simple_remove(&kmem_limited_groups, memcg->kmemcg_id);
> +		memcg_free_cache_id(memcg->kmemcg_id);
>  	}
>  	/*
>  	 * This check can't live in kmem destruction function,
> @@ -2892,19 +2894,45 @@ int memcg_cache_id(struct mem_cgroup *memcg)
>  	return memcg ? memcg->kmemcg_id : -1;
>  }
>  
> -static size_t memcg_caches_array_size(int num_groups)
> +static int memcg_alloc_cache_id(void)
>  {
> -	ssize_t size;
> -	if (num_groups <= 0)
> -		return 0;
> +	int id, size;
> +	int err;
> +
> +	id = ida_simple_get(&kmem_limited_groups,
> +			    0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL);
> +	if (id < 0)
> +		return id;
>  
> -	size = 2 * num_groups;
> +	if (id < memcg_limited_groups_array_size)
> +		return id;
> +
> +	/*
> +	 * There's no space for the new id in memcg_caches arrays,
> +	 * so we have to grow them.
> +	 */
> +
> +	size = 2 * (id + 1);
>  	if (size < MEMCG_CACHES_MIN_SIZE)
>  		size = MEMCG_CACHES_MIN_SIZE;
>  	else if (size > MEMCG_CACHES_MAX_SIZE)
>  		size = MEMCG_CACHES_MAX_SIZE;
>  
> -	return size;
> +	mutex_lock(&memcg_slab_mutex);
> +	err = memcg_update_all_caches(size);
> +	mutex_unlock(&memcg_slab_mutex);
> +
> +	if (err) {
> +		ida_simple_remove(&kmem_limited_groups, id);
> +		return err;
> +	}
> +	return id;
> +
> +}
> +
> +static void memcg_free_cache_id(int id)
> +{
> +	ida_simple_remove(&kmem_limited_groups, id);
>  }
>  
>  /*
> @@ -2914,60 +2942,7 @@ static size_t memcg_caches_array_size(int num_groups)
>   */
>  void memcg_update_array_size(int num)
>  {
> -	if (num > memcg_limited_groups_array_size)
> -		memcg_limited_groups_array_size = memcg_caches_array_size(num);
> -}
> -
> -int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
> -{
> -	struct memcg_cache_params *cur_params = s->memcg_params;
> -
> -	VM_BUG_ON(!is_root_cache(s));
> -
> -	if (num_groups > memcg_limited_groups_array_size) {
> -		int i;
> -		struct memcg_cache_params *new_params;
> -		ssize_t size = memcg_caches_array_size(num_groups);
> -
> -		size *= sizeof(void *);
> -		size += offsetof(struct memcg_cache_params, memcg_caches);
> -
> -		new_params = kzalloc(size, GFP_KERNEL);
> -		if (!new_params)
> -			return -ENOMEM;
> -
> -		new_params->is_root_cache = true;
> -
> -		/*
> -		 * There is the chance it will be bigger than
> -		 * memcg_limited_groups_array_size, if we failed an allocation
> -		 * in a cache, in which case all caches updated before it, will
> -		 * have a bigger array.
> -		 *
> -		 * But if that is the case, the data after
> -		 * memcg_limited_groups_array_size is certainly unused
> -		 */
> -		for (i = 0; i < memcg_limited_groups_array_size; i++) {
> -			if (!cur_params->memcg_caches[i])
> -				continue;
> -			new_params->memcg_caches[i] =
> -						cur_params->memcg_caches[i];
> -		}
> -
> -		/*
> -		 * Ideally, we would wait until all caches succeed, and only
> -		 * then free the old one. But this is not worth the extra
> -		 * pointer per-cache we'd have to have for this.
> -		 *
> -		 * It is not a big deal if some caches are left with a size
> -		 * bigger than the others. And all updates will reset this
> -		 * anyway.
> -		 */
> -		rcu_assign_pointer(s->memcg_params, new_params);
> -		if (cur_params)
> -			kfree_rcu(cur_params, rcu_head);
> -	}
> -	return 0;
> +	memcg_limited_groups_array_size = num;
>  }
>  
>  static void memcg_register_cache(struct mem_cgroup *memcg,
> @@ -4167,23 +4142,12 @@ static int __memcg_activate_kmem(struct mem_cgroup *memcg,
>  	if (err)
>  		goto out;
>  
> -	memcg_id = ida_simple_get(&kmem_limited_groups,
> -				  0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL);
> +	memcg_id = memcg_alloc_cache_id();
>  	if (memcg_id < 0) {
>  		err = memcg_id;
>  		goto out;
>  	}
>  
> -	/*
> -	 * Make sure we have enough space for this cgroup in each root cache's
> -	 * memcg_params.
> -	 */
> -	mutex_lock(&memcg_slab_mutex);
> -	err = memcg_update_all_caches(memcg_id + 1);
> -	mutex_unlock(&memcg_slab_mutex);
> -	if (err)
> -		goto out_rmid;
> -
>  	memcg->kmemcg_id = memcg_id;
>  	INIT_LIST_HEAD(&memcg->memcg_slab_caches);
>  
> @@ -4204,10 +4168,6 @@ static int __memcg_activate_kmem(struct mem_cgroup *memcg,
>  out:
>  	memcg_resume_kmem_account();
>  	return err;
> -
> -out_rmid:
> -	ida_simple_remove(&kmem_limited_groups, memcg_id);
> -	goto out;
>  }
>  
>  static int memcg_activate_kmem(struct mem_cgroup *memcg,
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 9c29ba792368..800314e2a075 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -120,6 +120,33 @@ static void memcg_free_cache_params(struct kmem_cache *s)
>  	kfree(s->memcg_params);
>  }
>  
> +static int memcg_update_cache_params(struct kmem_cache *s, int num_memcgs)
> +{
> +	int size;
> +	struct memcg_cache_params *new_params, *cur_params;
> +
> +	BUG_ON(!is_root_cache(s));
> +
> +	size = offsetof(struct memcg_cache_params, memcg_caches);
> +	size += num_memcgs * sizeof(void *);
> +
> +	new_params = kzalloc(size, GFP_KERNEL);
> +	if (!new_params)
> +		return -ENOMEM;
> +
> +	cur_params = s->memcg_params;
> +	memcpy(new_params->memcg_caches, cur_params->memcg_caches,
> +	       memcg_limited_groups_array_size * sizeof(void *));
> +
> +	new_params->is_root_cache = true;
> +
> +	rcu_assign_pointer(s->memcg_params, new_params);
> +	if (cur_params)
> +		kfree_rcu(cur_params, rcu_head);
> +
> +	return 0;
> +}
> +
>  int memcg_update_all_caches(int num_memcgs)
>  {
>  	struct kmem_cache *s;
> @@ -130,9 +157,8 @@ int memcg_update_all_caches(int num_memcgs)
>  		if (!is_root_cache(s))
>  			continue;
>  
> -		ret = memcg_update_cache_size(s, num_memcgs);
> +		ret = memcg_update_cache_params(s, num_memcgs);
>  		/*
> -		 * See comment in memcontrol.c, memcg_update_cache_size:
>  		 * Instead of freeing the memory, we'll just leave the caches
>  		 * up to this point in an updated state.
>  		 */
> -- 
> 1.7.10.4
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] memcg: move memcg_update_cache_size to slab_common.c

[Michal Hocko]

On Thu 18-09-14 19:50:20, Vladimir Davydov wrote:
> The only reason why this function lives in memcontrol.c is that it
> depends on memcg_caches_array_size. However, we can pass the new array
> size immediately to it instead of new_id+1 so that it will be free of
> any memcontrol.c dependencies.
> 
> So let's move this function to slab_common.c and make it static.

Why?

besides that the patch does more code reshuffling which should be
documented. I have got lost a bit to be honest.

> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.cz>
> Cc: Christoph Lameter <cl@linux.com>
> ---
>  include/linux/memcontrol.h |    1 -
>  mm/memcontrol.c            |  114 ++++++++++++++------------------------------
>  mm/slab_common.c           |   30 +++++++++++-
>  3 files changed, 65 insertions(+), 80 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 4d17242eeff7..19df5d857411 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -440,7 +440,6 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order);
>  
>  int memcg_cache_id(struct mem_cgroup *memcg);
>  
> -int memcg_update_cache_size(struct kmem_cache *s, int num_groups);
>  void memcg_update_array_size(int num_groups);
>  
>  struct kmem_cache *
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b6bbb1e3e2ab..9431024e490c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -646,11 +646,13 @@ int memcg_limited_groups_array_size;
>  struct static_key memcg_kmem_enabled_key;
>  EXPORT_SYMBOL(memcg_kmem_enabled_key);
>  
> +static void memcg_free_cache_id(int id);
> +
>  static void disarm_kmem_keys(struct mem_cgroup *memcg)
>  {
>  	if (memcg_kmem_is_active(memcg)) {
>  		static_key_slow_dec(&memcg_kmem_enabled_key);
> -		ida_simple_remove(&kmem_limited_groups, memcg->kmemcg_id);
> +		memcg_free_cache_id(memcg->kmemcg_id);
>  	}
>  	/*
>  	 * This check can't live in kmem destruction function,
> @@ -2892,19 +2894,45 @@ int memcg_cache_id(struct mem_cgroup *memcg)
>  	return memcg ? memcg->kmemcg_id : -1;
>  }
>  
> -static size_t memcg_caches_array_size(int num_groups)
> +static int memcg_alloc_cache_id(void)
>  {
> -	ssize_t size;
> -	if (num_groups <= 0)
> -		return 0;
> +	int id, size;
> +	int err;
> +
> +	id = ida_simple_get(&kmem_limited_groups,
> +			    0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL);
> +	if (id < 0)
> +		return id;
>  
> -	size = 2 * num_groups;
> +	if (id < memcg_limited_groups_array_size)
> +		return id;
> +
> +	/*
> +	 * There's no space for the new id in memcg_caches arrays,
> +	 * so we have to grow them.
> +	 */
> +
> +	size = 2 * (id + 1);
>  	if (size < MEMCG_CACHES_MIN_SIZE)
>  		size = MEMCG_CACHES_MIN_SIZE;
>  	else if (size > MEMCG_CACHES_MAX_SIZE)
>  		size = MEMCG_CACHES_MAX_SIZE;
>  
> -	return size;
> +	mutex_lock(&memcg_slab_mutex);
> +	err = memcg_update_all_caches(size);
> +	mutex_unlock(&memcg_slab_mutex);
> +
> +	if (err) {
> +		ida_simple_remove(&kmem_limited_groups, id);
> +		return err;
> +	}
> +	return id;
> +
> +}
> +
> +static void memcg_free_cache_id(int id)
> +{
> +	ida_simple_remove(&kmem_limited_groups, id);
>  }
>  
>  /*
> @@ -2914,60 +2942,7 @@ static size_t memcg_caches_array_size(int num_groups)
>   */
>  void memcg_update_array_size(int num)
>  {
> -	if (num > memcg_limited_groups_array_size)
> -		memcg_limited_groups_array_size = memcg_caches_array_size(num);
> -}
> -
> -int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
> -{
> -	struct memcg_cache_params *cur_params = s->memcg_params;
> -
> -	VM_BUG_ON(!is_root_cache(s));
> -
> -	if (num_groups > memcg_limited_groups_array_size) {
> -		int i;
> -		struct memcg_cache_params *new_params;
> -		ssize_t size = memcg_caches_array_size(num_groups);
> -
> -		size *= sizeof(void *);
> -		size += offsetof(struct memcg_cache_params, memcg_caches);
> -
> -		new_params = kzalloc(size, GFP_KERNEL);
> -		if (!new_params)
> -			return -ENOMEM;
> -
> -		new_params->is_root_cache = true;
> -
> -		/*
> -		 * There is the chance it will be bigger than
> -		 * memcg_limited_groups_array_size, if we failed an allocation
> -		 * in a cache, in which case all caches updated before it, will
> -		 * have a bigger array.
> -		 *
> -		 * But if that is the case, the data after
> -		 * memcg_limited_groups_array_size is certainly unused
> -		 */
> -		for (i = 0; i < memcg_limited_groups_array_size; i++) {
> -			if (!cur_params->memcg_caches[i])
> -				continue;
> -			new_params->memcg_caches[i] =
> -						cur_params->memcg_caches[i];
> -		}
> -
> -		/*
> -		 * Ideally, we would wait until all caches succeed, and only
> -		 * then free the old one. But this is not worth the extra
> -		 * pointer per-cache we'd have to have for this.
> -		 *
> -		 * It is not a big deal if some caches are left with a size
> -		 * bigger than the others. And all updates will reset this
> -		 * anyway.
> -		 */
> -		rcu_assign_pointer(s->memcg_params, new_params);
> -		if (cur_params)
> -			kfree_rcu(cur_params, rcu_head);
> -	}
> -	return 0;
> +	memcg_limited_groups_array_size = num;
>  }
>  
>  static void memcg_register_cache(struct mem_cgroup *memcg,
> @@ -4167,23 +4142,12 @@ static int __memcg_activate_kmem(struct mem_cgroup *memcg,
>  	if (err)
>  		goto out;
>  
> -	memcg_id = ida_simple_get(&kmem_limited_groups,
> -				  0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL);
> +	memcg_id = memcg_alloc_cache_id();
>  	if (memcg_id < 0) {
>  		err = memcg_id;
>  		goto out;
>  	}
>  
> -	/*
> -	 * Make sure we have enough space for this cgroup in each root cache's
> -	 * memcg_params.
> -	 */
> -	mutex_lock(&memcg_slab_mutex);
> -	err = memcg_update_all_caches(memcg_id + 1);
> -	mutex_unlock(&memcg_slab_mutex);
> -	if (err)
> -		goto out_rmid;
> -
>  	memcg->kmemcg_id = memcg_id;
>  	INIT_LIST_HEAD(&memcg->memcg_slab_caches);
>  
> @@ -4204,10 +4168,6 @@ static int __memcg_activate_kmem(struct mem_cgroup *memcg,
>  out:
>  	memcg_resume_kmem_account();
>  	return err;
> -
> -out_rmid:
> -	ida_simple_remove(&kmem_limited_groups, memcg_id);
> -	goto out;
>  }
>  
>  static int memcg_activate_kmem(struct mem_cgroup *memcg,
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 9c29ba792368..800314e2a075 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -120,6 +120,33 @@ static void memcg_free_cache_params(struct kmem_cache *s)
>  	kfree(s->memcg_params);
>  }
>  
> +static int memcg_update_cache_params(struct kmem_cache *s, int num_memcgs)
> +{
> +	int size;
> +	struct memcg_cache_params *new_params, *cur_params;
> +
> +	BUG_ON(!is_root_cache(s));
> +
> +	size = offsetof(struct memcg_cache_params, memcg_caches);
> +	size += num_memcgs * sizeof(void *);
> +
> +	new_params = kzalloc(size, GFP_KERNEL);
> +	if (!new_params)
> +		return -ENOMEM;
> +
> +	cur_params = s->memcg_params;
> +	memcpy(new_params->memcg_caches, cur_params->memcg_caches,
> +	       memcg_limited_groups_array_size * sizeof(void *));
> +
> +	new_params->is_root_cache = true;
> +
> +	rcu_assign_pointer(s->memcg_params, new_params);
> +	if (cur_params)
> +		kfree_rcu(cur_params, rcu_head);
> +
> +	return 0;
> +}
> +
>  int memcg_update_all_caches(int num_memcgs)
>  {
>  	struct kmem_cache *s;
> @@ -130,9 +157,8 @@ int memcg_update_all_caches(int num_memcgs)
>  		if (!is_root_cache(s))
>  			continue;
>  
> -		ret = memcg_update_cache_size(s, num_memcgs);
> +		ret = memcg_update_cache_params(s, num_memcgs);
>  		/*
> -		 * See comment in memcontrol.c, memcg_update_cache_size:
>  		 * Instead of freeing the memory, we'll just leave the caches
>  		 * up to this point in an updated state.
>  		 */
> -- 
> 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 1/2] memcg: move memcg_{alloc,free}_cache_params to slab_common.c

[Vladimir Davydov]

On Mon, Sep 22, 2014 at 03:52:45PM +0200, Michal Hocko wrote:
> On Thu 18-09-14 19:50:19, Vladimir Davydov wrote:
> > The only reason why they live in memcontrol.c is that we get/put css
> > reference to the owner memory cgroup in them. However, we can do that in
> > memcg_{un,}register_cache.
> > 
> > So let's move them to slab_common.c and make them static.
> 
> Why is it better?

First, I think that the less public interface functions we have in
memcontrol.h the better. Since the functions I move don't depend on
memcontrol, I think it's worth making them private to slab, especially
taking into account that the arrays are defined on the slab's side too.

Second, the way how per-memcg arrays are updated looks rather awkward:
it proceeds from memcontrol.c (__memcg_activate_kmem) to slab_common.c
(memcg_update_all_caches) and back to memcontrol.c again
(memcg_update_array_size). In the next patch I move the function
relocating the arrays (memcg_update_array_size) to slab_common.c and
therefore get rid this circular call path. I think we should have the
cache allocation stuff in the same place where we have relocation,
because it's easier to follow the code then. So I move arrays alloc/free
functions to slab_common.c too.

The third point isn't obvious. In the "Per memcg slab shrinkers" patch
set, which I sent recently, I have to update per-memcg list_lrus arrays
too. And it's much easier and cleaner to do it in list_lru.c rather than
in memcontrol.c. The current patchset makes cache arrays allocation path
conform that of the upcoming list_lru.

Thanks,
Vladimir

Re: [PATCH 1/2] memcg: move memcg_{alloc,free}_cache_params to slab_common.c

[Vladimir Davydov]

On Mon, Sep 22, 2014 at 03:52:45PM +0200, Michal Hocko wrote:
> On Thu 18-09-14 19:50:19, Vladimir Davydov wrote:
> > The only reason why they live in memcontrol.c is that we get/put css
> > reference to the owner memory cgroup in them. However, we can do that in
> > memcg_{un,}register_cache.
> > 
> > So let's move them to slab_common.c and make them static.
> 
> Why is it better?

First, I think that the less public interface functions we have in
memcontrol.h the better. Since the functions I move don't depend on
memcontrol, I think it's worth making them private to slab, especially
taking into account that the arrays are defined on the slab's side too.

Second, the way how per-memcg arrays are updated looks rather awkward:
it proceeds from memcontrol.c (__memcg_activate_kmem) to slab_common.c
(memcg_update_all_caches) and back to memcontrol.c again
(memcg_update_array_size). In the next patch I move the function
relocating the arrays (memcg_update_array_size) to slab_common.c and
therefore get rid this circular call path. I think we should have the
cache allocation stuff in the same place where we have relocation,
because it's easier to follow the code then. So I move arrays alloc/free
functions to slab_common.c too.

The third point isn't obvious. In the "Per memcg slab shrinkers" patch
set, which I sent recently, I have to update per-memcg list_lrus arrays
too. And it's much easier and cleaner to do it in list_lru.c rather than
in memcontrol.c. The current patchset makes cache arrays allocation path
conform that of the upcoming list_lru.

Thanks,
Vladimir

--
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 2/2] memcg: move memcg_update_cache_size to slab_common.c

[Vladimir Davydov]

On Mon, Sep 22, 2014 at 04:07:34PM +0200, Michal Hocko wrote:
> On Thu 18-09-14 19:50:20, Vladimir Davydov wrote:
> > The only reason why this function lives in memcontrol.c is that it
> > depends on memcg_caches_array_size. However, we can pass the new array
> > size immediately to it instead of new_id+1 so that it will be free of
> > any memcontrol.c dependencies.
> > 
> > So let's move this function to slab_common.c and make it static.
> 
> Why?

Jumping from memcontrol.c to slab_common.c and then back to memcontrol.c
while updating per-memcg caches looks ugly IMO. We can do the update on
the slab's side.

> besides that the patch does more code reshuffling which should be
> documented. I have got lost a bit to be honest.

It just makes it sane :-) Currently we walk over all slab caches each
time new kmemcg is created even if memcg_limited_groups_array_size
doesn't grow and we've actually nothing to do. So it moves cache id
allocation stuff to a separate function (memcg_alloc_cache_id) and
places the check there so that memcg_update_all_caches is only called
when it's really necessary.

I'm sorry if it confuses you. I thought the patch isn't big and rather
easy to understand :-/ Next time will split better.

Thanks,
Vladimir

Re: [PATCH 2/2] memcg: move memcg_update_cache_size to slab_common.c

[Vladimir Davydov]

On Mon, Sep 22, 2014 at 04:07:34PM +0200, Michal Hocko wrote:
> On Thu 18-09-14 19:50:20, Vladimir Davydov wrote:
> > The only reason why this function lives in memcontrol.c is that it
> > depends on memcg_caches_array_size. However, we can pass the new array
> > size immediately to it instead of new_id+1 so that it will be free of
> > any memcontrol.c dependencies.
> > 
> > So let's move this function to slab_common.c and make it static.
> 
> Why?

Jumping from memcontrol.c to slab_common.c and then back to memcontrol.c
while updating per-memcg caches looks ugly IMO. We can do the update on
the slab's side.

> besides that the patch does more code reshuffling which should be
> documented. I have got lost a bit to be honest.

It just makes it sane :-) Currently we walk over all slab caches each
time new kmemcg is created even if memcg_limited_groups_array_size
doesn't grow and we've actually nothing to do. So it moves cache id
allocation stuff to a separate function (memcg_alloc_cache_id) and
places the check there so that memcg_update_all_caches is only called
when it's really necessary.

I'm sorry if it confuses you. I thought the patch isn't big and rather
easy to understand :-/ Next time will split better.

Thanks,
Vladimir

--
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 2/2] memcg: move memcg_update_cache_size to slab_common.c

[Michal Hocko]

On Mon 22-09-14 18:20:35, Vladimir Davydov wrote:
> On Mon, Sep 22, 2014 at 04:07:34PM +0200, Michal Hocko wrote:
> > On Thu 18-09-14 19:50:20, Vladimir Davydov wrote:
> > > The only reason why this function lives in memcontrol.c is that it
> > > depends on memcg_caches_array_size. However, we can pass the new array
> > > size immediately to it instead of new_id+1 so that it will be free of
> > > any memcontrol.c dependencies.
> > > 
> > > So let's move this function to slab_common.c and make it static.
> > 
> > Why?
> 
> Jumping from memcontrol.c to slab_common.c and then back to memcontrol.c
> while updating per-memcg caches looks ugly IMO. We can do the update on
> the slab's side.

Then put this into the patch description. I am always kind of lost when
following all those slab <-> memcg jumps so I definitely do not have
anything against cleaning this up. But please be explicit about your
motivation about the change and put it into the changelog. Things might
be obvious for you when you are deeply familiar with the code but the
poor reviewer has to build up the whole thing from scratch usually.

> > besides that the patch does more code reshuffling which should be
> > documented. I have got lost a bit to be honest.
> 
> It just makes it sane :-) Currently we walk over all slab caches each
> time new kmemcg is created even if memcg_limited_groups_array_size
> doesn't grow and we've actually nothing to do. So it moves cache id
> allocation stuff to a separate function (memcg_alloc_cache_id) and
> places the check there so that memcg_update_all_caches is only called
> when it's really necessary.
> 
> I'm sorry if it confuses you. I thought the patch isn't big and rather
> easy to understand :-/ Next time will split better.

This would be worth a separate patch then and explain all of this.

Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] memcg: move memcg_update_cache_size to slab_common.c

[Michal Hocko]

On Mon 22-09-14 18:20:35, Vladimir Davydov wrote:
> On Mon, Sep 22, 2014 at 04:07:34PM +0200, Michal Hocko wrote:
> > On Thu 18-09-14 19:50:20, Vladimir Davydov wrote:
> > > The only reason why this function lives in memcontrol.c is that it
> > > depends on memcg_caches_array_size. However, we can pass the new array
> > > size immediately to it instead of new_id+1 so that it will be free of
> > > any memcontrol.c dependencies.
> > > 
> > > So let's move this function to slab_common.c and make it static.
> > 
> > Why?
> 
> Jumping from memcontrol.c to slab_common.c and then back to memcontrol.c
> while updating per-memcg caches looks ugly IMO. We can do the update on
> the slab's side.

Then put this into the patch description. I am always kind of lost when
following all those slab <-> memcg jumps so I definitely do not have
anything against cleaning this up. But please be explicit about your
motivation about the change and put it into the changelog. Things might
be obvious for you when you are deeply familiar with the code but the
poor reviewer has to build up the whole thing from scratch usually.

> > besides that the patch does more code reshuffling which should be
> > documented. I have got lost a bit to be honest.
> 
> It just makes it sane :-) Currently we walk over all slab caches each
> time new kmemcg is created even if memcg_limited_groups_array_size
> doesn't grow and we've actually nothing to do. So it moves cache id
> allocation stuff to a separate function (memcg_alloc_cache_id) and
> places the check there so that memcg_update_all_caches is only called
> when it's really necessary.
> 
> I'm sorry if it confuses you. I thought the patch isn't big and rather
> easy to understand :-/ Next time will split better.

This would be worth a separate patch then and explain all of this.

Thanks!
-- 
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 1/2] memcg: move memcg_{alloc,free}_cache_params to slab_common.c

[Michal Hocko]

On Mon 22-09-14 18:14:20, Vladimir Davydov wrote:
> On Mon, Sep 22, 2014 at 03:52:45PM +0200, Michal Hocko wrote:
> > On Thu 18-09-14 19:50:19, Vladimir Davydov wrote:
> > > The only reason why they live in memcontrol.c is that we get/put css
> > > reference to the owner memory cgroup in them. However, we can do that in
> > > memcg_{un,}register_cache.
> > > 
> > > So let's move them to slab_common.c and make them static.
> > 
> > Why is it better?
> 
> First, I think that the less public interface functions we have in
> memcontrol.h the better. Since the functions I move don't depend on
> memcontrol, I think it's worth making them private to slab, especially
> taking into account that the arrays are defined on the slab's side too.
> 
> Second, the way how per-memcg arrays are updated looks rather awkward:
> it proceeds from memcontrol.c (__memcg_activate_kmem) to slab_common.c
> (memcg_update_all_caches) and back to memcontrol.c again
> (memcg_update_array_size). In the next patch I move the function
> relocating the arrays (memcg_update_array_size) to slab_common.c and
> therefore get rid this circular call path. I think we should have the
> cache allocation stuff in the same place where we have relocation,
> because it's easier to follow the code then. So I move arrays alloc/free
> functions to slab_common.c too.
> 
> The third point isn't obvious. In the "Per memcg slab shrinkers" patch
> set, which I sent recently, I have to update per-memcg list_lrus arrays
> too. And it's much easier and cleaner to do it in list_lru.c rather than
> in memcontrol.c. The current patchset makes cache arrays allocation path
> conform that of the upcoming list_lru.

Exactly what I would love to have in the changelog...

Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] memcg: move memcg_{alloc,free}_cache_params to slab_common.c

[Michal Hocko]

On Mon 22-09-14 18:14:20, Vladimir Davydov wrote:
> On Mon, Sep 22, 2014 at 03:52:45PM +0200, Michal Hocko wrote:
> > On Thu 18-09-14 19:50:19, Vladimir Davydov wrote:
> > > The only reason why they live in memcontrol.c is that we get/put css
> > > reference to the owner memory cgroup in them. However, we can do that in
> > > memcg_{un,}register_cache.
> > > 
> > > So let's move them to slab_common.c and make them static.
> > 
> > Why is it better?
> 
> First, I think that the less public interface functions we have in
> memcontrol.h the better. Since the functions I move don't depend on
> memcontrol, I think it's worth making them private to slab, especially
> taking into account that the arrays are defined on the slab's side too.
> 
> Second, the way how per-memcg arrays are updated looks rather awkward:
> it proceeds from memcontrol.c (__memcg_activate_kmem) to slab_common.c
> (memcg_update_all_caches) and back to memcontrol.c again
> (memcg_update_array_size). In the next patch I move the function
> relocating the arrays (memcg_update_array_size) to slab_common.c and
> therefore get rid this circular call path. I think we should have the
> cache allocation stuff in the same place where we have relocation,
> because it's easier to follow the code then. So I move arrays alloc/free
> functions to slab_common.c too.
> 
> The third point isn't obvious. In the "Per memcg slab shrinkers" patch
> set, which I sent recently, I have to update per-memcg list_lrus arrays
> too. And it's much easier and cleaner to do it in list_lru.c rather than
> in memcontrol.c. The current patchset makes cache arrays allocation path
> conform that of the upcoming list_lru.

Exactly what I would love to have in the changelog...

Thanks!
-- 
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 1/2] memcg: move memcg_{alloc,free}_cache_params to slab_common.c

[Johannes Weiner]

On Thu, Sep 18, 2014 at 07:50:19PM +0400, Vladimir Davydov wrote:
> The only reason why they live in memcontrol.c is that we get/put css
> reference to the owner memory cgroup in them. However, we can do that in
> memcg_{un,}register_cache.
> 
> So let's move them to slab_common.c and make them static.
> 
> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.cz>
> Cc: Christoph Lameter <cl@linux.com>

Cool, so you get rid of the back-and-forth between memcg and slab, and
thereby also shrink the public memcg interface.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH 1/2] memcg: move memcg_{alloc,free}_cache_params to slab_common.c

[Johannes Weiner]

On Thu, Sep 18, 2014 at 07:50:19PM +0400, Vladimir Davydov wrote:
> The only reason why they live in memcontrol.c is that we get/put css
> reference to the owner memory cgroup in them. However, we can do that in
> memcg_{un,}register_cache.
> 
> So let's move them to slab_common.c and make them static.
> 
> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.cz>
> Cc: Christoph Lameter <cl@linux.com>

Cool, so you get rid of the back-and-forth between memcg and slab, and
thereby also shrink the public memcg interface.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

--
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 2/2] memcg: move memcg_update_cache_size to slab_common.c

[Johannes Weiner]

On Thu, Sep 18, 2014 at 07:50:20PM +0400, Vladimir Davydov wrote:
> The only reason why this function lives in memcontrol.c is that it
> depends on memcg_caches_array_size. However, we can pass the new array
> size immediately to it instead of new_id+1 so that it will be free of
> any memcontrol.c dependencies.
> 
> So let's move this function to slab_common.c and make it static.
> 
> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.cz>
> Cc: Christoph Lameter <cl@linux.com>

Looks good.  One nit below, but not a show stopper.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

> @@ -646,11 +646,13 @@ int memcg_limited_groups_array_size;
>  struct static_key memcg_kmem_enabled_key;
>  EXPORT_SYMBOL(memcg_kmem_enabled_key);
>  
> +static void memcg_free_cache_id(int id);

Any chance you could re-order this code to avoid the forward decl?
memcg_alloc_cache_id() and memcg_free_cache_id() are new functions
anyway, might as well put the definition above the callsites.

Thanks!

Re: [PATCH 2/2] memcg: move memcg_update_cache_size to slab_common.c

[Johannes Weiner]

On Thu, Sep 18, 2014 at 07:50:20PM +0400, Vladimir Davydov wrote:
> The only reason why this function lives in memcontrol.c is that it
> depends on memcg_caches_array_size. However, we can pass the new array
> size immediately to it instead of new_id+1 so that it will be free of
> any memcontrol.c dependencies.
> 
> So let's move this function to slab_common.c and make it static.
> 
> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.cz>
> Cc: Christoph Lameter <cl@linux.com>

Looks good.  One nit below, but not a show stopper.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

> @@ -646,11 +646,13 @@ int memcg_limited_groups_array_size;
>  struct static_key memcg_kmem_enabled_key;
>  EXPORT_SYMBOL(memcg_kmem_enabled_key);
>  
> +static void memcg_free_cache_id(int id);

Any chance you could re-order this code to avoid the forward decl?
memcg_alloc_cache_id() and memcg_free_cache_id() are new functions
anyway, might as well put the definition above the callsites.

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>

Re: [PATCH 2/2] memcg: move memcg_update_cache_size to slab_common.c

[Vladimir Davydov]

On Mon, Sep 22, 2014 at 04:11:37PM -0400, Johannes Weiner wrote:
> On Thu, Sep 18, 2014 at 07:50:20PM +0400, Vladimir Davydov wrote:
> > @@ -646,11 +646,13 @@ int memcg_limited_groups_array_size;
> >  struct static_key memcg_kmem_enabled_key;
> >  EXPORT_SYMBOL(memcg_kmem_enabled_key);
> >  
> > +static void memcg_free_cache_id(int id);
> 
> Any chance you could re-order this code to avoid the forward decl?

I'm going to move the call to memcg_free_cache_id() from the css free
path to css offline. Actually, this is what "[PATCH -mm 08/14] memcg:
release memcg_cache_id on css offline", which is a part of my "Per memcg
slab shrinkers" patch set, does. The css offline path is defined below
css_alloc/free_cache_id, so the forward declaration will be removed
then.

Thanks,
Vladimir

Re: [PATCH 2/2] memcg: move memcg_update_cache_size to slab_common.c

[Vladimir Davydov]

On Mon, Sep 22, 2014 at 04:11:37PM -0400, Johannes Weiner wrote:
> On Thu, Sep 18, 2014 at 07:50:20PM +0400, Vladimir Davydov wrote:
> > @@ -646,11 +646,13 @@ int memcg_limited_groups_array_size;
> >  struct static_key memcg_kmem_enabled_key;
> >  EXPORT_SYMBOL(memcg_kmem_enabled_key);
> >  
> > +static void memcg_free_cache_id(int id);
> 
> Any chance you could re-order this code to avoid the forward decl?

I'm going to move the call to memcg_free_cache_id() from the css free
path to css offline. Actually, this is what "[PATCH -mm 08/14] memcg:
release memcg_cache_id on css offline", which is a part of my "Per memcg
slab shrinkers" patch set, does. The css offline path is defined below
css_alloc/free_cache_id, so the forward declaration will be removed
then.

Thanks,
Vladimir

--
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 1/2] memcg: move memcg_{alloc,free}_cache_params to slab_common.c

[Vladimir Davydov]

On Mon, Sep 22, 2014 at 04:08:25PM -0400, Johannes Weiner wrote:
> On Thu, Sep 18, 2014 at 07:50:19PM +0400, Vladimir Davydov wrote:
> > The only reason why they live in memcontrol.c is that we get/put css
> > reference to the owner memory cgroup in them. However, we can do that in
> > memcg_{un,}register_cache.
> > 
> > So let's move them to slab_common.c and make them static.
> > 
> > Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Michal Hocko <mhocko@suse.cz>
> > Cc: Christoph Lameter <cl@linux.com>
> 
> Cool, so you get rid of the back-and-forth between memcg and slab, and
> thereby also shrink the public memcg interface.

It should be mentioned that we still call memcg_update_array_size()
(defined at memcontrol.c) from memcg_update_all_caches()
(slab_common.c), because we must hold the slab_mutex while updating
memcg_limited_groups_array_size. However, I'm going to remove this
requirement and get rid of memcg_update_array_size() too. This is what
"[PATCH -mm 10/14] memcg: add rwsem to sync against memcg_caches arrays
relocation", which is a part of my "Per memcg slab shrinkers" patch set,
does.

Thanks,
Vladimir

Re: [PATCH 1/2] memcg: move memcg_{alloc,free}_cache_params to slab_common.c

[Vladimir Davydov]

On Mon, Sep 22, 2014 at 04:08:25PM -0400, Johannes Weiner wrote:
> On Thu, Sep 18, 2014 at 07:50:19PM +0400, Vladimir Davydov wrote:
> > The only reason why they live in memcontrol.c is that we get/put css
> > reference to the owner memory cgroup in them. However, we can do that in
> > memcg_{un,}register_cache.
> > 
> > So let's move them to slab_common.c and make them static.
> > 
> > Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Michal Hocko <mhocko@suse.cz>
> > Cc: Christoph Lameter <cl@linux.com>
> 
> Cool, so you get rid of the back-and-forth between memcg and slab, and
> thereby also shrink the public memcg interface.

It should be mentioned that we still call memcg_update_array_size()
(defined at memcontrol.c) from memcg_update_all_caches()
(slab_common.c), because we must hold the slab_mutex while updating
memcg_limited_groups_array_size. However, I'm going to remove this
requirement and get rid of memcg_update_array_size() too. This is what
"[PATCH -mm 10/14] memcg: add rwsem to sync against memcg_caches arrays
relocation", which is a part of my "Per memcg slab shrinkers" patch set,
does.

Thanks,
Vladimir

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