Re: [PATCH] mm, vmscan: make kswapd unreserve highatomic pageblocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mm, vmscan: premature highatomic unreserve in balance_pgdat()
- [Medium] mm, vmscan: incomplete highatomic unreserve in balance_pgdat()
- [Low] mm, vmscan: contradictory migratetype comment in balance_pgdat()
--

commit 9f34d95b6ce713649b203e410b246f2a907d1814
Author: Vlastimil Babka (SUSE) <vbabka@kernel.org>

mm, vmscan: make kswapd unreserve highatomic pageblocks

This commit extracts unreserve_highatomic_pageblock_zone() and calls it from
balance_pgdat() when kswapd raises reclaim priority or reclaims no pages. This
allows kswapd to unreserve MIGRATE_HIGHATOMIC pageblocks on nodes where direct
reclaim does not run.

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 9959ca158ce83..91c622068d991 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -7460,8 +7460,22 @@ static int balance_pgdat(pg_data_t *pgdat, int order, int highest_zoneidx)
>  		if (nr_boost_reclaim && !nr_reclaimed)
>  			break;
>  
> -		if (raise_priority || !nr_reclaimed)
> +		if (raise_priority || !nr_reclaimed) {

[Severity: Medium]
Will gating this on raise_priority cause kswapd to prematurely unreserve
MIGRATE_HIGHATOMIC pageblocks during ordinary, successful reclaim passes at
DEF_PRIORITY?

In balance_pgdat(), raise_priority is initialized to true at the start of
each loop iteration and is only cleared if kswapd_shrink_node() returns true
(or at DEF_PRIORITY - 2 during watermark-boosted reclaim):

mm/vmscan.c:kswapd_shrink_node() {
	sc->nr_to_reclaim = 0;
	for_each_managed_zone_pgdat(zone, pgdat, z, sc->reclaim_idx) {
		sc->nr_to_reclaim += max(high_wmark_pages(zone), SWAP_CLUSTER_MAX);
	}
	...
	return max(sc->nr_scanned, sc->nr_reclaimed - nr_reclaimed) >= sc->nr_to_reclaim;
}

Because sc->nr_to_reclaim is set to the full sum of high_wmark_pages(zone)
rather than the deficit between low and high watermarks, a normal pass at
DEF_PRIORITY that scans lruvec_size >> DEF_PRIORITY pages and reclaims
enough pages to balance the node (or aborts scanning early in
should_abort_scan() under CONFIG_LRU_GEN once WMARK_HIGH + MIN_LRU_BATCH is
met) still has kswapd_shrink_node() return false.

Since pgdat_balanced() is only checked at the top of the do-while loop (and
is already true during watermark-boosted reclaim when nr_boost_reclaim > 0),
a first pass at DEF_PRIORITY that makes progress or has already balanced the
node will still enter this branch, unreserve highatomic pageblocks into
MIGRATE_UNMOVABLE across all eligible zones, and only then notice
pgdat_balanced() is true at the top of the next iteration.

Should highatomic unreservation in balance_pgdat() only happen when
!nr_reclaimed (or after rechecking !pgdat_balanced() when reclaim fails to
make progress)?

> +			/*
> +			 * Similarly to when direct reclaim is retrying, try to
> +			 * unreserve highatomic pageblocks on the node. Convert
> +			 * them to movable ones as kswapd doesn't have a target
> +			 * migratetype, and passing unmovable should lead to
> +			 * least permanent fragmentation.
> +			 */

[Severity: Low]
This isn't a bug, but the comment says "Convert them to movable ones" while
the second half of the sentence and the call below use MIGRATE_UNMOVABLE.
Should that say "Convert them to unmovable ones"?

> +			for (int i = 0; i <= highest_zoneidx; i++) {
> +				struct zone *zone = pgdat->node_zones + i;
> +
> +				unreserve_highatomic_pageblock_zone(zone,
> +						MIGRATE_UNMOVABLE, false);
> +			}

[Severity: Medium]
Can a node filled with unreclaimable 1G hugepages still fail to balance in
balance_pgdat() and hit kswapd_failures exhaustion with this change?

There appear to be three related cases in this path:

First, unreserve_highatomic_pageblock_zone() preserves the last pageblock
when force is false:

mm/page_alloc.c:unreserve_highatomic_pageblock_zone() {
	if (!force && zone->nr_reserved_highatomic <= pageblock_nr_pages)
		return false;
}

However, __zone_watermark_unusable_free() still subtracts all of
z->nr_free_highatomic, including free pages in that last reserved pageblock.
Unlike direct reclaim, which follows up with force = true in
should_reclaim_retry(), balance_pgdat() never passes force = true, so if the
single remaining highatomic pageblock keeps usable free pages below the
watermark, pgdat_balanced() will remain false.

Second, reserve_highatomic_pageblock() can reserve up to 1% of
zone_managed_pages(zone), which includes 1G hugetlbfs pages (for example, up
to 320 pageblocks on a 64GB zone). Because
unreserve_highatomic_pageblock_zone() unreserves at most one pageblock per
call, a full balance_pgdat() walk from DEF_PRIORITY down to 1 unreserves at
most 12 pageblocks per zone (or at most 6 under CONFIG_LRU_GEN when
set_initial_priority() starts at DEF_PRIORITY / 2 because reclaimable is 0).

Third, unreserving highatomic pageblocks does not increment sc.nr_reclaimed,
and if the final unreserve at sc.priority == 1 balances the node, the loop
exits before rechecking pgdat_balanced() at the top of the loop:

mm/vmscan.c:balance_pgdat() {
	if (!sc.nr_reclaimed && !boosted) {
		int fail_cnt = atomic_inc_return(&pgdat->kswapd_failures);
		/* kswapd context, low overhead to trace every failure */
		trace_mm_vmscan_kswapd_reclaim_fail(pgdat->node_id, fail_cnt);
	}
}

Once pgdat->kswapd_failures reaches MAX_RECLAIM_RETRIES, kswapd stops even if
unreserving highatomic pageblocks was making progress or more reserved
highatomic pageblocks remain.

Could balance_pgdat() pass force = true at low priority, and avoid
incrementing kswapd_failures (or restart/recheck balance) when
unreserve_highatomic_pageblock_zone() succeeds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-kswapd-highatomic-unreserve-v1-1-38be5106adf5@kernel.org?part=1