Re: [PATCH v3] mm/memcg: clear folio memcg after changing per memcg stats

[Kairui Song]

On Wed, Sep 2, 2026 at 10:07 AM Bingfang Guo via B4 Relay
<devnull+bingfangguo.tencent.com@xxxxxxxxxx> wrote:
>
> From: Bingfang Guo <bingfangguo@xxxxxxxxxxx>
>
> I notice extremely high swapcached count in the per memcg level
> memory.stat when running tests with cgroupv1 setup by swapping pages in
> and out.  It seems that the counter never gets decreased so the value is
> rather useless and confusing to users reading it. So I think fixing it
> so that the value can reflect the actual swapcache usage correctly could
> be helpful.

Thanks! Good catch, I missed the V1 case.

> __memcg1_swapout() transfers the memsw charge of a folio to its swap
> entry and clears folio->memcg_data as part of that.  In the vmscan
> swapout path it runs before __swap_cache_del_folio(), which then
> decrements the swapcache stats through lruvec_stat_mod_folio().  Since
> folio->memcg_data has already been cleared, folio_memcg() returns NULL
> and the NR_SWAPCACHE decrement only updates the node-level counter
> instead of the memcg's lruvec, leaking the per-memcg swapcache count.
>
> Move the __memcg1_swapout() call into __swap_cache_del_folio(), after
> the NR_FILE_PAGES and NR_SWAPCACHE updates but before
> __swap_cache_do_del_folio() removes the folio from the swap cache.  This
> keeps the stats attributed to the folio's memcg while still recording
> the swap cgroup with a valid folio->swap.  Add a swapout parameter so
> the plain swap_cache_del_folio() path is left unchanged.
>
> Fixes: b197d41462c20 ("mm/memcg, swap: store cgroup id in cluster table directly")

Is this the right Fixes? I think the problem could be introduced by
2732acda82c9?

> Signed-off-by: Bingfang Guo <bingfangguo@xxxxxxxxxxx>
> ---
> The problem is reproducible using the following script and program:
>
> ```
> #!/bin/bash
> set -e
>
> CG=/sys/fs/cgroup/memory/swapcache-leak-test
> SIZE=$((256 * 1024 * 1024))   # 256 MiB of anon memory
>
> [ "$(id -u)" -eq 0 ] || { echo "must run as root"; exit 1; }
> grep -q . /proc/swaps <<<"$(tail -n +2 /proc/swaps)" || { echo "no swap active; run: swapon <dev>"; exit 1; }
>
> cleanup() { rmdir "$CG" 2>/dev/null || true; }
> trap cleanup EXIT
>
> cc -O2 swapout.c -o swapout
>
> mkdir -p "$CG"
> echo "+memory" > /sys/fs/cgroup/cgroup.subtree_control 2>/dev/null || true
>
> echo "== before reclaim =="
> grep -E '^(swapcached|anon) ' "$CG/memory.stat"
>
> # Put ourselves in the cgroup, allocate & touch anon memory, then wait to be reclaimed.
> (
>         echo $BASHPID > "$CG/cgroup.procs"
>         # Allocate and dirty SIZE bytes of anonymous memory.
>         ./swapout
> ) &
> WORKER=$!
> sleep 2
>
> echo "== after reclaim (swap cache should drain to ~0) =="
> grep -E '^(swapcached|anon) ' "$CG/memory.stat"
>
> SWAPCACHED=$(awk '/^swapcached /{print $2}' "$CG/memory.stat")
> echo
> if [ "$SWAPCACHED" -gt $((1024 * 1024)) ]; then
>         echo "LEAK DETECTED: swapcached = $SWAPCACHED bytes (expected ~0)  [BUGGY kernel]"
>         RC=1
> else
>         echo "OK: swapcached = $SWAPCACHED bytes  [FIXED kernel]"
>         RC=0
> fi
>
> kill "$WORKER" 2>/dev/null || true
> wait "$WORKER" 2>/dev/null || true
> exit $RC
> ```
>
> swapout.c:
> ```
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> #include <sys/mman.h>
>
> int main(int argc, char **argv)
> {
>         size_t mib = (argc > 1) ? strtoul(argv[1], NULL, 10) : 256;
>         size_t size = mib * 1024UL * 1024UL;
>         long page = sysconf(_SC_PAGESIZE);
>         char *buf;
>         size_t i;
>
>         buf = mmap(NULL, size, PROT_READ | PROT_WRITE,
>                    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>         if (buf == MAP_FAILED) {
>                 perror("mmap");
>                 return 1;
>         }
>
>         /* Fault in and dirty every page so it becomes reclaimable anon. */
>         for (i = 0; i < size; i += page)
>                 buf[i] = 1;
>
>         printf("allocated and dirtied %zu MiB, paging out...\n", mib);
>
>         /* Force the whole range out to swap. */
>         if (madvise(buf, size, MADV_PAGEOUT)) {
>                 perror("madvise(MADV_PAGEOUT)");
>                 return 1;
>         }
>
>         /* Give reclaim a moment, then stay alive so the cgroup can be inspected. */
>         printf("paged out; sleeping so memory.stat can be read. pid=%d\n", getpid());
>         sleep(30);
>
>         munmap(buf, size);
>         return 0;
> }
> ```
>
> Test result:
>
> before:
> ```
> == before reclaim ==
> swapcached 0
> allocated and dirtied 256 MiB, paging out...
> paged out; sleeping so memory.stat can be read. pid=4778
> == after reclaim (swap cache should drain to ~0) ==
> swapcached 268435456
>
> LEAK DETECTED: swapcached = 268435456 bytes (expected ~0)  [BUGGY kernel]
> ```
>
> after the patch:
> ```
> == before reclaim ==
> swapcached 0
> allocated and dirtied 256 MiB, paging out...
> paged out; sleeping so memory.stat can be read. pid=2601
> == after reclaim (swap cache should drain to ~0) ==
> swapcached 0
>
> OK: swapcached = 0 bytes  [FIXED kernel]
> ```

