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

[Lorenzo Stoakes <ljs@kernel.org>]

On Fri, May 15, 2026 at 07:23:36AM +0000, ranxiaokai627@163.com wrote:
> >On Wed, May 13, 2026 at 09:45:07AM +0000, ranxiaokai627@163.com wrote:
> >> From: Ran Xiaokai <ran.xiaokai@zte.com.cn>
> >>
> >> 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.
> >
> >> +	/* 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;
> >>
> >> -	if (ret > 0) {
> >> -		int err = start_stop_khugepaged();
> >> +	spin_lock(&huge_shmem_orders_lock);
> >> +	for (m = 0; m < ARRAY_SIZE(huge_shmem_orders_by_mode); m++) {
> >> +		if (m == mode)
> >> +			changed |= !__test_and_set_bit(order, huge_shmem_orders_by_mode[m]);
> >> +		else
> >> +			changed |= __test_and_clear_bit(order, huge_shmem_orders_by_mode[m]);
> >> +	}
> >> +	spin_unlock(&huge_shmem_orders_lock);
> >
> >You're copy/pasta'ing anon_enabled_store() but not the nicer refactoring that's
> >there.
> >
> >Please split this out like set_anon_enabled_mode() does. And put:
> >
> >	static unsigned long *enabled_orders[] = {
> >		&huge_shmem_orders_always,
> >		&huge_shmem_orders_inherit,
> >		&huge_shmem_orders_within_size,
> >		&huge_shmem_orders_madvise,
> >	};
> >
> >At the start of it like that does also.
> >
> >Please don't reproduce the single letter var name though :)
> >
>
> Arrays huge_shmem_orders_by_mode are shared by both store() and
> show() in patch2, so we should keeping at file scope to avoid duplication,
> what do you think ?

Ah yeah ok, in that case keep that separated out, but the other points in the
review stand!

Thanks :)

>
> >> +};
> >
> >>
> >> +	if (changed) {
> >> +		err = start_stop_khugepaged();
> >>  		if (err)
> >>  			ret = err;
> >
> >
> >> +	} else {
> >> +		/*
> >> +		 * Recalculate watermarks even when the mode hasn't changed
> >> +		 * to preserve the legacy behavior, as this is always called
> >> +		 * inside start_stop_khugepaged().
> >> +		 */
> >> +		set_recommended_min_free_kbytes();
> >>  	}
> >> +
> >>  	return ret;
> >
> >return count;
> >
> >>  }
> >>
> >> --
> >> 2.25.1
> >>
> >>
> >
> >Cheers, Lorenzo
>