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

[Bingfang Guo]

On Fri, Sep 04, 2026 at 08:42:05PM +0800, Kairui Song wrote:
> 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.
> 

Hi, Kairui. Thanks for your reviewing!

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

You are right... I'll update it in the next version.

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

Of course, that will be better! I think I can do this:

---
diff --git a/mm/swap_state.c b/mm/swap_state.c
index 049635247964..9c37a0f5f049 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -313,6 +313,7 @@ static void __swap_cache_do_del_folio(struct swap_cluster_info *ci,
  *
  * Context: Caller must ensure the folio is locked and in the swap cache
  * using the index of @entry, and lock the cluster that holds the entries.
+ * @swapout should be set if the folio is being reclaimed.
  */
 void __swap_cache_del_folio(struct swap_cluster_info *ci, struct folio *folio,
                            swp_entry_t entry, void *shadow, bool swapout)