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 74F89C61DA4 for ; Thu, 16 Mar 2023 00:09:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DC0A76B0071; Wed, 15 Mar 2023 20:09:17 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D701F6B0072; Wed, 15 Mar 2023 20:09:17 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BEB796B0075; Wed, 15 Mar 2023 20:09:17 -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 AB0E26B0071 for ; Wed, 15 Mar 2023 20:09:17 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 6A8AE1405E3 for ; Thu, 16 Mar 2023 00:09:17 +0000 (UTC) X-FDA: 80572826754.08.51BDE12 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf11.hostedemail.com (Postfix) with ESMTP id 7510F40012 for ; Thu, 16 Mar 2023 00:09:15 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=Sho2Kozx; spf=pass (imf11.hostedemail.com: domain of longman@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=longman@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1678925355; 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=xoeBmzzz0FU9zZCf+NCdIJ+5XjbGzZmlZo/bkUXi60o=; b=BjWYFLCxc5z97lOivMZuycw4vXoEhjG6XjqAArsHwkpqwWCmpYklKZttfGPQE3hH5Mpjw+ XEGjzB3ujTKK66L5irFy1U1vHiUt/SRYqcBYRNsja+74R4jYZcidSX/enbOuRlPkQR7Svv MXySwFXGFRtqKRCKz/8CrrisTCGhkuM= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=Sho2Kozx; spf=pass (imf11.hostedemail.com: domain of longman@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=longman@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1678925355; a=rsa-sha256; cv=none; b=s5P4YjKFqwcUXyDBm2WXQEX3qIE0AkacUqbRboMsUzw7BM+zkwrKFFl9c2tjvQFGjE1JNZ gmL/VTgrLgknRh52uxmb+1CNqLXFgfrWkNBZO3Bs+amPggFDSNaSEdHfX3XHO4WzuSK16o EVD9/h5+e7ow+JmaZ9U0cvcUwAa+7Rw= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1678925354; h=from:from: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; bh=xoeBmzzz0FU9zZCf+NCdIJ+5XjbGzZmlZo/bkUXi60o=; b=Sho2KozxynxO1kXWtFF85tvKAgh1P1FMSvXUNmNQza27hZK8WQl6UW1Kx/9Yctj9Gdk+0e 376KKXhOjXeTNx799+Gf9kvnPqgP8S+Ir3ZNYVqG5Nx8Tf/2VS1RM4QnRqazRdPfoIPhKP 0jiY424Rsok4XrKnGrS+2K8sgQsCVXc= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-459-v8AEuQWUOgOLUsSnft__XA-1; Wed, 15 Mar 2023 20:09:06 -0400 X-MC-Unique: v8AEuQWUOgOLUsSnft__XA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B1E23801206; Thu, 16 Mar 2023 00:09:05 +0000 (UTC) Received: from [10.22.34.146] (unknown [10.22.34.146]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7F4A91121314; Thu, 16 Mar 2023 00:09:04 +0000 (UTC) Message-ID: <8d4e1b74-6ae8-4243-d5c2-e63e8046d355@redhat.com> Date: Wed, 15 Mar 2023 20:09:04 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 Subject: Re: [PATCH] memcg: page_cgroup_ino() get memcg from compound_head(page) Content-Language: en-US To: Yosry Ahmed , 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 References: <20230313083452.1319968-1-yosryahmed@google.com> <20230313124431.fe901d79bc8c7dc96582539c@linux-foundation.org> From: Waiman Long In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 X-Stat-Signature: jtpjhya98p7oim8f9eaxcp1n9g5eqs54 X-Rspam-User: X-Rspamd-Queue-Id: 7510F40012 X-Rspamd-Server: rspam06 X-HE-Tag: 1678925355-615791 X-HE-Meta: U2FsdGVkX1/CqS2g3LUtE8cWkDL9LVqm1HYz/OXNdQuwwJPm/o8SHF51KGkobnTTGR5BZKcd2MueB09x/4ZmWOg5XUp/IqAWve+GipuCBaE8DdanZeL2UgVwwQalm+yOJAAXpvsNJolaY8dYKHKbQpivSC1UpyLihhmL7xBE/BdLGQw2HI7pZhLNNiTLT8NaoduZhH/PY4zfvFhNzG7KOQdW+TeB/j7O3bgi5wPubCPkc3wNQp3NCH5vC6hEd7Uxq2y7gJCZButhN7DiASAwD8/MpJu/8i4O/i8wFiSFMz0bP7m96srtydrnFQ9+NoeU+o5EgerYM4PKyN0e38/TDdaceH59rEbWmTInbiBJa6RMyXma5hlITaFRVk/0oJDxeUv+U2FjeGj15ZD+fT64PNtU/ceBslwkCDpvKV8BrOKvljX4mce7ewtgoeu73GwwlrjEvcLr8ACjbVCP1zd2HPgq14OUXOJcefke3wqhN/Kwq44O0mfp3Pf0rZ5hO7Sjy0OGcSAspx8xAYYkqt6G/EvILhIcp83I+1FHaCxdJhtgGkaAlWOhrXEDHR44NDT6OHokyJz+Yf5C/jOWFJlcIL7XA1xD3HJ6TgkaWtmBqXcHxKrZMKT1gVnx7UFy8eeN6bNvUmumYf6OrAzwoUnJ8YjWZ0FP2C8z98o7Y+txF994YclKOWJwFj2nFCGjRr8LlK8iwfjDea/xnFZZXnb4zCCWDVprK1gUv00FErN4OxBD7Mxl10bB//smUkItcw0mmiZhhsnNMIAX1pEyS/VE7rvIhPoEx4njvTQjArVljSbOS2jBFFLNANT7M0dJpkmJ0d9o8pWkNiDMxJ5sksuI08k7Umwm/DG470RPYBhqNsIJJPpapyM8p1mvLgcGdj0MznDMR8QBbIZMGp8f3tA/UYhrpkyvkWhlb4Bwv0kuvLzw9HxH5vxt+ylmNhF9IEwjmVlZXoi6Dme03lMIFmM YPZoUMpV rI+0m4efjIig6EVK3Z++qhMt920uOcBP0G51qcS/2OmL4ZgqTmGD342J01dqAgynb9OJ6uhoX0huCsKh4TlNYwtf2wBYto5Z4G9+U3y4QZTU22ARZVy34AL2KID7RIUdd7L2Jo4Ootve7AHbhEXkQKVTsGWAR3BvgNkB1UaQhVpakzRGZ5tf9Lies2fSyws3Ks76q/31L/n07kNU/AoaftjdCG2TSQimzBMmNqMeQaZy7hLSvLSAM29aAQg25XB6HTplcStPmiDA4dleJ2KuwlY4HrSZob51yxW+EjB4c3LJVMrJ/+UjTkF5et76f+ZDLG98hRuyk9OMEJTQdNg962QKkfT/4MxpHSJErANNQQNBFJ30Kii+2DlnWTbKpndWYUjuCkY++tQKCh8Bj4vOSnHlUK8efnfwTk3l2kNorKa+RTzYPI0aaUd7eIg== 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 3/15/23 17:43, Yosry Ahmed wrote: > On Wed, Mar 15, 2023 at 5:19 AM Matthew Wilcox wrote: >> On Wed, Mar 15, 2023 at 12:04:10AM -0700, Yosry Ahmed wrote: >>> On Tue, Mar 14, 2023 at 9:54 PM Matthew Wilcox wrote: >>>> On Mon, Mar 13, 2023 at 02:08:53PM -0700, Yosry Ahmed wrote: >>>>> On Mon, Mar 13, 2023 at 12:44 PM 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 change >>>>> 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 page >>>> 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 this >>>> patch is the right fix; it simply papers over the problem and maybe >>>> creates a new one. Callers of page_memcg_check() should be eliminated, >>>> precisely because of this ambiguity. It's up to the people who understand >>>> 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 they >>>> want to preserve the existing behaviour of returning NULL for tail pages. >>>> >>>> So, I say NACK to this patch as it does not preserve existing behaviour, >>>> 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. Cheers, Longman