Re: [PATCH v1 5/8] mm: zswap: Store the folio's node id in the zswap_entry.
From: Kanchana P. Sridhar
Date: Thu Oct 15 2026 - 17:30:48 EST
On Wed, Oct 14, 2026 at 8:17 AM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
>
> 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.
Thanks for the suggestion. I have modified the layout such that the
size remains unchanged at 48 bytes. The "u8 pool_idx" requires byte
alignment, so I moved the 1-bit "referenced" field to be before this:
#define ZSWAP_MAX_COMPR_LENGTH_SHIFT 18U
struct zswap_entry {
swp_entry_t swpentry;
struct {
unsigned int length:ZSWAP_MAX_COMPR_LENGTH_SHIFT;
bool referenced:1U;
u8 pool_idx;
};
int nid;
unsigned long handle;
struct obj_cgroup *objcg;
struct list_head lru;
};
struct zswap_entry {
swp_entry_t swpentry; /* 0 8 */
struct {
unsigned int length:18; /* 8: 0 4 */
/* Bitfield combined with next fields */
bool referenced:1; /* 10: 2 1 */
/* XXX 5 bits hole, try to pack */
u8 pool_idx; /* 11 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 */
/* size: 48, cachelines: 1, members: 6 */
/* last cacheline: 48 bytes */
};
The macro is required to add a build time assertion in zswap_setup():
BUILD_BUG_ON(ZSWAP_MAX_COMPR_LENGTH_SHIFT < PAGE_SHIFT);
>
> 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.
This is all taken care of in my latest version.
>
> >
> > >
> > > 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.
With patch 0006, we try to bulk allocate all entries for the batch.
Only if this fails, we call zswap_entry_cache_alloc(gfp, nid). So
patch 0006 addresses this already, and the nid is used on in the
fallback case.
>
> >
> > 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.
I realize this means we trade off page migration for writeback for the
pages representing zswap compressed data. This might not necessarily
be a bad thing for 2 main reasons, the way I see it in this specific
context:
1) Page migration would only be transferring a memory pressure problem
elsewhere. Not sure how well this would work with memcgs or
containers. At some point someone would need to do writeback.
2) I don't believe there is a zswap specific page migration flow in
the kernel other than zsmalloc compaction. IOW, memory tiering
components like CXL are handled directly in the reclaim path. From
what I understand, things wouldn't break with slab memory for zswap
compressed storage, but I could be wrong. I hope to include the
architecture level considerations/trade-offs in the RFC.
>
> >
> > >
> > > > 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
> > > >