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

[Yosry Ahmed]

On Tue, Oct 13, 2026 at 6:17 PM Kanchana P. Sridhar
<kanchanapsridhar2026@xxxxxxxxx> wrote:
>
> 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.

Please look at the layout I pasted (quoted below). Your patch
increases the struct size from 48 bytes to 56 bytes, undoing the
optimization introduced by pool_idx. We need to pack length, pool_idx,
and referenced in a single u32 to keep the size constant.

FWIW, moving pool_idx to the end just trades holes for padding, I am
not sure if that's necessarily an improvement.

>
> >
> > 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.

In this case unfortunately we need to stick with bitfields. We should
probably add a build-time assertion that the width of length is enough
to cover PAGE_SIZE.

> 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.

I don't think we can put pool_idx to the end and keep the struct size
constant. For length, yeah let's use the minimum number of bits needed
and add an assertion, so that the free to use bits are obvious.

>
> >
> > 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.

We allocate the entry on the same node to avoid having to store nid in
the zswap entry (see entry_to_nid()). If we store it, then there's no
need to allocate on the node.

>
> 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.

Keep in mind that zsmalloc is movable memory, while slab memory isn't.
Zsmalloc may also be less likely to make a new allocation in the store
path. There are other considerations beyond memory overhead and
latency here.

>
> >
> > >       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
> > >