Re: [PATCH v6 3/4] mm: memcontrol: add interfaces for swap tier selection

[YoungJun Park]

On Tue, May 26, 2026 at 11:33:37PM +0800, Baoquan He wrote:
> On 04/21/26 at 02:53pm, Youngjun Park wrote:
> ...snip...
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index c3d98ab41f1f..0f67572e5e3e 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -68,6 +68,7 @@
> >  #include <net/ip.h>
> >  #include "slab.h"
> >  #include "memcontrol-v1.h"
> > +#include "swap_tier.h"
> >  
> >  #include <linux/uaccess.h>
> >  
> > @@ -4130,6 +4131,8 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
> >  	refcount_set(&memcg->id.ref, 1);
> >  	css_get(css);
> >  
> > +	swap_tiers_memcg_inherit_mask(memcg);
> > +
> >  	/*
> >  	 * Ensure mem_cgroup_from_private_id() works once we're fully online.
> >  	 *
> > @@ -5667,6 +5670,88 @@ static int swap_events_show(struct seq_file *m, void *v)
> >  	return 0;
> >  }
> >  
> > +static int swap_tier_show(struct seq_file *m, void *v)
> > +{
> > +	struct mem_cgroup *memcg = mem_cgroup_from_seq(m);
> > +
> > +	swap_tiers_mask_show(m, READ_ONCE(memcg->tier_mask));
> > +	return 0;
> > +}
> > +
> > +static ssize_t swap_tier_write(struct kernfs_open_file *of,
> > +				char *buf, size_t nbytes, loff_t off)
> > +{
> > +	struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
> > +	char *pos, *token;
> > +	int ret = 0;
> > +	int original_mask = 0;
> > +
> > +	pos = strstrip(buf);
> > +
> > +	spin_lock(&swap_tier_lock);
> > +	if (!*pos) {
> > +		WRITE_ONCE(memcg->tier_mask, TIER_ALL_MASK);
> > +		goto sync;
> > +	}
> > +
> > +	original_mask = memcg->tier_mask;
> > +
> > +	while ((token = strsep(&pos, " \t\n")) != NULL) {
> > +		int mask;
> > +
> > +		if (!*token)
> > +			continue;
> > +
> > +		if (token[0] != '-' && token[0] != '+') {
> > +			ret = -EINVAL;
> > +			goto err;
> > +		}
> > +
> > +		mask = swap_tiers_mask_lookup(token+1);
> > +		if (!mask) {
> > +			ret = -EINVAL;
> > +			goto err;
> > +		}
> > +
> > +		/*
> > +		 * if child already set, cannot add that tiers for hierarch mismatching.
> > +		 * parent compatible, child must respect parent selected swap device.
> > +		 */
> 
> This paragraph of code comment sounds a little unnatural. We are writing
> it into memcg, the child memcg is handled in
> swap_tiers_memcg_sync_mask(), isn't it? I don't get the 2nd sentence.
> Could you help explain?

Thanks for the review.

I think this sentence is not appropriate.
Maybe I forgot to remove this line after modifying the code many times.

This sentence is not correct.

I explain one by one for clarification.

"if child already set, cannot add that tiers for hierarch mismatching."

mask can be changed (can change the tier_mask). 
But the effective one (effective_mask) is recomputed with parent's effective_mask.

"parent compatible, child must respect parent selected swap device"

effective_mask must be restricted, which means child effective_mask
must be subset of parent's effective_mask.