Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\))
Subject: Re: [PATCH 1/2] mm, slab: Revert "extend slab/shrink to shrink all
 memcg caches"
From:   Qian Cai <cai@lca.pw>
In-Reply-To: <20200424151225.10966-1-longman@redhat.com>
Date:   Fri, 24 Apr 2020 12:41:34 -0400
Cc:     Christoph Lameter <cl@linux.com>,
        Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Juri Lelli <juri.lelli@redhat.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <23B286D4-831B-4788-95BA-438C88BB4EB0@lca.pw>
References: <20200424151225.10966-1-longman@redhat.com>
To:     Waiman Long <longman@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Apr 24, 2020, at 11:12 AM, Waiman Long <longman@redhat.com> wrote:
>=20
> When the slub shrink sysfs file is written into, the function call
> sequence is as follows:
>=20
>  kernfs_fop_write
>    =3D> slab_attr_store
>      =3D> shrink_store
>        =3D> kmem_cache_shrink_all
>=20
> It turns out that doing a memcg cache scan in kmem_cache_shrink_all()
> is redundant as the same memcg cache scan is being done in
> slab_attr_store(). So revert the commit 04f768a39d55 ("mm, slab: =
extend
> slab/shrink to shrink all memcg caches") except the documentation =
change
> which is still valid.

BTW, currently, do,

# echo 1 > /sys/kernel/slab/fs_cache/shrink

would crash the kernel stack probably due to large amount of memcg =
caches.

 I am still figuring out if the above commit 04f768a39d55 is the =
culprit.

[ 7938.979589][T106403] Kernel panic - not syncing: stack-protector: =
Kernel stack is corrupted in: __kmem_cache_create+0x7f8/0x800
[ 7938.979640][T106403] CPU: 80 PID: 106403 Comm: kworker/80:2 Not =
tainted 5.7.0-rc2-next-20200424 #5
[ 7938.979670][T106403] Workqueue: memcg_kmem_cache =
memcg_kmem_cache_create_func
[ 7938.979708][T106403] Call Trace:
[ 7938.979745][T106403] [c000200012e0f880] [c000000000716498] =
dump_stack+0xfc/0x174 (unreliable)
[ 7938.979789][T106403] [c000200012e0f8d0] [c00000000010d7d0] =
panic+0x224/0x4d4
[ 7938.979816][T106403] [c000200012e0f970] [c00000000010d05c] =
__stack_chk_fail+0x2c/0x30
[ 7938.979865][T106403] [c000200012e0f9d0] [c0000000004b1fb8] =
__kmem_cache_create+0x7f8/0x800
[ 7938.979914][T106403] [c000200012e0faf0] [4320383d35334320] =
0x4320383d35334320

>=20
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
> mm/slab.h        |  1 -
> mm/slab_common.c | 37 -------------------------------------
> mm/slub.c        |  2 +-
> 3 files changed, 1 insertion(+), 39 deletions(-)
>=20
> diff --git a/mm/slab.h b/mm/slab.h
> index 207c83ef6e06..0937cb2ae8aa 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -237,7 +237,6 @@ int __kmem_cache_shrink(struct kmem_cache *);
> void __kmemcg_cache_deactivate(struct kmem_cache *s);
> void __kmemcg_cache_deactivate_after_rcu(struct kmem_cache *s);
> void slab_kmem_cache_release(struct kmem_cache *);
> -void kmem_cache_shrink_all(struct kmem_cache *s);
>=20
> struct seq_file;
> struct file;
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 23c7500eea7d..2e367ab8c15c 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -995,43 +995,6 @@ int kmem_cache_shrink(struct kmem_cache *cachep)
> }
> EXPORT_SYMBOL(kmem_cache_shrink);
>=20
> -/**
> - * kmem_cache_shrink_all - shrink a cache and all memcg caches for =
root cache
> - * @s: The cache pointer
> - */
> -void kmem_cache_shrink_all(struct kmem_cache *s)
> -{
> -	struct kmem_cache *c;
> -
> -	if (!IS_ENABLED(CONFIG_MEMCG_KMEM) || !is_root_cache(s)) {
> -		kmem_cache_shrink(s);
> -		return;
> -	}
> -
> -	get_online_cpus();
> -	get_online_mems();
> -	kasan_cache_shrink(s);
> -	__kmem_cache_shrink(s);
> -
> -	/*
> -	 * We have to take the slab_mutex to protect from the memcg list
> -	 * modification.
> -	 */
> -	mutex_lock(&slab_mutex);
> -	for_each_memcg_cache(c, s) {
> -		/*
> -		 * Don't need to shrink deactivated memcg caches.
> -		 */
> -		if (s->flags & SLAB_DEACTIVATED)
> -			continue;
> -		kasan_cache_shrink(c);
> -		__kmem_cache_shrink(c);
> -	}
> -	mutex_unlock(&slab_mutex);
> -	put_online_mems();
> -	put_online_cpus();
> -}
> -
> bool slab_is_available(void)
> {
> 	return slab_state >=3D UP;
> diff --git a/mm/slub.c b/mm/slub.c
> index 9bf44955c4f1..183ccc364ccf 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5343,7 +5343,7 @@ static ssize_t shrink_store(struct kmem_cache =
*s,
> 			const char *buf, size_t length)
> {
> 	if (buf[0] =3D=3D '1')
> -		kmem_cache_shrink_all(s);
> +		kmem_cache_shrink(s);
> 	else
> 		return -EINVAL;
> 	return length;
> --=20
> 2.18.1
>=20
>=20