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

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

[ranxiaokai627]

>On Thu, 14 May 2026 09:26:39 +0000 ranxiaokai627@163.com wrote:
>
>> Thanks for the detailed and thoughtful review! I'll fix this as 
>> suggested in the next version.
>
>AI review is asking about a couple of atomicity issues - please check it out?
>	https://sashiko.dev/#/patchset/20260513094508.50888-1-ranxiaokai627@163.com

The huge_shmem_orders_xxx global variables are of type unsigned long,
which is 32-bit even on 32-bit systems. There is no reason for the
store in __test_and_set_bit() to be split into two instructions,
so I do not think this introduces a torn write issue.

David also mentioned this optimization here: 
https://lore.kernel.org/all/4f2abf42-983f-4cc2-92f5-c81827e7b7e2@kernel.org/

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

[Lorenzo Stoakes]

On Wed, May 20, 2026 at 10:29:22AM +0000, ranxiaokai627@163.com wrote:
> >On Thu, 14 May 2026 09:26:39 +0000 ranxiaokai627@163.com wrote:
> >
> >> Thanks for the detailed and thoughtful review! I'll fix this as
> >> suggested in the next version.
> >
> >AI review is asking about a couple of atomicity issues - please check it out?
> >	https://sashiko.dev/#/patchset/20260513094508.50888-1-ranxiaokai627@163.com
>
> The huge_shmem_orders_xxx global variables are of type unsigned long,
> which is 32-bit even on 32-bit systems. There is no reason for the
> store in __test_and_set_bit() to be split into two instructions,
> so I do not think this introduces a torn write issue.

Yep agreed, this doesn't look like a real issue.

>
> David also mentioned this optimization here:
> https://lore.kernel.org/all/4f2abf42-983f-4cc2-92f5-c81827e7b7e2@kernel.org/
>

Yup, I think it's fine as is!

Thanks, Lorenzo

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

[ranxiaokai627]

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.

Tested with selftests ./run_kselftest.sh -t mm:ksft_thp.sh,
all test cases passed.

Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
---
 mm/shmem.c | 94 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 43 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 3b5dc21b323c..60cb10854f11 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,42 @@ 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;
+	bool changed = false;
 
-	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;
 
-	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);
 
+	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;
 }
 
-- 
2.25.1

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

[Baolin Wang]

The subject line should be: "mm: shmem: xxx"

On 5/13/26 5:45 PM, 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.
> 
> Tested with selftests ./run_kselftest.sh -t mm:ksft_thp.sh,
> all test cases passed.
> 
> Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
> ---

You should document your changes under the '---' for the new version, or 
ideally, include a cover letter describing them.

I did some togging and works well (some nits as below). Feel free to add:
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
Tested-by: Baolin Wang <baolin.wang@linux.alibaba.com>

