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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm: zswap: off-by-one bitfield width for zswap_entry.length on 256KB pages
- [Medium] mm: zswap: shrink_memcg() skips non-N_NORMAL_MEMORY nodes in zswap_list_lru
--

commit c4aa889f59a75fcf871276fd68b4a4738afe132b
Author: Kanchana P. Sridhar <kanchanapsridhar2026@gmail.com>

mm: zswap: Store the folio's node id in the zswap_entry.

This commit stores the folio's NUMA node ID directly in struct zswap_entry
instead of deriving it from the slab page backing the entry, and converts
zswap_entry_cache_alloc() to use node-agnostic slab allocation. To keep the
structure size unchanged, it packs length and referenced into bitfields.

> diff --git a/mm/zswap.c b/mm/zswap.c
> index e1d57a7d95950..19b7d60bc7d2a 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -180,6 +180,7 @@ struct zswap_pool {
>   */
>  #define ZSWAP_FIRST_POOL_ID 1
>  #define ZSWAP_MAX_POOL_ID U8_MAX
> +#define ZSWAP_MAX_COMPR_LENGTH_SHIFT 18U
>  static DEFINE_XARRAY_FLAGS(zswap_pools, XA_FLAGS_ALLOC1 | XA_FLAGS_LOCK_BH);
>  static struct zswap_pool __rcu *zswap_current_pool;
>  
> @@ -206,15 +207,19 @@ static struct shrinker *zswap_shrinker;
>   *              logic if referenced is unset. See comments in the shrinker
>   *              section for context.
>   * pool_idx - id of the zswap_pool that the entry's data is in.
> + * nid - NUMA node id of the page for which this is the zswap entry.
>   * handle - zsmalloc allocation handle that stores the compressed page data
>   * objcg - the obj_cgroup that the compressed memory is charged to
>   * lru - handle to the pool's lru used to evict pages.
>   */
>  struct zswap_entry {
>  	swp_entry_t swpentry;
> -	unsigned int length;
> -	bool referenced;
> -	u8 pool_idx;
> +	struct {
> +		unsigned int length:ZSWAP_MAX_COMPR_LENGTH_SHIFT;
> +		bool referenced:1U;
> +		u8 pool_idx;
> +	};

[Severity: High]
Can entry->length overflow to 0 when storing an incompressible page on a
kernel with 256KB pages (PAGE_SHIFT == 18)?

With ZSWAP_MAX_COMPR_LENGTH_SHIFT defined as 18U, the 18-bit unsigned
bitfield can only hold values up to (1U << 18) - 1, whereas PAGE_SIZE is
(1UL << 18) when PAGE_SHIFT == 18.

When an incompressible page is stored with writeback enabled in
zswap_store_page() -> zswap_compress():

mm/zswap.c:zswap_compress() {
    ...
	if (comp_ret) {
		...
		comp_ret = 0;
		dlen = PAGE_SIZE;
		dst = kmap_local_folio(folio, index * PAGE_SIZE);
		mapped = true;
	}
	...
	entry->handle = handle;
	entry->length = dlen;
}

Assigning PAGE_SIZE to entry->length wraps the 18-bit bitfield to 0. In
zswap_store_page(), entry->length == 0 then skips initializing entry->lru and
adding it to the LRU:

mm/zswap.c:zswap_store_page() {
    ...
	if (entry->length == PAGE_SIZE)
		atomic_long_inc(&zswap_stored_incompressible_pages);
	...
	if (entry->length) {
		INIT_LIST_HEAD(&entry->lru);
		zswap_lru_add(entry);
	}
}

Later, zswap_entry_free() unconditionally calls zswap_lru_del(entry) on the
uninitialized entry->lru list head, and __zswap_decompress() fails on swapin
because entry->length == PAGE_SIZE is false.

Should ZSWAP_MAX_COMPR_LENGTH_SHIFT be at least 19U (and the check in
zswap_setup() be BUILD_BUG_ON(ZSWAP_MAX_COMPR_LENGTH_SHIFT <= PAGE_SHIFT)) so
that entry->length can represent PAGE_SIZE?

[ ... ]

> @@ -1507,9 +1506,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);
>  	if (!entry) {
>  		zswap_reject_kmemcache_fail++;
>  		return false;
> @@ -1524,6 +1524,7 @@ static bool zswap_store_page(struct folio *folio, long index,
>  	 * to an unrelated live pool.
>  	 */
>  	entry->pool_idx = pool->idx;
> +	entry->nid = nid;

[Severity: Medium]
What happens during cgroup or pool reclaim in shrink_memcg() if the swapped
folio resides on a NUMA node without normal memory (for example, a
ZONE_MOVABLE-only or ZONE_HIGHMEM-only node)?

Previously, entry_to_nid() derived the node from virt_to_page(entry), which
was allocated from slab with GFP_KERNEL and therefore always resided on a
node in N_NORMAL_MEMORY.

With entry->nid now set to folio_nid(folio) in zswap_store_page(), anonymous
and shmem folios allocated with GFP_HIGHUSER_MOVABLE can have an entry->nid
that is in N_MEMORY, but not in N_NORMAL_MEMORY. zswap_lru_add() inserts
those entries into zswap_list_lru for entry->nid, but shrink_memcg() only
walks N_NORMAL_MEMORY nodes:

mm/zswap.c:shrink_memcg() {
    ...
	for_each_node_state(nid, N_NORMAL_MEMORY) {
		unsigned long nr_to_walk = SWAP_CLUSTER_MAX;

		shrunk += list_lru_walk_one(&zswap_list_lru, nid, memcg,
					    &shrink_memcg_cb, NULL, &nr_to_walk);
		scanned += SWAP_CLUSTER_MAX - nr_to_walk;
	}
}

Should shrink_memcg() iterate over N_MEMORY instead of N_NORMAL_MEMORY so
that entries from movable-only or highmem-only nodes can be reclaimed by
shrink_worker() and mem_cgroup_try_charge_zswap()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028030503.17923-1-kanchanapsridhar2026@gmail.com?part=5