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 B889FD5E379 for ; Sat, 9 Nov 2024 18:59:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 31FBC6B008C; Sat, 9 Nov 2024 13:59:01 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 2B1B06B0092; Sat, 9 Nov 2024 13:59:01 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 121CC6B0096; Sat, 9 Nov 2024 13:59:01 -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 E47466B008C for ; Sat, 9 Nov 2024 13:59:00 -0500 (EST) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 99C821604DF for ; Sat, 9 Nov 2024 18:59:00 +0000 (UTC) X-FDA: 82767466194.17.AB89256 Received: from mail-ed1-f47.google.com (mail-ed1-f47.google.com [209.85.208.47]) by imf23.hostedemail.com (Postfix) with ESMTP id A96C7140007 for ; Sat, 9 Nov 2024 18:58:33 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="JqriQ1c/"; spf=pass (imf23.hostedemail.com: domain of joshua.hahnjy@gmail.com designates 209.85.208.47 as permitted sender) smtp.mailfrom=joshua.hahnjy@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1731178598; 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=YkXlVsWL0LDP2pYZSQamK9+dqruXX3tJOG4uBlgHeNY=; b=q5b2WNwqmeRkrNyDqTtZBqgEe1W5JMDiJHIpH5aYeay9avY1CP+mEuVpHTG3Q9DXJDUKq4 50IKjTlUbfIOEZueuug3/rzYeUZ7tyadswsTSNl50beJ7fRkjEgu1yTAeGXhVyOgwyqJcg FbZj7ttgF7+bwe+voBat24l5IvdQB3o= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1731178598; a=rsa-sha256; cv=none; b=1RvRY4zzDA8k2+ag30QLzZ6pvYHuyEraRjlYnEvVEu7eqLdiz/+GfoylnjjQvzLT3s4ATf xfzm0ND8oVCc0LjmqE7VpDEE4il0LPeQ+XO239kkBztfOZBO43x4oJ3OctePi352kM5VAb E/QYd7m07JPGrAJb2zEveMe4R8vZVME= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="JqriQ1c/"; spf=pass (imf23.hostedemail.com: domain of joshua.hahnjy@gmail.com designates 209.85.208.47 as permitted sender) smtp.mailfrom=joshua.hahnjy@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-ed1-f47.google.com with SMTP id 4fb4d7f45d1cf-5c941623a5aso7514384a12.0 for ; Sat, 09 Nov 2024 10:58:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1731178737; x=1731783537; 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=YkXlVsWL0LDP2pYZSQamK9+dqruXX3tJOG4uBlgHeNY=; b=JqriQ1c/7WhIFULsamqRHVwBdTjBOteSybPDt2OF53sYw6WviLqaDSfdtBICpjbhby 70hQz4yO6j3wVCL2FdojBz6Umi0fMFTqsWDugshglnqW5jda+WCGxHMU8HlZCtQ+6Vlk pkKGleCG2oNq6QJzFqoWl8SIxNrjQ0gpXFSvwb/Vht2R96ilIly8MCZ/+0Aoi2TAY5pY fqyLkii187MhPNoR/KkezoVNBUYwDqR58ScOJd67Bayk1GtSpbbfgwtlWQV8KzjSzYXw SygW7njP1Ywk7HZ+jpziW70Tzg8qL3xKK7N9/9X2sisM4jAu0XH87ordfWqTCeuJgHCP AE0w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731178737; x=1731783537; 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=YkXlVsWL0LDP2pYZSQamK9+dqruXX3tJOG4uBlgHeNY=; b=Z2P41sJU+4ehbuN+kflDtyebHyIC5yj/+EtjMN75Gl6VrMeP64431ix1ATgllG9bI+ xDwzjreGnxSKdl2izgbT+Cl+YNqXI8kFbMFzjvXOOf33j6zFIE24WjiXVr2eaOWX86g8 d9DwJILfw8BEgrOFm5NMTxFeCUP13UCSZgUfpV/NuqZKGwWsGGxQFfuE/OFugYD2ewtN 62DuPOLFo2Ozyg00zjrzcIA9EkDT8Wc2NITePFHnPWwyTRy6FYAhUpHHqRt6vaxbUn1D TYiRKQR8WS2WieqVlKOJ154TAmtmeNpcrwicEe7NZE8sVOHX0/C1NHfFEBiGsBYFvrqS yIBw== X-Forwarded-Encrypted: i=1; AJvYcCXjhJ+4ThPBeGPUNLNOpSoieSXYXKN3ZKWjXI2/8ykHy/KYsvkpop8OpetyYD28nxwnsu8Q0HnQMg==@kvack.org X-Gm-Message-State: AOJu0YzOfHMGtof26blZ3HBzrOVvCgbrNAqdptgz3BWAN0tATsfNwUqt DupTUF8ykOYYN8BbeApF8NGqvuHMYUQxA+V2tV5ku1MH2PCG20h9oW3bXytZz2Vo1HBUzFzTBBz OGgcBoInbe9Y4007krTQ3un9SkV8= X-Google-Smtp-Source: AGHT+IEMHnjxHRVQ6HWzYS26PEZzRqHV3qeSPEb7/kPtEKJPdNiw7SS5CEd5p4EzAj7+0bt3ktx36WVY1m/0KKac/QY= X-Received: by 2002:a17:907:9455:b0:a9a:b818:521d with SMTP id a640c23a62f3a-a9eec9cf03dmr802199566b.18.1731178736956; Sat, 09 Nov 2024 10:58:56 -0800 (PST) MIME-Version: 1.0 References: <20241108212946.2642085-1-joshua.hahnjy@gmail.com> <20241108212946.2642085-3-joshua.hahnjy@gmail.com> In-Reply-To: From: Joshua Hahn Date: Sat, 9 Nov 2024 13:58:46 -0500 Message-ID: Subject: Re: [PATCH 2/3] memcg/hugetlb: Introduce mem_cgroup_charge_hugetlb To: Shakeel Butt Cc: hannes@cmpxchg.org, mhocko@kernel.org, roman.gushchin@linux.dev, muchun.song@linux.dev, akpm@linux-foundation.org, cgroups@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-team@meta.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: A96C7140007 X-Stat-Signature: fj1wfbaky7brjbu3nodwuffs834391ig X-HE-Tag: 1731178713-446317 X-HE-Meta: U2FsdGVkX1872fU36lBK315icFUXe02Q6zurl3slJn9VV3PNzrP7tXseKiTRV2pphe5i9ZcGBG5iVnsFu7puiht4UtIVidFsymwcGdBOVw2VMvBW1VbsDRjwXhYLDmzgo6Id+2mqj5/m9H81s2n9FSZZTsN4xc5RSkELe7pOAAlUc7TTQUKia27NsIVvcqpLPKEPNjEtB+Bu7ZS/a7CJLKcbMl6nmAo4UmY4EP4DDWbOzBK/vCMyUaASSxOfOVRnZNKT8z5uW0a594tlGSn4uvJ49MJXDCH+RkqHg2jM9z6Bcxj+FxvjYsAQOlQ1s6dRLMhoaYG9Oq3MgR32+4CEmSqSg6CBZn5nslJHXE9Z6yspvYHC9Tr37X2Q4i6aLKddeymS8TVkJvGR2HzwHMTnEQ+T3ZHYmyTCw/LFgCPDCrx54crXuhA+Vvp4VLs5ZtnSnyLE6sL1VnyAPEaaodgivlgbgEOB06DCmEaijRT9GEjjvmKzz23TBLMoa1EmTS3FnoK8EPnY/ksTtnU9a8hpWk+LQ4ecCUMiJYlXeyKMssPcgl2xAFUWeb6lN7KyHLNPdnCcMOapHaOsrMz7XqWsgVidKTqATqh+A38izIbwG4TyJDzfeoQd5cMbiJ5mSUzFtlHb+Cw358937vW31DoSWfBWsnLY1uI7Dzf3l0E9a4iwZJZBxfoDSATPFmMfS/duP/oI/UTlR3XqTf6qNaCPAo6Vt1VAzJ0kVPwo9xthtb6azsoMEdjZ8uDE/offjzvRjCzI524BbsXPJuJx5dBXS4PQZ8pyRB4Xl/Jana7joG+dkDQjHdKoeAbH6XrwCjVBYjFXydEnq1IMKCYciultorDFnkZC7Z5XBqtsSN25E5hYOoJ+KNA8/3Qj9RP7+iiDlihSNhvKnv1VEGh6ukuH+2HGvEjtwQskBHcqwhHFrnv+HWz63t47wlHd7b96/7lJTJE3z15StUecXl3+Nra VvYpG/WN 95+98o7DL2t6GW9M5x7Kl5IuRIShHCg3Cf+SexJJofjDt+Y/Ts14xx6B3dHXOc5KpGUh4bAofneKioVS4zUSqQvrGgnN648ms7ySmpS65NwgapoD8WsOn7fqyZPjmdf8roeM6Ccd9pbHCx89h4C9bcbCeK+tJBoDjpRFNip9AoSIf7sCzd+Y/6sNfBRAtD/PfSZ1+8s8SPrQXL8XrAVqJSF5K0FSn2mQJAkoiBny5+FUALWEvLfgpikKQov307DG/ZTCSVIXxWYxg1YNJ8N60X5pChp8RJ2+bnOj69ASApxhLVRohXX8Xa4OIzaB6CSzRAgSXbithQb8aPcq4qQJJWyDh7OcvKroXwTWx9pngJx/l3k/z9iuXYCyp4O6eMpTZkO5E+g1Nagpf1NG51B7T5CvKnU3ENZC5XCHKBYFZt1s5DF8uwmE1eFQ4sqeJq9w4qzsXYQJJE/R6hrI= X-Bogosity: Ham, tests=bogofilter, spamicity=0.002566, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: Hello Shakeel, thank you for reviewing my patch! On Fri, Nov 8, 2024 at 5:43=E2=80=AFPM Shakeel Butt wrote: > > On Fri, Nov 08, 2024 at 01:29:45PM -0800, Joshua Hahn wrote: > > This patch introduces mem_cgroup_charge_hugetlb, which combines the > > logic of mem_cgroup{try,commit}_hugetlb. This reduces the footprint of > > memcg in hugetlb code, and also consolidates the error path that memcg > > can take into just one point. > > > > Signed-off-by: Joshua Hahn > > - if (!memcg_charge_ret) > > - mem_cgroup_commit_charge(folio, memcg); > > - lruvec_stat_mod_folio(folio, NR_HUGETLB, pages_per_huge_page(h)); > > - mem_cgroup_put(memcg); > > + ret =3D mem_cgroup_charge_hugetlb(folio, gfp); > > + if (ret =3D=3D -ENOMEM) { > > + spin_unlock_irq(&hugetlb_lock); > > spin_unlock_irq?? Thank you for the catch. I completely missed this after I swapped the position of mem_cgroup_charge_hugetlb to be called without the lock. I will fix this. > > + free_huge_folio(folio); > > free_huge_folio() will call lruvec_stat_mod_folio() unconditionally but > you are only calling it on success. This may underflow the metric. I was actually thinking about this too. I was wondering what would make sense -- in the original draft of this patch, I had the charge increment be called unconditionally as well. The idea was that even though it would not make sense to have the stat incremented when there is an error, it would eventually be corrected by free_huge_folio's decrement. However, because there is nothing stopping the user from checking the stat in this period, they may temporarily see that the value is incremented in memory.stat, even though they were not able to obtain this page. With that said, maybe it makes sense to increment unconditionally, if free also decrements unconditionally. This race condition is not something that will cause a huge problem for the user, although users relying on userspace monitors for memory.stat to handle memory management may see some problems. Maybe what would make the most sense is to do both incrementing & decrementing conditionally as well. Thank you for your feedback, I will iterate on this for the next version! > > +int mem_cgroup_charge_hugetlb(struct folio *folio, gfp_t gfp) > > +{ > > + struct mem_cgroup *memcg =3D get_mem_cgroup_from_current(); > > + int ret =3D 0; > > + > > + if (mem_cgroup_disabled() || !memcg_accounts_hugetlb() || > > + !memcg || !cgroup_subsys_on_dfl(memory_cgrp_subsys)) { > > + ret =3D -EOPNOTSUPP; > > why EOPNOTSUPP? You need to return 0 here. We do want > lruvec_stat_mod_folio() to be called. In this case, I was just preserving the original code's return statements. That is, in mem_cgroup_hugetlb_try_charge, the intended behavior was to return -EOPNOTSUPP if any of these conditions were met. If I understand the code correctly, calling lruvec_stat_mod_folio() on this failure will be a noop, since either memcg doesn't account hugetlb folios / there is no memcg / memcg is disabled. With all of this said, I think your feedback makes the most sense here, given the new semantics of the function: if there is no memcg or memcg doesn't account hugetlb, then there is no way that the limit can be reached! I will go forward with returning 0, and calling lruvec_stat_mod_folio (which will be a noop). Thank you for your detailed feedback. I wish I had caught these errors myself, thank you for your time in reviewing my patch. I hope you have a great rest of your weekend! Joshua