Re: [PATCH-next] mm, memcg: Add ":deact" tag for reparented kmem caches in memcg_slabinfo

[Roman Gushchin]

On Fri, Jun 21, 2019 at 01:30:05PM -0400, Waiman Long wrote:
> With Roman's kmem cache reparent patch, multiple kmem caches of the same
> type can be seen attached to the same memcg id. All of them, except
> maybe one, are reparent'ed kmem caches. It can be useful to tag those
> reparented caches by adding a new slab flag "SLAB_DEACTIVATED" to those
> kmem caches that will be reparent'ed if it cannot be destroyed completely.
> 
> For the reparent'ed memcg kmem caches, the tag ":deact" will now be
> shown in <debugfs>/memcg_slabinfo.
> 
> Signed-off-by: Waiman Long <longman@xxxxxxxxxx>

Hi Waiman!

Sorry for the late reply. The patch overall looks good to me,
except one nit. Please feel free to use my ack:
Acked-by: Roman Gushchin <guro@xxxxxx>

> ---
>  include/linux/slab.h |  4 ++++
>  mm/slab.c            |  1 +
>  mm/slab_common.c     | 14 ++++++++------
>  mm/slub.c            |  1 +
>  4 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index fecf40b7be69..19ab1380f875 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -116,6 +116,10 @@
>  /* Objects are reclaimable */
>  #define SLAB_RECLAIM_ACCOUNT	((slab_flags_t __force)0x00020000U)
>  #define SLAB_TEMPORARY		SLAB_RECLAIM_ACCOUNT	/* Objects are short-lived */
> +
> +/* Slab deactivation flag */
> +#define SLAB_DEACTIVATED	((slab_flags_t __force)0x10000000U)
> +
>  /*
>   * ZERO_SIZE_PTR will be returned for zero sized kmalloc requests.
>   *
> diff --git a/mm/slab.c b/mm/slab.c
> index a2e93adf1df0..e8c7743fc283 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2245,6 +2245,7 @@ int __kmem_cache_shrink(struct kmem_cache *cachep)
>  #ifdef CONFIG_MEMCG
>  void __kmemcg_cache_deactivate(struct kmem_cache *cachep)
>  {
> +	cachep->flags |= SLAB_DEACTIVATED;

A nit: it can be done from kmemcg_cache_deactivate() instead,
and then you don't have to do it in slab and slub separately.

Since it's not slab- or slub-specific code, it'd be better, IMO,
to put it into slab_common.c.

Thanks!

>  	__kmem_cache_shrink(cachep);
>  }
>  
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 146d8eaa639c..85cf0c374303 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -1533,7 +1533,7 @@ static int memcg_slabinfo_show(struct seq_file *m, void *unused)
>  	struct slabinfo sinfo;
>  
>  	mutex_lock(&slab_mutex);
> -	seq_puts(m, "# <name> <css_id[:dead]> <active_objs> <num_objs>");
> +	seq_puts(m, "# <name> <css_id[:dead|deact]> <active_objs> <num_objs>");
>  	seq_puts(m, " <active_slabs> <num_slabs>\n");
>  	list_for_each_entry(s, &slab_root_caches, root_caches_node) {
>  		/*
> @@ -1544,22 +1544,24 @@ static int memcg_slabinfo_show(struct seq_file *m, void *unused)
>  
>  		memset(&sinfo, 0, sizeof(sinfo));
>  		get_slabinfo(s, &sinfo);
> -		seq_printf(m, "%-17s root      %6lu %6lu %6lu %6lu\n",
> +		seq_printf(m, "%-17s root       %6lu %6lu %6lu %6lu\n",
>  			   cache_name(s), sinfo.active_objs, sinfo.num_objs,
>  			   sinfo.active_slabs, sinfo.num_slabs);
>  
>  		for_each_memcg_cache(c, s) {
>  			struct cgroup_subsys_state *css;
> -			char *dead = "";
> +			char *status = "";
>  
>  			css = &c->memcg_params.memcg->css;
>  			if (!(css->flags & CSS_ONLINE))
> -				dead = ":dead";
> +				status = ":dead";
> +			else if (c->flags & SLAB_DEACTIVATED)
> +				status = ":deact";
>  
>  			memset(&sinfo, 0, sizeof(sinfo));
>  			get_slabinfo(c, &sinfo);
> -			seq_printf(m, "%-17s %4d%5s %6lu %6lu %6lu %6lu\n",
> -				   cache_name(c), css->id, dead,
> +			seq_printf(m, "%-17s %4d%-6s %6lu %6lu %6lu %6lu\n",
> +				   cache_name(c), css->id, status,
>  				   sinfo.active_objs, sinfo.num_objs,
>  				   sinfo.active_slabs, sinfo.num_slabs);
>  		}
> diff --git a/mm/slub.c b/mm/slub.c
> index a384228ff6d3..c965b4413658 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -4057,6 +4057,7 @@ void __kmemcg_cache_deactivate(struct kmem_cache *s)
>  	 */
>  	slub_set_cpu_partial(s, 0);
>  	s->min_partial = 0;
> +	s->flags |= SLAB_DEACTIVATED;
>  }
>  #endif	/* CONFIG_MEMCG */
>  
> -- 
> 2.18.1
>