Re: [PATCH 2/4] mm/memcg: make memory.reclaim interface generic

[Klara Modin <klarasmodin@gmail.com>]

Hi,

On 2025-06-23 11:58:49 -0700, Davidlohr Bueso wrote:
> This adds a general call for both parsing as well as the
> common reclaim semantics. memcg is still the only user and
> no change in semantics.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  mm/internal.h   |  2 +
>  mm/memcontrol.c | 77 ++------------------------------------
>  mm/vmscan.c     | 98 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 104 insertions(+), 73 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 3823fb356d3b..fc4262262b31 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -517,6 +517,8 @@ extern unsigned long highest_memmap_pfn;
>  bool folio_isolate_lru(struct folio *folio);
>  void folio_putback_lru(struct folio *folio);
>  extern void reclaim_throttle(pg_data_t *pgdat, enum vmscan_throttle_state reason);
> +int user_proactive_reclaim(char *buf,
> +			   struct mem_cgroup *memcg, pg_data_t *pgdat);
>  
>  /*
>   * in mm/rmap.c:
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 902da8a9c643..015e406eadfa 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -51,7 +51,6 @@
>  #include <linux/spinlock.h>
>  #include <linux/fs.h>
>  #include <linux/seq_file.h>
> -#include <linux/parser.h>
>  #include <linux/vmpressure.h>
>  #include <linux/memremap.h>
>  #include <linux/mm_inline.h>
> @@ -4566,83 +4565,15 @@ static ssize_t memory_oom_group_write(struct kernfs_open_file *of,
>  	return nbytes;
>  }
>  
> -enum {
> -	MEMORY_RECLAIM_SWAPPINESS = 0,
> -	MEMORY_RECLAIM_SWAPPINESS_MAX,
> -	MEMORY_RECLAIM_NULL,
> -};
> -
> -static const match_table_t tokens = {
> -	{ MEMORY_RECLAIM_SWAPPINESS, "swappiness=%d"},
> -	{ MEMORY_RECLAIM_SWAPPINESS_MAX, "swappiness=max"},
> -	{ MEMORY_RECLAIM_NULL, NULL },
> -};
> -
>  static ssize_t memory_reclaim(struct kernfs_open_file *of, char *buf,
>  			      size_t nbytes, loff_t off)
>  {
>  	struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
> -	unsigned int nr_retries = MAX_RECLAIM_RETRIES;
> -	unsigned long nr_to_reclaim, nr_reclaimed = 0;
> -	int swappiness = -1;
> -	unsigned int reclaim_options;
> -	char *old_buf, *start;
> -	substring_t args[MAX_OPT_ARGS];
> -
> -	buf = strstrip(buf);
> -
> -	old_buf = buf;
> -	nr_to_reclaim = memparse(buf, &buf) / PAGE_SIZE;
> -	if (buf == old_buf)
> -		return -EINVAL;
> -
> -	buf = strstrip(buf);
> -
> -	while ((start = strsep(&buf, " ")) != NULL) {
> -		if (!strlen(start))
> -			continue;
> -		switch (match_token(start, tokens, args)) {
> -		case MEMORY_RECLAIM_SWAPPINESS:
> -			if (match_int(&args[0], &swappiness))
> -				return -EINVAL;
> -			if (swappiness < MIN_SWAPPINESS || swappiness > MAX_SWAPPINESS)
> -				return -EINVAL;
> -			break;
> -		case MEMORY_RECLAIM_SWAPPINESS_MAX:
> -			swappiness = SWAPPINESS_ANON_ONLY;
> -			break;
> -		default:
> -			return -EINVAL;
> -		}
> -	}
> -
> -	reclaim_options	= MEMCG_RECLAIM_MAY_SWAP | MEMCG_RECLAIM_PROACTIVE;
> -	while (nr_reclaimed < nr_to_reclaim) {
> -		/* Will converge on zero, but reclaim enforces a minimum */
> -		unsigned long batch_size = (nr_to_reclaim - nr_reclaimed) / 4;
> -		unsigned long reclaimed;
> -
> -		if (signal_pending(current))
> -			return -EINTR;
> -
> -		/*
> -		 * This is the final attempt, drain percpu lru caches in the
> -		 * hope of introducing more evictable pages for
> -		 * try_to_free_mem_cgroup_pages().
> -		 */
> -		if (!nr_retries)
> -			lru_add_drain_all();
> -
> -		reclaimed = try_to_free_mem_cgroup_pages(memcg,
> -					batch_size, GFP_KERNEL,
> -					reclaim_options,
> -					swappiness == -1 ? NULL : &swappiness);
> -
> -		if (!reclaimed && !nr_retries--)
> -			return -EAGAIN;
> +	int ret;
>  
> -		nr_reclaimed += reclaimed;
> -	}

