Re: [PATCH RFC 06/15] mm/mglru: use explicit tier range in read_ctrl_pos()

[Baoquan He]

On 08/04/26 at 03:47am, Kairui Song via B4 Relay wrote:
> From: Kairui Song <kasong@xxxxxxxxxxx>
> 
> read_ctrl_pos() encodes the tier range in a single "tier" parameter
> via "tier % MAX_NR_TIERS" as the start and "min(tier, MAX_NR_TIERS-1)"
> as the end.  This is hard to follow or maintain or extend. Tier
> values 0..3 select a single tier, while tier == MAX_NR_TIERS selects
> the full range.
> 
> Replace it with explicit (tier_min, tier_max) parameters using a
> half-open [tier_min, tier_max) interval, which is the conventional C
> idiom.  The call sites become self-documenting:
> 
>   - get_tier_idx:   (0, 1) for tier 0, (tier, tier+1) for each tier
>   - get_type_to_scan: (0, MAX_NR_TIERS) for the full range

I agree with the complexity, while I am starting to like the original
style once I understand it. Maybe add sentences to explain it?

Anyway, leave this to other reviewers.

> 
> No functional change.
> 
> Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> ---
>  mm/vmscan.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index c2ea92c2b69e..a359d5a1ff41 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -3192,8 +3192,8 @@ struct ctrl_pos {
>  	int gain;
>  };
>  
> -static void read_ctrl_pos(struct lruvec *lruvec, int type, int tier, int gain,
> -			  struct ctrl_pos *pos)
> +static void read_ctrl_pos(struct lruvec *lruvec, int type, int tier_min,
> +			  int tier_max, int gain, struct ctrl_pos *pos)
>  {
>  	int i;
>  	struct lru_gen_folio *lrugen = &lruvec->lrugen;
> @@ -3202,7 +3202,7 @@ static void read_ctrl_pos(struct lruvec *lruvec, int type, int tier, int gain,
>  	pos->gain = gain;
>  	pos->refaulted = pos->total = 0;
>  
> -	for (i = tier % MAX_NR_TIERS; i <= min(tier, MAX_NR_TIERS - 1); i++) {
> +	for (i = tier_min; i < tier_max; i++) {
>  		pos->refaulted += lrugen->avg_refaulted[type][i] +
>  				  atomic_long_read(&lrugen->refaulted[hist][type][i]);
>  		pos->total += lrugen->avg_total[type][i] +
> @@ -4805,9 +4805,9 @@ static int get_tier_idx(struct lruvec *lruvec, int type)
>  	 * This value is chosen because any other tier would have at least twice
>  	 * as many refaults as the first tier.
>  	 */
> -	read_ctrl_pos(lruvec, type, 0, 2, &sp);
> +	read_ctrl_pos(lruvec, type, 0, 1, 2, &sp);
>  	for (tier = 1; tier < MAX_NR_TIERS; tier++) {
> -		read_ctrl_pos(lruvec, type, tier, 3, &pv);
> +		read_ctrl_pos(lruvec, type, tier, tier + 1, 3, &pv);
>  		if (!positive_ctrl_err(&sp, &pv))
>  			break;
>  	}
> @@ -4828,8 +4828,8 @@ static int get_type_to_scan(struct lruvec *lruvec, int swappiness)
>  	 * Compare the sum of all tiers of anon with that of file to determine
>  	 * which type to scan.
>  	 */
> -	read_ctrl_pos(lruvec, LRU_GEN_ANON, MAX_NR_TIERS, swappiness, &sp);
> -	read_ctrl_pos(lruvec, LRU_GEN_FILE, MAX_NR_TIERS, MAX_SWAPPINESS - swappiness, &pv);
> +	read_ctrl_pos(lruvec, LRU_GEN_ANON, 0, MAX_NR_TIERS, swappiness, &sp);
> +	read_ctrl_pos(lruvec, LRU_GEN_FILE, 0, MAX_NR_TIERS, MAX_SWAPPINESS - swappiness, &pv);
>  
>  	return positive_ctrl_err(&sp, &pv);
>  }
> 
> -- 
> 2.55.0
> 
>