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 AFC39E743D5 for ; Fri, 29 Sep 2023 01:25:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 409998D00E2; Thu, 28 Sep 2023 21:25:23 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3B9B38D0002; Thu, 28 Sep 2023 21:25:23 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 25A6E8D00E2; Thu, 28 Sep 2023 21:25:23 -0400 (EDT) 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 CB9C98D0002 for ; Thu, 28 Sep 2023 21:25:22 -0400 (EDT) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id A05F31A0443 for ; Fri, 29 Sep 2023 01:25:22 +0000 (UTC) X-FDA: 81287892084.05.8796635 Received: from mail-io1-f44.google.com (mail-io1-f44.google.com [209.85.166.44]) by imf24.hostedemail.com (Postfix) with ESMTP id EBDBE180002 for ; Fri, 29 Sep 2023 01:25:20 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=gWzDvRJc; spf=pass (imf24.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.166.44 as permitted sender) smtp.mailfrom=nphamcs@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=1695950720; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=h3L1GAZy6HkuNwYvpeX+pKCoLERD/qgacGWNglWfEpU=; b=CW0nXWBv4LiXx2cqt37e/BdKTlugv/JvCzzFk0ESI/AnvJa58BM8NWvdrMRmzQdvarUdHt IhHrx9rCAbKUB3XTSJES8dZQXcWwW+loWWJ0O6CXzaH+CsRhBHxlnZco0MCklE7tBkWgHL WwGh/sXvSU8K70NHbGlaOroJgTH3xU8= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1695950721; a=rsa-sha256; cv=none; b=x2IOpNQ9oBtRi0tOWLh545QTVRqt93A6QHfj86Ah5yzqzyknvPgGNDkwthYA35eI7htIFH QWGB5AOoL6q86s3EfJs3fumwP0YkqoMSOMNH0bC5NdsBFezN1fWM8B6+5pQv11o7Y7QHZ1 MAccKavpV/CGHa+0JXuzB8lw6NS1SNY= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=gWzDvRJc; spf=pass (imf24.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.166.44 as permitted sender) smtp.mailfrom=nphamcs@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-io1-f44.google.com with SMTP id ca18e2360f4ac-79f95439795so3757439f.0 for ; Thu, 28 Sep 2023 18:25:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695950720; x=1696555520; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=h3L1GAZy6HkuNwYvpeX+pKCoLERD/qgacGWNglWfEpU=; b=gWzDvRJc+TbapTtPwFFrqiW3aivgVroReLbIY6+Eyx2Kmuj4rIrbiwCKtrFbhYJK2e ekdPze9xPsh4q/dNHXMz1fx2s7/G/07YGaWL+NIi7NJMOtf/FhpSvMA6rImhkOb91j7G 9IdFpobhdOd6dfOUevFvnA+IVh4FkZRDzI2WJ1a19b4l2UMsulh/+bsVBdZDkq8Jpve4 cZvS9n1NzsXUAe272OlOe4/dC5jEp/L49ivEu6Hoy+DCuiH1ER+UuSflulEACpxShiOg hQNNujYiUqGUPbLSNT1kmw+veQwAUcxn/KoEswU9e+8jhsVk7FElyHVJAsLSF+GAAET7 akiA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695950720; x=1696555520; h=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=h3L1GAZy6HkuNwYvpeX+pKCoLERD/qgacGWNglWfEpU=; b=u2bf7XaxZ9QpVoL2BsqJ1sEF+rWlh0F6DmEIVLRoPNGElJt1AlVzz2GTwthjue8W+t ae8ctpCsOXuH1vHKqfrpZC1ReXL1Q7SfgnBCdOtWVLbiI1TmcKFbQy0RT51TJ8RwBKc6 rjwt8xEal4P0c9EioMm37Hlm8Vk01018esupQ6k9BKVJvJX2/0XWmfHlCeYf9z2zOMQn htx1W00WjNCfrXG1WlSkgxl/iZxUbZe+nHyN88SCJeJjedTV8jwQmbeaA+NW0Tl+Q3Kc LCz4p85wsQt6Bgqp+Ye7oMd1TXEK3WcMWqk8nkmp9yNQ/MToaVzGrnhqcrhuvCrfYeCE qy1g== X-Gm-Message-State: AOJu0YyLavblOdZvwpEbq8pmhU3dTe0I9sVD12caoUryKdEdxB5po7Wu b19MEStzHWHa7Ceil8WKQQuewvXgMR52UVs/9Wk= X-Google-Smtp-Source: AGHT+IEwda/DGWDggLMyGn0jEcfY6v3nVqoYJjBTyyMI/nyS0erOB5U4GsQvgFt9/427oRBwkrXB6BmsbKE3BzZSQJ8= X-Received: by 2002:a05:6e02:12e5:b0:351:375f:2a31 with SMTP id l5-20020a056e0212e500b00351375f2a31mr2507519iln.6.1695950720072; Thu, 28 Sep 2023 18:25:20 -0700 (PDT) MIME-Version: 1.0 References: <20230928005723.1709119-1-nphamcs@gmail.com> <20230928005723.1709119-2-nphamcs@gmail.com> In-Reply-To: From: Nhat Pham Date: Thu, 28 Sep 2023 18:25:08 -0700 Message-ID: Subject: Re: [PATCH v2 1/2] hugetlb: memcg: account hugetlb-backed memory in memory controller To: Yosry Ahmed Cc: akpm@linux-foundation.org, cgroups@vger.kernel.org, hannes@cmpxchg.org, kernel-team@meta.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, lizefan.x@bytedance.com, mhocko@kernel.org, mike.kravetz@oracle.com, muchun.song@linux.dev, riel@surriel.com, roman.gushchin@linux.dev, shakeelb@google.com, shuah@kernel.org, tj@kernel.org Content-Type: multipart/alternative; boundary="0000000000003a01fb0606754cf2" X-Rspamd-Queue-Id: EBDBE180002 X-Rspam-User: X-Stat-Signature: 6y3g3asj89rk796ppogjfod616bjs5ee X-Rspamd-Server: rspam03 X-HE-Tag: 1695950720-41147 X-HE-Meta: U2FsdGVkX1+6sdU5VG/y1gF16d8YTmLONPQJfM1IPlwSm4kvq3V5IQp9WDCPtWOYxC/zslb6of5dnzqEyZVAhGkxAB1HyY2XcPpyje16I5Q/Y4KBCG4gbKEapbOsorFt5OGofoJyTvZuCVmcoIW1c8IkHcOPdWJBzirXl/91YfW/tumNSYHLNArSi2FsDM08yTRyhALtCCbJQoc0Ed6E81H7mYPAQv8XXknu4YlO/keehx6tseYqb4G09hxaNosYI6R7Sxnov5HX2vxXcpZljvjiejiuCmGHKDh1St1XUC4Dh1gKP9ILdhWEcEFhpoIfc+Sqjt/mwIfCGL95jtggFP4oGjMld8tS1P28E63IHd9ajr4Y7NARlz9U+isHXNz6GpcNi1A8iLMvpMZF8OM8WPAPkM/F0Gx/PdJ/5/uCFYU3G+T1d0nITyWeOToqsGQHWIOOR2zEeveXhLugDRs5n5qcXCJfGlYD/mKXilrijjrm0pk+52fGgmICboliqUmuiAyF1+09AZf5qa4qevImqSywK6Q5LftaGN2m/fzAeZ/k2Pe0Nslp5c+eIDIS9w7Gwop03Bb2hHF5MFZyUJlpvkc3PUnAzzQ2oGBdN0mbO3cnHOILrQGxvWeuLCxMKwfURpHJsrmHNkT1P/eDhOjVzLsfWWaLeXV08jiF+4isB82M5x9Z9Qs1OWUnyG+sZY92rkBvUfTdoijJo7Gf2XdOPHemj5vjsiKmAZDGIgJh/UZvTfkh8NevUYnD7Ke7tVkOx5R+oDMsNk2GBQAhcOrEQqcR14Cp+a6YqNbnYE7mSJt0Kko5zjKQb7JCyVa44qsC+xb/3XbaRp8362UhYV+b21eYWpjubKmJG/smfjNTbbU4wzWCp4vtvDxuMNmDIbasQqrAiZb3879UxsrD4/XcbUBXBlPHEZpAMcR3Kbz+BnO2i4kRlFADd19D9S8njqTgD5+OAa8wRJiuZ9o7rRc EhJkrqWs Bp3MYmoMv4/LIVHz2DS/R66FVHFfoPg3dgycyo7E4EPxTn4L5nCoiZ7uI/NjJx/pQahLK5mE7nfjOjqWdej+sYc/KHg6jUwM2Gqi2rKdHB6jO7ijz7J7DJQxSf6IBXptKRfPyBVlG3s/vbgrfnPz7eKregzrIOrh4JOU6q2HIJzxh2dW7+AQRiCqIFxijVHdoRRxGFggZzUN79E2OyFsp/Q8oMXzj1ZcdqvjKGPIfUW2eef6TZYfLHUZ8gMW1skPgVn3JgpJTNnvY+OjY9wawX2S9bWEHeL75qsGqjRPya0IFwsZjHOGkmTZGXd/ihSRFjMxxVzCCN0O6TbyArZvfroX4Q847m9Azy/XEWS7y69bXwgHBmLKLGHVPxHjH+ZGDMqg6sKMC5AbBX+nsffvFgFw1aUtY4dgLny5C7z7Bs+PkucguAH/bhGn3B8ul9h3+rf9SdrO1on1uFcSOyMMLP0qG670B3qDKLXy5aw24ni5pGVgk9fOLiZcuzQnnUvSHPgiO X-Bogosity: Ham, tests=bogofilter, spamicity=0.099186, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: --0000000000003a01fb0606754cf2 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Sep 28, 2023 at 18:18 Yosry Ahmed wrote: > On Thu, Sep 28, 2023 at 6:07=E2=80=AFPM Nhat Pham wro= te: > > > > On Thu, Sep 28, 2023 at 5:58=E2=80=AFPM Nhat Pham w= rote: > > > > > > On Thu, Sep 28, 2023 at 5:38=E2=80=AFPM Yosry Ahmed > wrote: > > > > > > > > > > > > > > > > > > > > > > + > > > > > +/** > > > > > + * mem_cgroup_hugetlb_charge_folio - Charge a newly allocated > hugetlb folio. > > > > > + * @folio: folio to charge. > > > > > + * @gfp: reclaim mode > > > > > + * > > > > > + * This function charges an allocated hugetlbf folio to the memc= g > of the > > > > > + * current task. > > > > > + * > > > > > + * Returns 0 on success. Otherwise, an error code is returned. > > > > > + */ > > > > > +int mem_cgroup_hugetlb_charge_folio(struct folio *folio, gfp_t > gfp) > > > > > +{ > > > > > + struct mem_cgroup *memcg; > > > > > + int ret; > > > > > + > > > > > + if (mem_cgroup_disabled() || > > > > > + !(cgrp_dfl_root.flags & > CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING)) > > > > > > > > What happens if the memory controller is mounted in a cgroup v1 > > > > hierarchy? It appears to me that we *will* go through with hugetlb > > > > charging in this case? > > > > > > Ah right, cgroup v1. Does it not work with mount flag guarding? > > > What's the behavior of cgroup v1 when it comes to memory > > > recursive protection for e.g (which this mount flag is based on)? > > > > > > If it doesn't work, we'll have to add a separate knob for v1 - > > > no biggies. > > > > But to be clear, my intention is that we're not adding this > > feature to v1 (which, to my understanding, has been > > deprecated). > > > > If it's added by virtue of it sharing infrastructure with v2, > > then it's fine, but only if the mount option still works to > > guard against unintentional enablement (if not we'll > > also short-circuit v1, or add knobs if ppl really want > > it in v1 as well). > > > > If it's not added at all, then I don't have any complaints :) > > > > > > > > Other than this concern, I don't have anything against cgroup v1 > > > having this feature per se - everything should still work. But let > > > I know if it can break cgroupv1 accounting otherwise :) > > > > > My concern is the scenario where the memory controller is mounted in > cgroup v1, and cgroup v2 is mounted with memory_hugetlb_accounting. Ohh I see. Lemme do some testing to double check :) > > In this case it seems like the current code will only check whether > memory_hugetlb_accounting was set on cgroup v2 or not, disregarding > the fact that cgroup v1 did not enable hugetlb accounting. > > I obviously prefer that any features are also added to cgroup v1, > because we still didn't make it to cgroup v2, especially when the > infrastructure is shared. On the other hand, I am pretty sure the > maintainers will not like what I am saying :) I can at least try to not break v1 for a start :) Thanks for pointing it out tho! > --0000000000003a01fb0606754cf2 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Thu, Sep 28, 2023 at 18:18 Yosry Ahmed <yosryahmed@google.com> wrote:
On Thu, Sep 28, 2023 at 6:07=E2=80=AFP= M Nhat Pham <npha= mcs@gmail.com> wrote:
>
> On Thu, Sep 28, 2023 at 5:58=E2=80=AFPM Nhat Pham <nphamcs@gmail.com> wrote:
> >
> > On Thu, Sep 28, 2023 at 5:38=E2=80=AFPM Yosry Ahmed <yosryahmed@google.com> wrote:
> > >
> > > <snip>
> > >
> > > >
> > > > +
> > > > +/**
> > > > + * mem_cgroup_hugetlb_charge_folio - Charge a newly al= located hugetlb folio.
> > > > + * @folio: folio to charge.
> > > > + * @gfp: reclaim mode
> > > > + *
> > > > + * This function charges an allocated hugetlbf folio t= o the memcg of the
> > > > + * current task.
> > > > + *
> > > > + * Returns 0 on success. Otherwise, an error code is r= eturned.
> > > > + */
> > > > +int mem_cgroup_hugetlb_charge_folio(struct folio *foli= o, gfp_t gfp)
> > > > +{
> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct mem_cgroup *memcg; > > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0int ret;
> > > > +
> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0if (mem_cgroup_disabled() |= |
> > > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0!(cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING))
> > >
> > > What happens if the memory controller is mounted in a cgroup= v1
> > > hierarchy? It appears to me that we *will* go through with h= ugetlb
> > > charging in this case?
> >
> > Ah right, cgroup v1. Does it not work with mount flag guarding? > > What's the behavior of cgroup v1 when it comes to memory
> > recursive protection for e.g (which this mount flag is based on)?=
> >
> > If it doesn't work, we'll have to add a separate knob for= v1 -
> > no biggies.
>
> But to be clear, my intention is that we're not adding this
> feature to v1 (which, to my understanding, has been
> deprecated).
>
> If it's added by virtue of it sharing infrastructure with v2,
> then it's fine, but only if the mount option still works to
> guard against unintentional enablement (if not we'll
> also short-circuit v1, or add knobs if ppl really want
> it in v1 as well).
>
> If it's not added at all, then I don't have any complaints :)<= br> >
> >
> > Other than this concern, I don't have anything against cgroup= v1
> > having this feature per se - everything should still work. But le= t
> > I know if it can break cgroupv1 accounting otherwise :)
> >

My concern is the scenario where the memory controller is mounted in
cgroup v1, and cgroup v2 is mounted with memory_hugetlb_accounting.

Ohh I see. Lemme do some= testing to double
check :)
<= br>



In this case it seems like the current code will only check whether
memory_hugetlb_accounting was set on cgroup v2 or not, disregarding
the fact that cgroup v1 did not enable hugetlb accounting.

I obviously prefer that any features are also added to cgroup v1,
because we still didn't make it to cgroup v2, especially when the
infrastructure is shared. On the other hand, I am pretty sure the
maintainers will not like what I am saying :)

I can at least try to not break v1 for a start= :)
Thanks for pointing it out tho!
--0000000000003a01fb0606754cf2--