Re: [PATCH v6 08/12] mm, swap: only charge physical swap entries

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mm, swap: folio_realloc_swap() allocates physical swap before charging memcg
- [Medium] mm: memcontrol-v1: __memcg1_swapout() omits physical vswap from MEMCG_SWAP
--

commit c922e31fae8f71fd3ae6707b90202f40d4d0babb
Author: Nhat Pham <nphamcs@gmail.com>

mm, swap: only charge physical swap entries

This commit changes swap memcg accounting so that virtual swap entries are
charged against a memcg's swap counter only when physical swap backing is
allocated rather than when the virtual swap entry is first allocated. It also
splits the swap memcg helpers into separate get, record, charge, uncharge,
and put operations.

> diff --git a/Documentation/admin-guide/cgroup-v1/memory.rst b/Documentation/admin-guide/cgroup-v1/memory.rst
> index 7d2a44af52c93..2a67356d25a0d 100644
> --- a/Documentation/admin-guide/cgroup-v1/memory.rst
> +++ b/Documentation/admin-guide/cgroup-v1/memory.rst
> @@ -553,7 +553,7 @@ memory.stat file includes following statistics:
>      pgpgout         # of uncharging events to the memory cgroup. The uncharging
>                      event happens each time a page is unaccounted from the
>                      cgroup.
> -    swap            # of bytes of swap usage
> +    swap            # of bytes of physical swap usage
>      swapcached      # of bytes of swap cached in memory
>      dirty           # of bytes that are waiting to get written back to the disk.
>      writeback       # of bytes of file/anon cache that are queued for syncing to

[ ... ]

> diff --git a/mm/memcontrol-v1.c b/mm/memcontrol-v1.c
> index b7f2868885071..4e4946728db5a 100644
> --- a/mm/memcontrol-v1.c
> +++ b/mm/memcontrol-v1.c
> @@ -15,6 +15,7 @@
>  #include "internal.h"
>  #include "swap.h"
>  #include "swap_table.h"
> +#include "vswap.h"
>  #include "memcontrol-v1.h"
>  
>  /* for OOM */
> @@ -299,7 +300,8 @@ void __memcg1_swapout(struct folio *folio, struct swap_cluster_info *ci)
>  	 */
>  	nr_entries = folio_nr_pages(folio);
>  	private_id = mem_cgroup_private_id_get(memcg, nr_entries);
> -	mod_memcg_state(memcg, MEMCG_SWAP, nr_entries);
> +	if (!is_vswap_entry(folio->swap))
> +		mod_memcg_state(memcg, MEMCG_SWAP, nr_entries);
>  
>  	__swap_cgroup_set(ci, swp_cluster_offset(folio->swap), nr_entries, private_id);

[Severity: Medium]
Documentation/admin-guide/cgroup-v1/memory.rst is updated above to describe
the swap counter in memory.stat as bytes of physical swap usage.

On cgroup v1 (where do_memsw_account() is true) with vswap and zswap enabled,
mem_cgroup_may_zswap() returns true when !cgroup_subsys_on_dfl(memory_cgrp_subsys),
so folio_may_vswap() returns true and folio_alloc_swap() allocates virtual
swap entries via vswap_alloc() where is_vswap_entry(folio->swap) is true.

When a vswap folio acquires physical swapfile backing via
folio_realloc_swap() -- either in swap_writeout() when zswap_store() rejects
the folio or in zswap_writeback_entry() when zswap writes an entry back to
disk -- folio->swap remains a virtual swap entry while the physical slot is
recorded in the cluster's virtual_table:

  folio_realloc_swap()
    __vtable_set(ci_dyn, voff + i, vtable_mk_phys(pe));

In folio_realloc_swap(), __mem_cgroup_swap_charge() returns 0 early because
do_memsw_account() is true (and memcg_id in ci->memcg_table is 0 while the
folio is still in the swap cache on cgroup v1).

When writeback completes and the folio is removed from the swap cache via
__swap_cache_del_folio() -> __memcg1_swapout(), __memcg1_swapout() checks
!is_vswap_entry(folio->swap) (and the subsequent commit "mm, swap: add vmstat
and memcg counters for vswap" branches on is_vswap_entry(folio->swap) between
the vswap and swap counters) and skips incrementing the swap state counter
even when the vswap entry is backed by physical swap. Likewise,
__mem_cgroup_swap_uncharge() and __mem_cgroup_swap_put() never decrement the
swap state counter for vswap entries on cgroup v1.

