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 1C375C25B10 for ; Mon, 6 May 2024 20:42:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 90EF06B0099; Mon, 6 May 2024 16:42:19 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8BF8F6B009A; Mon, 6 May 2024 16:42:19 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7874F6B009B; Mon, 6 May 2024 16:42:19 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 54B956B0099 for ; Mon, 6 May 2024 16:42:19 -0400 (EDT) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id F3F67A0328 for ; Mon, 6 May 2024 20:42:18 +0000 (UTC) X-FDA: 82089143556.09.7BE2D8D Received: from mail-ej1-f42.google.com (mail-ej1-f42.google.com [209.85.218.42]) by imf21.hostedemail.com (Postfix) with ESMTP id 28AE61C000B for ; Mon, 6 May 2024 20:42:16 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=FtwGayzl; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf21.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.42 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1715028137; 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=IqjodXzTW0rNwIpAxPfnGxHwUTylHmXn+/HV+LShC/Y=; b=nduR6N9L7Waitl3eXProC8KpFNw7ppg1tZq95akfbldyAXSHKkhXrmszDbJ7jyZ0SdfYav XlbwfvwOl+Zv+R24gDVOKiYrviQ8+tMbO63nGCfT3BjeujPAWG9QNrfEem44tYpH/w9UMs I6IIC3Rvf81aM6d1vihfwE21Gy4HxIM= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=FtwGayzl; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf21.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.42 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1715028137; a=rsa-sha256; cv=none; b=SZu8iQ22wDraed1mw8KjvHnvJ4xZCoaaSansKViRs5deMqHv8mUx3Ozes8NWZjpwt0MKuA rs6yf5xjB9HiCxS7p8ogq1RHYoC5Jjvu0dnHbImvjd30NhcLm0mQBFkh2vvUtHD8DYPhFf WxQ7qXsoLwQjmsRfL6ciN+J71y54ojY= Received: by mail-ej1-f42.google.com with SMTP id a640c23a62f3a-a59a5f81af4so609397966b.3 for ; Mon, 06 May 2024 13:42:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1715028135; x=1715632935; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=IqjodXzTW0rNwIpAxPfnGxHwUTylHmXn+/HV+LShC/Y=; b=FtwGayzlt6CB08XvNZsKUVRuA/6igUH5RvD1dx5LNcKv5uldU7w4mYhjTBEO0GX5IK FUa8At0lQYbz0VMWlVxM1uNABMTk+QgC/pM4X+44LYaE/mQEVLcqmJo1hbItFUV00pli gm7tjxCJUjGujOT1UBqIvyuT1eMdiukmpBwTVYLTnnH/KIydC0Pfk0xNWcJYhu2WKSCG pXStI4wGk07+jykMtqzCeyZ9Cp9AQIpWB4r9zYY2tlU2FZfQoPWN/TxKfU7jBa+wBR3A gcB1JfVvxSf3mXUPQlD+Ns3I/PERqtqgIR/FddRuqcW7YQ5oyo0piVY2E2vzDXNVwuCM gIqw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715028135; x=1715632935; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=IqjodXzTW0rNwIpAxPfnGxHwUTylHmXn+/HV+LShC/Y=; b=Mg8TwOVp8Y3MlWGh53S//m2/OnmbpGZr4Qh71BrCZfpJsbkHfYTbJ32hDfy/bNQSEz Am8lrejnkr/oxOOFRt7Sp+0kRL8ZEZNlhMJ4rvJy6CtYm88/YYYJmGhlnMzUPfTQEZOb qeWkMxyIxwoMjDluV9OgUgE0NgsxEOzOV83zWwdBb0yX2wy0pX+3ias/g3I4fnZceDxs HSbV86L4WqMLfIzcC6i6SUFyRYtaM1AXDLnC/oMBlUNKr5jl8QjQ0XNBTZbZN6cA7m3k 4Zr9togNuqXupFDaCBOPw75QLU6o9rHDGxN3EKVqkseYeDsIlaXpG7m6+6x4kTPeYKZo 6n0g== X-Forwarded-Encrypted: i=1; AJvYcCUdS/02M9Bk7HOxXrqWAHhIgOCG3NOOSV7vW83gOETLWX9ZWY9/LcPcp8oKcxcVMSIJS61z5aPUqbH0IKZ0KUo0OgI= X-Gm-Message-State: AOJu0YwsQTmWkVagJXb/gXPHQNeE+hDOvjGHFcVmKk9d8EEkIPdpL/Br Oqt/PnfTWTfWrmqznQfhA6FlNKRJCQZRSdFDI8r1AheGM0MSYuO39O4GIty3Ha6GSfx/E8NCkj8 VehncGO0I4jLfq+vlhrHcZeeWCxUCQ59Z3DEp X-Google-Smtp-Source: AGHT+IEWbexmwiqgS6EJbE9+us5EtjbzkfVqomXoksmrmMeAzkfSh6xHMTpgAE+iaoShueQeiS7QwhDw8vmoE4t1Tx8= X-Received: by 2002:a17:907:6d03:b0:a59:bc9d:a0a3 with SMTP id sa3-20020a1709076d0300b00a59bc9da0a3mr5109816ejc.75.1715028135229; Mon, 06 May 2024 13:42:15 -0700 (PDT) MIME-Version: 1.0 References: <20240506192924.271999-1-yosryahmed@google.com> <42b09dc5-cfb1-4bd7-b5e3-8b631498d08f@redhat.com> In-Reply-To: <42b09dc5-cfb1-4bd7-b5e3-8b631498d08f@redhat.com> From: Yosry Ahmed Date: Mon, 6 May 2024 13:41:38 -0700 Message-ID: Subject: Re: [PATCH v2] mm: do not update memcg stats for NR_{FILE/SHMEM}_PMDMAPPED To: David Hildenbrand Cc: Andrew Morton , Shakeel Butt , Johannes Weiner , Michal Hocko , Roman Gushchin , Muchun Song , linux-mm@kvack.org, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, syzbot+9319a4268a640e26b72b@syzkaller.appspotmail.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 28AE61C000B X-Stat-Signature: ya9sp17zndskr6nziqbgmz71piex35pp X-Rspam-User: X-HE-Tag: 1715028136-896198 X-HE-Meta: U2FsdGVkX18kgEn4gOe8XttuHkPjtEpPXb4SD4SggxhDhxn6G3qu6EhAt4/V7z+ig6/xRhyIUHdooeFekOsD5ZAHwcUpOkCjNjkmUSL/wFW+lQOO+Fuw7Yx+vVF+MOWvMUAcEudQ2TqZKcCCVApmDsXtSHLNzZgwCpPsbgfOQ8KVAwLlmE7w58r1WKJvU/18nXkYWx7Y1Z/PatqTvhExw7a02XkewoxASt7rPFtEczJRioQMaVgQtdtvsQ+GwSJDqbeUxWhXutG1uwXemkMFLJtpxDYIKPiax0KFt17tfIrmfj5OaVgDiFkm/1cZZXYiL2W0uq5FFcLD5aTHdFftWZZjti5sbHgZVMb/9VYl5Rq17Myja2xiHAhulhiM0LdmA+HgkEb4PljpF7PCxr1DdLKxzs93YbyKpoY+M90cC9V5ULbWxBHHkznek34h6SJN1SYUYi9j0Oxhfim93zyzsbWgbcDdP2LoSioi4Hflnhn1YR3YA6bnV7H7kAB2Uj0YQ60lTjI1uW235u6ISw8UWIDIG+fGz/MUW21Oijtob4QF4gE/Bh0RxXwTIvCvcNlrAU4kvzpMUd67uNQuT5wBthXRDQC1l873Fc9l8F8WZxzzvFZXQyRIv1vKFUExO2UYrPr0E/ujWEk3Ms6EvsiccX/PmPO3qQooLeK519S+ABwwwTJlKy6w5Wl5XhHztR9roIHE4VtFcO7q9LEl0zPPObj0vJR2L+HXx8Y/k1PNH5zRlUXFHJksmHMVlEzbsTkxOJ1AnqidTbApi5ELOlFOs4NGXBudO7h9KW/fvjDnrh6g1aPTrTGK58Oqob1epRCTNjWQLEoJ4enz9LiEERIob8lrR+1hzXbcrV0t2XX9IA/KyxkZ8hJMYF4gUXqLd16MA4QTvjqvHYJtBR5YXJQ+iAIx1vsbcMvoV2FMzB1rlRUAwnhE9Hl+BFi1LQT93Zj0m0GgsF+0FEh9MCyI+8o +vgu0hoP zvNBbeZjH4TriiZ6CQQ38uNhjQHw8fl3uRsTs84wqKUiQxc//yCqCDYtOxsCFL7NqU+CYXFhBsOyEgJgy8NF1Wc6WaaESGJnH2Nm2U12tTaa5G7Oljxv9DpuXAUSDrRpMMX5J1upFtqe+JJ4W9L2kSHP0ChAoAhTiqJiy7WwnPFr+4bFy47pNeAjTRMw2KZiNAKOKY8emjWU8Q/7pZ+L0uy1MRFU6vcpEG1PvzyPEk4Lc+x+mhmDlHw1IRCaMfmILTiiuAGu1FCMkgkk/YPOKraaAubWwBW9hgeEfU+1sCVM2O+ryanOlKt1uT3dWSnzKnFpQdvQFFbTZgiXZofVD4Ogf3yg/ZCpX9oEmexz+g2Qm8R8K071gY6fC1m72CTS8QKPTHo1kFHexQ+LYlIQPFUp7XTEwiNA7LiiNxYrt0AKR7G0HJlaxil4fMllEzveZZmfuKx/Bp5zL0lLT9kId3cSUqoGJOL0vY7h7PzcNsD52xJv8zc3j47pVhr57k8O4OIUFYT0upmLruOgMm8DkwQ248zbBAvrTBzL8 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 Mon, May 6, 2024 at 1:31=E2=80=AFPM David Hildenbrand = wrote: > > On 06.05.24 22:18, Yosry Ahmed wrote: > > On Mon, May 06, 2024 at 09:50:10PM +0200, David Hildenbrand wrote: > >> On 06.05.24 21:29, Yosry Ahmed wrote: > >>> Previously, all NR_VM_EVENT_ITEMS stats were maintained per-memcg, > >>> although some of those fields are not exposed anywhere. Commit > >>> 14e0f6c957e39 ("memcg: reduce memory for the lruvec and memcg stats") > >>> changed this such that we only maintain the stats we actually expose > >>> per-memcg via a translation table. > >>> > >>> Additionally, commit 514462bbe927b ("memcg: warn for unexpected event= s > >>> and stats") added a warning if a per-memcg stat update is attempted f= or > >>> a stat that is not in the translation table. The warning started firi= ng > >>> for the NR_{FILE/SHMEM}_PMDMAPPED stat updates in the rmap code. Thes= e > >>> stats are not maintained per-memcg, and hence are not in the translat= ion > >>> table. > >>> > >>> Do not use __lruvec_stat_mod_folio() when updating NR_FILE_PMDMAPPED = and > >>> NR_SHMEM_PMDMAPPED. Use __mod_node_page_state() instead, which update= s > >>> the global per-node stats only. > >>> > >>> Reported-by: syzbot+9319a4268a640e26b72b@syzkaller.appspotmail.com > >>> Closes: https://lore.kernel.org/lkml/0000000000001b9d500617c8b23c@goo= gle.com > >>> Fixes: 514462bbe927 ("memcg: warn for unexpected events and stats") > >>> Acked-by: Shakeel Butt > >>> Signed-off-by: Yosry Ahmed > >>> --- > >>> mm/rmap.c | 15 +++++++++------ > >>> 1 file changed, 9 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/mm/rmap.c b/mm/rmap.c > >>> index 12be4241474ab..ed7f820369864 100644 > >>> --- a/mm/rmap.c > >>> +++ b/mm/rmap.c > >>> @@ -1435,13 +1435,14 @@ static __always_inline void __folio_add_file_= rmap(struct folio *folio, > >>> struct page *page, int nr_pages, struct vm_area_struct *v= ma, > >>> enum rmap_level level) > >>> { > >>> + pg_data_t *pgdat =3D folio_pgdat(folio); > >>> int nr, nr_pmdmapped =3D 0; > >>> VM_WARN_ON_FOLIO(folio_test_anon(folio), folio); > >>> nr =3D __folio_add_rmap(folio, page, nr_pages, level, &nr_pmdmapp= ed); > >>> if (nr_pmdmapped) > >>> - __lruvec_stat_mod_folio(folio, folio_test_swapbacked(foli= o) ? > >>> + __mod_node_page_state(pgdat, folio_test_swapbacked(folio)= ? > >>> NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED, nr_pmdmap= ped); > >>> if (nr) > >>> __lruvec_stat_mod_folio(folio, NR_FILE_MAPPED, nr); > >>> @@ -1493,6 +1494,7 @@ static __always_inline void __folio_remove_rmap= (struct folio *folio, > >>> enum rmap_level level) > >>> { > >>> atomic_t *mapped =3D &folio->_nr_pages_mapped; > >>> + pg_data_t *pgdat =3D folio_pgdat(folio); > >>> int last, nr =3D 0, nr_pmdmapped =3D 0; > >>> bool partially_mapped =3D false; > >>> enum node_stat_item idx; > >>> @@ -1540,13 +1542,14 @@ static __always_inline void __folio_remove_rm= ap(struct folio *folio, > >>> } > >>> if (nr_pmdmapped) { > >>> + /* NR_{FILE/SHMEM}_PMDMAPPED are not maintained per-memcg= */ > >>> if (folio_test_anon(folio)) > >>> - idx =3D NR_ANON_THPS; > >>> - else if (folio_test_swapbacked(folio)) > >>> - idx =3D NR_SHMEM_PMDMAPPED; > >>> + __lruvec_stat_mod_folio(folio, NR_ANON_THPS, -nr_= pmdmapped); > >>> else > >>> - idx =3D NR_FILE_PMDMAPPED; > >>> - __lruvec_stat_mod_folio(folio, idx, -nr_pmdmapped); > >>> + __mod_node_page_state(pgdat, > >> > >> folio_pgdat(folio) should fit here easily. :) > >> > >> But I would actually suggest something like the following in mm/rmap.c > > > > I am not a big fan of this. Not because I don't like the abstraction, > > but because I think it doesn't go all the way. It abstracts a very > > certain case: updating nr_pmdmapped for file folios. > > > > Right. It only removes some of the ugliness ;) I think if we do this we just add one unnecessary layer of indirection to one case. If anything people will wonder why we have a helper only for this case. Just my 2c :) > > > I think if we opt for abstracting the stats updates in mm/rmap.c, we > > should go all the way with something like the following (probably split > > as two patches: refactoring then bug fix). WDYT about the below? > > > > diff --git a/mm/rmap.c b/mm/rmap.c > > index 12be4241474ab..70d6f6309da01 100644 > > --- a/mm/rmap.c > > +++ b/mm/rmap.c > > @@ -1269,6 +1269,28 @@ static void __page_check_anon_rmap(struct folio = *folio, struct page *page, > > page); > > } > > > > +static void __foio_mod_stat(struct folio *folio, int nr, int nr_pmdmap= ped) > > +{ > > + int idx; > > + > > + if (nr) { > > + idx =3D folio_test_anon(folio) ? NR_ANON_MAPPED : NR_FILE= _MAPPED; > > + __lruvec_stat_mod_folio(folio, idx, nr); > > + } > > + if (nr_pmdmapped) { > > + if (folio_test_anon(folio)) { > > + idx =3D NR_ANON_THPS; > > + __lruvec_stat_mod_folio(folio, idx, nr_pmdmapped)= ; > > + } else { > > + /* NR_*_PMDMAPPED are not maintained per-memcg */ > > + idx =3D folio_test_swapbacked(folio) ? > > + NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED; > > + __mod_node_page_state(folio_pgdat(folio), idx, > > + nr_pmdmapped); > > + } > > + } > > +} > > + > > I didn't suggest that, because in the _anon and _file functions we'll > end up introducing unnecessary folio_test_anon() checks that the > compiler cannot optimize out. I convinced myself that the folio_test_anon() will be #free because the struct folio should be already in the cache at this point, of course I may be delusional :) We can pass in an @anon boolean parameter, but it becomes an ugliness tradeoff at this point :) Anyway, I don't feel strongly either way. I am fine with keeping the patch as-is, the diff I proposed above, or the diff I proposed with an @anon parameter of folio_test_anon(). The only option I don't really like is adding a helper just for the file pmdmapped case. > > But at least in the removal path it's a clear win. > > -- > Cheers, > > David / dhildenb >