Refactor enabled_store() to use a new change_enabled() helper that
reuses the shared enum enabled_mode and enabled_mode_strings[]
introduced in the previous commit.
The helper uses the same loop pattern as change_anon_orders(),
iterating over an array of flag bit positions and using
test_and_set_bit()/test_and_clear_bit() to track whether the state
actually changed. ENABLED_INHERIT is rejected since it is not a
valid mode for the global THP setting.
Signed-off-by: Breno Leitao <leitao@debian.org>
---
mm/huge_memory.c | 51 ++++++++++++++++++++++++++++++++++++---------------
1 file changed, 36 insertions(+), 15 deletions(-)
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 19619213f54d1..dd5011cf36839 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -330,30 +330,51 @@ static const char * const enabled_mode_strings[] = {
[ENABLED_NEVER] = "never",
};
+static bool change_enabled(enum enabled_mode mode)
+{
+ static const unsigned long thp_flags[] = {
+ TRANSPARENT_HUGEPAGE_FLAG,
+ TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG,
+ };
+ bool changed = false;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(thp_flags); i++) {
+ if (i == mode)
+ changed |= !test_and_set_bit(thp_flags[i],
+ &transparent_hugepage_flags);
+ else
+ changed |= test_and_clear_bit(thp_flags[i],
+ &transparent_hugepage_flags);
+ }
+
+ return changed;
+}
+
static ssize_t enabled_store(struct kobject *kobj,
struct kobj_attribute *attr,
const char *buf, size_t count)
{
- ssize_t ret = count;
+ int mode;
- if (sysfs_streq(buf, "always")) {
- clear_bit(TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG, &transparent_hugepage_flags);
- set_bit(TRANSPARENT_HUGEPAGE_FLAG, &transparent_hugepage_flags);
- } else if (sysfs_streq(buf, "madvise")) {
- clear_bit(TRANSPARENT_HUGEPAGE_FLAG, &transparent_hugepage_flags);
- set_bit(TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG, &transparent_hugepage_flags);
- } else if (sysfs_streq(buf, "never")) {
- clear_bit(TRANSPARENT_HUGEPAGE_FLAG, &transparent_hugepage_flags);
- clear_bit(TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG, &transparent_hugepage_flags);
- } else
- ret = -EINVAL;
+ mode = sysfs_match_string(enabled_mode_strings, buf);
+ if (mode < 0 || mode == ENABLED_INHERIT)
+ return -EINVAL;
- if (ret > 0) {
+ if (change_enabled(mode)) {
int err = start_stop_khugepaged();
+
if (err)
- ret = err;
+ return err;
+ } else {
+ /*
+ * Recalculate watermarks even when the mode didn't
+ * change, as the previous code always called
+ * start_stop_khugepaged() which does this internally.
+ */
+ set_recommended_min_free_kbytes();
}
- return ret;
+ return count;
}
static struct kobj_attribute enabled_attr = __ATTR_RW(enabled);
--
2.47.3On Thu, Mar 05, 2026 at 06:04:55AM -0800, Breno Leitao wrote:
> Refactor enabled_store() to use a new change_enabled() helper that
> reuses the shared enum enabled_mode and enabled_mode_strings[]
> introduced in the previous commit.
>
> The helper uses the same loop pattern as change_anon_orders(),
> iterating over an array of flag bit positions and using
> test_and_set_bit()/test_and_clear_bit() to track whether the state
> actually changed. ENABLED_INHERIT is rejected since it is not a
> valid mode for the global THP setting.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> mm/huge_memory.c | 51 ++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 19619213f54d1..dd5011cf36839 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -330,30 +330,51 @@ static const char * const enabled_mode_strings[] = {
> [ENABLED_NEVER] = "never",
> };
>
> +static bool change_enabled(enum enabled_mode mode)
> +{
> + static const unsigned long thp_flags[] = {
> + TRANSPARENT_HUGEPAGE_FLAG,
> + TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG,
> + };
> + bool changed = false;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(thp_flags); i++) {
> + if (i == mode)
> + changed |= !test_and_set_bit(thp_flags[i],
> + &transparent_hugepage_flags);
> + else
> + changed |= test_and_clear_bit(thp_flags[i],
> + &transparent_hugepage_flags);
> + }
> +
> + return changed;
> +}
> +
> static ssize_t enabled_store(struct kobject *kobj,
> struct kobj_attribute *attr,
> const char *buf, size_t count)
> {
> - ssize_t ret = count;
> + int mode;
>
> - if (sysfs_streq(buf, "always")) {
> - clear_bit(TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG, &transparent_hugepage_flags);
> - set_bit(TRANSPARENT_HUGEPAGE_FLAG, &transparent_hugepage_flags);
> - } else if (sysfs_streq(buf, "madvise")) {
> - clear_bit(TRANSPARENT_HUGEPAGE_FLAG, &transparent_hugepage_flags);
> - set_bit(TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG, &transparent_hugepage_flags);
> - } else if (sysfs_streq(buf, "never")) {
> - clear_bit(TRANSPARENT_HUGEPAGE_FLAG, &transparent_hugepage_flags);
> - clear_bit(TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG, &transparent_hugepage_flags);
> - } else
> - ret = -EINVAL;
> + mode = sysfs_match_string(enabled_mode_strings, buf);
> + if (mode < 0 || mode == ENABLED_INHERIT)
> + return -EINVAL;
The mode == ENABLED_INHERIT check is weird, it reads like 'oh a user CAN specify
this, but if they do we error out', but in reality
/sys/kernel/mm/transparent_hugepage/enabled explicitly outputs only always,
inherit, maddvise.
So, even though it's duplicative, I think it's probably saner to have:
static const char * const anon_enabled_mode_strings[] = {
[ENABLED_ALWAYS] = "always",
[ENABLED_MADVISE] = "madvise",
[ENABLED_INHERIT] = "inherit",
[ENABLED_NEVER] = "never",
};
static const char * const global_enabled_mode_strings[] = {
[ENABLED_ALWAYS] = "always",
[ENABLED_MADVISE] = "madvise",
[ENABLED_NEVER] = "never",
};
Just to make it clear what you're doing.
And if you add something for shmem add one for that too.
With that fixed, feel free to add:
Reviewed-by: Lorenzo Stoakes (Oracle) <ljs@kernel.org>
>
> - if (ret > 0) {
> + if (change_enabled(mode)) {
> int err = start_stop_khugepaged();
> +
> if (err)
> - ret = err;
> + return err;
> + } else {
> + /*
> + * Recalculate watermarks even when the mode didn't
> + * change, as the previous code always called
> + * start_stop_khugepaged() which does this internally.
> + */
> + set_recommended_min_free_kbytes();
> }
> - return ret;
> + return count;
> }
>
> static struct kobj_attribute enabled_attr = __ATTR_RW(enabled);
>
> --
> 2.47.3
>
Cheers, Lorenzo
On Fri, Mar 06, 2026 at 11:39:28AM +0000, Lorenzo Stoakes (Oracle) wrote: > On Thu, Mar 05, 2026 at 06:04:55AM -0800, Breno Leitao wrote: > > + mode = sysfs_match_string(enabled_mode_strings, buf); > > + if (mode < 0 || mode == ENABLED_INHERIT) > > + return -EINVAL; > > The mode == ENABLED_INHERIT check is weird, it reads like 'oh a user CAN specify > this, but if they do we error out', but in reality > /sys/kernel/mm/transparent_hugepage/enabled explicitly outputs only always, > inherit, maddvise. > > So, even though it's duplicative, I think it's probably saner to have: Agree. That would be a better approach. > With that fixed, feel free to add: > > Reviewed-by: Lorenzo Stoakes (Oracle) <ljs@kernel.org> Ack, thanks for the review, --breno
© 2016 - 2026 Red Hat, Inc.