Re: [PATCH] mm/migrate: don't move memcg NR_FILE_PAGES for swapcache

[Joshua Hahn]

On Tue, Nov 3, 2026 at 11:43 PM Song Hu <husong@xxxxxxxxxx> wrote:
>
> NR_FILE_PAGES lives on both the node and the memcg plane for page
> cache folios: filemap_add_folio() and filemap_unaccount_folio() charge
> it to the folio's node and memcg, while __swap_cache_add_folio() and
> __swap_cache_del_folio() maintain the node plane of NR_FILE_PAGES and
> the memcg plane of NR_SWAPCACHE.  A folio in the swap cache therefore
> carries no per-memcg NR_FILE_PAGES count: shmem folios drop their
> memcg file accounting when swapped out, and anon folios never had any.
>
> The cross-zone arm of __folio_migrate_mapping() moves NR_FILE_PAGES on
> both planes unconditionally.  For swapcache folios the memcg plane is
> empty, so migrating one misaccounts it: the destination accrues
> nr_pages that were never accounted there while the source counter
> underflows.  The wrong counts persist until the memcg is destroyed,
> but the aggregate in memory.stat stays correct, so the drift is easy
> to miss.  NUMA balancing, migratepages, compaction and memory hotplug
> can all migrate such folios.

Hi Song, thanks for the patch!

I think this description was a little bit hard to read @_@ so the core
problem is that swapcache folios are accounted differently in per-node
and per-memcg, right? And that swapcache is not accounted towards
memcg's NR_FILE_PAGES, so we need to split up the mod_lruvec_state
into the node and memcg accounting separately. This makes sense to me.

> Reproducer, on a 2-node QEMU guest with cgroup v2:
>
>   1. create a cgroup with memory.max = 50M and swap enabled;
>   2. memset 128M of anonymous memory in it, so reclaim swaps most of
>      it out;
>   3. fork to raise the swap entry refcount, then fault the memory
>      back in, so the resident folios are swapcache ones;
>   4. migrate_pages(pid, node0 -> node1).
>
> "file" row of memory.numa_stat (bytes) after step 4:
>
>                     without this patch      with this patch
>                       N0          N1          N0          N1
>                      0 (*)  51691520         0           0
>
>   (*) the source counter has gone negative; negative values read as 0.

Thanks for adding the reproducer. This helps a lot for reviewing!

> Keep moving the node plane via mod_node_page_state() and skip only
> the empty memcg plane, mirroring the swapcache exclusion the NR_SHMEM
> handling in the same block already applies.  It accompanies

So you note here that the block already has a kind of swapcache
exclusion. To me I think the code block we are working with here is a
little bit complicated with all the cases we have to look into (and
three separate folio_test_swapcache() calls...). At a future time I
think we should explicitly lay out all the cases (swapcache /
non-swapcache shmem / normal file) to make it a bit easier to follow
where everything gets accounted for.

I say this because it took me a second to realize that the #ifdef
CONFIG_SWAP should only apply to the NR_SWAPCACHE modification and not
other swapcache conditionals since it protects the enum and nothing
else.

Anyways, none of that should block this clearly user-visible bug.
Thanks! I hope you have a great day,
Reviewed-by: Joshua Hahn <joshua.hahnjy@xxxxxxxxx>

> commit 4eef27af2932 ("mm/migrate: move NR_FILE_THPS on cross-node migration")
> and commit c5eef2e03618 ("mm/migrate: move NR_KERNEL_FILE_PAGES on cross-node migration").
>
> Fixes: 0d1c20722ab3 ("mm: memcontrol: switch to native NR_FILE_PAGES and NR_SHMEM counters")
> Signed-off-by: Song Hu <husong@xxxxxxxxxx>
> ---
>  mm/migrate.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 28a8d7734735..bc60f6587d1b 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -690,8 +690,20 @@ static int __folio_migrate_mapping(struct address_space *mapping,
>                 old_lruvec = mem_cgroup_lruvec(memcg, oldzone->zone_pgdat);
>                 new_lruvec = mem_cgroup_lruvec(memcg, newzone->zone_pgdat);
>
> -               mod_lruvec_state(old_lruvec, NR_FILE_PAGES, -nr);
> -               mod_lruvec_state(new_lruvec, NR_FILE_PAGES, nr);
> +               /*
> +                * Swapcache folios carry NR_FILE_PAGES on the node plane
> +                * only (their memcg footprint is NR_SWAPCACHE, see
> +                * swap_state.c), so move only the node plane for them.
> +                */
> +               if (folio_test_swapcache(folio)) {
> +                       mod_node_page_state(oldzone->zone_pgdat, NR_FILE_PAGES,
> +                                           -nr);
> +                       mod_node_page_state(newzone->zone_pgdat, NR_FILE_PAGES,
> +                                           nr);
> +               } else {
> +                       mod_lruvec_state(old_lruvec, NR_FILE_PAGES, -nr);
> +                       mod_lruvec_state(new_lruvec, NR_FILE_PAGES, nr);
> +               }
>                 if (folio_test_swapbacked(folio) && !folio_test_swapcache(folio)) {
>                         mod_lruvec_state(old_lruvec, NR_SHMEM, -nr);
>                         mod_lruvec_state(new_lruvec, NR_SHMEM, nr);
> --
> 2.53.0
>