LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andrew Morton <akpm@linux-foundation.org>
To: Shakeel Butt <shakeelb@google.com>
Cc: Michal Hocko <mhocko@kernel.org>,
	Greg Thelen <gthelen@google.com>,
	Christoph Lameter <cl@linux.com>,
	Pekka Enberg <penberg@kernel.org>,
	David Rientjes <rientjes@google.com>,
	Joonsoo Kim <iamjoonsoo.kim@lge.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Vladimir Davydov <vdavydov.dev@gmail.com>,
	Tejun Heo <tj@kernel.org>, Linux MM <linux-mm@kvack.org>,
	cgroups@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] mm: fix race between kmem_cache destroy, create and deactivate
Date: Mon, 21 May 2018 11:42:27 -0700	[thread overview]
Message-ID: <20180521114227.233983ac7038a9f4bf5b7066@linux-foundation.org> (raw)
In-Reply-To: <20180521174116.171846-1-shakeelb@google.com>

On Mon, 21 May 2018 10:41:16 -0700 Shakeel Butt <shakeelb@google.com> wrote:

> The memcg kmem cache creation and deactivation (SLUB only) is
> asynchronous. If a root kmem cache is destroyed whose memcg cache is in
> the process of creation or deactivation, the kernel may crash.
> 
> Example of one such crash:
> 	general protection fault: 0000 [#1] SMP PTI
> 	CPU: 1 PID: 1721 Comm: kworker/14:1 Not tainted 4.17.0-smp
> 	...
> 	Workqueue: memcg_kmem_cache kmemcg_deactivate_workfn
> 	RIP: 0010:has_cpu_slab
> 	...
> 	Call Trace:
> 	? on_each_cpu_cond
> 	__kmem_cache_shrink
> 	kmemcg_cache_deact_after_rcu
> 	kmemcg_deactivate_workfn
> 	process_one_work
> 	worker_thread
> 	kthread
> 	ret_from_fork+0x35/0x40
> 
> This issue is due to the lack of reference counting for the root
> kmem_caches. There exist a refcount in kmem_cache but it is actually a
> count of aliases i.e. number of kmem_caches merged together.
> 
> This patch make alias count explicit and adds reference counting to the
> root kmem_caches. The reference of a root kmem cache is elevated on
> merge and while its memcg kmem_cache is in the process of creation or
> deactivation.
> 

The patch seems depressingly complex.

And a bit underdocumented...

> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -674,6 +674,8 @@ struct memcg_cache_params {
>  };
>  
>  int memcg_update_all_caches(int num_memcgs);
> +bool kmem_cache_tryget(struct kmem_cache *s);
> +void kmem_cache_put(struct kmem_cache *s);
>  
>  /**
>   * kmalloc_array - allocate memory for an array.
> diff --git a/include/linux/slab_def.h b/include/linux/slab_def.h
> index d9228e4d0320..4bb22c89a740 100644
> --- a/include/linux/slab_def.h
> +++ b/include/linux/slab_def.h
> @@ -41,7 +41,8 @@ struct kmem_cache {
>  /* 4) cache creation/removal */
>  	const char *name;
>  	struct list_head list;
> -	int refcount;
> +	refcount_t refcount;
> +	int alias_count;

The semantic meaning of these two?  What locking protects alias_count?

>  	int object_size;
>  	int align;
>  
> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
> index 3773e26c08c1..532d4b6f83ed 100644
> --- a/include/linux/slub_def.h
> +++ b/include/linux/slub_def.h
> @@ -97,7 +97,8 @@ struct kmem_cache {
>  	struct kmem_cache_order_objects max;
>  	struct kmem_cache_order_objects min;
>  	gfp_t allocflags;	/* gfp flags to use on each alloc */
> -	int refcount;		/* Refcount for slab cache destroy */
> +	refcount_t refcount;	/* Refcount for slab cache destroy */
> +	int alias_count;	/* Number of root kmem caches merged */

"merged" what with what in what manner?

>  	void (*ctor)(void *);
>  	unsigned int inuse;		/* Offset to metadata */
>  	unsigned int align;		/* Alignment */
>
> ...
>
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -25,7 +25,8 @@ struct kmem_cache {
>  	unsigned int useroffset;/* Usercopy region offset */
>  	unsigned int usersize;	/* Usercopy region size */
>  	const char *name;	/* Slab name for sysfs */
> -	int refcount;		/* Use counter */
> +	refcount_t refcount;	/* Use counter */
> +	int alias_count;

Semantic meaning/usage of alias_count?  Locking for it?

>  	void (*ctor)(void *);	/* Called on object slot creation */
>  	struct list_head list;	/* List of all slab caches on the system */
>  };
>
> ...
>
> +bool kmem_cache_tryget(struct kmem_cache *s)
> +{
> +	if (is_root_cache(s))
> +		return refcount_inc_not_zero(&s->refcount);
> +	return false;
> +}
> +
> +void kmem_cache_put(struct kmem_cache *s)
> +{
> +	if (is_root_cache(s) &&
> +	    refcount_dec_and_test(&s->refcount))
> +		__kmem_cache_destroy(s, true);
> +}
> +
> +void kmem_cache_put_locked(struct kmem_cache *s)
> +{
> +	if (is_root_cache(s) &&
> +	    refcount_dec_and_test(&s->refcount))
> +		__kmem_cache_destroy(s, false);
> +}

Some covering documentation for the above would be useful.  Why do they
exist, why do they only operate on the root cache? etc.

>
> ...
>

  reply	other threads:[~2018-05-21 18:42 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-21 17:41 Shakeel Butt
2018-05-21 18:42 ` Andrew Morton [this message]
2018-05-21 20:12   ` Shakeel Butt
2018-05-22 16:43   ` Christopher Lameter
2018-05-22 21:14 ` kbuild test robot
2018-05-22 21:39   ` Andrew Morton
2018-05-22 22:07 ` kbuild test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180521114227.233983ac7038a9f4bf5b7066@linux-foundation.org \
    --to=akpm@linux-foundation.org \
    --cc=cgroups@vger.kernel.org \
    --cc=cl@linux.com \
    --cc=gthelen@google.com \
    --cc=hannes@cmpxchg.org \
    --cc=iamjoonsoo.kim@lge.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@kernel.org \
    --cc=penberg@kernel.org \
    --cc=rientjes@google.com \
    --cc=shakeelb@google.com \
    --cc=tj@kernel.org \
    --cc=vdavydov.dev@gmail.com \
    --subject='Re: [PATCH] mm: fix race between kmem_cache destroy, create and deactivate' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).