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 70826C6FD1D for ; Wed, 15 Mar 2023 12:19:49 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id F36676B0072; Wed, 15 Mar 2023 08:19:48 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id EE65E6B0074; Wed, 15 Mar 2023 08:19:48 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DAEB16B0075; Wed, 15 Mar 2023 08:19:48 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id CB0BD6B0072 for ; Wed, 15 Mar 2023 08:19:48 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id A1920AB821 for ; Wed, 15 Mar 2023 12:19:48 +0000 (UTC) X-FDA: 80571038856.01.8F4FADD Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf28.hostedemail.com (Postfix) with ESMTP id 73333C001D for ; Wed, 15 Mar 2023 12:19:44 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=ibZv7O8A; dmarc=none; spf=none (imf28.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1678882785; 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=YEHaS1jbcFt6XFTHtzvHwBBDGnKA7GP1NQZcvCyxIVc=; b=kXWt0Q+OT8Oh31WeHOzym4D18BVjGHGWwW7lTMbFmemo69s1NSLsVe6F3e+yj5FbAIez6Q ToqarWJ8Lh6Vx8XQ52HIjmeweTuHTz9tFERyYxdyzo3HD1dcOnXGX370WWZoV47jqiUw1B rBa+cdV4KxBGNIxnYZZdD9Vr0/ju/XE= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=ibZv7O8A; dmarc=none; spf=none (imf28.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1678882785; a=rsa-sha256; cv=none; b=5F4PIe1AODZiA1KfsrAokkvRqfXf7bYncUpk+l0+F0W2YGy2X4NlBCvyQahul54num08qP 6Opc9GAQKFo/WsqrfHJH010JrScPg3LiQaIXjnMStWItguUD9NK99F/jEluXhxkW39XL1d 6AAQjAceltnkvZ4MgYWjb9qYuGyRz1k= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Transfer-Encoding: Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: Sender:Reply-To:Content-ID:Content-Description; bh=YEHaS1jbcFt6XFTHtzvHwBBDGnKA7GP1NQZcvCyxIVc=; b=ibZv7O8AfRIpW1okpk0auST6ju QGUQE09QAEhtE72dYe30m3Dx/TOe/gWHxKNlcCcZ9+EUAiQ2s+8VPtKAkZlSmetzcOHPSTNtoV5vA ZyUgKojS86wmMbFQw0TSsvCFRTsuh9xBIQmKbJcHkSRfozwqbyLbIBPkV3NyIaHQM55O98xXphp1a zWzRoqY9juw8uxfElduAWBKt7cU6+xxk17aQQeXoLIsR4ElNNMLqtkx9VbmHi8G67IY1jSHrSgg0Y +mD2lwCmLX5Xms/qJDQg6vHlcKDWhlRph2sAKiurG5rDNTp7J4rUQRu4llhEaK6aiKd82vJa78t8T Q550uQfA==; Received: from willy by casper.infradead.org with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1pcQ6B-00Dolz-7a; Wed, 15 Mar 2023 12:19:19 +0000 Date: Wed, 15 Mar 2023 12:19:19 +0000 From: Matthew Wilcox To: Yosry Ahmed 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 Subject: Re: [PATCH] memcg: page_cgroup_ino() get memcg from compound_head(page) Message-ID: References: <20230313083452.1319968-1-yosryahmed@google.com> <20230313124431.fe901d79bc8c7dc96582539c@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 73333C001D X-Stat-Signature: dxmbxsrdj433dzmn15t4859fa5m61ocp X-HE-Tag: 1678882784-532765 X-HE-Meta: U2FsdGVkX1/T+IulQVkL5/K4yKU46I8fge9kvrGbXZarEjyvBD1tnhKZkfPX+goD9KUKVsWXyoEMUNsUIAR2BCfc+ITwDppgULNvTEa7HUYCiM+krvZ/XirqDRBEbyPUN3bd/qK5hxbInttGoXg2B5iYxbje95RSMnp1z/i+kgOf1Ltg6ZC0fW0icNgftGObd4qOZABR/epZ+f08y7Ety3SR7SsORx2eAXnJPjq7TFqW/Rd9HtPk+iTuWsOiDjlMvfjy34KARmcHYMVuTIduz/guysBKp1RMhFuRqPLTW6LZfHFs27rQogKD89zcKV6nuiFZ6kW734FPLP4p9GX/wmymHFyLD3vLs/F6GRgrlId0ZSaLHa89z5232r12qLpbEroCqW4ulRwrNyMl2WY0OxwT/a0Hdr44UYeV8+mwRXiCq/PIKoVoIGOblJ81SvHZ3HmWZYD8AqQTJtEHMYJAEonCvY384I3SQjeRfyiquy+GrPks7TxEAf90ww3K17Fbqt5k+h065n2tHt5TL++JMztd2/+aGD+CFizBhDlRImtDELJHVFNGFGQ7vYh0CVM4vTTXZxf4CEUNnxLfUY0u7YtFoqp78rhP7x3zftiYlExk/4IcXc54aSSi1KF+psX1xxb8rNBj3EHQflH6LzIF2mDCfam5MHc6uk1UUoseZZR2LEpD0zXliam+9q4N/4eBNYSF/SfNphUJKMRd7ILr4reHKxEMPrQjNxIFzm1Inv5nAINj7Z1EeDsfRo66HhnDgOFQnBMqpXJrIGHJ4bW7bYthX/kgO34PrfCQ+67hBOMHMnRWnjBTMUwbCtFYhTs+yvYfEhQUCFMSnYvRy7ID8Jck+4tH6ZDAh4L9Ip/CSGCc+JfxMzVGsJq62vTyfDwBw7qgu6x9VsmmDAdMv86mGEXtWRPw/R4O4k0QQaJQL6foKuNvrtbbZXNRni2O3egpW+MXgxxg6qQKMlw8NtD MBhIbBuK EMcKbAgzxZNVTWkpk3TFlaTquJ5cYdQ42jRTMVFsSI6O7xul+T9GF+MBvSgSZLpE/M9ckx9D4CDf9UuUTgeFGhcOcblfMkOUDDqyqDrFEmAX7uAqEXVRgtvbY18/+rwR6UsAK4Wb8BkEwd8DcFk47mIj4ByaiCMZJfl+dBzY4nv+1+9NS0NyYpQ2Z2FlE3FY7URqErH+ZUCGwyRKZdgmGEk4GlM35fYYc2bmGirgWuLxFuil4dXYcBKfz9Nga3wtAD6LCr0mPKpCg3Ar7xKsVBjua7iSzmqcNI98Jbho7ekecHC4AUt0J+c+9gER9LHLGy7QvPyy9uCNT4T/eiit5mY4yv3U53ZrnaXhaPkDjWYZnixS3/YlkjpDRhbdrtsSX6c02C++vkjqdJ7TYDqh9D5CxRL8QzNC5EL2x 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 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.