Re: [PATCH 3/8] memcg, slab: never try to merge memcg caches

[Michal Hocko]

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

I cannot tell I understand the above but I am totally not sure about the
statement bellow.

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

How come the code was unused? find_mergeable called cache_match_memcg...

> Signed-off-by: Vladimir Davydov <vdavydov@xxxxxxxxxxxxx>
> ---
>  mm/slab.h        |   21 ++++-----------------
>  mm/slab_common.c |    8 +++++---
>  mm/slub.c        |   19 +++++++++----------
>  3 files changed, 18 insertions(+), 30 deletions(-)
> 
> diff --git a/mm/slab.h b/mm/slab.h
> index 8184a7cde272..3045316b7c9d 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -55,12 +55,12 @@ extern void create_boot_cache(struct kmem_cache *, const char *name,
>  struct mem_cgroup;
>  #ifdef CONFIG_SLUB
>  struct kmem_cache *
> -__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
> -		   size_t align, unsigned long flags, void (*ctor)(void *));
> +__kmem_cache_alias(const char *name, size_t size, size_t align,
> +		   unsigned long flags, void (*ctor)(void *));
>  #else
>  static inline struct kmem_cache *
> -__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
> -		   size_t align, unsigned long flags, void (*ctor)(void *))
> +__kmem_cache_alias(const char *name, size_t size, size_t align,
> +		   unsigned long flags, void (*ctor)(void *))
>  { return NULL; }
>  #endif
>  
> @@ -119,13 +119,6 @@ static inline bool is_root_cache(struct kmem_cache *s)
>  	return !s->memcg_params || s->memcg_params->is_root_cache;
>  }
>  
> -static inline bool cache_match_memcg(struct kmem_cache *cachep,
> -				     struct mem_cgroup *memcg)
> -{
> -	return (is_root_cache(cachep) && !memcg) ||
> -				(cachep->memcg_params->memcg == memcg);
> -}
> -
>  static inline void memcg_bind_pages(struct kmem_cache *s, int order)
>  {
>  	if (!is_root_cache(s))
> @@ -204,12 +197,6 @@ static inline bool is_root_cache(struct kmem_cache *s)
>  	return true;
>  }
>  
> -static inline bool cache_match_memcg(struct kmem_cache *cachep,
> -				     struct mem_cgroup *memcg)
> -{
> -	return true;
> -}
> -
>  static inline void memcg_bind_pages(struct kmem_cache *s, int order)
>  {
>  }
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 152d9b118b7a..a75834bb966d 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -200,9 +200,11 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size,
>  	 */
>  	flags &= CACHE_CREATE_MASK;
>  
> -	s = __kmem_cache_alias(memcg, name, size, align, flags, ctor);
> -	if (s)
> -		goto out_unlock;
> +	if (!memcg) {
> +		s = __kmem_cache_alias(name, size, align, flags, ctor);
> +		if (s)
> +			goto out_unlock;
> +	}
>  
>  	err = -ENOMEM;
>  	s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL);
> diff --git a/mm/slub.c b/mm/slub.c
> index 2b1a6970e46f..962abfdfde06 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3686,9 +3686,8 @@ static int slab_unmergeable(struct kmem_cache *s)
>  	return 0;
>  }
>  
> -static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
> -		size_t align, unsigned long flags, const char *name,
> -		void (*ctor)(void *))
> +static struct kmem_cache *find_mergeable(size_t size, size_t align,
> +		unsigned long flags, const char *name, void (*ctor)(void *))
>  {
>  	struct kmem_cache *s;
>  
> @@ -3707,11 +3706,14 @@ static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
>  		if (slab_unmergeable(s))
>  			continue;
>  
> +		if (!is_root_cache(s))
> +			continue;
> +
>  		if (size > s->size)
>  			continue;
>  
>  		if ((flags & SLUB_MERGE_SAME) != (s->flags & SLUB_MERGE_SAME))
> -				continue;
> +			continue;
>  		/*
>  		 * Check if alignment is compatible.
>  		 * Courtesy of Adrian Drzewiecki
> @@ -3722,21 +3724,18 @@ static struct kmem_cache *find_mergeable(struct mem_cgroup *memcg, size_t size,
>  		if (s->size - size >= sizeof(void *))
>  			continue;
>  
> -		if (!cache_match_memcg(s, memcg))
> -			continue;
> -
>  		return s;
>  	}
>  	return NULL;
>  }
>  
>  struct kmem_cache *
> -__kmem_cache_alias(struct mem_cgroup *memcg, const char *name, size_t size,
> -		   size_t align, unsigned long flags, void (*ctor)(void *))
> +__kmem_cache_alias(const char *name, size_t size, size_t align,
> +		   unsigned long flags, void (*ctor)(void *))
>  {
>  	struct kmem_cache *s;
>  
> -	s = find_mergeable(memcg, size, align, flags, name, ctor);
> +	s = find_mergeable(size, align, flags, name, ctor);
>  	if (s) {
>  		s->refcount++;
>  		/*
> -- 
> 1.7.10.4
> 

-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/