> +	ret = user_proactive_reclaim(buf, memcg, NULL);

This is outside CONFIG_NUMA.

> +	if (ret)
> +		return ret;
>  
>  	return nbytes;
>  }
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index c13c01eb0b42..63ddec550c3b 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -57,6 +57,7 @@
>  #include <linux/rculist_nulls.h>
>  #include <linux/random.h>
>  #include <linux/mmu_notifier.h>
> +#include <linux/parser.h>
>  
>  #include <asm/tlbflush.h>
>  #include <asm/div64.h>
> @@ -6714,6 +6715,15 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
>  
>  	return nr_reclaimed;
>  }
> +#else
> +unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *memcg,
> +					   unsigned long nr_pages,
> +					   gfp_t gfp_mask,
> +					   unsigned int reclaim_options,
> +					   int *swappiness)
> +{
> +	return 0;
> +}
>  #endif
>  
>  static void kswapd_age_node(struct pglist_data *pgdat, struct scan_control *sc)
> @@ -7708,6 +7718,94 @@ int node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned int order)
>  
>  	return ret;
>  }
> +
> +enum {
> +	MEMORY_RECLAIM_SWAPPINESS = 0,
> +	MEMORY_RECLAIM_SWAPPINESS_MAX,
> +	MEMORY_RECLAIM_NULL,
> +};
> +static const match_table_t tokens = {
> +	{ MEMORY_RECLAIM_SWAPPINESS, "swappiness=%d"},
> +	{ MEMORY_RECLAIM_SWAPPINESS_MAX, "swappiness=max"},
> +	{ MEMORY_RECLAIM_NULL, NULL },
> +};
> +
> +int user_proactive_reclaim(char *buf, struct mem_cgroup *memcg, pg_data_t *pgdat)
> +{
> +	unsigned int nr_retries = MAX_RECLAIM_RETRIES;
> +	unsigned long nr_to_reclaim, nr_reclaimed = 0;
> +	int swappiness = -1;
> +	char *old_buf, *start;
> +	substring_t args[MAX_OPT_ARGS];
> +
> +	if (!buf || (!memcg && !pgdat))
> +		return -EINVAL;
> +
> +	buf = strstrip(buf);
> +
> +	old_buf = buf;
> +	nr_to_reclaim = memparse(buf, &buf) / PAGE_SIZE;
> +	if (buf == old_buf)
> +		return -EINVAL;
> +
> +	buf = strstrip(buf);
> +
> +	while ((start = strsep(&buf, " ")) != NULL) {
> +		if (!strlen(start))
> +			continue;
> +		switch (match_token(start, tokens, args)) {
> +		case MEMORY_RECLAIM_SWAPPINESS:
> +			if (match_int(&args[0], &swappiness))
> +				return -EINVAL;
> +			if (swappiness < MIN_SWAPPINESS ||
> +			    swappiness > MAX_SWAPPINESS)
> +				return -EINVAL;
> +			break;
> +		case MEMORY_RECLAIM_SWAPPINESS_MAX:
> +			swappiness = SWAPPINESS_ANON_ONLY;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	while (nr_reclaimed < nr_to_reclaim) {
> +		/* Will converge on zero, but reclaim enforces a minimum */
> +		unsigned long batch_size = (nr_to_reclaim - nr_reclaimed) / 4;
> +		unsigned long reclaimed;
> +
> +		if (signal_pending(current))
> +			return -EINTR;
> +
> +		/*
> +		 * This is the final attempt, drain percpu lru caches in the
> +		 * hope of introducing more evictable pages.
> +		 */
> +		if (!nr_retries)
> +			lru_add_drain_all();
> +
> +		if (memcg) {
> +			unsigned int reclaim_options;
> +
> +			reclaim_options = MEMCG_RECLAIM_MAY_SWAP |
> +					  MEMCG_RECLAIM_PROACTIVE;
> +			reclaimed = try_to_free_mem_cgroup_pages(memcg,
> +						 batch_size, GFP_KERNEL,
> +						 reclaim_options,
> +						 swappiness == -1 ? NULL : &swappiness);
> +		} else {
> +			return -EINVAL;
> +		}
> +
> +		if (!reclaimed && !nr_retries--)
> +			return -EAGAIN;
> +
> +		nr_reclaimed += reclaimed;
> +	}
> +
> +	return 0;
> +}
> +
>  #endif

Should this really be inside CONFIG_NUMA? It was moved from outside of
CONFIG_NUMA where it's now called which results in a build failure if
it's disabled. Or is there a stub missing?

>  
>  /**
> -- 
> 2.39.5
> 

Regards,
Klara Modin