Does this cause swap and total_swap in cgroup v1 memory.stat to remain at 0
for all vswap-allocated pages backed by physical swap slots?

[ ... ]

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 640e38a55497e..341077fee23be 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c

[ ... ]

> @@ -2297,18 +2353,33 @@ swp_entry_t folio_realloc_swap(struct folio *folio)
>  	VM_BUG_ON_FOLIO(!folio_test_swapcache(folio), folio);
>  	VM_WARN_ON(!is_vswap_entry(vswap_entry));
>  
> -	phys_entry = vswap_to_phys(vswap_entry);
> -	if (phys_entry.val)
> -		return phys_entry;
> +	voff = swp_cluster_offset(vswap_entry);
> +	ci = __swap_entry_to_cluster(vswap_entry);
> +	ci_dyn = container_of(ci, struct swap_cluster_info_dynamic, ci);
> +
> +	spin_lock(&ci->lock);
> +	vt = __vtable_get(ci_dyn, voff);
> +	if (vtable_type(vt) == VSWAP_SWAPFILE) {
> +		spin_unlock(&ci->lock);
> +		return vtable_to_phys(vt);
> +	}
> +	memcg_id = __swap_cgroup_get(ci, voff);
> +	spin_unlock(&ci->lock);
>  
>  	phys_entry = folio_alloc_phys_swap(folio);
> -	if (!phys_entry.val)
> +	if (!phys_entry.val) {
> +		mem_cgroup_swap_fail(memcg_id);
>  		return (swp_entry_t){};
> +	}
>  
> -	voff = swp_cluster_offset(vswap_entry);
> +	if (mem_cgroup_swap_charge(memcg_id, nr)) {
> +		__swap_cluster_free_phys_backing(__swap_entry_to_info(phys_entry),
> +						 __swap_entry_to_cluster(phys_entry),
> +						 swp_cluster_offset(phys_entry),
> +						 nr);
> +		return (swp_entry_t){};
> +	}

[Severity: Medium]
With vswap and zswap enabled, a cgroup with memory.swap.max set to 0 (or at
its physical swap limit) can reclaim anonymous pages via:

  mem_cgroup_can_swap()
  shrink_folio_list()
    folio_alloc_swap()

which allocates a vswap entry without charging memcg->swap. When zswap_store()
subsequently fails in swap_writeout() (for example, when the global zswap pool
is full or memory.zswap.max is reached), swap_writeout() calls
folio_realloc_swap(folio). Similarly, zswap_writeback_entry() calls
folio_realloc_swap(folio) if a cgroup reaches memory.swap.max during a
writeback walk.

In folio_realloc_swap(), memcg_id and nr are already known before calling
folio_alloc_phys_swap(folio), yet physical swap is allocated before
mem_cgroup_swap_charge(memcg_id, nr) is checked. When
mem_cgroup_swap_charge() fails, __swap_cluster_free_phys_backing() frees the
newly allocated physical slot(s):

  __swap_cluster_free_phys_backing()
    pci->count -= nr_pages;
    ...
    __swap_cluster_finish_free()
      if (!ci->count)
        free_cluster(si, ci);

Whenever folio_alloc_phys_swap() isolated a fresh empty cluster from
si->free_clusters (which cluster_alloc_swap_entry() prefers on discard-enabled
swap devices), pci->count drops from nr back to 0 and free_cluster() is
called. On discard-enabled swap devices, free_cluster() marks the entire
cluster for discard and schedules discard_work; on non-discard devices,
__free_cluster() frees pci->table via call_rcu(). In both cases,
percpu_swap_cluster becomes unusable (!cluster_is_usable()) for the next page,
forcing every subsequent rejected page into swap_alloc_slow() to populate and
immediately tear down or discard another free cluster.

Could folio_realloc_swap() charge the memcg via
mem_cgroup_swap_charge(memcg_id, nr) before calling folio_alloc_phys_swap(),
and uncharge if physical swap allocation fails, so physical swap clusters are
not allocated and immediately destroyed when the cgroup is at memory.swap.max?

[ ... ]

([c922e31fae8f ("mm, swap: only charge physical swap entries")](https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c922e31fae8f71fd3ae6707b90202f40d4d0babb))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021060323.553433-1-nphamcs@gmail.com?part=8