Re: [PATCH v2 09/11] mm/memcg, swap: store cgroup id in cluster table directly

[Kairui Song]

On Sat, Apr 18, 2026 at 10:05 PM YoungJun Park <youngjun.park@xxxxxxx> wrote:
>
>
> > +             if (IS_ENABLED(CONFIG_MEMCG) && !memcg_table) {
> > +                     swap_table_free(table);
> > +                     return -ENOMEM;
> > +             }
>
> Hi Kairui,

Thanks YoungJun,

>
> Nit:
> (Just a readability nit. purely my preference, feel free to ignore.)
> the checks around swap_memcg_table_alloc() reduce to two
> equivalent forms of the same memcg success/failure question:
>
> (!IS_ENABLED(CONFIG_MEMCG) || memcg_table)   /* success */
> (IS_ENABLED(CONFIG_MEMCG) && !memcg_table)   /* failure */
>
> A macro for the failure side would let the call sites read as plain
> positive/negative:

Your suggestion is really helpful! I also thought about maybe I should
do some cleanup of metadata first before adding more metadata cleanup
for a memcg table. In this V2 it's still not that hard to follow, but
in V3 if we have an optional zero map bitmap, things may get really
messy if I just keep piling up things like this.

>
> #define SWAP_MEMCG_TABLE_ALLOC_FAILED(t) \
>         (IS_ENABLED(CONFIG_MEMCG) && !(t))
>
> SWAP_MEMCG_TABLE_ALLOC_FAILED(memcg_table)     /* failure */
> !SWAP_MEMCG_TABLE_ALLOC_FAILED(memcg_table)    /* success */
>
> Equivalently, the same macro can be expressed by splitting on
> CONFIG_MEMCG.
>
> #ifdef CONFIG_MEMCG
> #define SWAP_MEMCG_TABLE_ALLOC_FAILED(t)  (!(t))
> #else
> #define SWAP_MEMCG_TABLE_ALLOC_FAILED(t)  (0)
> #endif
>
> What do you think?

Good idea. I also agree this part needs some cleanup. I will take your
suggestion and do a few more cleanup in a seperate commit. Maybe just
let the helper return early if MEMCG is not defined or something like
you suggested.

Thanks for the review!