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

On Fri, 17 Jul 2026 23:08:08 +0200 Johannes Weiner <[email protected]> wrote:

> 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.
> 

Thanks for the prototype and the above reivew comments!

I integrated all of them and sent them as v4.

And best part of it is, sashiko is happy as well now with v4 lol