Gregory Price gourry@gourry.net writes:
On Sun, Oct 27, 2024 at 10:24:10PM -0700, Shakeel Butt wrote:
On Fri, Oct 25, 2024 at 10:17:24AM GMT, Gregory Price wrote:
When numa balancing is enabled with demotion, vmscan will call migrate_pages when shrinking LRUs. Successful demotions will cause node vmstat numbers to double-decrement, leading to an imbalanced page count. The result is dmesg output like such:
$ cat /proc/sys/vm/stat_refresh
[77383.088417] vmstat_refresh: nr_isolated_anon -103212 [77383.088417] vmstat_refresh: nr_isolated_file -899642
This negative value may impact compaction and reclaim throttling.
The double-decrement occurs in the migrate_pages path:
caller to shrink_folio_list decrements the count shrink_folio_list demote_folio_list migrate_pages migrate_pages_batch migrate_folio_move migrate_folio_done mod_node_page_state(-ve) <- second decrement
This path happens for SUCCESSFUL migrations, not failures. Typically callers to migrate_pages are required to handle putback/accounting for failures, but this is already handled in the shrink code.
When accounting for migrations, instead do not decrement the count when the migration reason is MR_DEMOTION. As of v6.11, this demotion logic is the only source of MR_DEMOTION.
Signed-off-by: Gregory Price gourry@gourry.net Fixes: 26aa2d199d6f2 ("mm/migrate: demote pages during reclaim") Cc: stable@vger.kernel.org
Reviewed-by: Shakeel Butt shakeel.butt@linux.dev
This patch looks good for stable backports. For future I wonder if instead of migrate_pages(), the caller providing the isolated folios, manages the isolated stats (increments and decrements) similar to how reclaim does it.
Note that even if you provided the folios, you'd likely still end up in migrate_pages_batch/migrate_folio_move and subsequently the same accounting path. Probably there's some refactoring we can do to make the accounting more obvious - it is very subtle here.
I agree with Shakeel here. It's better for the caller who isolates the folios to increase and decrease the isolation counter. And yes, some refactoring is required.
-- Best Regards, Huang, Ying
mm/migrate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/migrate.c b/mm/migrate.c index 923ea80ba744..e3aac274cf16 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1099,7 +1099,7 @@ static void migrate_folio_done(struct folio *src, * not accounted to NR_ISOLATED_*. They can be recognized * as __folio_test_movable */
- if (likely(!__folio_test_movable(src)))
- if (likely(!__folio_test_movable(src)) && reason != MR_DEMOTION) mod_node_page_state(folio_pgdat(src), NR_ISOLATED_ANON + folio_is_file_lru(src), -folio_nr_pages(src));
2.43.0