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 17FEDC3DA63 for ; Wed, 24 Jul 2024 15:20:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 954746B0093; Wed, 24 Jul 2024 11:20:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8DE786B0095; Wed, 24 Jul 2024 11:20:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 77E436B0096; Wed, 24 Jul 2024 11:20:20 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 565796B0093 for ; Wed, 24 Jul 2024 11:20:20 -0400 (EDT) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 02972A0986 for ; Wed, 24 Jul 2024 15:20:19 +0000 (UTC) X-FDA: 82375007400.05.72E9DFD Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf28.hostedemail.com (Postfix) with ESMTP id 9F665C001C for ; Wed, 24 Jul 2024 15:20:16 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=foq0aGZe; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf28.hostedemail.com: domain of vbabka@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=vbabka@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1721834363; 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=HOUCCircbK86b7pBqfWyM/xIlPFPN3qFTUVajVoP5eI=; b=hJXvdlhNLMKur/X2P/BsyZyL/tzWrY3lcldOS6TLrKebXxyTaidV0GITI5mLZB9dUJbxom 6a5ZS4OLy1JlHbMNAhz974nLoeKET+TewY7TdBjIRz3WEcnOeBfE1pAoWPA3Nx29kr4xMb RCnLaWJ3wmDm7nGpUbRW/1RYXSUS3cc= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721834363; a=rsa-sha256; cv=none; b=DqSiR84Uht89JoRw1q7940n6QGhn2lEbsk6IH8WfL3QbfowntbYAuZ4tw0ZbmkgpvNqcMY +E2fjg0/cEauGuNBAdlC0b5NV6PYhji4PCI4CCx2InAtahi2hKiTI7+AMvZG+D7a+qnzDi qXcEXJ9iyYXs2CnRFjBehvfB38fazck= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=foq0aGZe; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf28.hostedemail.com: domain of vbabka@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=vbabka@kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 722DF611EE; Wed, 24 Jul 2024 15:20:15 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2D6D3C32781; Wed, 24 Jul 2024 15:20:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1721834415; bh=qeXLUpoZGsZRPbAT3HnF46SuxY7YTRrZMXLruO62MZ4=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=foq0aGZehOfXyqCf95kJdLCejTjAEe6euW8dlRJJ7u9hGgh36hPbajH3m3Suml4LV z61gO5OonvF224YmrWKT1dYlX1XAI9bZezjUI1jfVVqUwY0zGPvG/kP8ttPu6VOokS JxfEX1hGt9bq1tbyACJJi4J+fCwrXPziPYtniQ+HUUSWstnFvwG4x7L15Ec36jyO5u itQ87DTpaNObbcl+W8KnQuLwyng0WIngITLq/J4qvh6TScZSsWmTPYSS3OxKkAOvwq lpFkpGr8FTJXHPniV87OFfsML7dm5u2ppm6NkzcnH964YfzNxuIKgVNohknyOXu2jw rGxJomXCH6pPw== Message-ID: <610dc6fa-6681-4c9e-bffb-ef6d299dd169@kernel.org> Date: Wed, 24 Jul 2024 17:20:09 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] mm: kmem: add lockdep assertion to obj_cgroup_memcg Content-Language: en-US To: Muchun Song , hannes@cmpxchg.org, mhocko@kernel.org, roman.gushchin@linux.dev, shakeel.butt@linux.dev, muchun.song@linux.dev, akpm@linux-foundation.org Cc: cgroups@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20240724095307.81264-1-songmuchun@bytedance.com> From: "Vlastimil Babka (SUSE)" In-Reply-To: <20240724095307.81264-1-songmuchun@bytedance.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 9F665C001C X-Stat-Signature: o6koga4up8qhf88mq6smk16j4g4wdnkn X-Rspam-User: X-HE-Tag: 1721834416-28587 X-HE-Meta: U2FsdGVkX1+EusZhqSc8uTvzyHDmVL9A74VUtCZjkStCVc8wH25WE5HGw4MK5LXAKTBvS6Y1SaWCMj0CkPC5Cz7DVBWQLnlEQMHUhyM7QE0snxN1HW/LV0R0IxHn9suDYbK1g7Ry3H00R6kfvM/aNXNaYkmosdGvzX/HM5/SgR0AuVKjwCrSpru+YHYpdfW8k+SXewALZSPesbkoJxWF+j4RuMR8qzIlqDn8IpLBKlJWiLtK7I3112JmoKpRHywb3m9Fcg5LGgCJywmqEUf6phnOQAK5Kn7oY2rzMkGUGVJfKOoVZ4IusMxnhllFtsvcF3ehO1i4U5WOLqwE/z/aLhHQIkS7P08k6UPlEHAbt9VDw9XI3u9yzUOA81Kwq6pQmiEMfQcbWs/6jg4wgI5DXlaAbxJ+ZQ1G1LNfknDlwEFkZg9dcvku8cWgVQBx3HDD1a+6hYJvo1dMHDvCPOF5p2vjXhHQr+RL/i/PAM51QcsNOy/7wb7nUxtV08JDumBAWeQMOe+rBO4suoL6xXZTZzOZB6FE6ig3Kd1zfiVoWMhVSBi+l2S7DJEAzyy0A4m48vQXnTsxfiRQcEsbRoQLT/5XB4dJqZYbErU7VQDMFPWS79MdQHZaM4phv6AFfrEBieydAy3/iOKtyc4iexnUzf9xox9QcYdA0PiXxmykafWUJsVdfaiHA5nFuxAVYVbKhLNKqwe8JHvEycwClknSq7FSZZDWQAyYuWDJXlTsV8o3q9ykjIHuzjxAzcqV/tEoh+r7NvMtgZN2X3qpgTDjS16wPm/mLaFnWcuM30O8UlwF+rrUGuT09uMeqcwwZ4oogp9DNgjZChDWLpkW6ovBoECG0BeWW3WPy6xESJwbI6hCNQeCGNjI3s4JurhDaWZjDZMGYrpyRJvjAgwFura+MWfRhhtKyKT7b1BRg/jR/EZjFZAX2NbcG9+WXqYUqgrqY/wqbtkWzh9dNq50nPe c3IX1mQ4 YuqTB+Tg73TzCYC3H+Z7mVcCWbeT6S2AiRiMQ/9MC6JngJ2qJ+6gfR0MJzidSlntMO0M9hs9xkuVS3zuwEwLLQIYkyAUj+OpGd3pA0gvp4z3ozbRnVLKsy6WH5DLk8VGxAgzDawRLVqni+GWsS+775skyRCGELIH9AgI83atRvCmm9mDKJGibeB4ftpfl1Ty+MJWlB15HnxOkIo2J6Dab4xWsAojh/oxlN8s3RWIAdrzcPlgJGEXJHFnwwPfx1ao06fKB2838Q5v9fPitPFpD0+Ve2eG8mkTWXsSUXK//jhF3qaD9F6hyfB+lVnjCYmy9ssQf1Tm4pViGtCTdrN3bUldhiBOUU64PTv0Q3bzewUh8WzeSIYt2+A9iIhtM2hRAKACG65R+nZ35J7o= 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 7/24/24 11:53 AM, 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. > > 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). > Additionally, this is some internal implementation detail of memcg > and should not be accessible outside memcg code. > > 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.com/ > Signed-off-by: Muchun Song > --- > v2: > - Remove mention of objcg_lock in obj_cgroup_memcg()(Shakeel Butt). > > include/linux/memcontrol.h | 20 +++++++++++++++++--- > mm/memcontrol.c | 6 +++--- > 2 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index fc94879db4dff..742351945f683 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -360,11 +360,11 @@ 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(&cgroup_mutex)); Maybe lockdep_assert_once() would be a better fit? > return READ_ONCE(objcg->memcg); > } > > @@ -438,6 +438,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) != NULL; > + return __folio_memcg(folio) != NULL; > +} > + > /** > * folio_memcg_rcu - Locklessly get the memory cgroup associated with a folio. > * @folio: Pointer to the folio. > @@ -454,7 +467,6 @@ static inline struct mem_cgroup *folio_memcg_rcu(struct folio *folio) > unsigned long memcg_data = 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 +475,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 = folio_memcg(old);