Re: [PATCH v3 2/2] mm/vmscan: reduce lru_lock contention via vmstat-derived scan-balance cost

[Johannes Weiner <hannes@cmpxchg.org>]

On Fri, Jul 17, 2026 at 06:57:32AM -0700, Usama Arif wrote:
> @@ -2303,12 +2301,63 @@ static void prepare_scan_control(pg_data_t *pgdat, struct scan_control *sc)
>  	mem_cgroup_flush_stats_ratelimited(sc->target_mem_cgroup);
>  
>  	/*
> -	 * Determine the scan balance between anon and file LRUs.
> +	 * Determine the scan balance between anon and file LRUs from per-LRU
> +	 * vmstat counters. The raw cost per side is:
> +	 *
> +	 *	PGROTATE	   - reclaim-driven rotations, bumped from both
> +	 *			     shrink_inactive_list and shrink_active_list
> +	 *			     (CPU work).
> +	 *	NR_VMSCAN_WRITE    - reclaim-driven anon pageout IO.
> +	 *	WORKINGSET_RESTORE - refaults of previously-workingset pages.
> +	 *
> +	 * The two IO terms are weighted by SWAP_CLUSTER_MAX to reflect the
> +	 * higher cost of an IO over a rotation.
> +	 *
> +	 * Reads are lock-free per-cpu sum collations, rstat-aggregated up
> +	 * the memcg hierarchy by mem_cgroup_flush_stats_ratelimited() above.
> +	 * Use lruvec_page_state_monotonic() so the unsigned subtraction
> +	 * `now - prev_cost[f]` yields the correct delta across a signed-long
> +	 * wraparound of the underlying counter (a real hazard on 32-bit that
> +	 * the clamp in lruvec_page_state() would otherwise turn into a huge
> +	 * spurious delta).
> +	 *
> +	 * The delta against prev_cost is folded into cost_accum, which is
> +	 * halved on both sides until their sum is within lrusize/4.
> +	 * cost_lock serialises concurrent reclaimers in the same memcg+node.

IMO that's a lot of describing what the code does. Why not stick
closer to the original comments?

>  	 */
> -	spin_lock_irq(&target_lruvec->lru_lock);
> -	sc->anon_cost = target_lruvec->anon_cost;
> -	sc->file_cost = target_lruvec->file_cost;
> -	spin_unlock_irq(&target_lruvec->lru_lock);
> +	spin_lock(&target_lruvec->cost_lock);
> +	for (int f = 0; f <= 1; f++) {
> +		unsigned long now, delta;
> +
> +		now = lruvec_page_state_monotonic(target_lruvec, PGROTATE_ANON + f) +
> +		      lruvec_page_state_monotonic(target_lruvec,
> +						  WORKINGSET_RESTORE_BASE + f) *
> +				SWAP_CLUSTER_MAX;
> +		if (f == WORKINGSET_ANON)
> +			now += lruvec_page_state_monotonic(target_lruvec,
> +							   NR_VMSCAN_WRITE) *
> +				SWAP_CLUSTER_MAX;

It's hard to prove overflow behavior is correct. I would keep the
delta extraction dead simple, then do the weight math on the delta.

> +		delta = now - target_lruvec->prev_cost[f];
> +		target_lruvec->prev_cost[f] = now;
> +		target_lruvec->cost_accum[f] += delta;
> +	}
> +	unsigned long lrusize =
> +		lruvec_page_state(target_lruvec, NR_INACTIVE_ANON) +
> +		lruvec_page_state(target_lruvec, NR_ACTIVE_ANON) +
> +		lruvec_page_state(target_lruvec, NR_INACTIVE_FILE) +
> +		lruvec_page_state(target_lruvec, NR_ACTIVE_FILE);
> +	unsigned long cost_limit = lrusize / 4;
> +
> +	while (target_lruvec->cost_accum[WORKINGSET_ANON] > cost_limit ||
> +	       target_lruvec->cost_accum[WORKINGSET_FILE] > cost_limit ||
> +	       target_lruvec->cost_accum[WORKINGSET_ANON] +
> +	       target_lruvec->cost_accum[WORKINGSET_FILE] > cost_limit) {
> +		target_lruvec->cost_accum[WORKINGSET_ANON] /= 2;
> +		target_lruvec->cost_accum[WORKINGSET_FILE] /= 2;

Why do you need to check them individually? Between reclaim cycles,
there are no scans->rotations. And I don't see how you could get
refault events several times the size of the LRU, let alone in excess
of ULONG_MAX.

> +	}
> +	sc->anon_cost = target_lruvec->cost_accum[WORKINGSET_ANON];
> +	sc->file_cost = target_lruvec->cost_accum[WORKINGSET_FILE];
> +	spin_unlock(&target_lruvec->cost_lock);

I realize these long descriptor names make it hard, but I think this
can be cleaned up a bit and written in a more idiomatic way.

How about this (totally untested):

struct lru_cost {
	unsigned long count;
	unsigned long last_rotated;
	unsigned long last_io;
};

struct lruvec {
	struct lru_cost cost[2];
};

	struct lru_cost *anon_cost, *file_cost;
	unsigned long lrusize;
	
	/*
	 * Determine the scan balance between anon and file LRUs.
	 *
	 * The cost model is based on rotations, refaults and
	 * reclaim-driven writes (anon only) on each side.
	 *
	 * These event counters are monotonic, so each reclaim cycle
	 * the delta since the last scan is extracted and incorporated
	 * into a decaying average. This ensures currency, as workloads
	 * change over time, and avoids overflow in the calculations.
	 */
	spin_lock_irq(&target_lruvec->cost_lock);

	for (file = 0; file <= 1; file++) {
		struct lru_cost *cost = &target_lruvec->cost[file];
		unsigned long rotated, io, nr_rotated, nr_io;

		rotated = lruvec_page_state_monotonic(target_lruvec,
						PGROTATE_ANON + file);
		io = lruvec_page_state_monotonic(target_lruvec,
						WORKINGSET_RESTORE_BASE + file);
		if (!file)
			io += lruvec_page_state_monotonic(target_lruvec,
						NR_VMSCAN_WRITE);

		nr_rotated = rotated - cost->last_rotated;
		nr_io = io - cost->last_io;

		/*
		 * Reflect the relative cost of incurring IO and spending CPU
		 * time on rotations. This doesn't attempt to make a precise
		 * comparison, it just says: if reloads are about comparable
		 * between the LRU lists, or rotations are overwhelmingly
		 * different between them, adjust scan balance for CPU work.
		 */
		cost->count += nr_io * SWAP_CLUSTER_MAX + nr_rotated;

		cost->last_rotated = rotated;
		cost->last_io = io;
	}

	anon_cost = &target_lruvec->cost[WORKINGSET_ANON];
	file_cost = &target_lruvec->cost[WORKINGSET_FILE];

	lrusize = lruvec_page_state(target_lruvec, NR_INACTIVE_ANON) +
		  lruvec_page_state(target_lruvec, NR_ACTIVE_ANON) +
		  lruvec_page_state(target_lruvec, NR_INACTIVE_FILE) +
		  lruvec_page_state(target_lruvec, NR_ACTIVE_FILE);

	while (anon_cost->count + file_cost->count > lrusize / 4) {
		anon_cost->count /= 2;
		file_cost->count /= 2;
	}

	sc->anon_cost = anon_cost->count;
	sc->file_cost = file_cost->count;

	spin_unlock(&target_lruvec->cost_lock);