Received-SPF: pass (google.com: domain of linux-kernel+bounces-172619-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH v1 2/4] mm: memcg: merge multiple page_counters into a single structure
From: Roman Gushchin <roman.gushchin@linux.dev>
In-Reply-To: <CABdmKX1NfkogiKQ4VGC8WoK4WLEG552QttNSUzLWRT46i2mQmw@mail.gmail.com>
Date: Tue, 7 May 2024 19:07:22 -0700
Cc: Shakeel Butt <shakeel.butt@linux.dev>,
 Andrew Morton <akpm@linux-foundation.org>,
 Muchun Song <muchun.song@linux.dev>, Johannes Weiner <hannes@cmpxchg.org>,
 Michal Hocko <mhocko@kernel.org>, Frank van der Linden <fvdl@google.com>,
 linux-mm@kvack.org, linux-kernel@vger.kernel.org
Message-Id: <C4D5522D-592F-4E55-AEFD-845D7165F89D@linux.dev>
References: <CABdmKX1NfkogiKQ4VGC8WoK4WLEG552QttNSUzLWRT46i2mQmw@mail.gmail.com>
To: "T.J. Mercier" <tjmercier@google.com>


> On May 7, 2024, at 5:26=E2=80=AFPM, T.J. Mercier <tjmercier@google.com> wr=
ote:
>=20
> =EF=BB=BFOn Fri, May 3, 2024 at 2:18=E2=80=AFPM Roman Gushchin <roman.gush=
chin@linux.dev> wrote:
>>=20
>>> On Fri, May 03, 2024 at 02:11:17PM -0700, Shakeel Butt wrote:
>>>> On Fri, May 03, 2024 at 01:18:33PM -0700, Roman Gushchin wrote:
>>> [...]
>>>> enum mem_counter_type {
>>>> +#ifdef CONFIG_MEMCG
>>>>    MCT_MEMORY,             /* cgroup v1 and v2 */
>>>>    MCT_SWAP,               /* cgroup v2 only */
>>>>    MCT_MEMSW =3D MCT_SWAP,   /* cgroup v1 only */
>>>>    MCT_KMEM,               /* cgroup v1 only */
>>>>    MCT_TCPMEM,             /* cgroup v1 only */
>>>> +#endif
>>>> +#ifdef CONFIG_CGROUP_HUGETLB
>>>> +   MCT_HUGETLB_MAX =3D __MCT_HUGETLB_MAX,
>>>> +#endif
>>>> +   __MCT_NR_ITEMS,
>>>> };
>>>>=20
>>>=20
>>> Thanks for the awesome work. I haven't gone through all the patches yet
>>> but wanted to ask a quick question. In the above enum are you trying to
>>> do a union between memcg and hugetlb? It gave me a big pause to
>>> understand what you are trying to do.
>>=20
>> Yep, sort of. So the page_counter structure supports N independent
>> counters, where N is sufficient enough for both memcg and hugetlb cases.
>>=20
>> MCT_MEMORY, MCT_SWAP etc are used directly in the memcontrol.c code,
>> while hugetlb code just indexes. MCT_HUGETLB_MAX magic is needed to defin=
e
>> N at the compile time.
>=20
> Where N is __MCT_NR_ITEMS for all the counter array lengths? That
> doesn't look like it works if MCT_HUGETLB_MAX is small... i.e. there
> is both CONFIG_MEMCG and CONFIG_CGROUP_HUGETLB and (__MCT_HUGETLB_MAX
> =3D 1 or 3) since MCT_HUGETLB_MAX would be < MCT_TCPMEM and then
> __MCT_NR_ITEMS would be wrong?
>=20
> If so, what about:
>=20
> enum mem_counter_type {
> #ifdef CONFIG_MEMCG
>    MCT_MEMORY, /* cgroup v1 and v2 */
>    MCT_SWAP, /* cgroup v2 only */
>    MCT_MEMSW =3D MCT_SWAP, /* cgroup v1 only */
>    MCT_KMEM, /* cgroup v1 only */
>    MCT_TCPMEM, /* cgroup v1 only */
> #endif
>    MCT_MEMCG_NR_ITEMS,
> #ifdef CONFIG_CGROUP_HUGETLB
>    MCT_HUGETLB_MAX =3D MCT_MEMCG_NR_ITEMS + __MCT_HUGETLB_MAX,
> #else
>    MCT_HUGETLB_MAX =3D 0,
> #endif
>    __MCT_NR_ITEMS =3D MAX(MCT_MEMCG_NR_ITEMS, MCT_HUGETLB_MAX)
> };

The page_counter structure is not shared between memory and hugetlb cgroups,=
 so N should be big enough to accommodate 4 memcg counters __or__ 2 * HUGE_M=
AX_STATE hugetlb counters. Your version has enough space for both.

Thanks!=