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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 11/4/26 16:54, Joshua Hahn wrote:
> On Tue, Nov 3, 2026 at 11:43 PM Song Hu <husong@kylinos.cn> 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@gmail.com>

This sounds like something we want to fix in stable?

-- 
Cheers,

David