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 A4209C4332F for ; Thu, 14 Dec 2023 01:46:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 26DA18D0088; Wed, 13 Dec 2023 20:46:32 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 1F8428D0083; Wed, 13 Dec 2023 20:46:32 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 06F468D0088; Wed, 13 Dec 2023 20:46:31 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id E499D8D0083 for ; Wed, 13 Dec 2023 20:46:31 -0500 (EST) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id B4E62160307 for ; Thu, 14 Dec 2023 01:46:31 +0000 (UTC) X-FDA: 81563734182.25.7B61674 Received: from out-171.mta1.migadu.com (out-171.mta1.migadu.com [95.215.58.171]) by imf22.hostedemail.com (Postfix) with ESMTP id 7B0CBC000E for ; Thu, 14 Dec 2023 01:46:29 +0000 (UTC) Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=lOa+ybgT; spf=pass (imf22.hostedemail.com: domain of roman.gushchin@linux.dev designates 95.215.58.171 as permitted sender) smtp.mailfrom=roman.gushchin@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1702518390; 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=iadDOZ7uuah36iqkq6OTgveCGZQqV5YcB5cpZRi30I0=; b=0OEsiR1G9CarpCbo7ostRqu4JYUFRxThoAmAe6lhrrjQX+b/YbKChHyErooOxdde1bUuB9 GNxFGz4ALC5S46inAqBm/fK+umY1DYb/U65QcavjnkY4tqqsik06mD8DfNtEsVnp+kzGgQ ikqcr3H7XdaAbT67pBnkyzmgefDoHvY= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=lOa+ybgT; spf=pass (imf22.hostedemail.com: domain of roman.gushchin@linux.dev designates 95.215.58.171 as permitted sender) smtp.mailfrom=roman.gushchin@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1702518390; a=rsa-sha256; cv=none; b=sU97Yb/ZjWTXs7DGYbuQp3iv/x6aMGBJ5wohCzMe+nixZ7ejbJz7vQEXYRqte29h3KKGV9 NE2K6LlWyJrOOlmYtg1Rt/05PdmUtv58l4YTzE5Worc01ti0L6FUdMSbOGNai+pehNBylW h0DLw9zk9e0xoQ2l4oPbYo1RQY9WaA0= Date: Wed, 13 Dec 2023 17:46:21 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1702518387; 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=iadDOZ7uuah36iqkq6OTgveCGZQqV5YcB5cpZRi30I0=; b=lOa+ybgTpb+IhZ18G8/ofVgOlfldaF1OrOPmKFWFcJsnwgDqdnW3cZB3KEbJ8tjJXSEJ2J LpS2N5kjZqU51N3WNzXhqWYXynBb2zQ+9s8dB76Y+nsiMk4LRnW2Mut4+BitJIHpnG5BZF qUx8Dt0/W1spYf5LkumbjuMi59fOF10= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Roman Gushchin To: Shakeel Butt Cc: Yosry Ahmed , Matthew Wilcox , Andrew Morton , Johannes Weiner , Michal Hocko , Muchun Song , cgroups@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page Message-ID: References: <20231213202325.2cq3hwpycsvxcote@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Migadu-Flow: FLOW_OUT X-Rspamd-Queue-Id: 7B0CBC000E X-Rspam-User: X-Stat-Signature: 3y5cutnn4ybapsxtjuwh8m9wphww3smj X-Rspamd-Server: rspam01 X-HE-Tag: 1702518389-811921 X-HE-Meta: U2FsdGVkX1+hCgcVDdFv9wnhIycTywOYv7nf+k6rcY+7fph2m0VYUHeiw/UHM31KbIS0fbO2LkivHPxMzRZZUwOGdtKbwLlRQxDta2tgopjt/cOYPiZpgL6B3Ta0yV1ITMz1bGABswVQT3/83CKYyO+xgV/wCGJx/zPosaOV4pZQ4E1P5g5Vao+FBgyk/xeEhys/uLLSiVtHY3tcAaVG0urVBwF5uQkWkS1Bs0hC1h3hpuHF4UWw2vSkp0fThEPbgNM+OehxUpccIanM4UkrtDip0d5RQg9L908JCTRARYzyOr2hq81B8LHUexG1oXBOoWHN/TKPwM2EsvoxN7JZ77Fm85r+FAec7bFdAyDcU6epyeCn6UReSdxCzCd1nzwLxvUEwSaKTgpzOAJgOmC0GeQSRxsKOwi2ESEPzPPthiQ0/6ettHxNG1EM5o7zBRaLJ6sJpeAftU71+WE8wDRCZ7GyhGek/9XbDIkKMeD6EHaTTf1gi0TPcn5uQPrag0/CDg2T06uMs8sTP2Ih9Q9wu6mVhZ+d7NdMldIoty7bH9+SctiT4YSpIu9N8hXgAVlWBeK2Ha8q1eUL6rHhFGBQAV++XN5YbCrQg0ku6ekbR6dmpSvt9mjSZYcx1nRexeGMYvQ3QWeHsGS3tiKTj/QeBD6eWqomvShXifwbzYI1CvKqyt25w3snXCUj68YJRmTngj2ejsJHlbPAEeVhaT9KjsBtA2W+dUaJRWUwWevkfwbIhqZfir3TU9Gjgai/GYaPJNp43go4DLqcCFdI6kvnl7hBUdIoRU+IfctaY8aOVMRQfAU9u6qFgM2mNMYumvpLwoywGKy+PIqWMo6ANQnZXe4teciq9xqbr8VGhRxBsxLM3/okz6B4hUlKC3D5fHUaAd4jK5/FfM9AOaOXiXxwJb0LUB8wzg0LAObaRl4PHF3meXVsi+8jnna7s6yrUSxfN7KQPiPz9t5zYKDC4WL E5dowDbv grsDI6TjV3i2AtqJU47XuYG6wNoNmfKqy6sUkt1AAYwANMq48KOMOIbnlvgUrtNS3RjbKm7dRzIDkGt/jYNxgWdQWJnoVb3WTHOkQJQgWzHS2vzsv3/u+zr3bQCqVdWVrJF5dpuGrpBAsXbYxn1d24THC6evoz5SEfpUi0ZdTl0q70qouwZmsncGdQMXNM5rrmsuqJ9Y7LtMFZCqE5ESvNT3jOEIZBbeyyWj1z+1KOX8koW5y9p38Uvcp/9AlXIw9NitsEhVyYjFfg0k= 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 Wed, Dec 13, 2023 at 02:12:35PM -0800, Shakeel Butt wrote: > On Wed, Dec 13, 2023 at 2:05 PM Yosry Ahmed wrote: > > > > On Wed, Dec 13, 2023 at 12:58 PM Shakeel Butt wrote: > > > > > > On Wed, Dec 13, 2023 at 12:41 PM Shakeel Butt wrote: > > > > > > > > On Wed, Dec 13, 2023 at 12:27 PM Matthew Wilcox wrote: > > > > > > > > > > On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote: > > > > > > On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote: > > > > > > > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox wrote: > > > > > > > > > > > > > > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote: > > > > > > > > > I doubt an extra compound_head() will matter in that path, but if you > > > > > > > > > feel strongly about it that's okay. It's a nice cleanup that's all. > > > > > > > > > > > > > > > > i don't even understand why you think it's a nice cleanup. > > > > > > > > > > > > > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page() > > > > > > > instead of memcg_kmem_uncharge_page(), and open-coding checks that > > > > > > > already exist in both of them to avoid the unnecessary function call > > > > > > > if possible. I think this should be the job of > > > > > > > memcg_kmem_uncharge_page(), but it's currently missing the > > > > > > > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()). > > > > > > > > > > > > > > So I think moving that check to the wrapper allows > > > > > > > free_pages_prepare() to call memcg_kmem_uncharge_page() and without > > > > > > > worrying about those memcg-specific checks. > > > > > > > > > > > > There is a (performance) reason these open coded check are present in > > > > > > page_alloc.c and that is very clear for __memcg_kmem_charge_page() but > > > > > > not so much for __memcg_kmem_uncharge_page(). So, for uncharge path, > > > > > > this seems ok. Now to resolve Willy's concern for the fork() path, I > > > > > > think we can open code the checks there. > > > > > > > > > > > > Willy, any concern with that approach? > > > > > > > > > > The justification for this change is insufficient. Or really any change > > > > > in this area. It's fine the way it is. "The check is done twice" is > > > > > really weak, when the check is so cheap (much cheaper than calling > > > > > compound_head!) > > > > > > > > I think that is what Yosry is trying i.e. reducing two calls to > > > > page_folio() to one in the page free path. > > > > > > Actually no, there will still be two calls to page_folio() even after > > > Yosry's change. One for PageMemcgKmem() and second in > > > __memcg_kmem_uncharge_page(). > > > > > > I think I agree with Willy that this patch is actually adding one more > > > page_folio() call to the fork code path. > > > > It is adding one more page_folio(), yes, but to the process exit path. > > > > > > > > Maybe we just need to remove PageMemcgKmem() check in the > > > free_pages_prepare() and that's all. > > > > You mean call memcg_kmem_charge_page() directly in > > free_pages_prepare() without the PageMemcgKmem()? I think we can do > > that. My understanding is that this is not the case today because we > > want to avoid the function call if !PageMemcgKmem(). Do you believe > > the cost of the function call is negligible? > > The compiler can potentially inline that function but on the other > hand we will do twice reads of page->compound_head due to READ_ONCE(). > > We don't have data to support one option or the other. Unless we can > show perf difference between the two, I think doing nothing (leave it > as is) will be the better use of our time. +1, especially given how controversial the change is.