Nice, the reproducer is under "---" so won't be included in the commit
message but anyone can find it on lore.

> ---
> Changes in v3:
> - Add doc for the new parameter.
> - Link to v2: https://lore.kernel.org/r/20260901-memcg-swapcache-stats-fix-v2-1-9caad330459b@xxxxxxxxxxx
>
> Changes in v2:
> - Update the commit message to describe the problem in the beginnning.
> - Change function declaration for !CONFIG_SWAP as well.
> - Link to v1: https://lore.kernel.org/r/20260831-memcg-swapcache-stats-fix-v1-1-1c0819ebdb86@xxxxxxxxxxx
> ---
>  mm/swap.h       |  6 ++++--
>  mm/swap_state.c | 11 ++++++++---
>  mm/vmscan.c     |  3 +--
>  3 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/mm/swap.h b/mm/swap.h
> index 0b5d507739bcb..b3b54c28929a1 100644
> --- a/mm/swap.h
> +++ b/mm/swap.h
> @@ -319,7 +319,8 @@ struct folio *swap_cache_alloc_folio(swp_entry_t target_entry, gfp_t gfp_mask,
>  void __swap_cache_add_folio(struct swap_cluster_info *ci,
>                             struct folio *folio, swp_entry_t entry);
>  void __swap_cache_del_folio(struct swap_cluster_info *ci,
> -                           struct folio *folio, swp_entry_t entry, void *shadow);
> +                           struct folio *folio, swp_entry_t entry, void *shadow,
> +                           bool swapout);
>  void __swap_cache_replace_folio(struct swap_cluster_info *ci,
>                                 struct folio *old, struct folio *new);
>
> @@ -452,7 +453,8 @@ static inline void swap_cache_del_folio(struct folio *folio)
>  }
>
>  static inline void __swap_cache_del_folio(struct swap_cluster_info *ci,
> -               struct folio *folio, swp_entry_t entry, void *shadow)
> +               struct folio *folio, swp_entry_t entry, void *shadow,
> +               bool swapout)
>  {
>  }
>
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 305877e1f4d7b..99985208b529b 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -306,6 +306,7 @@ static void __swap_cache_do_del_folio(struct swap_cluster_info *ci,
>   * @folio: The folio.
>   * @entry: The first swap entry that the folio corresponds to.
>   * @shadow: shadow value to be filled in the swap cache.
> + * @swapout: whether this operation swaps out the folio.
>   *
>   * Removes a folio from the swap cache and fills a shadow in place.
>   * This won't put the folio's refcount. The caller has to do that.
> @@ -314,13 +315,17 @@ static void __swap_cache_do_del_folio(struct swap_cluster_info *ci,
>   * using the index of @entry, and lock the cluster that holds the entries.
>   */

Perhaps the Context: part of kdoc could briefly mention that for
"swapout = true" case, the folio should be a reclaiming one?