> On May 7, 2024, at 5:26 PM, T.J. Mercier <tjmercier@xxxxxxxxxx> wrote:
>
> On Fri, May 3, 2024 at 2:18 PM Roman Gushchin <roman.gushchin@xxxxxxxxx> wrote:
>>
>>> 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 = MCT_SWAP, /* cgroup v1 only */
>>>> MCT_KMEM, /* cgroup v1 only */
>>>> MCT_TCPMEM, /* cgroup v1 only */
>>>> +#endif
>>>> +#ifdef CONFIG_CGROUP_HUGETLB
>>>> + MCT_HUGETLB_MAX = __MCT_HUGETLB_MAX,
>>>> +#endif
>>>> + __MCT_NR_ITEMS,
>>>> };
>>>>
>>>
>>> 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.
>>
>> Yep, sort of. So the page_counter structure supports N independent
>> counters, where N is sufficient enough for both memcg and hugetlb cases.
>>
>> 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 define
>> N at the compile time.
>
> 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
> = 1 or 3) since MCT_HUGETLB_MAX would be < MCT_TCPMEM and then
> __MCT_NR_ITEMS would be wrong?
>
> If so, what about:
>
> enum mem_counter_type {
> #ifdef CONFIG_MEMCG
> MCT_MEMORY, /* cgroup v1 and v2 */
> MCT_SWAP, /* cgroup v2 only */
> MCT_MEMSW = 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 = MCT_MEMCG_NR_ITEMS + __MCT_HUGETLB_MAX,
> #else
> MCT_HUGETLB_MAX = 0,
> #endif
> __MCT_NR_ITEMS = 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_MAX_STATE hugetlb counters. Your version has enough space for both.
Thanks!
