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 25D60C6FD1D for ; Thu, 16 Mar 2023 00:26:33 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 466596B0075; Wed, 15 Mar 2023 20:26:32 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3EDA76B0078; Wed, 15 Mar 2023 20:26:32 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 290186B007B; Wed, 15 Mar 2023 20:26:32 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 178696B0075 for ; Wed, 15 Mar 2023 20:26:32 -0400 (EDT) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id D4D188133C for ; Thu, 16 Mar 2023 00:26:31 +0000 (UTC) X-FDA: 80572870182.13.0320AE0 Received: from mail-ed1-f49.google.com (mail-ed1-f49.google.com [209.85.208.49]) by imf19.hostedemail.com (Postfix) with ESMTP id 14FCD1A000C for ; Thu, 16 Mar 2023 00:26:28 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=UdYkNlgR; spf=pass (imf19.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.49 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1678926389; 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=qmCQ5bc+C9iO3H73ytNfZkYAlpZcblBVxwQDXjSAzCk=; b=1sujTVUdOUeQ65yZWqAZTkFKNaQGqMkzmS3mFuD5OvV92PcIpdla9CkUUhJFPrG9FoXAiL fhaWlU7yOXuCDbsMpRiv6lO3HxZGdgii0AOqf7gvmHVBzo62SGA7kbd/idftih0smMYZI8 7bSgMXp0nkiuU4dJL8WN/GzbN/Co5qI= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=UdYkNlgR; spf=pass (imf19.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.49 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1678926389; a=rsa-sha256; cv=none; b=mMk3w2fzhK8hs57EscjaasDro3DKc4Jx8OvokhMXY5y7kJD2eG1ksnf5Yugzp3XdxhsqY7 WRPZ5lpf3gaXVWLfU9SCTpy1XmIJkCusLeANr5WNzUWzzg+/3blDNj1WJ3rSc+rorY7uQm I2PY0DN6V58HIGOQlfbcDNw9+fs5Ugc= Received: by mail-ed1-f49.google.com with SMTP id o12so1230255edb.9 for ; Wed, 15 Mar 2023 17:26:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1678926387; 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=qmCQ5bc+C9iO3H73ytNfZkYAlpZcblBVxwQDXjSAzCk=; b=UdYkNlgR1Wtz/47/irEnt549HqrEdeO4feJz++7oONmHwyUT/Ut+foiSaou2eZrBWL TaKG9TBACFTi1lGwnfAvFhsGAHL1A2OOFE6DzQdbtmsz2tGK7w52hzCch/ab9zrKzVbm PERzqoBCV/zdgFWi8HqoelQBGIsAcutQ88iGFaHJNpzwBLgCLoo5BBLtdo3OVgNgiPtl 1vNUQyuQ6wO/42+/aNHQuhRYjZG3dbZLuoCdF9VE43IKnxSwT4oHo+wRtB6c8QRnTwmh MVRqel6WHmdebLAvFSV1/NnRIlimvBL6hyIK9UcHzxOt2ebUDKoFRZ7OfY/h9/lj+jvY qDmw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678926387; 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=qmCQ5bc+C9iO3H73ytNfZkYAlpZcblBVxwQDXjSAzCk=; b=l4S224X8JjnzZ2HmYaUGRpB0KtxJXC1LNO6i0PkBqVU33YejjJ2myruykxrb7SdkFF bkAyNyr6mPAJRpYf5+dbmV1KcHKCj2eQxg3Zcg1x6vm+XPfqFCmQ1ve8VoxioHRr7XTM gvCFezn5K5xhhvZU7HFHAffAoCgzMDzv13rKOMmAjyaIhVWk5aMd8VWfEcLgQ3uiIUWo Zb2D4N9m/GLQppq1/XLUReAn/64RwZT3Pda1sOR6qJxp8bk2ctTURzgcxyO3VLkNgf9m O9zMboQu8MEh9E3ker5jbzJhV39oW/3hjK3yqFgu02NWB+Ajke1u6GVfo4e38huYyb5L 7Shg== X-Gm-Message-State: AO0yUKUS/wgNDEP7SENYOdGMrKLn4Ou/ig38RKwiYMTMy88I+VbaCK06 DEyrpN6G2QUCufEFjX29Av4ePwertfHLjYwPw3o5uw== X-Google-Smtp-Source: AK7set9jkIOt4pi7NvFPVNNngZMHFf4dK7LYw5SaHhw1cQY8z8D+JIGqsxygKFk4F5L6wjlP+kZcS2hDsbIwbhobslw= X-Received: by 2002:a17:907:2069:b0:8af:4963:fb08 with SMTP id qp9-20020a170907206900b008af4963fb08mr4333797ejb.15.1678926387257; Wed, 15 Mar 2023 17:26:27 -0700 (PDT) MIME-Version: 1.0 References: <20230313083452.1319968-1-yosryahmed@google.com> <20230313124431.fe901d79bc8c7dc96582539c@linux-foundation.org> <8d4e1b74-6ae8-4243-d5c2-e63e8046d355@redhat.com> In-Reply-To: <8d4e1b74-6ae8-4243-d5c2-e63e8046d355@redhat.com> From: Yosry Ahmed Date: Wed, 15 Mar 2023 17:25:49 -0700 Message-ID: Subject: Re: [PATCH] memcg: page_cgroup_ino() get memcg from compound_head(page) To: Waiman Long , Matthew Wilcox 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-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 14FCD1A000C X-Rspam-User: X-Stat-Signature: tyrsxwxtthbtuae7keedyo3hcr48cjqn X-HE-Tag: 1678926388-997899 X-HE-Meta: U2FsdGVkX1/pjjaQWrwMbUKZEX/CgangonO/FDg752mnz1i9lNdQqRG4xmaeqpJ0XK7RKgTNA7eavhyO6wrg9RrWfxkt8Vl+Fu8wETi66S/+I/EzIUcoqDa2U3IeAIHHNNH8KFq/XT26qa/XtZxKeBkIo9nZdmBkdf3pylXp4vawU9uMjuX+ADoXaanw0QWI8F3cSYuCeCWKVN3AaoVo+a/5XhmGz9xYlNPbQ8+IRamW2rquYiqk+ZRDnkj1CYp7Krk4t2aFskBSKD10aLTZ6TMFqMJvPyexhFi+VrcJbs9OsmWhA9vSkuVT0Nzcwzy3NAcMttQOdnOjOs9N+sROEIBuLNlCQVWvoQ2fZ/mZpj0fYBqKN+ahWu0cb+UBXrLxpQCWE6TB5ivTGYQW73t8ADBIx0voF7ur3GlPcyyJ3zCEzAFGGe9Vb1GxDy7kv1tLAtW8jnOBDFIGvRAjkjzImyy9c3p77WSwIhgBOho/XUa5BB2XfWh9osVk6MkABznPDbvDd57wRv24SxuFQoszC4Pm9EZ3ylJlD1Bhc3bwTbLMgMhX0Vf7c5Rqa4URmpuE37jCnpq20sPVzkpib3qCrSvzGhNrZD3ym3oXsHj1PKx507Ypv/5mAGR1WONy+YPRHswLdp5QNpkKB2+ism7LE0PwS2+To1Fkz5/JD6mb6QYDMih2CGXFJIaH4tVpx5cFyC/QMn9jMWBVN+Dzbk9cC9qzd2KX2l4l91WeImYpXQMoAFS7tTJahWfIxkvTVR+I9XGdOlCHSoOgPFzHVcgqBFHlnyFU8jxD9n1tGJuORZLM9eCG5ccpIoIUa1cafxKA9O5Jh1xItUrC/9YcQBFe3+VNqBJg3cp9z101o9ClpMmlRX4vb93yal1eMRQ8PSJe9FuvpCS142+aBUKqIv2kNx+6AZLu6X3VGx59xFW4drFpJ9LMCiC6dUPp1CP8lbuk+SiN88EdH3hNCO0z3XM G9uKZ+j2 xTWDIY09+1ud2xjGIBMG7khgCQ6p9jiKKvnBK8C1IqZgpXvuNaAWMS+RlJDlrrHij8msZoQiDXmeHtYIlGpyBsYUCL5vY/KWErThBDd3+LRDq2DPK3wynv73478VqB/At+5fXf37pkFxKJBjfxTk20jGmyIDpdj45xWwq2lqREqF5CdRoQbaocFB1BfqM78E3DMXfFQ2gqahVTAbA4wySjQ/ia9mUQsRQKf9pbjSpKU+un1tJ/iwjSEk9HHQOWmNji8Vyjnv2eQc+iHxesGYn2f/QfdjMtfCqIYF1fVr7ytG4iymfly2UmBw5IveKmbyHmrK/+cOJeW7shSJAnkwfJHVhAJRp0wQpU4Up+N3PJg5nlZVAR5Bf0SkYlOnzEUb13xqk4AxMKy3ckSwFDiuc9r8CGcVN2StuyMGLQdMbhLGDrHYr3hODL6Vq7SykgrzNS5c13PaVDz6VMBatlUP7QK8S0HmPXPO3Axct3/WJ2CB5662briMdtJSAoNhOOeZCMJQz1NDRNf2dRzaGGZXk7JVblQ== 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:09=E2=80=AFPM Waiman Long wr= ote: > > > On 3/15/23 17:43, Yosry Ahmed wrote: > > 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_check= (), we > >>>>>>> observed a warning from page_cgroup_ino() when reading > >>>>>>> /proc/kpagecgroup. > >>>>>> If this is the only known situation in which page_memcg_check() is > >>>>>> 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 pa= ge > >>>> 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 th= is > >>>> patch is the right fix; it simply papers over the problem and maybe > >>>> creates a new one. Callers of page_memcg_check() should be eliminat= ed, > >>>> precisely because of this ambiguity. It's up to the people who unde= rstand > >>>> each of the callers who need to make the decision to always convert = the > >>>> page that they have to a folio and ask about its memcg, or whether t= hey > >>>> want to preserve the existing behaviour of returning NULL for tail p= ages. > >>>> > >>>> So, I say NACK to this patch as it does not preserve existing behavi= our, > >>>> 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? > > I prefer the current behavior of printing information for the head page > only. I am open to suggestion of the best APIs to use. I think instead of explicitly checking page->memcg_data, we can check PageTail() and return explicitly for tail pages tails, check PageSlab() to print the message for slab pages, then get the page's memcg through folio_memcg_check(page_folio(page)). Something like: static inline int print_page_owner_memcg(char *kbuf, size_t count, int ret, struct page *page) { ... rcu_read_lock(); /* Only head pages hold refs to a memcg */ if (PageTail(page)) goto out_unlock; if (PageSlab(page)) ret +=3D scnprintf(kbuf + ret, count - ret, "Slab cache page\n"); memcg =3D folio_memcg_check(page_folio(page)); if (!memcg) goto out_unlock; ... } Matthew, What do you think? > > Cheers, > Longman >