>   mm/shmem.c | 94 +++++++++++++++++++++++++++++-------------------------
>   1 file changed, 51 insertions(+), 43 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 3b5dc21b323c..60cb10854f11 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,42 @@ 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;
> +	bool changed = false;
>   
> -	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;
>   
> -	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);
>   
> +	if (changed) {
> +		err = start_stop_khugepaged();
>   		if (err)
>   			ret = err;

Nit: just return 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;

Nit: return count, then you can remove the 'ret' variable.

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

[ranxiaokai627]

>The subject line should be: "mm: shmem: xxx"

Yes, of course.
Thanks for your review.

>On 5/13/26 5:45 PM, 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.
>> 
>> Tested with selftests ./run_kselftest.sh -t mm:ksft_thp.sh,
>> all test cases passed.
>> 
>> Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
>> ---
>
>You should document your changes under the '---' for the new version, or 
>ideally, include a cover letter describing them.

Thanks for the reminder!
Will do in the next version.

>I did some togging and works well (some nits as below). Feel free to add:
>Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>Tested-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>
>>   mm/shmem.c | 94 +++++++++++++++++++++++++++++-------------------------
>>   1 file changed, 51 insertions(+), 43 deletions(-)
>> 
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 3b5dc21b323c..60cb10854f11 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,42 @@ 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;
>> +	bool changed = false;
>> +	/* 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);
>>   
>> +	if (changed) {
>> +		err = start_stop_khugepaged();
>>   		if (err)
>>   			ret = err;
>
>Nit: just return 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;
>
>Nit: return count, then you can remove the 'ret' variable.

Agreed.
I'll wait for more feedback and send a new version.

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

[Lorenzo Stoakes]

On Thu, May 14, 2026 at 10:36:34AM +0800, Baolin Wang wrote:
> The subject line should be: "mm: shmem: xxx"
>
> On 5/13/26 5:45 PM, 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.
> >
> > Tested with selftests ./run_kselftest.sh -t mm:ksft_thp.sh,
> > all test cases passed.
> >
> > Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
> > ---
>
> You should document your changes under the '---' for the new version, or
> ideally, include a cover letter describing them.

Well this should have been a cover letter anyway! :)

Ran - please don't send series with >1 patch with 2/2 in-reply-to 1/2, it's a
pain to deal with.

Send a cover letter if >1 patch and have both in-reply-to the cover letter.

Cheers, Lorenzo

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

[Breno Leitao]

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.
> 
> Tested with selftests ./run_kselftest.sh -t mm:ksft_thp.sh,
> all test cases passed.
> 
> Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>

Reviewed-by: Breno Leitao <leitao@debian.org>

> +	/* 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;

This identation seems a bit broken. You probably want something like:

	if (mode == HUGE_SHMEM_ENABLED_INHERIT &&
	    shmem_huge == SHMEM_HUGE_FORCE && !is_pmd_order(order))
		return -EINVAL;

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

[ranxiaokai627]

>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.
>> 
>> Tested with selftests ./run_kselftest.sh -t mm:ksft_thp.sh,
>> all test cases passed.
>> 
>> Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
>
>Reviewed-by: Breno Leitao <leitao@debian.org>
>
>> +	/* 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;
>
>This identation seems a bit broken. You probably want something like:
>
>	if (mode == HUGE_SHMEM_ENABLED_INHERIT &&
>	    shmem_huge == SHMEM_HUGE_FORCE && !is_pmd_order(order))
>		return -EINVAL;

Thanks for the detailed and thoughtful review! I'll fix this as 
suggested in the next version.

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

[Lorenzo Stoakes]

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.

You are not mentioning that you're changing the stop/start behaviour. This
is a fundamental part of the change that should be documented in the commit
message.

>
> Tested with selftests ./run_kselftest.sh -t mm:ksft_thp.sh,
> all test cases passed.
>
> Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
> ---
>  mm/shmem.c | 94 +++++++++++++++++++++++++++++-------------------------
>  1 file changed, 51 insertions(+), 43 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 3b5dc21b323c..60cb10854f11 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,
> +};

I think it's perfectly cromulent to put this within a function rather than as a
static value at file scope. See below.

> +
>  static ssize_t thpsize_shmem_enabled_show(struct kobject *kobj,
>  					  struct kobj_attribute *attr, char *buf)
>  {
> @@ -5551,57 +5574,42 @@ 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;

I hate single letter variable names (yes I see set_anon_enabled_mode() has it, I
should have reviewed against that, oops :).

Please give it its type of enum huge_shmem_enalbed_mode.

>  	ssize_t ret = count;
> +	int err;

This is silly, we don't need ret and err (the ret is not your fault
obviously...)

On success, we return count, on failure we return err. Drop the ret here.

(I think Baolin reviewed similarly :)

> +	bool changed = false;
>
> -	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;

I don't know why we filter this error, it returns -EINVAL on error anyway, just
return mode in this case.

>
> -		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;
>
> -	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 :)

> +};

>
> +	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

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

[ranxiaokai627]

>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.
>
>You are not mentioning that you're changing the stop/start behaviour. This
>is a fundamental part of the change that should be documented in the commit
>message.

Hi, Lorenzo,
Thanks for your detailed review.

I'll update the commit message to document this:

"Behavioral change:
Call start_stop_khugepaged() only when the mode actually changes.
If unchanged, call set_recommended_min_free_kbytes() to preserve legacy
watermark behavior. This avoids unnecessary khugepaged restarts."

>>
>> Tested with selftests ./run_kselftest.sh -t mm:ksft_thp.sh,
>> all test cases passed.
>>
>> Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
>> ---
>>  mm/shmem.c | 94 +++++++++++++++++++++++++++++-------------------------
>>  1 file changed, 51 insertions(+), 43 deletions(-)
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 3b5dc21b323c..60cb10854f11 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,
>> +};
>
>I think it's perfectly cromulent to put this within a function rather than as a
>static value at file scope. See below.
>
>> +
>>  static ssize_t thpsize_shmem_enabled_show(struct kobject *kobj,
>>  					  struct kobj_attribute *attr, char *buf)
>>  {
>> @@ -5551,57 +5574,42 @@ 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;
>
>I hate single letter variable names (yes I see set_anon_enabled_mode() has it, I
>should have reviewed against that, oops :).
>
>Please give it its type of enum huge_shmem_enalbed_mode.

int main()
{
    enum huge_shmem_enabled_mode mode = 0;
    const char* type_name = _Generic((mode),
        int: "signed int",
        unsigned int: "unsigned int",
        default: "other"
    );
    printf("enum huge_shmem_enabled_mode underlying type is: %s\n", type_name);
    return 0;
}
 ./a.out
enum huge_shmem_enabled_mode underlying type is: unsigned int

This will cause the following check to always evaluate to false:
if (mode < 0)
	return mode;

So i think we should keep mode as integer.

>>  	ssize_t ret = count;
>> +	int err;
>
>This is silly, we don't need ret and err (the ret is not your fault
>obviously...)
>
>On success, we return count, on failure we return err. Drop the ret here.
>
>(I think Baolin reviewed similarly :)

Agreed, will simplify return logic and return count in v3.

>> +	bool changed = false;
>>
>> -	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;
>
>I don't know why we filter this error, it returns -EINVAL on error anyway, just
>return mode in this case.

yes, will return mode directly, 
but the type of mode needs to remain an integer variable as replied above.

>>
>> -		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;
>>
>> -	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 :)

Agreed, will extract a set_shmem_enabled_mode() helper with local array.

>> +};
>
>>
>> +	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

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

[ranxiaokai627]

>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 ?

>> +};
>
>>
>> +	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

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

[Lorenzo Stoakes]

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
>

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

[ranxiaokai627]

>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.
>
>>  static ssize_t thpsize_shmem_enabled_show(struct kobject *kobj,
>>  					  struct kobj_attribute *attr, char *buf)
>>  {
>> @@ -5551,57 +5574,42 @@ 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;
>
>I hate single letter variable names (yes I see set_anon_enabled_mode() has it, I
>should have reviewed against that, oops :).
>
>Please give it its type of enum huge_shmem_enalbed_mode.

Hi, Lorenzo,

int main()
{
    enum huge_shmem_enabled_mode mode = 0;
    const char* type_name = _Generic((mode),
        int: "signed int",
        unsigned int: "unsigned int",
        default: "other"
    );
    printf("enum huge_shmem_enabled_mode underlying type is: %s\n", type_name);
    return 0;
}
 ./a.out
enum huge_shmem_enabled_mode underlying type is: unsigned int

This will cause the following check to always evaluate to false and
does not validate input parameters like echo "xxx" > .../hugepages-xxkB/shmem_enable
if (mode < 0)
	return mode;

So i think we should keep mode as integer.

>> +	mode = sysfs_match_string(huge_shmem_enabled_mode_strings, buf);
>> +	if (mode < 0)
>> +		return -EINVAL;

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

[Lorenzo Stoakes]

On Mon, May 18, 2026 at 02:02:08AM +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.
> >
> >>  static ssize_t thpsize_shmem_enabled_show(struct kobject *kobj,
> >>  					  struct kobj_attribute *attr, char *buf)
> >>  {
> >> @@ -5551,57 +5574,42 @@ 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;
> >
> >I hate single letter variable names (yes I see set_anon_enabled_mode() has it, I
> >should have reviewed against that, oops :).
> >
> >Please give it its type of enum huge_shmem_enalbed_mode.
>
> Hi, Lorenzo,
>
> int main()
> {
>     enum huge_shmem_enabled_mode mode = 0;
>     const char* type_name = _Generic((mode),
>         int: "signed int",
>         unsigned int: "unsigned int",
>         default: "other"
>     );
>     printf("enum huge_shmem_enabled_mode underlying type is: %s\n", type_name);
>     return 0;
> }
>  ./a.out
> enum huge_shmem_enabled_mode underlying type is: unsigned int
>
> This will cause the following check to always evaluate to false and
> does not validate input parameters like echo "xxx" > .../hugepages-xxkB/shmem_enable
> if (mode < 0)
> 	return mode;
>
> So i think we should keep mode as integer.
>
> >> +	mode = sysfs_match_string(huge_shmem_enabled_mode_strings, buf);
> >> +	if (mode < 0)
> >> +		return -EINVAL;

OK fair enough. The C type system strikes again :)

>

Cheers, Lorenzo