Re: [PATCH v1 5/8] mm: zswap: Store the folio's node id in the zswap_entry.

[Kanchana P. Sridhar]

On Tue, Oct 13, 2026 at 5:18 PM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
>
> On Thu, Oct 08, 2026 at 11:28:11AM -0700, Kanchana P. Sridhar wrote:
> > When storing a folio in zswap, save the folio's node id in the zswap
> > entry instead of relying on the zswap_entry being allocated on the same
> > node. This enables optimizations such as batch allocation of zswap
> > entries for large folios using kmem_cache_alloc_bulk() that does not
> > support specifying a node id.
> >
> > The size of the zswap_entry remains unchanged as 'referenced' is lumped
> > in with the 'length' (as it doesn't need a full unsigned int anyway).
> >
> > Suggested-by: Nhat Pham <nphamcs@xxxxxxxxx>
> > Suggested-by: Yosry Ahmed <yosry.ahmed@xxxxxxxxx>
> > Signed-off-by: Kanchana P Sridhar <kanchana.p.sridhar@xxxxxxxxx>
> > Signed-off-by: Kanchana P. Sridhar <kanchanapsridhar2026@xxxxxxxxx>
> > ---
> >  mm/zswap.c | 23 +++++++++++------------
> >  1 file changed, 11 insertions(+), 12 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index ca513a9f89e3..c2731d3acb2c 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -194,6 +194,7 @@ static struct shrinker *zswap_shrinker;
> >   *              writeback logic. The entry is only reclaimed by the writeback
> >   *              logic if referenced is unset. See comments in the shrinker
> >   *              section for context.
> > + * nid - NUMA node id of the page for which this is the zswap entry.
> >   * pool_idx - id of the zswap_pool that the entry's data is in.
> >   * handle - zsmalloc allocation handle that stores the compressed page data
> >   * objcg - the obj_cgroup that the compressed memory is charged to
> > @@ -201,8 +202,11 @@ static struct shrinker *zswap_shrinker;
> >   */
> >  struct zswap_entry {
> >       swp_entry_t swpentry;
> > -     unsigned int length;
> > -     bool referenced;
> > +     struct {
> > +             unsigned int length:31;
> > +             bool referenced:1;
> > +     };
> > +     int nid;
>
> This will increase the size of the struct now with pool_idx, right?

I was gathering the struct size with the struct {unsigned int
length:31; bool referenced:1} as suggested in v14, for my before/after
comparisons:

before:
struct zswap_entry {
        swp_entry_t                swpentry;             /*     0     8 */
        struct {
                unsigned int       length:31;            /*     8: 0  4 */

                /* Bitfield combined with next fields */

                bool               referenced:1;         /*    11: 7  1 */
        };                                               /*     8     4 */
        int                        nid;                  /*    12     4 */
        u8                         pool_idx;             /*    16     1 */

        /* XXX 7 bytes hole, try to pack */

        long unsigned int          handle;               /*    24     8 */
        struct obj_cgroup *        objcg;                /*    32     8 */
        struct list_head           lru;                  /*    40    16 */

        /* size: 56, cachelines: 1, members: 7 */
        /* sum members: 49, holes: 1, sum holes: 7 */
        /* last cacheline: 56 bytes */
};

after:
struct zswap_entry {
        swp_entry_t                swpentry;             /*     0     8 */
        struct {
                unsigned int       length:31;            /*     8: 0  4 */

                /* Bitfield combined with next fields */

                bool               referenced:1;         /*    11: 7  1 */
        };                                               /*     8     4 */
        int                        nid;                  /*    12     4 */
        long unsigned int          handle;               /*    16     8 */
        struct obj_cgroup *        objcg;                /*    24     8 */
        struct list_head           lru;                  /*    32    16 */
        u8                         pool_idx;             /*    48     1 */

        /* size: 56, cachelines: 1, members: 7 */
        /* padding: 7 */
        /* last cacheline: 56 bytes */
};

And decided to go with the placement in patch 7, since it also
separates out members by compress/decompress path accesses. pool_idx
is set once, and never changes. Further, moving pool_idx to the end
gets rid of holes.

>
> Before you patch on x86_64:
>
> struct zswap_entry {
>         swp_entry_t                swpentry;             /*     0     8 */
>         unsigned int               length;               /*     8     4 */
>         bool                       referenced;           /*    12     1 */
>         u8                         pool_idx;             /*    13     1 */
>
>         /* XXX 2 bytes hole, try to pack */
>
>         unsigned long              handle;               /*    16     8 */
>         struct obj_cgroup *        objcg;                /*    24     8 */
>         struct list_head           lru;                  /*    32    16 */
>
>         /* size: 48, cachelines: 1, members: 7 */
>         /* sum members: 46, holes: 1, sum holes: 2 */
>         /* last cacheline: 48 bytes */
> };
>
> After:
>
> struct zswap_entry {
>         swp_entry_t                swpentry;             /*     0     8 */
>         struct {
>                 unsigned int       length:31;            /*     8: 0  4 */
>
>                 /* Bitfield combined with next fields */
>
>                 bool               referenced:1;         /*    11: 7  1 */
>         };                                               /*     8     4 */
>         struct {
>                 unsigned int               length:31;            /*     0: 0  4 */
>
>                 /* Bitfield combined with next fields */
>
>                 bool                       referenced:1;         /*     3: 7  1 */
>
>                 /* size: 4, cachelines: 1, members: 2 */
>                 /* last cacheline: 4 bytes */
>         };
>
>         int                        nid;                  /*    12     4 */
>         u8                         pool_idx;             /*    16     1 */
>
>         /* XXX 7 bytes hole, try to pack */
>
>         unsigned long              handle;               /*    24     8 */
>         struct obj_cgroup *        objcg;                /*    32     8 */
>         struct list_head           lru;                  /*    40    16 */
>
>         /* size: 56, cachelines: 1, members: 7 */
>         /* sum members: 49, holes: 1, sum holes: 7 */
>         /* last cacheline: 56 bytes */
> };
>
> I think we want to reorder the fields. We can probably also avoid the
> bitfield if the maximum page size is 64K (is it?)

It seems on ARM64, the max page size is 64K, however on PPC, it can be
256K. So it appears we need at least 18 bits on currently supported
architectures.
If you agree with the pool_idx placement to be moved to the end as
proposed in patch 7, I can try and optimize this further, by limiting
the "unsigned int length:18", if you think this makes sense.

>
> struct zswap_entry {
>         swp_entry_t                swpentry;             /*     0     8 */
>         u16                        length;               /*     8     2 */
>         u8                         pool_idx;             /*    10     1 */
>         bool                       referenced;           /*    11     1 */
>         int                        nid;                  /*    12     4 */
>         unsigned long              handle;               /*    16     8 */
>         struct obj_cgroup *        objcg;                /*    24     8 */
>         struct list_head           lru;                  /*    32    16 */
>
>         /* size: 48, cachelines: 1, members: 8 */
>         /* last cacheline: 48 bytes */
> };
>
>
> >       u8 pool_idx;
> >       unsigned long handle;
> >       struct obj_cgroup *objcg;
> > @@ -672,14 +676,8 @@ static int zswap_enabled_param_set(const char *val,
> >  * lru functions
> >  **********************************/
> >
> > -static inline int entry_to_nid(struct zswap_entry *entry)
> > -{
> > -     return page_to_nid(virt_to_page(entry));
> > -}
> > -
> >  static void zswap_lru_add(struct zswap_entry *entry)
> >  {
> > -     int nid = entry_to_nid(entry);
> >       struct mem_cgroup *memcg;
> >
> >       /*
> > @@ -696,19 +694,18 @@ static void zswap_lru_add(struct zswap_entry *entry)
> >       rcu_read_lock();
> >       memcg = obj_cgroup_memcg(entry->objcg);
> >       /* will always succeed */
> > -     list_lru_add(&zswap_list_lru, &entry->lru, nid, memcg);
> > +     list_lru_add(&zswap_list_lru, &entry->lru, entry->nid, memcg);
> >       rcu_read_unlock();
> >  }
> >
> >  static void zswap_lru_del(struct zswap_entry *entry)
> >  {
> > -     int nid = entry_to_nid(entry);
> >       struct mem_cgroup *memcg;
> >
> >       rcu_read_lock();
> >       memcg = obj_cgroup_memcg(entry->objcg);
> >       /* will always succeed */
> > -     list_lru_del(&zswap_list_lru, &entry->lru, nid, memcg);
> > +     list_lru_del(&zswap_list_lru, &entry->lru, entry->nid, memcg);
> >       rcu_read_unlock();
> >  }
> >
> > @@ -1449,9 +1446,10 @@ static bool zswap_store_page(struct folio *folio, long index,
> >  {
> >       swp_entry_t page_swpentry = folio_swap_entry(folio, index);
> >       struct zswap_entry *entry, *old;
> > +     int nid = folio_nid(folio);
> >
> >       /* allocate entry */
> > -     entry = zswap_entry_cache_alloc(GFP_KERNEL, folio_nid(folio));
> > +     entry = zswap_entry_cache_alloc(GFP_KERNEL, nid);
>
> We don't need to allocate the entry on the same node anymore, should we
> drop this and let the slab allocator handle it as it sees fit?

Was this a recent change? I see the folio_nid(folio) on my version of
mm-unstable as of 10-3-2026. Let me check, and yes, allowing the slab
allocated handle this is probably a good idea.

Along the same lines, I have been working on replacing zsmalloc with
direct SLAB allocations for the zswap compressed data storage and see
very promising results: memory savings and latency improvements. I
will submit an RFC shortly.

>
> >       if (!entry) {
> >               zswap_reject_kmemcache_fail++;
> >               return false;
> > @@ -1466,6 +1464,7 @@ static bool zswap_store_page(struct folio *folio, long index,
> >        * to an unrelated live pool.
> >        */
> >       entry->pool_idx = pool->idx;
> > +     entry->nid = nid;
> >
> >       old = xa_store(swap_zswap_tree(page_swpentry),
> >                      swp_offset(page_swpentry),
> > --
> > 2.39.5
> >