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

[Davidlohr Bueso]

On Wed, 16 Jul 2025, Roman Gushchin wrote:

> Davidlohr Bueso <dave@xxxxxxxxxxxx> writes:
>
> > 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@xxxxxxxxxxxx>
> > ---
> >  mm/internal.h   |  2 +
> >  mm/memcontrol.c | 77 ++------------------------------------
> >  mm/vmscan.c     | 98 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 104 insertions(+), 73 deletions(-)
> > ...
> > 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);
>
> To be honest, not a big fan of this refactoring. Effectively parts of
> the memcg user interface are moved into mm/vmscan.c. I get that you want
> to use the exact same interface somewhere else, but still...

I disagree, further this is no different than other callers in vmscan.c
around memcg.

> Is it possible to keep it in mm/memcontrol.c?

Why? now proactive reclaim is not special to memcg... unless strong reasons
it makes little sense to keep it there.

> Also maybe split the actual reclaim mechanism and user's input parsing?

I tried something like this initially, and ended up prefering this way.

Further, this approach limits the reachability of the input parsing logic, and
the interface is already being an exception to the one-value per file "rule".

Thanks,
Davidlohr