From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 26B8AC3DA59 for ; Mon, 22 Jul 2024 07:54:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AFE2F6B0085; Mon, 22 Jul 2024 03:54:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AAF096B0088; Mon, 22 Jul 2024 03:54:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 94E536B0089; Mon, 22 Jul 2024 03:54:41 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 770596B0085 for ; Mon, 22 Jul 2024 03:54:41 -0400 (EDT) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 278A1C1569 for ; Mon, 22 Jul 2024 07:54:41 +0000 (UTC) X-FDA: 82366626762.03.0E3674C Received: from out-186.mta1.migadu.com (out-186.mta1.migadu.com [95.215.58.186]) by imf11.hostedemail.com (Postfix) with ESMTP id BFE904000D for ; Mon, 22 Jul 2024 07:54:38 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=T2fCpkk1; spf=pass (imf11.hostedemail.com: domain of muchun.song@linux.dev designates 95.215.58.186 as permitted sender) smtp.mailfrom=muchun.song@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1721634843; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=vv6YSv7sgoYsC82DCWX0dpCMhAkkJP3/WhxIR4lZbC4=; b=vFHvqSWmdna1xp0vYvTZJ6IH7cA2+3pJGqSustU74OGiYeAkWdMxH5Le0UoOu1zRLFOFVR rGFM9ce6+ny2D/dX2VfHwSac4sYzeox5AXwTPLcx87JIJKv1U+3LoIPXwfDE1c6NlS1Smw jp11oUwc4HOSgdw/+lxksnxfvvkdpus= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=T2fCpkk1; spf=pass (imf11.hostedemail.com: domain of muchun.song@linux.dev designates 95.215.58.186 as permitted sender) smtp.mailfrom=muchun.song@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721634843; a=rsa-sha256; cv=none; b=fsW+fZo17SaQ3kSNtqUcHD5PB8vdnFaH7bSCT1JHeasmI8n7suUAODN3z+EH6k6/XLQ3mO IAifY2zmxpMINA30QIAR760HaPWpuFjKvjyIHhNDwyEExuHlCMqwm4v5omHBe/zXZerw50 1lG6f3WijhFMlaAo0aKwFiDWjzy5lZg= X-Envelope-To: chengming.zhou@linux.dev DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1721634877; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=vv6YSv7sgoYsC82DCWX0dpCMhAkkJP3/WhxIR4lZbC4=; b=T2fCpkk1j4FP2yq5WJCBo0k3j5rCbkl6drvNgvl+8viZJIcKX2HAxXJHMRZyt+DXRWa71I 6oZqvvMzDMsRq6sOndjrHm20hdtZAo5z8i7jBvdIcYgHfwXCcc48QIizJCg+dckry8Dm7f L+ZMYiKiAVoAJpmydLk++QwoT9wtuic= X-Envelope-To: songmuchun@bytedance.com X-Envelope-To: hannes@cmpxchg.org X-Envelope-To: mhocko@kernel.org X-Envelope-To: roman.gushchin@linux.dev X-Envelope-To: shakeel.butt@linux.dev X-Envelope-To: akpm@linux-foundation.org X-Envelope-To: cgroups@vger.kernel.org X-Envelope-To: linux-mm@kvack.org X-Envelope-To: linux-kernel@vger.kernel.org Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.600.62\)) Subject: Re: [PATCH] mm: kmem: add lockdep assertion to obj_cgroup_memcg X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Muchun Song In-Reply-To: Date: Mon, 22 Jul 2024 15:53:57 +0800 Cc: Muchun Song , Johannes Weiner , Michal Hocko , Roman Gushchin , shakeel.butt@linux.dev, Andrew Morton , cgroups@vger.kernel.org, Linux Memory Management List , LKML Content-Transfer-Encoding: quoted-printable Message-Id: <20859F67-A80C-4FD0-990C-40C70905E55B@linux.dev> References: <20240722070810.46016-1-songmuchun@bytedance.com> To: Chengming Zhou X-Migadu-Flow: FLOW_OUT X-Stat-Signature: 7qt79xe4d3h8eira5rgsx74a5zb9cwp5 X-Rspam-User: X-Rspamd-Queue-Id: BFE904000D X-Rspamd-Server: rspam02 X-HE-Tag: 1721634878-575644 X-HE-Meta: U2FsdGVkX19Zw43vneIGPCLpyWWQWZCF9eNXuaiD9NwqGCwAN8ficPpEquB/qHdcXkAxyexMNteAy9pc9waG1Xra1lNY+IbXie1cIjB+Cy4TinS8wr3PcocSagU2EAzh+F49fXsefyITdP8wyiOQUuROUGlEEzGMO4zTndidP/6KwCUrCTzbB/eAhiAsATDslhm9GhH6Ofd+ywapoUIJxpJc1sK3iNNyvrRSd/KoHlAJ9eqwCVO3vVJxpComl5QdhO+KX8VUylpEpFeT+M1EbU7SO/RrAGgZPRjAo+iG+MPEIdaGJnLRW5BBmQ44I+rTwhKCAsMlz9marAKDjQKCgLZBbaV3LcZ3J1hB1QbW1twQE2jrOBQR9TSYMR1NJwxXkEG9FG9DQrKweWqkPLLpQPU9Bw5ihrwjI8b5f+VLcUla3n9WRpraTta3Q3kLCkHn4RTEIoxiDmUeJkEd7htiA0f/ti8BWSnoYjiVHMSU8Wmj8+De/LaK3HtpuTlleK4fMJ5CGeHT1W2GrkySTTiUkG77/LAxp5yvlPo5wvaKumxdAMBRPu0egSp0hjent8IE4QeeHwZxJwEqbzO5rDLhRjUhd1HqV9k8tmfpt4uCwHZiBCPJIfl/MYWgf2Xo9vLwOUVvO+xxImKrxkvMjNdDlO3u50ryFG3ZoGr+cHQAq1xmH2zaYTGFrdSqzvBIru7jX7GVkiqe6qb6dLDlS3iRAyB/t0r2MzKzePCq6SqIm8QlStHjKWl6htFu/rJM44TK9MX5cs8E4/e+rJ50S3nwWQbcOD6ENlLMv/zgN6GXgZofqfOIkcVuO/jBjs3YAIVc6BRVoQW9iSeoYBHR2u+6EnjTPs6zymg2GYz1C/8eW5lQBYBCUT69Y3hhprz5ym/pM0cTFz9diMb1kDsEe035JjsLAccnAHYmBTh9x4nMdiicOXeXsRshvROlxcjJh9xB6U/lVFVWxdcGDbAsmaA SGeQOHIc CIsXF0APguhosy5lptvPBgth3vdZeOU7JolBGoeSu8QUlv9f+3oVB5CuSQhUFBumFYRuTvUOcNKMVFnhE2f86JeSr/1gONq+JVov8+aQJDYPpTEYXrH6YLJgGVfplTbs2oORpv8GtDifpgHAGj+8xP5zjdkMl225Z/VBmX4HWuVMxCxH7LHlXaQsF9Ls2v+GLSpuhzU2hoRlCoChA8zNv9x4Dk5CXyaKZYHcSn5UC/SoOhdGC4s2Haw1ljD/gGtrkZ+HPCliLTfYQk9XXi2JkC4GwBx/WqGcK3mx+ElUtm83H+IAXkIRTL0SduMj/D1TzA+4R X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: > On Jul 22, 2024, at 15:46, Chengming Zhou = wrote: >=20 > On 2024/7/22 15:08, Muchun Song wrote: >> The obj_cgroup_memcg() is supposed to safe to prevent the returned >> memory cgroup from being freed only when the caller is holding the >> rcu read lock or objcg_lock or cgroup_mutex. It is very easy to >> ignore thoes conditions when users call some upper APIs which call >> obj_cgroup_memcg() internally like mem_cgroup_from_slab_obj() (See >> the link below). So it is better to add lockdep assertion to >> obj_cgroup_memcg() to find those issues ASAP. >=20 > Yeah, some users care about the lifetime of returned memcg, while > some other users maybe not. >=20 > Maybe a dumb question, can we just make objcg hold the refcount of > its pointed memcg? So the users of that objcg don't need to care > about the refcount of memcg? (We could switch the refcount from > old memcg to the new memcg when objcg switch memcg pointer, right?) You mean the memcg is pinned if objcg is pinned, right? If yes, in which case, reparenting of memcg cannot make memcg being freed ASAP. >=20 > Thanks. >=20 >> Because there is no user of obj_cgroup_memcg() holding objcg_lock >> to make the returned memory cgroup safe, do not add objcg_lock >> assertion (We should export objcg_lock if we really want to do) >> and leave a comment to indicate it is intentional. >> Some users like __mem_cgroup_uncharge() do not care the lifetime >> of the returned memory cgroup, which just want to know if the >> folio is charged to a memory cgroup, therefore, they do not need >> to hold the needed locks. In which case, introduce a new helper >> folio_memcg_charged() to do this. Compare it to folio_memcg(), it >> could eliminate a memory access of objcg->memcg for kmem, actually, >> a really small gain. >> Link: = https://lore.kernel.org/all/20240718083607.42068-1-songmuchun@bytedance.co= m/ >> Signed-off-by: Muchun Song >> --- >> include/linux/memcontrol.h | 22 +++++++++++++++++++--- >> mm/memcontrol.c | 6 +++--- >> 2 files changed, 22 insertions(+), 6 deletions(-) >> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h >> index fc94879db4dff..d616c50025098 100644 >> --- a/include/linux/memcontrol.h >> +++ b/include/linux/memcontrol.h >> @@ -360,11 +360,13 @@ static inline bool folio_memcg_kmem(struct = folio *folio); >> * After the initialization objcg->memcg is always pointing at >> * a valid memcg, but can be atomically swapped to the parent memcg. >> * >> - * The caller must ensure that the returned memcg won't be released: >> - * e.g. acquire the rcu_read_lock or css_set_lock. >> + * The caller must ensure that the returned memcg won't be released. >> */ >> static inline struct mem_cgroup *obj_cgroup_memcg(struct obj_cgroup = *objcg) >> { >> + WARN_ON_ONCE(!rcu_read_lock_held() && >> + /* !lockdep_is_held(&objcg_lock) && */ >> + !lockdep_is_held(&cgroup_mutex)); >> return READ_ONCE(objcg->memcg); >> } >> @@ -438,6 +440,19 @@ static inline struct mem_cgroup = *folio_memcg(struct folio *folio) >> return __folio_memcg(folio); >> } >> +/* >> + * folio_memcg_charged - If a folio is charged to a memory cgroup. >> + * @folio: Pointer to the folio. >> + * >> + * Returns true if folio is charged to a memory cgroup, otherwise = returns false. >> + */ >> +static inline bool folio_memcg_charged(struct folio *folio) >> +{ >> + if (folio_memcg_kmem(folio)) >> + return __folio_objcg(folio) !=3D NULL; >> + return __folio_memcg(folio) !=3D NULL; >> +} >> + >> /** >> * folio_memcg_rcu - Locklessly get the memory cgroup associated = with a folio. >> * @folio: Pointer to the folio. >> @@ -454,7 +469,6 @@ static inline struct mem_cgroup = *folio_memcg_rcu(struct folio *folio) >> unsigned long memcg_data =3D READ_ONCE(folio->memcg_data); >> VM_BUG_ON_FOLIO(folio_test_slab(folio), folio); >> - WARN_ON_ONCE(!rcu_read_lock_held()); >> if (memcg_data & MEMCG_DATA_KMEM) { >> struct obj_cgroup *objcg; >> @@ -463,6 +477,8 @@ static inline struct mem_cgroup = *folio_memcg_rcu(struct folio *folio) >> return obj_cgroup_memcg(objcg); >> } >> + WARN_ON_ONCE(!rcu_read_lock_held()); >> + >> return (struct mem_cgroup *)(memcg_data & ~OBJEXTS_FLAGS_MASK); >> } >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index 622d4544edd24..3da0284573857 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -2366,7 +2366,7 @@ void mem_cgroup_cancel_charge(struct mem_cgroup = *memcg, unsigned int nr_pages) >> static void commit_charge(struct folio *folio, struct mem_cgroup = *memcg) >> { >> - VM_BUG_ON_FOLIO(folio_memcg(folio), folio); >> + VM_BUG_ON_FOLIO(folio_memcg_charged(folio), folio); >> /* >> * Any of the following ensures page's memcg stability: >> * >> @@ -4617,7 +4617,7 @@ void __mem_cgroup_uncharge(struct folio *folio) >> struct uncharge_gather ug; >> /* Don't touch folio->lru of any random page, pre-check: */ >> - if (!folio_memcg(folio)) >> + if (!folio_memcg_charged(folio)) >> return; >> uncharge_gather_clear(&ug); >> @@ -4662,7 +4662,7 @@ void mem_cgroup_replace_folio(struct folio = *old, struct folio *new) >> return; >> /* Page cache replacement: new folio already charged? */ >> - if (folio_memcg(new)) >> + if (folio_memcg_charged(new)) >> return; >> memcg =3D folio_memcg(old);