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 BFD76C3DA49 for ; Fri, 26 Jul 2024 08:09:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 46D0D6B008A; Fri, 26 Jul 2024 04:09:42 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 41EE76B008C; Fri, 26 Jul 2024 04:09:42 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2E49E6B0092; Fri, 26 Jul 2024 04:09:42 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 133856B008A for ; Fri, 26 Jul 2024 04:09:42 -0400 (EDT) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 921B01A16B5 for ; Fri, 26 Jul 2024 08:09:41 +0000 (UTC) X-FDA: 82381179762.02.C73E81E Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf06.hostedemail.com (Postfix) with ESMTP id BAFBD180015 for ; Fri, 26 Jul 2024 08:09:39 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=uZqdhyYl; spf=pass (imf06.hostedemail.com: domain of vbabka@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=vbabka@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1721981331; 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=VF/+lKgvewoP3+7eT6VWHsesZJ9Uq+FhlvXjSp47dHo=; b=KnvEWhP7pvMU7B4NDNYfZjBTZpZW4pw8d7NT+qp4z+BFP6aR/2/JHItxjNKWKYXdhT32sw 5jkNBp39wGD7HWH4+oAnM9jUMgPbgcmdVY1aiTdckS5PPxIDpyi5nUoECfJPOeKdnkkxb6 Qe6i7j9R7HRU2WuPGX9tdN9G+XsLgKs= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721981331; a=rsa-sha256; cv=none; b=L8ojJpjH8mwf/6XGLPdR6GhwDvV0iBvu5yWG/GWRmxCYB6moa/R1riIuXZ70r9f5kOHQQL /ApWhXV54kQMk+uF9NeplmXzH5P+awjAGrVDBk4hXBubmJhEsMhbyg+Z8ib6wwDIlT6HK0 mCwSIqOOqa0l3APh9T6znk1ne3kHszk= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=uZqdhyYl; spf=pass (imf06.hostedemail.com: domain of vbabka@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=vbabka@kernel.org; dmarc=pass (policy=none) header.from=kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id BA9B8616F8; Fri, 26 Jul 2024 08:09:38 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 81299C32782; Fri, 26 Jul 2024 08:09:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1721981378; bh=Zs672yaxzd0Uq4FobmiIgckn9CgVGQcY++GMEOQ5twQ=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=uZqdhyYl1etnNDCMv+ojEUu/AP3TY0ch2kY/gAw9QJtuF5V/4yg4UUwGWWQ2vOGZr WEo+MJ2BTXcdrcz6YZhU8t+NHkGCEB6ka8vvH6dlr/zGYOVfXP9vaKoH5u6Xg7RgOj jjgHv5jkbtNybRXs1qHoY1oN1UmT0HhZ2GlZ9y56Yp0oX58ltNLkJK6xyLc4Aj03mn DIAUO4WdQGHd8omj8GsZiYiS/iciOd76+edMCyUmsI2XEIZ46HRSlT6stpHiVyou7j IEtzGqWpViVcV1m6oI9+h0ZrjM38kfKi1ZNbKKjvYIood4RqJiLERJk1v5HtFMMKq3 +imaony6tHIig== Message-ID: <8b04dbe3-cbf1-4d97-ab42-ebaae5974b40@kernel.org> Date: Fri, 26 Jul 2024 10:09:34 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3] 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: <20240725094330.72537-1-songmuchun@bytedance.com> From: "Vlastimil Babka (SUSE)" In-Reply-To: <20240725094330.72537-1-songmuchun@bytedance.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: BAFBD180015 X-Stat-Signature: 5zr6ayoad9hsh4scchbmiuxbuhxfmc5w X-Rspamd-Server: rspam09 X-Rspam-User: X-HE-Tag: 1721981379-762052 X-HE-Meta: U2FsdGVkX19JeQHm8HmTH/He7BbwjlBTDE8JioHtqfk8vBPPeGKftX01qZ1JZccze9CIklTd56oT6RlxVPSTdnrLC59zQtUu/x2Ayww/UMpUSPDxc3MzaxT393hKXVenGlgk8qfpfABLUuNtpbmBNjVJVmsn8J2LgINB+foTrgrU4T+UnRkgpziIAt9NiIS5ahFJVDsZzxfunNZC9T8vxP4Mz7y5LfWHKMElIho7MWvTolLsPN8oFue4stzdQOVZCnKbfcXd77DphPCtC+8YcfTU14BIt4aZqDVeZwE67FMH7wf/H2gQv7sbzFws/ewQu5mwSnVDs/4HhKFNM4bi3Bd+Szd8vl7OYSv2UgbHOwMxt/FwUa2rpwYge33kbHODSdkHYS4aEvUm/DAMIC4nO2Fm0pXbLEs8sC6fT0yg3bdjcu3ufikXD1tgUQNxTDVITnFFJoHr5r0qSB9cRTOop4MbJRz/WsHtnMRlAvg+5zbLW0e+Q1rck0679sAEaa571AkaViY800M7A7iGac6b+xVCeix/h66eGsSuu47Gg/W5KVrgiBMqHHwzeWvvbaJDsZdV/S9YaUehrukoYgvBK5JycltLdheeRB2H9zZIN8BdQmLx5fw+fJq2l0dYd/oVzP5EfDW3uRWIcZnZxP8Cuz//4a4hqSASEdapyxcyhBAClv9phhXGFFb68A7vTcdR8s35apG3J6FiiMGlZIGfy01R+zlHaHA4q4+jzoFgPYB8gbYm8JRvJK+AvTCyCLLxBmLHNs2S9hvPKT1olAfeC2QiU8CROdEYepn9LrhTMrtY+UskGds2+BGoWXChbq9YrBJk4h1jnkaX48z/a/9PURPRkK7d8xXhLo84K75N4eqIu+FQwBgpFlqUE0ZUyAftGNmJZI/DSmDE5w7Nhaagw/4UfcmE4a8WzmU//atOEgtArdS7D8o+NUJWrRm+/6YQmi5ILbl2F0LZBe2YyNy ecIiAzya ecazVl5sTNKiuJaHpcOcSA4EXcry/+jjaCIa3M5Gx/45YvmP57mfMYjiExMkAOU+mXl0v+kV1sQQxigl01OjZZCawux04Ssf5Bs75R5zTf3940DnZAjOKmbwCgjw6TzdRXzAauCceE0ebKmIEiawrFe+nDsVFwcqUvCRIvD2r9/2CYrippXV2IKo/6lY6SxjNKh2JYITa5PPsf+tgXigfRbds8FDrjnQsub+3aUBLlCT+jFE13ubTmZ5CVPWOzxIyAz6+a6D6zfo2cKCM3qDMGq0mLjcbbd8gwgy++596EqvSGh6aY3PMBuErppE5VUg2KK8VNdPbCvXFF0l8hEfsqcX//JyO7dggcYLUcGoqC2DpOUGzWO6YR6w5MERBXm3WUK1s7j1i7YLzHnw= 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/25/24 11:43 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 Acked-by: Vlastimil Babka > --- > v3: > - Use lockdep_assert_once(Vlastimil). > > 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..95f823deafeca 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) > { > + lockdep_assert_once(rcu_read_lock_held() || lockdep_is_held(&cgroup_mutex)); > 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);