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

[Baoquan He]

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?

> +		switch (token[0]) {
> +		case '-':
> +			WRITE_ONCE(memcg->tier_mask,
> +				   memcg->tier_mask & ~mask);
> +			break;
> +		case '+':
> +			WRITE_ONCE(memcg->tier_mask,
> +				   memcg->tier_mask | mask);
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		if (ret)
> +			goto err;
> +	}
> +
> +sync:
> +	swap_tiers_memcg_sync_mask(memcg);
> +err:
> +	if (ret)
> +		WRITE_ONCE(memcg->tier_mask, original_mask);
> +	spin_unlock(&swap_tier_lock);
> +	return ret ? ret : nbytes;
> +}
> +
...snip...