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 6CF00C6FD1D for ; Wed, 15 Mar 2023 21:44:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E389B6B007D; Wed, 15 Mar 2023 17:44:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id DE8C06B007E; Wed, 15 Mar 2023 17:44:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CB0C76B0080; Wed, 15 Mar 2023 17:44:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id BC79F6B007D for ; Wed, 15 Mar 2023 17:44:31 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 8B71416098C for ; Wed, 15 Mar 2023 21:44:31 +0000 (UTC) X-FDA: 80572461942.04.F47DF99 Received: from mail-ed1-f54.google.com (mail-ed1-f54.google.com [209.85.208.54]) by imf20.hostedemail.com (Postfix) with ESMTP id 9E5461C000F for ; Wed, 15 Mar 2023 21:44:29 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=KE+5xcsZ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf20.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.54 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=1678916669; 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=xS9QHmxXIu3DIhidQgO6UvYgPUAw45nJ7B6l2iIDBks=; b=vxYm2oGaR2epFxUDDaDD4DjBMkFEc/lRVBsNx3Ie1OIhhLcj85Qopu8sW5dDnsdv86JkJ2 QfjH4PpR2K2ipZRDL3L/ybEvLJr7o0QHFRShBNM3xKO0684sTQ1gaOByMXXcQ0lIPVMJjF v6o5+AOwxAXXmpyrZIaoUBQCiz8nUw4= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=KE+5xcsZ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf20.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.54 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1678916669; a=rsa-sha256; cv=none; b=SzMfreSPzb6mQUBAlPOW6u3gcK/TNWTgJ3f8FvfHtE8YHOxQQeVUqtCv4rMIAcGwmszYoK +ZJQ3ztS05IT82OF7d3mHXbuvUfwBqAlOaqMwJV1StXecATHJT9vzuYC0LlFwDMVuEPI5Y Ct+hn66lI6+jPcwQkrYsXi2DptQKgNo= Received: by mail-ed1-f54.google.com with SMTP id y4so115636edo.2 for ; Wed, 15 Mar 2023 14:44:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1678916668; 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=xS9QHmxXIu3DIhidQgO6UvYgPUAw45nJ7B6l2iIDBks=; b=KE+5xcsZLzBqQb6gvedBFvhYUI2Ss6NdsJFxy6K3P9O+4zmRIYaXSgsfg3KQ1ZYzew 28CbANzT0zZNc+HzsUBw47D/7Y8RXIE5Nq8IDIj2usRfHWlBlHXyCtQswgVc26h6dAjh 2ijXhxdbSSVFo/ajPzw+P9M4kHMuFuHIyTtlvbsbIrqBIYa+3ujV6lU4tqqCdopOoFSt MiEoh5FBxBp1NCeyzJGfaZWmV2NNm43KcjxGtoZy09MOEi+vZFiu4yC0y/tMg9v6s9L1 XjZ8q1u114WkY585sov7cbjFe5Z2ccXxZeBCltCz2VHhxAmPpeK09YfbGp88/LNez5Us TljA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678916668; 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=xS9QHmxXIu3DIhidQgO6UvYgPUAw45nJ7B6l2iIDBks=; b=tJxUuu1WQINGLgk7dsOX+SE2qkb1kUcLT6uvMXYqr3+V4WpE8gAiNZsZyAi+dMG+7k vJrUwb6O/Gld6tk1F9E5GR6Ts5S4GOY4uaVBkWQ6Na2F4FuxeDjo156bw+aPdTtm4R2h kwxX4BX6hF3tZXu4pspEOklNiQj44t5OGSyu/dXCtddculgXZhn0KlW1Rd/ESwg0ca/l oJD4xdFDLuTdl7LVCi1i2Hn2BeP9CmWYSGW0dqS59zGwNWlgfP6HyQWMA7y+4qO2j85M n1dUGS/ayBMTllqxkMqfn34fmGsWgZX9yLjpZZVI9VaXxYcSINSgLllqQoQqCcmdUczS 4bVA== X-Gm-Message-State: AO0yUKXG9X/jkeNNLwWvA6JofuVc+P5rHGMMNAFWyK+Xg82GFJzVh8lC GGdGB9Y26SXsYC0MHu9C7KyCVpL8Hp0SI/5zgShX+g== X-Google-Smtp-Source: AK7set/BTdj+LtbJR4/bFPCrNgmDa7VLYAoNE/dvKKcduNoEj3PrXV+57EiLZqthSyPbIcvFu+TWKJRrsyOZXfS2SOE= X-Received: by 2002:a50:a412:0:b0:4fb:9735:f917 with SMTP id u18-20020a50a412000000b004fb9735f917mr2285440edb.8.1678916666848; Wed, 15 Mar 2023 14:44:26 -0700 (PDT) MIME-Version: 1.0 References: <20230313083452.1319968-1-yosryahmed@google.com> <20230313124431.fe901d79bc8c7dc96582539c@linux-foundation.org> In-Reply-To: From: Yosry Ahmed Date: Wed, 15 Mar 2023 14:43:50 -0700 Message-ID: Subject: Re: [PATCH] memcg: page_cgroup_ino() get memcg from compound_head(page) To: Matthew Wilcox , Waiman Long Cc: Andrew Morton , Hugh Dickins , Johannes Weiner , Michal Hocko , Roman Gushchin , Shakeel Butt , Muchun Song , Naoya Horiguchi , Miaohe Lin , Vladimir Davydov , linux-mm@kvack.org, cgroups@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 9E5461C000F X-Stat-Signature: q95778j61dk6f9wb8suksot9j35w35z9 X-HE-Tag: 1678916669-258838 X-HE-Meta: U2FsdGVkX1/pLGrrX2YaCtB9uk/F1ok3rOorMwYWgkMdTHDMzBHjTeQNDJG45vf3Gk/fQoL/qtkBul9spHzUiyRpnVpUT+4VAuazmt5+xXAOnHRZRZYGFeWgGACCZPI/RILPNQjQhQoMJ1jBvPOVD+gclh19SKvYPRjKz67gkvK7AD4LkrEAuCCFH1oMTLu6k1fOFgCy7qCFoEQRFBFIFAvkD4sxRiMXO9lMWswI7YVS7nuo6Hn16SBhF0zaR+qwwbXQ9LeK7yKxpIMk4H7zjIinHDlvTBZq0qFpjgoGSbNg/eLMa/skRf3YAuCWi6z0gfU3LxfeL2BhHZgdddllsec99zPv2+H4PgQchSo1gAvVO+olXG3Kmptwo2uGfIx6gwGlISDKuoidzIdgn6nrS+iNhk2gntxAOM4i7od+rgPKddvXLcodaRyUGMEbAbB9Jn1HsKSA1C37scvHr0vgEhF0jTlhzFvihEgvCAVJ06oTY6vjaIHB7HtGZz1knZ8HBeeyhjU/AudWvKrJJhLuj4rTPZGbbiCiVLOyMxzoS+HfghWAeILILEBdwLhPNofO35kzs0EbJKYHuaSqX8ZeGFK6OcIvKB/pftl7rUiD8bB4AIsNsKP10cgaPSdTvZISfvWJFxqUGyypwQb7Wp9rfHZA30hCNq+MnPXK7onWZxZyFNS/mCR+xcx8M7jXr2Ay7M8MdzyBAQZwfNG0DNNP9S8bmxn0qRRMihuQst3StLKveoEOLtB7rMwCYwVqmaN/vUIWxEVCtGPKSfbhxTPiEd07JMgyaDtParILQ3f61jXuh+8R1dMl5Wp5I1Ui/RIhDKx+qUW711WEoWtjDEw09U8FHbF2vXq4uFiUSkcg1TPmf3O0rYeJOaFWjztsGd14ukdOen0MerwpOQRc5XtCpy7KaN0EdfoMImMthgApz6AHrvLIGxsslvwXCHvwwij8hG8MalsVe3R9o1aND27 xr4c2Qc0 1dPcpgA2lpY8o6hfab4I5LLtb1cDTnsGp8kVt71IPxkmHMMj4fescGU4p5xWmjhPmlIIJDxzSBk55ZEuGMXNkrWHB25vMwK4APjg6Wuns3iIuH+KKKo3lIO382Joz+VTwpJQe+GokDR5HUBrwrRHjCzVu9Y6dlr6AHtBEoHvO4fZ9OIkB8gxcbcjsP27ln9ZPlKYtQAPu8clPHp7NIEex+FLTBhqcdQHUqarTfe50KED/5xWEDHsGHwsTSESw5yqanImG8CVHTyLe3FblpK7T5h+WGEmRQU/n6fRsec6II5LiKudYH4MDxylKCnUh8RKfVrTwWf1qYquKJMHKJARsKvP7DPyxNaM1isDyumvDEZpKr6in5fqF/EB2lStHMelwq9+Rd8aMM3VlS4IHM93hyRMdny3xDpMmckusXLwZ5dHRYafCOo9bAI0wGeQefTnsSDmLUtcy9qfCqhUMG85AsCPJ3ekkFGJ/QEKkeGlhdDqZwvNkXGHQ/XUnYg== 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: On Wed, Mar 15, 2023 at 5:19=E2=80=AFAM Matthew Wilcox wrote: > > On Wed, Mar 15, 2023 at 12:04:10AM -0700, Yosry Ahmed wrote: > > On Tue, Mar 14, 2023 at 9:54=E2=80=AFPM Matthew Wilcox wrote: > > > > > > On Mon, Mar 13, 2023 at 02:08:53PM -0700, Yosry Ahmed wrote: > > > > On Mon, Mar 13, 2023 at 12:44=E2=80=AFPM Andrew Morton > > > > wrote: > > > > > > > > > > On Mon, 13 Mar 2023 08:34:52 +0000 Yosry Ahmed wrote: > > > > > > > > > > > From: Hugh Dickins > > > > > > > > > > > > In a kernel with added WARN_ON_ONCE(PageTail) in page_memcg_che= ck(), we > > > > > > observed a warning from page_cgroup_ino() when reading > > > > > > /proc/kpagecgroup. > > > > > > > > > > If this is the only known situation in which page_memcg_check() i= s > > > > > passed a tail page, why does page_memcg_check() have > > > > > > > > > > if (PageTail(page)) > > > > > return NULL; > > > > > > > > > > ? Can we remove this to simplify, streamline and clarify? > > > > > > > > I guess it's a safety check so that we don't end up trying to cast = a > > > > tail page to a folio. My opinion is to go one step further and chan= ge > > > > page_memcg_check() to do return the memcg of the head page, i.e: > > > > > > > > static inline struct mem_cgroup *page_memcg_check(struct page *page= ) > > > > { > > > > return folio_memcg_check(page_folio(page)); > > > > } > > > > > > If you look at my commit becacb04fdd4, I was preserving the existing > > > behaviour of page_memcg_check() when passed a tail page. It would > > > previously, rightly or wrongly, read the memcg_data from the tail pag= e > > > and get back NULL. > > > > Right, I looked at that. I also looked at 1b7e4464d43a which added > > folio_memcg() and changed page_memcg()'s behavior to use page_folio() > > to retrieve the memcg from the head, which made me wonder why > > different decisions were made for these 2 helpers. > > > > Were the users of page_memcg() already passing in head pages only? > > There were 18 months between those commits ... I'd learned to be > more careful about maintaining the semantics instead of changing > them to "what they should have been". > > > > > > > I suspect that was not the intended behaviour, but I do not think thi= s > > > patch is the right fix; it simply papers over the problem and maybe > > > creates a new one. Callers of page_memcg_check() should be eliminate= d, > > > precisely because of this ambiguity. It's up to the people who under= stand > > > each of the callers who need to make the decision to always convert t= he > > > page that they have to a folio and ask about its memcg, or whether th= ey > > > want to preserve the existing behaviour of returning NULL for tail pa= ges. > > > > > > So, I say NACK to this patch as it does not preserve existing behavio= ur, > > > and does not advance our understanding of what we have wrought. > > > > I am not sure which patch you are NACKing, the original patch from > > Hugh (adding compound_head() to page_cgroup_ino()) or the suggested > > alternative patch which changes page_memcg_check() to use > > page_folio(). > > Both patches are NACKed. page_memcg_check() needs to go away > because it has the tail page ambiguity. Both callers should be using > folio_memcg_check() directly and resolving for themselves what the > correct behaviour is when seeing a tail page. > I agree. I even suggested this to Michal in one of the replies. For page_cgroup_ino() we can simply pass in folio_memcg(page_folio(page)), which would mimic what Hugh's patch is doing for page_cgroup_ino(). For page owner, I am not sure if we want to do something similar (which would start printing the memcg for tail pages as well), or explicitly excluding tail pages and THEN do folio_memcg(page_folio(page)) to get the memcg of head pages. Waiman, what do you think?