Re: [PATCH 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string()

[Lorenzo Stoakes]

+cc David also!

Unfortunately we put some hugetlb logic in mm/shmem.c too which makes
get_maintainers.pl not do the right thing.

I wonder if we should add mm/shmem.c to the THP section _also_ just to cover
this off?

On Wed, May 13, 2026 at 11:04:12AM +0800, Baolin Wang wrote:
> CC Breno and Lorenzo

Ran - since you're going to need to do a respin anyway, please cc- Breno, David,
myself and probably worth adding the THP people in general:

R:	Zi Yan <ziy@xxxxxxxxxx>
R:	Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> (Baolin already cc'd of course :)
R:	Liam R. Howlett <liam@xxxxxxxxxxxxx>
R:	Nico Pache <npache@xxxxxxxxxx>
R:	Ryan Roberts <ryan.roberts@xxxxxxx>
R:	Dev Jain <dev.jain@xxxxxxx>
R:	Barry Song <baohua@xxxxxxxxxx>
R:	Lance Yang <lance.yang@xxxxxxxxx>

Thanks!

>
> On 5/12/26 8:05 PM, ranxiaokai627@xxxxxxx wrote:
> > From: Ran Xiaokai <ran.xiaokai@xxxxxxxxxx>
> >
> > Inspired by commit 82d9ff648c6c ("mm: huge_memory: refactor
> > anon_enabled_store() with set_anon_enabled_mode()"), refactor
> > thpsize_shmem_enabled_store() using sysfs_match_string().
> > This eliminates the duplicated spin_lock/unlock(), set/clear_bit(),
> > calls across all branches, reducing code duplication.
> >
> > Tested with selftests ./run_kselftest.sh -t mm:ksft_thp.sh,
> > all test cases passed.
> >
> > Signed-off-by: Ran Xiaokai <ran.xiaokai@xxxxxxxxxx>
> > ---
>
> Thanks for doing this.
>
> >   mm/shmem.c | 88 ++++++++++++++++++++++++++----------------------------
> >   1 file changed, 43 insertions(+), 45 deletions(-)
> >
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 3b5dc21b323c..0cc7872cc576 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -5526,6 +5526,29 @@ static ssize_t shmem_enabled_store(struct kobject *kobj,
> >   struct kobj_attribute shmem_enabled_attr = __ATTR_RW(shmem_enabled);
> >   static DEFINE_SPINLOCK(huge_shmem_orders_lock);
> > +enum huge_shmem_enabled_mode {
> > +	HUGE_SHMEM_ENABLED_ALWAYS = 0,
> > +	HUGE_SHMEM_ENABLED_INHERIT,
> > +	HUGE_SHMEM_ENABLED_WITHIN_SIZE,
> > +	HUGE_SHMEM_ENABLED_ADVISE,
> > +	HUGE_SHMEM_ENABLED_NEVER,
> > +};
> > +
> > +static const char * const huge_shmem_enabled_mode_strings[] = {
> > +	[HUGE_SHMEM_ENABLED_ALWAYS]      = "always",
> > +	[HUGE_SHMEM_ENABLED_INHERIT]     = "inherit",
> > +	[HUGE_SHMEM_ENABLED_WITHIN_SIZE] = "within_size",
> > +	[HUGE_SHMEM_ENABLED_ADVISE]      = "advise",
> > +	[HUGE_SHMEM_ENABLED_NEVER]       = "never",
> > +};
> > +
> > +static unsigned long * const huge_shmem_orders_by_mode[] = {
> > +	[HUGE_SHMEM_ENABLED_ALWAYS]      = &huge_shmem_orders_always,
> > +	[HUGE_SHMEM_ENABLED_INHERIT]     = &huge_shmem_orders_inherit,
> > +	[HUGE_SHMEM_ENABLED_WITHIN_SIZE] = &huge_shmem_orders_within_size,
> > +	[HUGE_SHMEM_ENABLED_ADVISE]      = &huge_shmem_orders_madvise,
> > +};
> > +
> >   static ssize_t thpsize_shmem_enabled_show(struct kobject *kobj,
> >   					  struct kobj_attribute *attr, char *buf)
> >   {
> > @@ -5551,57 +5574,32 @@ static ssize_t thpsize_shmem_enabled_store(struct kobject *kobj,
> >   					   const char *buf, size_t count)
> >   {
> >   	int order = to_thpsize(kobj)->order;
> > +	int mode, m;
> >   	ssize_t ret = count;
> > +	int err;
> > -	if (sysfs_streq(buf, "always")) {
> > -		spin_lock(&huge_shmem_orders_lock);
> > -		clear_bit(order, &huge_shmem_orders_inherit);
> > -		clear_bit(order, &huge_shmem_orders_madvise);
> > -		clear_bit(order, &huge_shmem_orders_within_size);
> > -		set_bit(order, &huge_shmem_orders_always);
> > -		spin_unlock(&huge_shmem_orders_lock);
> > -	} else if (sysfs_streq(buf, "inherit")) {
> > -		/* Do not override huge allocation policy with non-PMD sized mTHP */
> > -		if (shmem_huge == SHMEM_HUGE_FORCE && !is_pmd_order(order))
> > -			return -EINVAL;
> > +	mode = sysfs_match_string(huge_shmem_enabled_mode_strings, buf);
> > +	if (mode < 0)
> > +		return -EINVAL;
> > -		spin_lock(&huge_shmem_orders_lock);
> > -		clear_bit(order, &huge_shmem_orders_always);
> > -		clear_bit(order, &huge_shmem_orders_madvise);
> > -		clear_bit(order, &huge_shmem_orders_within_size);
> > -		set_bit(order, &huge_shmem_orders_inherit);
> > -		spin_unlock(&huge_shmem_orders_lock);
> > -	} else if (sysfs_streq(buf, "within_size")) {
> > -		spin_lock(&huge_shmem_orders_lock);
> > -		clear_bit(order, &huge_shmem_orders_always);
> > -		clear_bit(order, &huge_shmem_orders_inherit);
> > -		clear_bit(order, &huge_shmem_orders_madvise);
> > -		set_bit(order, &huge_shmem_orders_within_size);
> > -		spin_unlock(&huge_shmem_orders_lock);
> > -	} else if (sysfs_streq(buf, "advise")) {
> > -		spin_lock(&huge_shmem_orders_lock);
> > -		clear_bit(order, &huge_shmem_orders_always);
> > -		clear_bit(order, &huge_shmem_orders_inherit);
> > -		clear_bit(order, &huge_shmem_orders_within_size);
> > -		set_bit(order, &huge_shmem_orders_madvise);
> > -		spin_unlock(&huge_shmem_orders_lock);
> > -	} else if (sysfs_streq(buf, "never")) {
> > -		spin_lock(&huge_shmem_orders_lock);
> > -		clear_bit(order, &huge_shmem_orders_always);
> > -		clear_bit(order, &huge_shmem_orders_inherit);
> > -		clear_bit(order, &huge_shmem_orders_within_size);
> > -		clear_bit(order, &huge_shmem_orders_madvise);
> > -		spin_unlock(&huge_shmem_orders_lock);
> > -	} else {
> > -		ret = -EINVAL;
> > +	/* Do not override huge allocation policy with non-PMD sized mTHP */
> > +	if (mode == HUGE_SHMEM_ENABLED_INHERIT &&
> > +		shmem_huge == SHMEM_HUGE_FORCE && !is_pmd_order(order))
> > +		return -EINVAL;
> > +
> > +	spin_lock(&huge_shmem_orders_lock);
> > +	for (m = 0; m < ARRAY_SIZE(huge_shmem_orders_by_mode); m++) {
> > +		if (m == mode)
> > +			set_bit(order, huge_shmem_orders_by_mode[m]);
> > +		else
> > +			clear_bit(order, huge_shmem_orders_by_mode[m]);
> >   	}
>
> We are already under the lock, so you can use non-atomic functions like
> commit 82d9ff648c6c does: __test_and_set_bit/__test_and_clear_bit.

Agreed. I guess you're going with the RMW form so we can conditionally figure
out if we need to start_stop_khugepaged()?

>
> > +	spin_unlock(&huge_shmem_orders_lock);
> > -	if (ret > 0) {
> > -		int err = start_stop_khugepaged();
> > +	err = start_stop_khugepaged();
> > +	if (err)
>
> Moreover, I think we can follow commit 82d9ff648c6c's approach: if nothing
> changed, we don't need to call start_stop_khugepaged().

Agreed.

Cheers, Lorenzo