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

Kairui Song
2026-04-19T15:56:15Z

On Sat, Apr 18, 2026 at 10:05 PM YoungJun Park <youngjun.park@lge.com> 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!