[PATCH v2 0/3] memcg: trivial cleanups in kmemcg core

[PATCH v2 0/3] memcg: trivial cleanups in kmemcg core

[Vladimir Davydov]

This patch set does exactly the same thing as

https://lkml.org/lkml/2014/9/18/315

but it's commented better (thanks to Michal).

Vladimir Davydov (3):
  memcg: move memcg_{alloc,free}_cache_params to slab_common.c
  memcg: don't call memcg_update_all_caches if new cache id fits
  memcg: move memcg_update_cache_size to slab_common.c

 include/linux/memcontrol.h |   15 -----
 mm/memcontrol.c            |  154 ++++++++++++--------------------------------
 mm/slab_common.c           |   74 ++++++++++++++++++++-
 3 files changed, 111 insertions(+), 132 deletions(-)

-- 
1.7.10.4

[PATCH v2 1/3] 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. OTOH, there are several reasons to move them
to slab_common.c.

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 following patches 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. I'm going to make the list_lru structure
per-memcg to allow targeted kmem reclaim. That means we will have
per-memcg arrays in list_lrus too. It turns out that it's much easier to
update these arrays in list_lru.c rather than in memcontrol.c, because
all the stuff we need is defined there. This patch makes memcg caches
arrays allocation path conform that of the upcoming list_lru.

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

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

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

[Michal Hocko]

On Mon 22-09-14 20:00:44, 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. OTOH, there are several reasons to move them
> to slab_common.c.
> 
> 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 following patches 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. I'm going to make the list_lru structure
> per-memcg to allow targeted kmem reclaim. That means we will have
> per-memcg arrays in list_lrus too. It turns out that it's much easier to
> update these arrays in list_lru.c rather than in memcontrol.c, because
> all the stuff we need is defined there. This patch makes memcg caches
> arrays allocation path conform that of the upcoming list_lru.
> 
> So let's move these functions to slab_common.c and make them static.
> 
> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>

Excelent and thanks a lot for updating the changelog.
Acked-by: Michal Hocko <mhocko@suse.cz>

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

[PATCH v2 2/3] memcg: don't call memcg_update_all_caches if new cache id fits

[Vladimir Davydov]

memcg_update_all_caches grows arrays of per-memcg caches, so we only
need to call it when memcg_limited_groups_array_size is increased.
However, currently we invoke it each time a new kmem-active memory
cgroup is created. Then it just iterates over all slab_caches and does
nothinng (memcg_update_cache_size returns immediately).

