Re: [PATCH v3 12/12] mm, swap: merge zeromap into swap table

[Kairui Song]

On Thu, May 14, 2026 at 2:33 AM YoungJun Park <youngjun.park@xxxxxxx> wrote:
>
> On Tue, Apr 21, 2026 at 02:16:56PM +0800, Kairui Song via B4 Relay wrote:
>
> Nice! LGTM
>
> Reviewed-by: Youngjun Park <youngjun.park@xxxxxxx>
>
> A few nitpicks follow. take them if you find them useful. :)

Thanks!

>
> > +static inline void __swap_table_clear_zero(struct swap_cluster_info *ci,
> > +                                        unsigned int ci_off)
> > +{
> > +
>
> trailing blank line.
>
> > +#if SWAP_TABLE_HAS_ZEROFLAG
> > +     unsigned long swp_tb = __swap_table_get(ci, ci_off);
> > +
> > +     VM_WARN_ON(!swp_tb_is_countable(swp_tb));
> > +     swp_tb &= ~SWP_TB_ZERO_FLAG;
> > +     __swap_table_set(ci, ci_off, swp_tb);
> > +#else
> > +     lockdep_assert_held(&ci->lock);
> > +     __clear_bit(ci_off, ci->zero_bitmap);
> > +#endif
> > +}
> ...
> > +
> >       table = (struct swap_table *)rcu_access_pointer(ci->table);
> >       if (!table)
> >               return;
> > @@ -470,6 +475,13 @@ static int swap_cluster_alloc_table(struct swap_cluster_info *ci, gfp_t gfp)
> >       if (!ci->memcg_table)
> >               ret = -ENOMEM;
> >  #endif
> > +
> > +#if !SWAP_TABLE_HAS_ZEROFLAG
> > +     ci->zero_bitmap = bitmap_zalloc(SWAPFILE_CLUSTER, gfp);
> > +     if (!ci->zero_bitmap)
> > +             ret = -ENOMEM;
> > +#endif
> > +
>
> memcg_table above uses `if (!ci->memcg_table)` before
> kzalloc, but ci->zero_bitmap is assigned unconditionally.  Both
> are NULL on entry today (swap_cluster_free_table nullifies them),
> so either form works but the asymmetry reads oddly.  Either
> drop the memcg guard (with an entry VM_WARN_ON asserting both
> NULL by design), or mirror the guard here.

Good idea. Let me turn them into VM_WARN_ON. They should never be
non-NULL right now, and proceeding after a WARN shouldn't be
catastrophic.

I also notice I better let them do something like goto err_free on the
failure case, where err_free aborts early and frees everything. If any
allocation fails, the entire cluster should be freed for cleanliness
instead of proceeding. I will update this part.