linux-kernel - Re: [PATCH 3/9] slab: simplify shutdown_memcg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 14 Jan 2017 16:27:22 +0300
From:   Vladimir Davydov <vdavydov@...antool.org>
To:     Tejun Heo <tj@...nel.org>
Cc:     cl@...ux.com, penberg@...nel.org, rientjes@...gle.com,
        iamjoonsoo.kim@....com, akpm@...ux-foundation.org, jsvana@...com,
        hannes@...xchg.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, cgroups@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH 3/9] slab: simplify shutdown_memcg_caches()

On Sat, Jan 14, 2017 at 12:54:43AM -0500, Tejun Heo wrote:
> shutdown_memcg_caches() shuts down all memcg caches associated with a
> root cache.  It first walks the index table clearing and shutting down
> each entry and then shuts down the ones on
> root_cache->memcg_params.list.  As active caches are on both the table
> and the list, they're stashed away from the list to avoid shutting
> down twice and then get spliced back later.
> 
> This is unnecessarily complication.  All memcg caches are on
> root_cache->memcg_params.list.  The function can simply clear the
> index table and shut down all caches on the list.  There's no need to
> muck with temporary stashing.
> 
> Simplify the code.
> 
> Signed-off-by: Tejun Heo <tj@...nel.org>
> Cc: Vladimir Davydov <vdavydov.dev@...il.com>
> Cc: Christoph Lameter <cl@...ux.com>
> Cc: Pekka Enberg <penberg@...nel.org>
> Cc: David Rientjes <rientjes@...gle.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@....com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> ---
>  mm/slab_common.c | 32 +++++---------------------------
>  1 file changed, 5 insertions(+), 27 deletions(-)
> 
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 851c75e..45aa67c 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -634,48 +634,26 @@ static int shutdown_memcg_caches(struct kmem_cache *s)
>  {
>  	struct memcg_cache_array *arr;
>  	struct kmem_cache *c, *c2;
> -	LIST_HEAD(busy);
>  	int i;
>  
>  	BUG_ON(!is_root_cache(s));
>  
>  	/*
> -	 * First, shutdown active caches, i.e. caches that belong to online
> -	 * memory cgroups.
> +	 * First, clear the pointers to all memcg caches so that they will
> +	 * never be accessed even if the root cache stays alive.
>  	 */
>  	arr = rcu_dereference_protected(s->memcg_params.memcg_caches,
>  					lockdep_is_held(&slab_mutex));
> -	for_each_memcg_cache_index(i) {
> -		c = arr->entries[i];
> -		if (!c)
> -			continue;
> -		if (shutdown_cache(c))
> -			/*
> -			 * The cache still has objects. Move it to a temporary
> -			 * list so as not to try to destroy it for a second
> -			 * time while iterating over inactive caches below.
> -			 */
> -			list_move(&c->memcg_params.list, &busy);
> -		else
> -			/*
> -			 * The cache is empty and will be destroyed soon. Clear
> -			 * the pointer to it in the memcg_caches array so that
> -			 * it will never be accessed even if the root cache
> -			 * stays alive.
> -			 */
> -			arr->entries[i] = NULL;
> -	}
> +	for_each_memcg_cache_index(i)
> +		arr->entries[i] = NULL;
>  
>  	/*
> -	 * Second, shutdown all caches left from memory cgroups that are now
> -	 * offline.
> +	 * Shutdown all caches.
>  	 */
>  	list_for_each_entry_safe(c, c2, &s->memcg_params.list,
>  				 memcg_params.list)
>  		shutdown_cache(c);

The point of this complexity was to leave caches that happen to have
objects when kmem_cache_destroy() is called on the list, so that they
could be reused later. This behavior was inherited from the global
caches - if kmem_cache_destroy() is called on a cache that still has
object, we print a warning message and don't destroy the cache. This
patch changes this behavior.

>  
> -	list_splice(&busy, &s->memcg_params.list);
> -
>  	/*
>  	 * A cache being destroyed must be empty. In particular, this means
>  	 * that all per memcg caches attached to it must be empty too.