This patch fixes this insanity. In the meantime it moves the code
dealing with id allocations to separate functions, memcg_alloc_cache_id
and memcg_free_cache_id.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 mm/memcontrol.c |  136 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 72 insertions(+), 64 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b6bbb1e3e2ab..55d131645b45 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,44 @@ 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,59 +2941,55 @@ 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);
+	memcg_limited_groups_array_size = num;
 }
 
 int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
 {
 	struct memcg_cache_params *cur_params = s->memcg_params;
+	struct memcg_cache_params *new_params;
+	size_t size;
+	int i;
 
 	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 = num_groups * sizeof(void *);
+	size += offsetof(struct memcg_cache_params, memcg_caches);
 
-		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;
+	new_params = kzalloc(size, GFP_KERNEL);
+	if (!new_params)
+		return -ENOMEM;
 
-		/*
-		 * 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];
-		}
+	new_params->is_root_cache = true;
 
-		/*
-		 * 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);
+	/*
+	 * 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;
 }
 
@@ -4167,23 +4190,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 +4216,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,
-- 
1.7.10.4

Re: [PATCH v2 2/3] memcg: don't call memcg_update_all_caches if new cache id fits

[Michal Hocko]

On Mon 22-09-14 20:00:45, Vladimir Davydov wrote:
> memcg_update_all_caches grows arrays of per-memcg caches, so we only
> need to call it when memcg_limited_groups_array_size is increased.
> However, currently we invoke it each time a new kmem-active memory
> cgroup is created. Then it just iterates over all slab_caches and does
> nothinng (memcg_update_cache_size returns immediately).
> 
> This patch fixes this insanity. In the meantime it moves the code
> dealing with id allocations to separate functions, memcg_alloc_cache_id
> and memcg_free_cache_id.
> 
> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>

OK, looks good to me
Acked-by: Michal Hocko <mhocko@suse.cz>

Thanks!
> ---
>  mm/memcontrol.c |  136 +++++++++++++++++++++++++++++--------------------------
>  1 file changed, 72 insertions(+), 64 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b6bbb1e3e2ab..55d131645b45 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,44 @@ 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,59 +2941,55 @@ 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);
> +	memcg_limited_groups_array_size = num;
>  }
>  
>  int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
>  {
>  	struct memcg_cache_params *cur_params = s->memcg_params;
> +	struct memcg_cache_params *new_params;
> +	size_t size;
> +	int i;
>  
>  	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 = num_groups * sizeof(void *);
> +	size += offsetof(struct memcg_cache_params, memcg_caches);
>  
> -		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;
> +	new_params = kzalloc(size, GFP_KERNEL);
> +	if (!new_params)
> +		return -ENOMEM;
>  
> -		/*
> -		 * 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];
> -		}
> +	new_params->is_root_cache = true;
>  
> -		/*
> -		 * 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);
> +	/*
> +	 * 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;
>  }
>  
> @@ -4167,23 +4190,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 +4216,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,
> -- 
> 1.7.10.4
> 

-- 
Michal Hocko
SUSE Labs

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

[Vladimir Davydov]

While growing per memcg caches arrays, we jump between memcontrol.c and
slab_common.c in a weird way:

  memcg_alloc_cache_id - memcontrol.c
    memcg_update_all_caches - slab_common.c
      memcg_update_cache_size - memcontrol.c

There's absolutely no reason why memcg_update_cache_size can't live on
the slab's side though. So let's move it there and settle it comfortably
amid per-memcg cache allocation functions.

Besides, this patch cleans this function up a bit, removing all the
useless comments from it, and renames it to memcg_update_cache_params to
conform to memcg_alloc/free_cache_params, which we already have in
slab_common.c.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 include/linux/memcontrol.h |    1 -
 mm/memcontrol.c            |   49 --------------------------------------------
 mm/slab_common.c           |   30 +++++++++++++++++++++++++--
 3 files changed, 28 insertions(+), 52 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 55d131645b45..1ec22bf380d0 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2944,55 +2944,6 @@ void memcg_update_array_size(int num)
 	memcg_limited_groups_array_size = num;
 }
 
-int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
-{
-	struct memcg_cache_params *cur_params = s->memcg_params;
-	struct memcg_cache_params *new_params;
-	size_t size;
-	int i;
-
-	VM_BUG_ON(!is_root_cache(s));
-
-	size = num_groups * 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;
-}
-
 static void memcg_register_cache(struct mem_cgroup *memcg,
 				 struct kmem_cache *root_cache)
 {
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

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

[Michal Hocko]

On Mon 22-09-14 20:00:46, Vladimir Davydov wrote:
> While growing per memcg caches arrays, we jump between memcontrol.c and
> slab_common.c in a weird way:
> 
>   memcg_alloc_cache_id - memcontrol.c
>     memcg_update_all_caches - slab_common.c
>       memcg_update_cache_size - memcontrol.c
> 
> There's absolutely no reason why memcg_update_cache_size can't live on
> the slab's side though. So let's move it there and settle it comfortably
> amid per-memcg cache allocation functions.
> 
> Besides, this patch cleans this function up a bit, removing all the
> useless comments from it, and renames it to memcg_update_cache_params to
> conform to memcg_alloc/free_cache_params, which we already have in
> slab_common.c.
> 
> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>

I found new_params->memcg_caches[i] = ... style of initialization easier
to read and understand than memcpy. This is not something to block
this cleanup but I would be happier to have the array style back ;)

Acked-by: Michal Hocko <mhocko@suse.cz>

> ---
>  include/linux/memcontrol.h |    1 -
>  mm/memcontrol.c            |   49 --------------------------------------------
>  mm/slab_common.c           |   30 +++++++++++++++++++++++++--
>  3 files changed, 28 insertions(+), 52 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 55d131645b45..1ec22bf380d0 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2944,55 +2944,6 @@ void memcg_update_array_size(int num)
>  	memcg_limited_groups_array_size = num;
>  }
>  
> -int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
> -{
> -	struct memcg_cache_params *cur_params = s->memcg_params;
> -	struct memcg_cache_params *new_params;
> -	size_t size;
> -	int i;
> -
> -	VM_BUG_ON(!is_root_cache(s));
> -
> -	size = num_groups * 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;
> -}
> -
>  static void memcg_register_cache(struct mem_cgroup *memcg,
>  				 struct kmem_cache *root_cache)
>  {
> 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