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

[YoungJun Park]

On Wed, May 27, 2026 at 07:56:37AM +0800, Baoquan He wrote:
> On 04/21/26 at 02:53pm, Youngjun Park wrote:
> ...snip...
> > diff --git a/mm/swap_tier.c b/mm/swap_tier.c
> > index 9019b8978770..b53d6cc67d1e 100644
> > --- a/mm/swap_tier.c
> > +++ b/mm/swap_tier.c
> ...snip...
> > @@ -374,6 +404,80 @@ bool swap_tiers_update(void)
> >  			break;
> >  		swap_tiers_assign_dev(swp);
> >  	}
> > +	/*
> > +	 * XXX: Unused tiers default to ON, disabled after next tier added.
> > +	 * Use removed tier mask to clear settings for removed/re-added tiers.
> > +	 * (Could hold tier refs, but better to keep cgroup config independent)
> > +	 */
> > +	if (mask)
> > +		swap_tier_memcg_propagate(mask);
> 
> Code is neat, and high efficiency, while it's not easy to understand. I
> just sat in front of my computer the whole day yesterday to recall and
> understand why it's done like this, even though I made it clear to my
> self in the past. I think more words would be helpful to decrease the
> difficulty for people to undersntand it. E.g below two paragraphes. Just
> a suggestion and for your reference.

Thanks for the review again :)

Okay I see.
This explanation seems heavily abbreviated.

> diff --git a/mm/swap_tier.c b/mm/swap_tier.c
> index b53d6cc67d1e..0a6adf14ab91 100644
> --- a/mm/swap_tier.c
> +++ b/mm/swap_tier.c
> @@ -405,9 +405,17 @@ bool swap_tiers_update(int mask)
>  		swap_tiers_assign_dev(swp);
>  	}
>  	/*
> -	 * XXX: Unused tiers default to ON, disabled after next tier added.
> -	 * Use removed tier mask to clear settings for removed/re-added tiers.
> -	 * (Could hold tier refs, but better to keep cgroup config independent)
> +	 * When a tier is removed, its index (bit position in the mask) becomes
> +	 * free for reassignment to a future tier.  If a memcg had previously
> +	 * disabled this tier (cleared the bit in its swap.tiers file), the
> +	 * effective mask would keep that bit clear -- meaning the new tier at
> +	 * the same index would be silently unavailable, an invisible cgroup
> +	 * constraint left behind by a tier that no longer exists.
> +	 *
> +	 * To prevent this, OR the removed tier's mask bit into every memcg's
> +	 * tier_mask and tier_effective_mask.  This resets the bit so the new
> +	 * tier is accessible by default; users who want to restrict it must
> +	 * explicitly disable it after the tier is re-created.
>  	 */

The explanation seems good.

it explains why we must clear the "disabled bit",
when a new tier comes and we don't automatically diable,
it can implicitly disable "new tier" which user might not want.

This comes from that fact we don't have a cgroup refecounting on the tier land.
it is the corner case, we must sync for correct behavior.

I will change the comments as like it is.