Re: [PATCH v2 11/11] mm, swap: merge zeromap into swap table

YoungJun Park
2026-04-19T12:50:54Z

On Sat, Apr 18, 2026 at 09:34:35PM +0800, Kairui Song wrote:
> On Sat, Apr 18, 2026 at 8:28 PM YoungJun Park <youngjun.park@lge.com> wrote:
> >
> > On Fri, Apr 17, 2026 at 02:34:41AM +0800, Kairui Song via B4 Relay wrote:
> >
> > > *
> > > * Usages:
> > > *
> > > @@ -74,17 +76,22 @@ struct swap_memcg_table {
> > > #define SWP_TB_PFN_MARK_BITS 2
> > > #define SWP_TB_PFN_MARK_MASK (BIT(SWP_TB_PFN_MARK_BITS) - 1)
> > >
> > > -/* SWAP_COUNT part for PFN or shadow, the width can be shrunk or extended */
> > > -#define SWP_TB_COUNT_BITS min(4, BITS_PER_LONG - SWP_TB_PFN_BITS)
> > > +/* SWAP_COUNT and flags for PFN or shadow, width can be shrunk or extended */
> > > +#define SWP_TB_FLAGS_BITS min(5, BITS_PER_LONG - SWP_TB_PFN_BITS)
> > > +#define SWP_TB_COUNT_BITS (SWP_TB_FLAGS_BITS - 1)
> >
> > Hi Kairui :)
> >
> > Would this break the build on 32-bit arches with 40-bit phys
> > addrs (MAX_POSSIBLE_PHYSMEM_BITS = 40)?
> >
> > Architectures I checked.
> > - ARM LPAE (CONFIG_ARM_LPAE=y)
> > - ARC PAE40 (CONFIG_ARC_HAS_PAE40=y)
> > - MIPS XPA (CONFIG_XPA=y)
> >
> > Calculations.
> >
> > SWP_TB_PFN_BITS = 28 + 2 = 30
> > SWP_TB_FLAGS_BITS = min(5, 32 - 30) = 2
> > SWP_TB_COUNT_BITS = 2 - 1 = 1
> >
> > The BUILD_BUG_ON looks like the real problem. it needs at
> > least 3 count values (free/used/overflow).
> >
> > BUILD_BUG_ON(SWP_TB_COUNT_MAX < 2 || SWP_TB_COUNT_BITS < 2);
> >
> > Confirmed with a cross build (multi_v7_defconfig + lpae.config).
> >
> > error: BUILD_BUG_ON failed: SWP_TB_COUNT_MAX < 2 || SWP_TB_COUNT_BITS < 2
> > at __count_to_swp_tb (mm/swap_table.h:227)
>
> Hi YoungJun
>
> Nice catch! Thanks a lot :)
>
> > I think the right fix is widening swap_tb to 64 bits
> > unconditionally (atomic64_t).
>
> I'm a bit concerned that memory usage on 32 bits will bloat up...
>
> >
> > (Or, uglier, these arches could always route counts through the
> > extend table.)
> >
>
> Seems not ugly with a ci->zero_bitmap, looks clean to me, the
> definition will be:
>
> SWP_TABLE_USE_INLINE_ZEROMAP is true when BITS_PER_LONG is not enough
> for SWP_TB_FLAGS_BITS, then:
>
> struct swap_cluster_info {
> ...
> #ifndef SWP_TABLE_USE_INLINE_ZEROMAP
> unsigned long *zero_bitmap;
> #endif
> ...
> };
>
> And helpers will be:
> static inline void __swap_table_set_zero(struct swap_cluster_info *ci,
> unsigned int ci_off)
> {
> unsigned long swp_tb;
>
> #ifdef SWP_TABLE_USE_INLINE_ZEROMAP
> return bitmap_set(&ci->zeromap);
> #else
>
> swp_tb = __swap_table_get(ci, ci_off);
> VM_WARN_ON(!swp_tb_is_countable(swp_tb));
> swp_tb |= SWP_TB_ZERO_MARK;
> __swap_table_set(ci, ci_off, swp_tb);
> }
>
> There are only three helpers in total, looks fine. Allocation part is
> just like the memcg_table. Compared to this version only it seems
> onlys needs a few dozen lines change (A few #ifdef
> SWP_TABLE_USE_INLINE_ZEROMAP) and not hard to understand. How do you
> think?

Hi Kairui,

Sounds good. easy to understand and not many changes.

Another option could be using a 64-bit entry only on LPAE-like arches,
(not including every 32-bit arch)
though that would mean adding a separate set of atomic64 ops.

The direction you proposed seems cleaner, so I'm on board.

Thanks,
YoungJun Park