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 45685C4332F for ; Wed, 13 Dec 2023 22:12:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B6FB36B0096; Wed, 13 Dec 2023 17:12:50 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B1E9A6B00C4; Wed, 13 Dec 2023 17:12:50 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9C3406B00C5; Wed, 13 Dec 2023 17:12:50 -0500 (EST) 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 899F76B0096 for ; Wed, 13 Dec 2023 17:12:50 -0500 (EST) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 58ED01A0330 for ; Wed, 13 Dec 2023 22:12:50 +0000 (UTC) X-FDA: 81563195700.29.7998C27 Received: from mail-il1-f181.google.com (mail-il1-f181.google.com [209.85.166.181]) by imf12.hostedemail.com (Postfix) with ESMTP id 9E87D4000E for ; Wed, 13 Dec 2023 22:12:48 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=d9wCheJg; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf12.hostedemail.com: domain of shakeelb@google.com designates 209.85.166.181 as permitted sender) smtp.mailfrom=shakeelb@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1702505568; 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=WV8hyND0FtQ75xxFPin42YXNFIjC4p2jnNaORqS/TvY=; b=esQJpsOBfR0IcJtnE02z1Gp9g1mhnZDtFBd5W89WilofkcnUASEQEH/XxG+qnQUrzjxzTh 1LyWDM44TgAsH2BUAf75XNYGpusSlzilleXoCOvja9sd45aS+ErGP7tlcHbk8PNnZuvb5V gT0ZhcP9Uuz21WsfVue5EeVjjHQjc64= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=d9wCheJg; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf12.hostedemail.com: domain of shakeelb@google.com designates 209.85.166.181 as permitted sender) smtp.mailfrom=shakeelb@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1702505568; a=rsa-sha256; cv=none; b=QK+Sd2d+q3JKBrNnjHgqji9/pQXDTJ2xb6gUanWBV72NvAZSh4b+C8MdZh4EbBE7qEIfXm QFU74mgD5ZGVL6PXjQuimi6P85eJARsc2EfszbofxfOKEq3hSwNgKrVu/Uf11+gOv+SIgy 6HSDlxJB+mBu4t6FLbpmjjkuHrG5Ycg= Received: by mail-il1-f181.google.com with SMTP id e9e14a558f8ab-35f7ce02937so24205ab.0 for ; Wed, 13 Dec 2023 14:12:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1702505567; x=1703110367; 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=WV8hyND0FtQ75xxFPin42YXNFIjC4p2jnNaORqS/TvY=; b=d9wCheJgwiqZZzMoV6GxF7ef6tTCxdPRD0mwpsPWq45OfyKYLHG4e6gJMFPAIJjDr0 2jdEWihw+vRLmgLcTfWZzIObqBUOfUxefZhm9Zgx7OOoSmBtiMTzdN0R8nKcdElOo0d7 wp02rQSZnWWGt+BdHEQqxpdUnVwDlb+1ZQ4kYgT+Z/LczAFf65kRrebj3hs6EVfYjkbf FaZgvUtoL1Grj5M93SrBRdYsfSlX72Hz4gYDdm2PuRVwttE3Zt+LR5+mF41xuO6wSxve 961SlJc7Fdw6+72V8kZOIhS7XRTlYy/IY4RwtOBfDLFwa/1z9sLHRbA1VfkTkEnM0QQC kt2w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702505567; x=1703110367; 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=WV8hyND0FtQ75xxFPin42YXNFIjC4p2jnNaORqS/TvY=; b=Q9cs6QExNhUr/gRt9zKCWgO40i/7oFg9n5Jeecls+LgCzX1WtDxSa2dsLQtwD0JXW/ qfikzIYSaLbWiROGrF2/E/7sw3+eNPEx8vplJOnkMOkdo9tBQ1TAnsyZU1Sp8ljRsW1i vRtDPoftyIc0Y0ejMshKj3J/yVvMiWqPoydHFyNjsJN+dRN9RIosTdCEv9evR0ElLTXG O89cOZq1dUpsBeNxhNIKijCRhk7Lyd6eeQhKt1FZyFe6SF83jHdxvXe9nS0NuXalXa// UoZvwinPGT1sLN2qyGVIwtVGttMlUHc4+ARAhBpADYNucqHLJstAsd7GdrfwzW7gdJGa W6Fg== X-Gm-Message-State: AOJu0YxBc/cgXNE+tfBGltdhhHdx11FoS61QbLKgAn1Gy2YV5tmw9ylr iQAg3g1JpwFOCZ9cZjE8m02qZMFKhxIcp7Q/iSrmVg== X-Google-Smtp-Source: AGHT+IENYcrxYE5ozTDxyGwMECNbjPG9m12x3ZX2sXfvOtjNSL0c9ZjSTfiKOVAwGHqGdEKIRiaNxhzGaPEG6BwgHnI= X-Received: by 2002:a05:6e02:20eb:b0:35f:79b5:b980 with SMTP id q11-20020a056e0220eb00b0035f79b5b980mr155440ilv.27.1702505567505; Wed, 13 Dec 2023 14:12:47 -0800 (PST) MIME-Version: 1.0 References: <20231213130414.353244-1-yosryahmed@google.com> <20231213202325.2cq3hwpycsvxcote@google.com> In-Reply-To: From: Shakeel Butt Date: Wed, 13 Dec 2023 14:12:35 -0800 Message-ID: Subject: Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page To: Yosry Ahmed 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: 9E87D4000E X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: 9bxwk1eeqr4d74yd4jj1c965sei5y878 X-HE-Tag: 1702505568-215523 X-HE-Meta: U2FsdGVkX19fj8OPuEhnIQuw4kLSPasC5gazWqf6OBfiYsX7ONNNnAiz1cryFqLwhMLp/w6MwxwKIlf0JTGWtUBa8jqsESP1v/Ptj+e+5OrNvXxeEKnkmq1Q7yiPtaWlQSBd6KAskgse/h782fIGgoyQgDkZqllqB9jqsjCf1B2HXcE2Ubr278zZL0ZvBEB1CHkmiH5VFdI/L387KhcrRsEGstgpdLghBchzVSPc2NMXAL+doRmlMPzDMDwmMWABeD4KXaMHtPFnMgC+wOv9BGmQv3cmtAlRUnPCchERTxPOYrU9KXjzExf1q8tC2Em+zJ9qReBZ5Sp1TTcOkHb1McWe2N6P78wtKNrJrGrkYajOQN03ci8fD7aiKLAQHGoz1I9YlcLebcvRILedjHfZAB1x/cLFfB6o5KavIObiHR0J6dc/2Nx/j+VdOhvpER8EedfVbfrWg3kuHrjeHtBxk9PZ9ogv36YiCHvIMSrKbF2nv7rJqmXcDr/9wn/7o/8XHJFjzTxEOFzBk5EG8XRNOGrc9yaJeXft4bm/aZkARLG0T2cZ06Kogtxd20XdEXzEs5/HIcLNz2mnicIhjheKyyn10011dxEGhGxgTdSYOzIBlyKopWAwe+eXXxd5SH2hxzWCdelle81vdR92vvUR2SZoOcF1q8UCUsjAlsQ8wYN2AUI0UyiXhnqJr6umw/Ti8Z8+FDDXUi6iIWEVZp/LsPi1B6yjeVwkrBWJE9rcmcFgvvmgzcMtpXrD1bmg5hzDRAjNZyQpn9xtNVHgKgBcjeNXR72CLQ3hKtcKO8dkbq3fg0Bg/1Tx9uQnqRNE46DfPdjX/rdNXJxQotBAhtj+vn4zzc5qJbisazlB9IrbwI35XihG5I6vjOWnTi9lGlux7lY323ek+UJz2mFiiCIB9ySlaKhpYp0rjFI4bYuDIk+rNcgglmUvN9bnmvOIOjT83fa/fET5cvKfv4XVfKw +XMyzk7Z Y3u/9hS1RD+8AWcIcmpSQVO5ADx8NXa8wMwcN/TYyM38ORdO8kZHi5dBaAtmxrXg2ESgOHz6vsTSECjs6VQ1oKO8eb9tDsr5RhAjxSXWOi/agHd5IMUvB+ZbgRN78FfwpTcJIykhHgtb/nFPb4iYm5Z8rg+Bum6oh0esqeyThc0ezLvXxf4BkOPc53p12gpUKIhAzP0honX73vEuyTcATB88xEdyfCTLBrVO+nkFjobeBryzw7GIw0w+PiNmeHoo7d/nACOGH58z3NpJYV8S6bhj772mLRTPAMZUALEPfYb5JoTMDZ2ZEwRmrS1iQmmIcRx5Js4dWaGnhmUA85djsFI5tctw1Hai98zZUEI9apuYD8TM= 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: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 tha= t'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 t= hat > > > > > > 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 wit= hout > > > > > > worrying about those memcg-specific checks. > > > > > > > > > > There is a (performance) reason these open coded check are presen= t 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 pa= th, > > > > > 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 c= hange > > > > 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.