Re: [PATCH V3 2/3] mm: add max swappiness arg to lru_gen for anonymous memory only

[Andrew Morton <akpm@linux-foundation.org>]

On Wed,  9 Apr 2025 15:06:19 +0800 Zhongkun He <hezhongkun.hzk@bytedance.com> wrote:

> The MGLRU

paging yuzhao?

> already supports reclaiming only from anonymous memory
> via the /sys/kernel/debug/lru_gen interface. Now, memory.reclaim
> also supports the swappiness=max parameter to enable reclaiming
> solely from anonymous memory. To unify the semantics of proactive
> reclaiming from anonymous folios, the max parameter is introduced.
> 
> Additionally, the use of SWAPPINESS_ANON_ONLY in place of
> 'MAX_SWAPPINESS + 1' improves code clarity and makes the intention
> more explicit.
> 
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2697,8 +2697,11 @@ static bool should_clear_pmd_young(void)
>  		READ_ONCE((lruvec)->lrugen.min_seq[LRU_GEN_FILE]),	\
>  	}
>  
> +#define max_evictable_type(swappiness)						\
> +	((swappiness) != SWAPPINESS_ANON_ONLY)
> +
>  #define evictable_min_seq(min_seq, swappiness)				\
> -	min((min_seq)[!(swappiness)], (min_seq)[(swappiness) <= MAX_SWAPPINESS])
> +	min((min_seq)[!(swappiness)], (min_seq)[max_evictable_type(swappiness)])

Why oh why did we implement these in cpp?

>  
> @@ -3857,7 +3860,7 @@ static bool inc_min_seq(struct lruvec *lruvec, int type, int swappiness)
>  	int hist = lru_hist_from_seq(lrugen->min_seq[type]);
>  	int new_gen, old_gen = lru_gen_from_seq(lrugen->min_seq[type]);
>  
> -	if (type ? swappiness > MAX_SWAPPINESS : !swappiness)
> +	if (type ? (swappiness == SWAPPINESS_ANON_ONLY) : !swappiness)

This expression makes my brain bleed.

	if (type) {
		if (swappiness == SWAPPINESS_ANON_ONLY) {
			/*
			 * Nice comment explaining why we're doing this
			 */
			goto done;;
		}
	} else {
		if (!swappiness) {
			/*
			 * Nice comment explaining why we're doing this
			 */
			goto done;
		}
	}

or

	if (type && (swappiness == SWAPPINESS_ANON_ONLY)) {
		/*
		 * Nice comment explaining why we're doing this
		 */
		goto done;
	}

	if (!type && !swappiness) {
		/*
		 * Nice comment explaining why we're doing this
		 */
		goto done;
	}

It's much more verbose, but it has the huge advantage that it creates
locations where we can add comments which tell readers what's going on.
Which is pretty important, no?
	
>  		goto done;
>  
>  	/* prevent cold/hot inversion if the type is evictable */
> @@ -5523,7 +5526,7 @@ static int run_cmd(char cmd, int memcg_id, int nid, unsigned long seq,
>  
>  	if (swappiness < MIN_SWAPPINESS)
>  		swappiness = get_swappiness(lruvec, sc);
> -	else if (swappiness > MAX_SWAPPINESS + 1)
> +	else if (swappiness > SWAPPINESS_ANON_ONLY)
>  		goto done;
>  
>  	switch (cmd) {
> @@ -5580,7 +5583,7 @@ static ssize_t lru_gen_seq_write(struct file *file, const char __user *src,
>  	while ((cur = strsep(&next, ",;\n"))) {
>  		int n;
>  		int end;
> -		char cmd;
> +		char cmd, swap_string[5];
>  		unsigned int memcg_id;
>  		unsigned int nid;
>  		unsigned long seq;
> @@ -5591,13 +5594,22 @@ static ssize_t lru_gen_seq_write(struct file *file, const char __user *src,
>  		if (!*cur)
>  			continue;
>  
> -		n = sscanf(cur, "%c %u %u %lu %n %u %n %lu %n", &cmd, &memcg_id, &nid,
> -			   &seq, &end, &swappiness, &end, &opt, &end);
> +		n = sscanf(cur, "%c %u %u %lu %n %4s %n %lu %n", &cmd, &memcg_id, &nid,
> +			   &seq, &end, swap_string, &end, &opt, &end);

Permits userspace to easily overrun swap_string[].  OK, it's root-only,
but still, why permit this?

>  		if (n < 4 || cur[end]) {
>  			err = -EINVAL;
>  			break;
>  		}
>  
> +		/* set by userspace for anonymous memory only */
> +		if (!strncmp("max", swap_string, sizeof("max"))) {

Can sscanf() give us a non null-terminated string?

> +			swappiness = SWAPPINESS_ANON_ONLY;
> +		} else {
> +			err = kstrtouint(swap_string, 0, &swappiness);
> +			if (err)
> +				break;
> +		}
> +
>  		err = run_cmd(cmd, memcg_id, nid, seq, &sc, swappiness, opt);
>  		if (err)
>  			break;