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 5E2B7C4332F for ; Wed, 13 Dec 2023 22:16:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D1F448D006A; Wed, 13 Dec 2023 17:16:09 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id CA9E88D0062; Wed, 13 Dec 2023 17:16:09 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B21008D006A; Wed, 13 Dec 2023 17:16:09 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 9B4D98D0062 for ; Wed, 13 Dec 2023 17:16:09 -0500 (EST) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 5D8F814035D for ; Wed, 13 Dec 2023 22:16:09 +0000 (UTC) X-FDA: 81563204058.12.918DBAC Received: from mail-ej1-f53.google.com (mail-ej1-f53.google.com [209.85.218.53]) by imf24.hostedemail.com (Postfix) with ESMTP id 79AFD18001F for ; Wed, 13 Dec 2023 22:16:07 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=T5+WSThh; spf=pass (imf24.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.53 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=1702505767; 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=vLTmz5NciLF4rNowjjDuTk50N1sFl4Unka4K7FuS3Xc=; b=GAMofJyAPPfCuLK8rSHlf+XwM9MdPrV9sMRdf16sxgxdSRWPOf2ZohH9UkFtH0T8Y+uIEn anuWehW/jiz0bV/H+BIjqsWdDZpCKnj2K6ubjU4WDa8KGLnSSbdA1yecHOoiF7xXvyjj7i cfCkwKkYX5jSaRo/+oXyV0yEYEqHWbY= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=T5+WSThh; spf=pass (imf24.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.53 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=1702505767; a=rsa-sha256; cv=none; b=6EE8qXnCs2d/lLz+weUKVTxvWjGR1btn7hVAaa92ujamQH1X87ynMRleBAGZKthBWAvYsR XuBYloiOuZY4ullBjC/SaiLWrjaBa8N3HnEKG9QDUjR/Fi97k1+PvshVmqnOKiMgdBxyH1 rmWNpbMsi5hok8pXgRbYYyelzs7k618= Received: by mail-ej1-f53.google.com with SMTP id a640c23a62f3a-a1f47f91fc0so897624366b.0 for ; Wed, 13 Dec 2023 14:16:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1702505766; x=1703110566; darn=kvack.org; 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=vLTmz5NciLF4rNowjjDuTk50N1sFl4Unka4K7FuS3Xc=; b=T5+WSThhnQseKtsCKJPPS+/l9l4mfkkbNLE9OM6nsSJporx30RgwkC53UgINCZf8LV YIxZ6KbXzViRSjkFV/nsPa/8RbHFfaP6j37GLE8AEOd8jMiDRROQ+2vmyXx873Yd3sxS C9qiyhc8NXQKTQygpppnX13VgHYAeyeBG4Pd7ZCnPhTJQqkI23KKc7U21qog+SLhPoPO 1JW5/zetvXqODrvtqubIlH3xyLazPlDnip6K5mwO4Mq8VbVr4MocZIkYO078MdpUOXkf vffdph0+2I6uE4aMTBMAZ8MXElSqeY0Leg0TM5BCComvO87JFBeUH/zSfKXAzSEN0N0V Z1cw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702505766; x=1703110566; 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=vLTmz5NciLF4rNowjjDuTk50N1sFl4Unka4K7FuS3Xc=; b=cLjzQfvFMfAoY7lULty1uGOJaVm8aAtrXesjbDkPtFHpcb+5F97g7qZQaR/IVIFquh dsvzlxxMhpelcKNdKXjN5G9sgIYqGq16n87MlVdq+zm0klZMKP8v7iqPC9RCuTGVO9L2 33aAmsWs4vcphOzlyfEXQ1lbGsndCHbYZY1lj0QzLI+e0XfjZhFZK88f9wuA9KO5/5Up c6+24EsJP1DIeg7uK6Er6gnyGbyzsxJYC47I8n2494jPDchr3hty4vXeZGJYe22D9VBQ Rzxx+O2qmuLzvKWfgA2PdItMm2YgndRP6AJhiRmVb3JtADyFk+qct8ahsw4zVLW6Ge9Q Mreg== X-Gm-Message-State: AOJu0YyvGn20eeT97h6fviiTHeYKQ1hxHnqp7cirIaClIHggSnIxNlZF Kcte3GpyPrxACt3fTpodxVPUO4lKwNQkC1mGweR9fw== X-Google-Smtp-Source: AGHT+IFofV/+NPAePxHDP3xt0CYJYYFDLSmDOHSDQIVKivkX4dTllwijliAvjMwADo/c/q/pSQ1c0opm9Gl0ScjilW0= X-Received: by 2002:a17:907:971d:b0:a19:a19b:55f1 with SMTP id jg29-20020a170907971d00b00a19a19b55f1mr6336000ejc.129.1702505765965; Wed, 13 Dec 2023 14:16:05 -0800 (PST) MIME-Version: 1.0 References: <20231213130414.353244-1-yosryahmed@google.com> <20231213202325.2cq3hwpycsvxcote@google.com> In-Reply-To: From: Yosry Ahmed Date: Wed, 13 Dec 2023 14:15:29 -0800 Message-ID: Subject: Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page To: Shakeel Butt Cc: Matthew Wilcox , Andrew Morton , Johannes Weiner , Michal Hocko , Roman Gushchin , Muchun Song , cgroups@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 79AFD18001F X-Rspam-User: X-Stat-Signature: 1rxr744nziq4j46xht89gu87rgarphxr X-Rspamd-Server: rspam01 X-HE-Tag: 1702505767-215135 X-HE-Meta: U2FsdGVkX18QbHJxQyQx9qBp/6Ixe7KjcvrXiYbJYUi4WNm6HwPxvMbW8bJNem1e2u1dHFEYJmTMxsIAusXVBp6YquUjfD0xhKVi+JGQgO3J4syYBFwiHNB3Fp4Gwdi/2T4wyz7LxvxTHnrXQqhdVIKJzp/XPrhBsaK+eMpc0e0s3H6hfWpyjvI5wUScQWtYg/U6DfxrchDQhlEpV6DClQexsvK4Q7y1NftPZkmYvU7c1akzxYWWKkl3fTViEI43MxVSle8EMND9QIJNavd/WxBaAH02KIWZqhZFwypq2sphwYQ5kh/90v/FNRlZIyrD1fom3+yZ+YvQ8ih42rcdCu3Xu/q6+K+22tAP5eKuHt4pJFWr+HwV/T2XMAHSlTeAszwleUHYBpUzRZtnVP2tFftdRaKHxYxWYo/LbYgY77k4skCZxCFh9vSpf4mc8qYUSkj3W5DSk1d2rB/qm1E+rTClaQPGMOtcYA5+XzKW6hp7/D8xs7YlruFXWCFRsxLh4+I6wx6L6p3rvwWPg5oqEUjD/vykYmA2Au6A9EbrlVVLijdHvB3RgamgB+wUIcdulJ+9IZkOylluGstMfQjX/QYylBlyTiOrFUtYQi+/Mw1sXfNUYfyuf/EYhs4wUDlP7A6y+yR/Cp0uMN23RYQZER7ppRNYu3lS9OWRLCGlZHbBykpzAuyj1Dp/gU5mSMDPGOjihGeu6JiUEsTuWpkrwTJPoqWwk7H72gtDka8oCKlRrX1pLLs1qGCGuYhH+95JdPDRc91Ynqmi0IMf8mMxvzP4z16OqcO6YCOqkPRCf7uxzntFg4CAYk4Lg6iN8r3m2XLBwo6WFxVY9kOUYh3lUYTb/P+Vg0RcFg1tI3p3ycCwuxiCuCm0gt/2I+cNiyJKxcR1tzvsC1ZscvpHBR2edHiVe2MiwAwcE/tXUGm0m/g30xlPWX2JdtRov/jI8naNE1S2UnEThHN8qLcL48A GIvdio/Z Aav4aNMYN8/9+0pYyRz5tKepYqSJc5uJmBpx2u3DcuAwYdNlZME6zx0cBV8wqZCc0znIY1Fj8dUrqC7WLYyNb2nMC5g6ir+lWsaWsdVFfcSxJ7mxmXI1eQG2arrNPPtMHGsVoQIZ5DFUUKszTaapld2e+Q4l+6XpPo+CNdO+updTdELTRV5XD0Rqc68RnPNwMA/a3ntllBcoYwTY6gFnWKxPgv+Dl9o3Lt7plOASjEQ0l9qhAUVEDSbbiTq/ntSJNaxpP7V0Ki0/i+P3jSDxwnbMXn2lYEP4sQQzX4uaGKwcvpz2WjDFEP4T1qnCrY5nCUOZpV9EL38KmdKs4OrBwYsFedPJ+aYWiMjEb 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 2:12=E2=80=AFPM Shakeel Butt = wrote: > > On Wed, Dec 13, 2023 at 2:05=E2=80=AFPM Yosry Ahmed wrote: > > > > On Wed, Dec 13, 2023 at 12:58=E2=80=AFPM Shakeel Butt wrote: > > > > > > On Wed, Dec 13, 2023 at 12:41=E2=80=AFPM Shakeel Butt wrote: > > > > > > > > On Wed, Dec 13, 2023 at 12:27=E2=80=AFPM 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=E2=80=AFAM 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 t= hat's all. > > > > > > > > > > > > > > > > i don't even understand why you think it's a nice cleanup. > > > > > > > > > > > > > > free_pages_prepare() is directly calling __memcg_kmem_uncharg= e_page() > > > > > > > instead of memcg_kmem_uncharge_page(), and open-coding checks= that > > > > > > > already exist in both of them to avoid the unnecessary functi= on 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 w= ithout > > > > > > > worrying about those memcg-specific checks. > > > > > > > > > > > > There is a (performance) reason these open coded check are pres= ent in > > > > > > page_alloc.c and that is very clear for __memcg_kmem_charge_pag= e() 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() pa= th, 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 callin= g > > > > > 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 mor= e > > > 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. Ack, let's just leave it for now. FWIW, I believe what this patch is doing will eventually be the right thing to do once the code is folio-ized and the calls to page_folio() disappear naturally anyway.