* [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string()
@ 2026-05-13 9:45 ranxiaokai627
2026-05-13 9:45 ` [PATCH v2 2/2] mm: huge_memory: refactor thpsize_shmem_enabled_show() with helper arrays ranxiaokai627
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: ranxiaokai627 @ 2026-05-13 9:45 UTC (permalink / raw)
To: hughd, baolin.wang, akpm
Cc: leitao, ljs, linux-mm, linux-kernel, ran.xiaokai, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] mm: huge_memory: refactor thpsize_shmem_enabled_show() with helper arrays
2026-05-13 9:45 [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string() ranxiaokai627
@ 2026-05-13 9:45 ` ranxiaokai627
2026-05-14 2:41 ` Baolin Wang
` (2 more replies)
2026-05-14 2:36 ` [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string() Baolin Wang
` (2 subsequent siblings)
3 siblings, 3 replies; 11+ messages in thread
From: ranxiaokai627 @ 2026-05-13 9:45 UTC (permalink / raw)
To: hughd, baolin.wang, akpm
Cc: leitao, ljs, linux-mm, linux-kernel, ran.xiaokai, ranxiaokai627
From: Ran Xiaokai <ran.xiaokai@zte.com.cn>
Replace the hardcoded if/else chain of test_bit() calls and string
literals in thpsize_shmem_enabled_show() with a loop over
huge_shmem_orders_by_mode[] and huge_shmem_enabled_mode_strings[] arrays.
This makes thpsize_shmem_enabled_show() consistent with
thpsize_shmem_enabled_store() and eliminates duplicated mode name strings.
Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
---
mm/shmem.c | 36 +++++++++++++++++++++++-------------
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index 60cb10854f11..086762e6de71 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -5553,20 +5553,30 @@ static ssize_t thpsize_shmem_enabled_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
{
int order = to_thpsize(kobj)->order;
- const char *output;
-
- if (test_bit(order, &huge_shmem_orders_always))
- output = "[always] inherit within_size advise never";
- else if (test_bit(order, &huge_shmem_orders_inherit))
- output = "always [inherit] within_size advise never";
- else if (test_bit(order, &huge_shmem_orders_within_size))
- output = "always inherit [within_size] advise never";
- else if (test_bit(order, &huge_shmem_orders_madvise))
- output = "always inherit within_size [advise] never";
- else
- output = "always inherit within_size advise [never]";
+ int active = HUGE_SHMEM_ENABLED_NEVER;
+ int len = 0;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(huge_shmem_orders_by_mode); i++) {
+ if (test_bit(order, huge_shmem_orders_by_mode[i])) {
+ active = i;
+ break;
+ }
+ }
+
+ for (i = 0; i < ARRAY_SIZE(huge_shmem_enabled_mode_strings); i++) {
+ if (i == active)
+ len += sysfs_emit_at(buf, len, "[%s] ",
+ huge_shmem_enabled_mode_strings[i]);
+ else
+ len += sysfs_emit_at(buf, len, "%s ",
+ huge_shmem_enabled_mode_strings[i]);
+ }
+
+ /* Replace trailing space with newline */
+ buf[len - 1] = '\n';
- return sysfs_emit(buf, "%s\n", output);
+ return len;
}
static ssize_t thpsize_shmem_enabled_store(struct kobject *kobj,
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string()
2026-05-13 9:45 [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string() ranxiaokai627
2026-05-13 9:45 ` [PATCH v2 2/2] mm: huge_memory: refactor thpsize_shmem_enabled_show() with helper arrays ranxiaokai627
@ 2026-05-14 2:36 ` Baolin Wang
2026-05-14 10:10 ` ranxiaokai627
2026-05-14 12:05 ` Lorenzo Stoakes
2026-05-14 8:33 ` Breno Leitao
2026-05-14 12:48 ` Lorenzo Stoakes
3 siblings, 2 replies; 11+ messages in thread
From: Baolin Wang @ 2026-05-14 2:36 UTC (permalink / raw)
To: ranxiaokai627, hughd, akpm
Cc: leitao, ljs, linux-mm, linux-kernel, ran.xiaokai
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] mm: huge_memory: refactor thpsize_shmem_enabled_show() with helper arrays
2026-05-13 9:45 ` [PATCH v2 2/2] mm: huge_memory: refactor thpsize_shmem_enabled_show() with helper arrays ranxiaokai627
@ 2026-05-14 2:41 ` Baolin Wang
2026-05-14 9:08 ` Breno Leitao
2026-05-14 12:22 ` Lorenzo Stoakes
2 siblings, 0 replies; 11+ messages in thread
From: Baolin Wang @ 2026-05-14 2:41 UTC (permalink / raw)
To: ranxiaokai627, hughd, akpm
Cc: leitao, ljs, linux-mm, linux-kernel, ran.xiaokai
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>
>
> Replace the hardcoded if/else chain of test_bit() calls and string
> literals in thpsize_shmem_enabled_show() with a loop over
> huge_shmem_orders_by_mode[] and huge_shmem_enabled_mode_strings[] arrays.
>
> This makes thpsize_shmem_enabled_show() consistent with
> thpsize_shmem_enabled_store() and eliminates duplicated mode name strings.
>
> Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
> ---
LGTM. Thanks.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
Tested-by: Baolin Wang <baolin.wang@linux.alibaba.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string()
2026-05-13 9:45 [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string() ranxiaokai627
2026-05-13 9:45 ` [PATCH v2 2/2] mm: huge_memory: refactor thpsize_shmem_enabled_show() with helper arrays ranxiaokai627
2026-05-14 2:36 ` [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string() Baolin Wang
@ 2026-05-14 8:33 ` Breno Leitao
2026-05-14 9:26 ` ranxiaokai627
2026-05-14 12:48 ` Lorenzo Stoakes
3 siblings, 1 reply; 11+ messages in thread
From: Breno Leitao @ 2026-05-14 8:33 UTC (permalink / raw)
To: ranxiaokai627
Cc: hughd, baolin.wang, akpm, ljs, linux-mm, linux-kernel,
ran.xiaokai
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;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] mm: huge_memory: refactor thpsize_shmem_enabled_show() with helper arrays
2026-05-13 9:45 ` [PATCH v2 2/2] mm: huge_memory: refactor thpsize_shmem_enabled_show() with helper arrays ranxiaokai627
2026-05-14 2:41 ` Baolin Wang
@ 2026-05-14 9:08 ` Breno Leitao
2026-05-14 12:22 ` Lorenzo Stoakes
2 siblings, 0 replies; 11+ messages in thread
From: Breno Leitao @ 2026-05-14 9:08 UTC (permalink / raw)
To: ranxiaokai627
Cc: hughd, baolin.wang, akpm, ljs, linux-mm, linux-kernel,
ran.xiaokai
On Wed, May 13, 2026 at 09:45:08AM +0000, ranxiaokai627@163.com wrote:
> From: Ran Xiaokai <ran.xiaokai@zte.com.cn>
>
> Replace the hardcoded if/else chain of test_bit() calls and string
> literals in thpsize_shmem_enabled_show() with a loop over
> huge_shmem_orders_by_mode[] and huge_shmem_enabled_mode_strings[] arrays.
>
> This makes thpsize_shmem_enabled_show() consistent with
> thpsize_shmem_enabled_store() and eliminates duplicated mode name strings.
>
> Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
Reviewed-by: Breno Leitao <leitao@debian.org>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string()
2026-05-14 8:33 ` Breno Leitao
@ 2026-05-14 9:26 ` ranxiaokai627
0 siblings, 0 replies; 11+ messages in thread
From: ranxiaokai627 @ 2026-05-14 9:26 UTC (permalink / raw)
To: leitao
Cc: akpm, baolin.wang, hughd, linux-kernel, linux-mm, ljs,
ran.xiaokai, 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string()
2026-05-14 2:36 ` [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string() Baolin Wang
@ 2026-05-14 10:10 ` ranxiaokai627
2026-05-14 12:05 ` Lorenzo Stoakes
1 sibling, 0 replies; 11+ messages in thread
From: ranxiaokai627 @ 2026-05-14 10:10 UTC (permalink / raw)
To: baolin.wang
Cc: akpm, hughd, leitao, linux-kernel, linux-mm, ljs, ran.xiaokai,
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string()
2026-05-14 2:36 ` [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string() Baolin Wang
2026-05-14 10:10 ` ranxiaokai627
@ 2026-05-14 12:05 ` Lorenzo Stoakes
1 sibling, 0 replies; 11+ messages in thread
From: Lorenzo Stoakes @ 2026-05-14 12:05 UTC (permalink / raw)
To: ranxiaokai627
Cc: Baolin Wang, hughd, akpm, leitao, linux-mm, linux-kernel,
ran.xiaokai
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] mm: huge_memory: refactor thpsize_shmem_enabled_show() with helper arrays
2026-05-13 9:45 ` [PATCH v2 2/2] mm: huge_memory: refactor thpsize_shmem_enabled_show() with helper arrays ranxiaokai627
2026-05-14 2:41 ` Baolin Wang
2026-05-14 9:08 ` Breno Leitao
@ 2026-05-14 12:22 ` Lorenzo Stoakes
2 siblings, 0 replies; 11+ messages in thread
From: Lorenzo Stoakes @ 2026-05-14 12:22 UTC (permalink / raw)
To: ranxiaokai627
Cc: hughd, baolin.wang, akpm, leitao, linux-mm, linux-kernel,
ran.xiaokai
(As I said in the 1/2)
Please don't send 2/2 in response to 1/2, and use a cover letter if you send
more than 1 patch!
On Wed, May 13, 2026 at 09:45:08AM +0000, ranxiaokai627@163.com wrote:
> From: Ran Xiaokai <ran.xiaokai@zte.com.cn>
>
> Replace the hardcoded if/else chain of test_bit() calls and string
> literals in thpsize_shmem_enabled_show() with a loop over
> huge_shmem_orders_by_mode[] and huge_shmem_enabled_mode_strings[] arrays.
>
> This makes thpsize_shmem_enabled_show() consistent with
> thpsize_shmem_enabled_store() and eliminates duplicated mode name strings.
>
> Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
The logic looks good, I wish we could de-duplicate. But for now maybe better to
get this refactored first.
So:
Reviewed-by: Lorenzo Stoakes <ljs@kernel.org>
> ---
> mm/shmem.c | 36 +++++++++++++++++++++++-------------
> 1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 60cb10854f11..086762e6de71 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -5553,20 +5553,30 @@ static ssize_t thpsize_shmem_enabled_show(struct kobject *kobj,
> struct kobj_attribute *attr, char *buf)
> {
> int order = to_thpsize(kobj)->order;
> - const char *output;
> -
> - if (test_bit(order, &huge_shmem_orders_always))
> - output = "[always] inherit within_size advise never";
> - else if (test_bit(order, &huge_shmem_orders_inherit))
> - output = "always [inherit] within_size advise never";
> - else if (test_bit(order, &huge_shmem_orders_within_size))
> - output = "always inherit [within_size] advise never";
> - else if (test_bit(order, &huge_shmem_orders_madvise))
> - output = "always inherit within_size [advise] never";
> - else
> - output = "always inherit within_size advise [never]";
> + int active = HUGE_SHMEM_ENABLED_NEVER;
> + int len = 0;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(huge_shmem_orders_by_mode); i++) {
> + if (test_bit(order, huge_shmem_orders_by_mode[i])) {
> + active = i;
> + break;
> + }
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(huge_shmem_enabled_mode_strings); i++) {
> + if (i == active)
> + len += sysfs_emit_at(buf, len, "[%s] ",
> + huge_shmem_enabled_mode_strings[i]);
> + else
> + len += sysfs_emit_at(buf, len, "%s ",
> + huge_shmem_enabled_mode_strings[i]);
> + }
> +
> + /* Replace trailing space with newline */
> + buf[len - 1] = '\n';
>
> - return sysfs_emit(buf, "%s\n", output);
> + return len;
> }
This is pretty mcuh a one-for-one copy/pasta of defrag_show(), I don't love that
we have the exact same code duplicated across two files like that.
You could write something like:
static ssize_t thp_sysfs_enabled_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf,
const char *names, int names_len,
const char *orders_by_mode, int orders_by_mode_len,
int default_mode)
{
...
}
To abstract it, but that's kind of a horrible signature isn't it? :)
Could use a helper struct, but that feels a bit overkill for this hmm...
Really I wonder if we shouldn't have this in huge_memory.c anyway, it's a bit of
a weird thing to put it in mm/shmem.c, it's more huge pages than shmem imo.
Anyway. The logic itself looks fine so LGTM!
>
> static ssize_t thpsize_shmem_enabled_store(struct kobject *kobj,
> --
> 2.25.1
>
>
Cheers, Lorenzo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string()
2026-05-13 9:45 [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string() ranxiaokai627
` (2 preceding siblings ...)
2026-05-14 8:33 ` Breno Leitao
@ 2026-05-14 12:48 ` Lorenzo Stoakes
3 siblings, 0 replies; 11+ messages in thread
From: Lorenzo Stoakes @ 2026-05-14 12:48 UTC (permalink / raw)
To: ranxiaokai627
Cc: hughd, baolin.wang, akpm, leitao, linux-mm, linux-kernel,
ran.xiaokai
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-05-14 12:48 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-13 9:45 [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string() ranxiaokai627
2026-05-13 9:45 ` [PATCH v2 2/2] mm: huge_memory: refactor thpsize_shmem_enabled_show() with helper arrays ranxiaokai627
2026-05-14 2:41 ` Baolin Wang
2026-05-14 9:08 ` Breno Leitao
2026-05-14 12:22 ` Lorenzo Stoakes
2026-05-14 2:36 ` [PATCH v2 1/2] mm: huge_memory: refactor thpsize_shmem_enabled_store() with sysfs_match_string() Baolin Wang
2026-05-14 10:10 ` ranxiaokai627
2026-05-14 12:05 ` Lorenzo Stoakes
2026-05-14 8:33 ` Breno Leitao
2026-05-14 9:26 ` ranxiaokai627
2026-05-14 12:48 ` Lorenzo Stoakes
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox