* [PATCH 1/5] prctl: extend PR_SET_THP_DISABLE to optionally exclude VM_HUGEPAGE
2025-07-25 16:22 [PATCH 0/5] prctl: extend PR_SET_THP_DISABLE to only provide THPs when advised Usama Arif
@ 2025-07-25 16:22 ` Usama Arif
2025-07-30 19:31 ` Lorenzo Stoakes
2025-07-25 16:22 ` [PATCH 2/5] mm/huge_memory: convert "tva_flags" to "enum tva_type" for thp_vma_allowable_order*() Usama Arif
` (3 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Usama Arif @ 2025-07-25 16:22 UTC (permalink / raw)
To: Andrew Morton, david, linux-mm
Cc: linux-fsdevel, corbet, rppt, surenb, mhocko, hannes, baohua,
shakeel.butt, riel, ziy, laoar.shao, dev.jain, baolin.wang,
npache, lorenzo.stoakes, Liam.Howlett, ryan.roberts, vbabka,
jannh, Arnd Bergmann, sj, linux-kernel, linux-doc, kernel-team,
Usama Arif, Matthew Wilcox
From: David Hildenbrand <david@redhat.com>
People want to make use of more THPs, for example, moving from
the "never" system policy to "madvise", or from "madvise" to "always".
While this is great news for every THP desperately waiting to get
allocated out there, apparently there are some workloads that require a
bit of care during that transition: individual processes may need to
opt-out from this behavior for various reasons, and this should be
permitted without needing to make all other workloads on the system
similarly opt-out.
The following scenarios are imaginable:
(1) Switch from "none" system policy to "madvise"/"always", but keep THPs
disabled for selected workloads.
(2) Stay at "none" system policy, but enable THPs for selected
workloads, making only these workloads use the "madvise" or "always"
policy.
(3) Switch from "madvise" system policy to "always", but keep the
"madvise" policy for selected workloads: allocate THPs only when
advised.
(4) Stay at "madvise" system policy, but enable THPs even when not advised
for selected workloads -- "always" policy.
Once can emulate (2) through (1), by setting the system policy to
"madvise"/"always" while disabling THPs for all processes that don't want
THPs. It requires configuring all workloads, but that is a user-space
problem to sort out.
(4) can be emulated through (3) in a similar way.
Back when (1) was relevant in the past, as people started enabling THPs,
we added PR_SET_THP_DISABLE, so relevant workloads that were not ready
yet (i.e., used by Redis) were able to just disable THPs completely. Redis
still implements the option to use this interface to disable THPs
completely.
With PR_SET_THP_DISABLE, we added a way to force-disable THPs for a
workload -- a process, including fork+exec'ed process hierarchy.
That essentially made us support (1): simply disable THPs for all workloads
that are not ready for THPs yet, while still enabling THPs system-wide.
The quest for handling (3) and (4) started, but current approaches
(completely new prctl, options to set other policies per process,
alternatives to prctl -- mctrl, cgroup handling) don't look particularly
promising. Likely, the future will use bpf or something similar to
implement better policies, in particular to also make better decisions
about THP sizes to use, but this will certainly take a while as that work
just started.
Long story short: a simple enable/disable is not really suitable for the
future, so we're not willing to add completely new toggles.
While we could emulate (3)+(4) through (1)+(2) by simply disabling THPs
completely for these processes, this is a step backwards, because these
processes can no longer allocate THPs in regions where THPs were
explicitly advised: regions flagged as VM_HUGEPAGE. Apparently, that
imposes a problem for relevant workloads, because "not THPs" is certainly
worse than "THPs only when advised".
Could we simply relax PR_SET_THP_DISABLE, to "disable THPs unless not
explicitly advised by the app through MAD_HUGEPAGE"? *maybe*, but this
would change the documented semantics quite a bit, and the versatility
to use it for debugging purposes, so I am not 100% sure that is what we
want -- although it would certainly be much easier.
So instead, as an easy way forward for (3) and (4), add an option to
make PR_SET_THP_DISABLE disable *less* THPs for a process.
In essence, this patch:
(A) Adds PR_THP_DISABLE_EXCEPT_ADVISED, to be used as a flag in arg3
of prctl(PR_SET_THP_DISABLE) when disabling THPs (arg2 != 0).
prctl(PR_SET_THP_DISABLE, 1, PR_THP_DISABLE_EXCEPT_ADVISED).
(B) Makes prctl(PR_GET_THP_DISABLE) return 3 if
PR_THP_DISABLE_EXCEPT_ADVISED was set while disabling.
Previously, it would return 1 if THPs were disabled completely. Now
it returns the set flags as well: 3 if PR_THP_DISABLE_EXCEPT_ADVISED
was set.
(C) Renames MMF_DISABLE_THP to MMF_DISABLE_THP_COMPLETELY, to express
the semantics clearly.
Fortunately, there are only two instances outside of prctl() code.
(D) Adds MMF_DISABLE_THP_EXCEPT_ADVISED to express "no THP except for VMAs
with VM_HUGEPAGE" -- essentially "thp=madvise" behavior
Fortunately, we only have to extend vma_thp_disabled().
(E) Indicates "THP_enabled: 0" in /proc/pid/status only if THPs are
disabled completely
Only indicating that THPs are disabled when they are really disabled
completely, not only partially.
For now, we don't add another interface to obtained whether THPs
are disabled partially (PR_THP_DISABLE_EXCEPT_ADVISED was set). If
ever required, we could add a new entry.
The documented semantics in the man page for PR_SET_THP_DISABLE
"is inherited by a child created via fork(2) and is preserved across
execve(2)" is maintained. This behavior, for example, allows for
disabling THPs for a workload through the launching process (e.g.,
systemd where we fork() a helper process to then exec()).
For now, MADV_COLLAPSE will *fail* in regions without VM_HUGEPAGE and
VM_NOHUGEPAGE. As MADV_COLLAPSE is a clear advise that user space
thinks a THP is a good idea, we'll enable that separately next
(requiring a bit of cleanup first).
There is currently not way to prevent that a process will not issue
PR_SET_THP_DISABLE itself to re-enable THP. There are not really known
users for re-enabling it, and it's against the purpose of the original
interface. So if ever required, we could investigate just forbidding to
re-enable them, or make this somehow configurable.
Acked-by: Usama Arif <usamaarif642@gmail.com>
Tested-by: Usama Arif <usamaarif642@gmail.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Cc: Zi Yan <ziy@nvidia.com>
Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: "Liam R. Howlett" <Liam.Howlett@oracle.com>
Cc: Nico Pache <npache@redhat.com>
Cc: Ryan Roberts <ryan.roberts@arm.com>
Cc: Dev Jain <dev.jain@arm.com>
Cc: Barry Song <baohua@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Mike Rapoport <rppt@kernel.org>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Usama Arif <usamaarif642@gmail.com>
Cc: SeongJae Park <sj@kernel.org>
Cc: Jann Horn <jannh@google.com>
Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
Cc: Yafang Shao <laoar.shao@gmail.com>
Cc: Matthew Wilcox <willy@infradead.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
At first, I thought of "why not simply relax PR_SET_THP_DISABLE", but I
think there might be real use cases where we want to disable any THPs --
in particular also around debugging THP-related problems, and
"never" not meaning ... "never" anymore ever since we add MADV_COLLAPSE.
PR_SET_THP_DISABLE will also block MADV_COLLAPSE, which can be very
helpful for debugging purposes. Of course, I thought of having a
system-wide config option to modify PR_SET_THP_DISABLE behavior, but
I just don't like the semantics.
"prctl: allow overriding system THP policy to always"[1] proposed
"overriding policies to always", which is just the wrong way around: we
should not add mechanisms to "enable more" when we already have an
interface/mechanism to "disable" them (PR_SET_THP_DISABLE). It all gets
weird otherwise.
"[PATCH 0/6] prctl: introduce PR_SET/GET_THP_POLICY"[2] proposed
setting the default of the VM_HUGEPAGE, which is similarly the wrong way
around I think now.
The ideas explored by Lorenzo to extend process_madvise()[3] and mctrl()[4]
similarly were around the "default for VM_HUGEPAGE" idea, but after the
discussion, I think we should better leave VM_HUGEPAGE untouched.
Happy to hear naming suggestions for "PR_THP_DISABLE_EXCEPT_ADVISED" where
we essentially want to say "leave advised regions alone" -- "keep THP
enabled for advised regions",
The only thing I really dislike about this is using another MMF_* flag,
but well, no way around it -- and seems like we could easily support
more than 32 if we want to (most users already treat it like a proper
bitmap).
I think this here (modifying an existing toggle) is the only prctl()
extension that we might be willing to accept. In general, I agree like
most others, that prctl() is a very bad interface for that -- but
PR_SET_THP_DISABLE is already there and is getting used.
Long-term, I think the answer will be something based on bpf[5]. Maybe
in that context, I there could still be value in easily disabling THPs for
selected workloads (esp. debugging purposes).
Jann raised valid concerns[6] about new flags that are persistent across
exec[6]. As this here is a relaxation to existing PR_SET_THP_DISABLE I
consider it having a similar security risk as our existing
PR_SET_THP_DISABLE, but devil is in the detail.
[1] https://lore.kernel.org/r/20250507141132.2773275-1-usamaarif642@gmail.com
[2] https://lkml.kernel.org/r/20250515133519.2779639-2-usamaarif642@gmail.com
[3] https://lore.kernel.org/r/cover.1747686021.git.lorenzo.stoakes@oracle.com
[4] https://lkml.kernel.org/r/85778a76-7dc8-4ea8-8827-acb45f74ee05@lucifer.local
[5] https://lkml.kernel.org/r/20250608073516.22415-1-laoar.shao@gmail.com
[6] https://lore.kernel.org/r/CAG48ez3-7EnBVEjpdoW7z5K0hX41nLQN5Wb65Vg-1p8DdXRnjg@mail.gmail.com
Signed-off-by: David Hildenbrand <david@redhat.com>
---
Documentation/filesystems/proc.rst | 5 +--
fs/proc/array.c | 2 +-
include/linux/huge_mm.h | 20 ++++++++---
include/linux/mm_types.h | 13 +++----
include/uapi/linux/prctl.h | 10 ++++++
kernel/sys.c | 58 +++++++++++++++++++++++-------
mm/khugepaged.c | 2 +-
7 files changed, 81 insertions(+), 29 deletions(-)
diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
index 2971551b7235..915a3e44bc12 100644
--- a/Documentation/filesystems/proc.rst
+++ b/Documentation/filesystems/proc.rst
@@ -291,8 +291,9 @@ It's slow but very precise.
HugetlbPages size of hugetlb memory portions
CoreDumping process's memory is currently being dumped
(killing the process may lead to a corrupted core)
- THP_enabled process is allowed to use THP (returns 0 when
- PR_SET_THP_DISABLE is set on the process
+ THP_enabled process is allowed to use THP (returns 0 when
+ PR_SET_THP_DISABLE is set on the process to disable
+ THP completely, not just partially)
Threads number of threads
SigQ number of signals queued/max. number for queue
SigPnd bitmap of pending signals for the thread
diff --git a/fs/proc/array.c b/fs/proc/array.c
index d6a0369caa93..c4f91a784104 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -422,7 +422,7 @@ static inline void task_thp_status(struct seq_file *m, struct mm_struct *mm)
bool thp_enabled = IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE);
if (thp_enabled)
- thp_enabled = !test_bit(MMF_DISABLE_THP, &mm->flags);
+ thp_enabled = !test_bit(MMF_DISABLE_THP_COMPLETELY, &mm->flags);
seq_printf(m, "THP_enabled:\t%d\n", thp_enabled);
}
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index 7748489fde1b..71db243a002e 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -318,16 +318,26 @@ struct thpsize {
(transparent_hugepage_flags & \
(1<<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG))
+/*
+ * Check whether THPs are explicitly disabled for this VMA, for example,
+ * through madvise or prctl.
+ */
static inline bool vma_thp_disabled(struct vm_area_struct *vma,
vm_flags_t vm_flags)
{
+ /* Are THPs disabled for this VMA? */
+ if (vm_flags & VM_NOHUGEPAGE)
+ return true;
+ /* Are THPs disabled for all VMAs in the whole process? */
+ if (test_bit(MMF_DISABLE_THP_COMPLETELY, &vma->vm_mm->flags))
+ return true;
/*
- * Explicitly disabled through madvise or prctl, or some
- * architectures may disable THP for some mappings, for
- * example, s390 kvm.
+ * Are THPs disabled only for VMAs where we didn't get an explicit
+ * advise to use them?
*/
- return (vm_flags & VM_NOHUGEPAGE) ||
- test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags);
+ if (vm_flags & VM_HUGEPAGE)
+ return false;
+ return test_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, &vma->vm_mm->flags);
}
static inline bool thp_disabled_by_hw(void)
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 1ec273b06691..123fefaa4b98 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -1743,19 +1743,16 @@ enum {
#define MMF_VM_MERGEABLE 16 /* KSM may merge identical pages */
#define MMF_VM_HUGEPAGE 17 /* set when mm is available for khugepaged */
-/*
- * This one-shot flag is dropped due to necessity of changing exe once again
- * on NFS restore
- */
-//#define MMF_EXE_FILE_CHANGED 18 /* see prctl_set_mm_exe_file() */
+#define MMF_HUGE_ZERO_PAGE 18 /* mm has ever used the global huge zero page */
#define MMF_HAS_UPROBES 19 /* has uprobes */
#define MMF_RECALC_UPROBES 20 /* MMF_HAS_UPROBES can be wrong */
#define MMF_OOM_SKIP 21 /* mm is of no interest for the OOM killer */
#define MMF_UNSTABLE 22 /* mm is unstable for copy_from_user */
-#define MMF_HUGE_ZERO_PAGE 23 /* mm has ever used the global huge zero page */
-#define MMF_DISABLE_THP 24 /* disable THP for all VMAs */
-#define MMF_DISABLE_THP_MASK (1 << MMF_DISABLE_THP)
+#define MMF_DISABLE_THP_EXCEPT_ADVISED 23 /* no THP except when advised (e.g., VM_HUGEPAGE) */
+#define MMF_DISABLE_THP_COMPLETELY 24 /* no THP for all VMAs */
+#define MMF_DISABLE_THP_MASK ((1 << MMF_DISABLE_THP_COMPLETELY) |\
+ (1 << MMF_DISABLE_THP_EXCEPT_ADVISED))
#define MMF_OOM_REAP_QUEUED 25 /* mm was queued for oom_reaper */
#define MMF_MULTIPROCESS 26 /* mm is shared between processes */
/*
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index 43dec6eed559..9c1d6e49b8a9 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -177,7 +177,17 @@ struct prctl_mm_map {
#define PR_GET_TID_ADDRESS 40
+/*
+ * Flags for PR_SET_THP_DISABLE are only applicable when disabling. Bit 0
+ * is reserved, so PR_GET_THP_DISABLE can return "1 | flags", to effectively
+ * return "1" when no flags were specified for PR_SET_THP_DISABLE.
+ */
#define PR_SET_THP_DISABLE 41
+/*
+ * Don't disable THPs when explicitly advised (e.g., MADV_HUGEPAGE /
+ * VM_HUGEPAGE).
+ */
+# define PR_THP_DISABLE_EXCEPT_ADVISED (1 << 1)
#define PR_GET_THP_DISABLE 42
/*
diff --git a/kernel/sys.c b/kernel/sys.c
index b153fb345ada..b87d0acaab0b 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2423,6 +2423,50 @@ static int prctl_get_auxv(void __user *addr, unsigned long len)
return sizeof(mm->saved_auxv);
}
+static int prctl_get_thp_disable(unsigned long arg2, unsigned long arg3,
+ unsigned long arg4, unsigned long arg5)
+{
+ unsigned long *mm_flags = ¤t->mm->flags;
+
+ if (arg2 || arg3 || arg4 || arg5)
+ return -EINVAL;
+
+ if (test_bit(MMF_DISABLE_THP_COMPLETELY, mm_flags))
+ return 1;
+ else if (test_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, mm_flags))
+ return 1 | PR_THP_DISABLE_EXCEPT_ADVISED;
+ return 0;
+}
+
+static int prctl_set_thp_disable(bool thp_disable, unsigned long flags,
+ unsigned long arg4, unsigned long arg5)
+{
+ unsigned long *mm_flags = ¤t->mm->flags;
+
+ if (arg4 || arg5)
+ return -EINVAL;
+
+ /* Flags are only allowed when disabling. */
+ if ((!thp_disable && flags) || (flags & ~PR_THP_DISABLE_EXCEPT_ADVISED))
+ return -EINVAL;
+ if (mmap_write_lock_killable(current->mm))
+ return -EINTR;
+ if (thp_disable) {
+ if (flags & PR_THP_DISABLE_EXCEPT_ADVISED) {
+ clear_bit(MMF_DISABLE_THP_COMPLETELY, mm_flags);
+ set_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, mm_flags);
+ } else {
+ set_bit(MMF_DISABLE_THP_COMPLETELY, mm_flags);
+ clear_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, mm_flags);
+ }
+ } else {
+ clear_bit(MMF_DISABLE_THP_COMPLETELY, mm_flags);
+ clear_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, mm_flags);
+ }
+ mmap_write_unlock(current->mm);
+ return 0;
+}
+
SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
unsigned long, arg4, unsigned long, arg5)
{
@@ -2596,20 +2640,10 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
return -EINVAL;
return task_no_new_privs(current) ? 1 : 0;
case PR_GET_THP_DISABLE:
- if (arg2 || arg3 || arg4 || arg5)
- return -EINVAL;
- error = !!test_bit(MMF_DISABLE_THP, &me->mm->flags);
+ error = prctl_get_thp_disable(arg2, arg3, arg4, arg5);
break;
case PR_SET_THP_DISABLE:
- if (arg3 || arg4 || arg5)
- return -EINVAL;
- if (mmap_write_lock_killable(me->mm))
- return -EINTR;
- if (arg2)
- set_bit(MMF_DISABLE_THP, &me->mm->flags);
- else
- clear_bit(MMF_DISABLE_THP, &me->mm->flags);
- mmap_write_unlock(me->mm);
+ error = prctl_set_thp_disable(arg2, arg3, arg4, arg5);
break;
case PR_MPX_ENABLE_MANAGEMENT:
case PR_MPX_DISABLE_MANAGEMENT:
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 1ff0c7dd2be4..2c9008246785 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -410,7 +410,7 @@ static inline int hpage_collapse_test_exit(struct mm_struct *mm)
static inline int hpage_collapse_test_exit_or_disable(struct mm_struct *mm)
{
return hpage_collapse_test_exit(mm) ||
- test_bit(MMF_DISABLE_THP, &mm->flags);
+ test_bit(MMF_DISABLE_THP_COMPLETELY, &mm->flags);
}
static bool hugepage_pmd_enabled(void)
--
2.47.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 1/5] prctl: extend PR_SET_THP_DISABLE to optionally exclude VM_HUGEPAGE
2025-07-25 16:22 ` [PATCH 1/5] prctl: extend PR_SET_THP_DISABLE to optionally exclude VM_HUGEPAGE Usama Arif
@ 2025-07-30 19:31 ` Lorenzo Stoakes
2025-07-30 19:42 ` Usama Arif
0 siblings, 1 reply; 20+ messages in thread
From: Lorenzo Stoakes @ 2025-07-30 19:31 UTC (permalink / raw)
To: Usama Arif
Cc: Andrew Morton, david, linux-mm, linux-fsdevel, corbet, rppt,
surenb, mhocko, hannes, baohua, shakeel.butt, riel, ziy,
laoar.shao, dev.jain, baolin.wang, npache, Liam.Howlett,
ryan.roberts, vbabka, jannh, Arnd Bergmann, sj, linux-kernel,
linux-doc, kernel-team, Matthew Wilcox
An aside - we should update the man page for this - see
https://man7.org/linux/man-pages/man2/PR_SET_THP_DISABLE.2const.html
This has to be done separately from the series I think.
On Fri, Jul 25, 2025 at 05:22:40PM +0100, Usama Arif wrote:
> From: David Hildenbrand <david@redhat.com>
>
> People want to make use of more THPs, for example, moving from
> the "never" system policy to "madvise", or from "madvise" to "always".
>
> While this is great news for every THP desperately waiting to get
> allocated out there, apparently there are some workloads that require a
> bit of care during that transition: individual processes may need to
> opt-out from this behavior for various reasons, and this should be
> permitted without needing to make all other workloads on the system
> similarly opt-out.
>
> The following scenarios are imaginable:
>
> (1) Switch from "none" system policy to "madvise"/"always", but keep THPs
> disabled for selected workloads.
>
> (2) Stay at "none" system policy, but enable THPs for selected
> workloads, making only these workloads use the "madvise" or "always"
> policy.
>
> (3) Switch from "madvise" system policy to "always", but keep the
> "madvise" policy for selected workloads: allocate THPs only when
> advised.
>
> (4) Stay at "madvise" system policy, but enable THPs even when not advised
> for selected workloads -- "always" policy.
>
> Once can emulate (2) through (1), by setting the system policy to
> "madvise"/"always" while disabling THPs for all processes that don't want
> THPs. It requires configuring all workloads, but that is a user-space
> problem to sort out.
>
> (4) can be emulated through (3) in a similar way.
>
> Back when (1) was relevant in the past, as people started enabling THPs,
> we added PR_SET_THP_DISABLE, so relevant workloads that were not ready
> yet (i.e., used by Redis) were able to just disable THPs completely. Redis
> still implements the option to use this interface to disable THPs
> completely.
>
> With PR_SET_THP_DISABLE, we added a way to force-disable THPs for a
> workload -- a process, including fork+exec'ed process hierarchy.
> That essentially made us support (1): simply disable THPs for all workloads
> that are not ready for THPs yet, while still enabling THPs system-wide.
>
> The quest for handling (3) and (4) started, but current approaches
> (completely new prctl, options to set other policies per process,
> alternatives to prctl -- mctrl, cgroup handling) don't look particularly
> promising. Likely, the future will use bpf or something similar to
> implement better policies, in particular to also make better decisions
> about THP sizes to use, but this will certainly take a while as that work
> just started.
>
> Long story short: a simple enable/disable is not really suitable for the
> future, so we're not willing to add completely new toggles.
>
> While we could emulate (3)+(4) through (1)+(2) by simply disabling THPs
> completely for these processes, this is a step backwards, because these
> processes can no longer allocate THPs in regions where THPs were
> explicitly advised: regions flagged as VM_HUGEPAGE. Apparently, that
> imposes a problem for relevant workloads, because "not THPs" is certainly
> worse than "THPs only when advised".
>
> Could we simply relax PR_SET_THP_DISABLE, to "disable THPs unless not
> explicitly advised by the app through MAD_HUGEPAGE"? *maybe*, but this
> would change the documented semantics quite a bit, and the versatility
> to use it for debugging purposes, so I am not 100% sure that is what we
> want -- although it would certainly be much easier.
>
> So instead, as an easy way forward for (3) and (4), add an option to
> make PR_SET_THP_DISABLE disable *less* THPs for a process.
>
> In essence, this patch:
>
> (A) Adds PR_THP_DISABLE_EXCEPT_ADVISED, to be used as a flag in arg3
> of prctl(PR_SET_THP_DISABLE) when disabling THPs (arg2 != 0).
>
> prctl(PR_SET_THP_DISABLE, 1, PR_THP_DISABLE_EXCEPT_ADVISED).
>
> (B) Makes prctl(PR_GET_THP_DISABLE) return 3 if
> PR_THP_DISABLE_EXCEPT_ADVISED was set while disabling.
>
> Previously, it would return 1 if THPs were disabled completely. Now
> it returns the set flags as well: 3 if PR_THP_DISABLE_EXCEPT_ADVISED
> was set.
>
> (C) Renames MMF_DISABLE_THP to MMF_DISABLE_THP_COMPLETELY, to express
> the semantics clearly.
>
> Fortunately, there are only two instances outside of prctl() code.
>
> (D) Adds MMF_DISABLE_THP_EXCEPT_ADVISED to express "no THP except for VMAs
> with VM_HUGEPAGE" -- essentially "thp=madvise" behavior
>
> Fortunately, we only have to extend vma_thp_disabled().
>
> (E) Indicates "THP_enabled: 0" in /proc/pid/status only if THPs are
> disabled completely
>
> Only indicating that THPs are disabled when they are really disabled
> completely, not only partially.
>
> For now, we don't add another interface to obtained whether THPs
> are disabled partially (PR_THP_DISABLE_EXCEPT_ADVISED was set). If
> ever required, we could add a new entry.
>
> The documented semantics in the man page for PR_SET_THP_DISABLE
> "is inherited by a child created via fork(2) and is preserved across
> execve(2)" is maintained. This behavior, for example, allows for
> disabling THPs for a workload through the launching process (e.g.,
> systemd where we fork() a helper process to then exec()).
>
> For now, MADV_COLLAPSE will *fail* in regions without VM_HUGEPAGE and
> VM_NOHUGEPAGE. As MADV_COLLAPSE is a clear advise that user space
> thinks a THP is a good idea, we'll enable that separately next
> (requiring a bit of cleanup first).
>
> There is currently not way to prevent that a process will not issue
> PR_SET_THP_DISABLE itself to re-enable THP. There are not really known
> users for re-enabling it, and it's against the purpose of the original
> interface. So if ever required, we could investigate just forbidding to
> re-enable them, or make this somehow configurable.
>
> Acked-by: Usama Arif <usamaarif642@gmail.com>
> Tested-by: Usama Arif <usamaarif642@gmail.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: "Liam R. Howlett" <Liam.Howlett@oracle.com>
> Cc: Nico Pache <npache@redhat.com>
> Cc: Ryan Roberts <ryan.roberts@arm.com>
> Cc: Dev Jain <dev.jain@arm.com>
> Cc: Barry Song <baohua@kernel.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Mike Rapoport <rppt@kernel.org>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Usama Arif <usamaarif642@gmail.com>
> Cc: SeongJae Park <sj@kernel.org>
> Cc: Jann Horn <jannh@google.com>
> Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
> Cc: Yafang Shao <laoar.shao@gmail.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>
>
> ---
>
> At first, I thought of "why not simply relax PR_SET_THP_DISABLE", but I
> think there might be real use cases where we want to disable any THPs --
> in particular also around debugging THP-related problems, and
> "never" not meaning ... "never" anymore ever since we add MADV_COLLAPSE.
> PR_SET_THP_DISABLE will also block MADV_COLLAPSE, which can be very
> helpful for debugging purposes. Of course, I thought of having a
> system-wide config option to modify PR_SET_THP_DISABLE behavior, but
> I just don't like the semantics.
>
> "prctl: allow overriding system THP policy to always"[1] proposed
> "overriding policies to always", which is just the wrong way around: we
> should not add mechanisms to "enable more" when we already have an
> interface/mechanism to "disable" them (PR_SET_THP_DISABLE). It all gets
> weird otherwise.
>
> "[PATCH 0/6] prctl: introduce PR_SET/GET_THP_POLICY"[2] proposed
> setting the default of the VM_HUGEPAGE, which is similarly the wrong way
> around I think now.
>
> The ideas explored by Lorenzo to extend process_madvise()[3] and mctrl()[4]
> similarly were around the "default for VM_HUGEPAGE" idea, but after the
> discussion, I think we should better leave VM_HUGEPAGE untouched.
>
> Happy to hear naming suggestions for "PR_THP_DISABLE_EXCEPT_ADVISED" where
> we essentially want to say "leave advised regions alone" -- "keep THP
> enabled for advised regions",
>
> The only thing I really dislike about this is using another MMF_* flag,
> but well, no way around it -- and seems like we could easily support
> more than 32 if we want to (most users already treat it like a proper
> bitmap).
>
> I think this here (modifying an existing toggle) is the only prctl()
> extension that we might be willing to accept. In general, I agree like
> most others, that prctl() is a very bad interface for that -- but
> PR_SET_THP_DISABLE is already there and is getting used.
>
> Long-term, I think the answer will be something based on bpf[5]. Maybe
> in that context, I there could still be value in easily disabling THPs for
> selected workloads (esp. debugging purposes).
>
> Jann raised valid concerns[6] about new flags that are persistent across
> exec[6]. As this here is a relaxation to existing PR_SET_THP_DISABLE I
> consider it having a similar security risk as our existing
> PR_SET_THP_DISABLE, but devil is in the detail.
>
> [1] https://lore.kernel.org/r/20250507141132.2773275-1-usamaarif642@gmail.com
> [2] https://lkml.kernel.org/r/20250515133519.2779639-2-usamaarif642@gmail.com
> [3] https://lore.kernel.org/r/cover.1747686021.git.lorenzo.stoakes@oracle.com
> [4] https://lkml.kernel.org/r/85778a76-7dc8-4ea8-8827-acb45f74ee05@lucifer.local
> [5] https://lkml.kernel.org/r/20250608073516.22415-1-laoar.shao@gmail.com
> [6] https://lore.kernel.org/r/CAG48ez3-7EnBVEjpdoW7z5K0hX41nLQN5Wb65Vg-1p8DdXRnjg@mail.gmail.com
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> Documentation/filesystems/proc.rst | 5 +--
> fs/proc/array.c | 2 +-
> include/linux/huge_mm.h | 20 ++++++++---
> include/linux/mm_types.h | 13 +++----
> include/uapi/linux/prctl.h | 10 ++++++
> kernel/sys.c | 58 +++++++++++++++++++++++-------
> mm/khugepaged.c | 2 +-
> 7 files changed, 81 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> index 2971551b7235..915a3e44bc12 100644
> --- a/Documentation/filesystems/proc.rst
> +++ b/Documentation/filesystems/proc.rst
> @@ -291,8 +291,9 @@ It's slow but very precise.
> HugetlbPages size of hugetlb memory portions
> CoreDumping process's memory is currently being dumped
> (killing the process may lead to a corrupted core)
> - THP_enabled process is allowed to use THP (returns 0 when
> - PR_SET_THP_DISABLE is set on the process
> + THP_enabled process is allowed to use THP (returns 0 when
> + PR_SET_THP_DISABLE is set on the process to disable
> + THP completely, not just partially)
> Threads number of threads
> SigQ number of signals queued/max. number for queue
> SigPnd bitmap of pending signals for the thread
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index d6a0369caa93..c4f91a784104 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -422,7 +422,7 @@ static inline void task_thp_status(struct seq_file *m, struct mm_struct *mm)
> bool thp_enabled = IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE);
>
> if (thp_enabled)
> - thp_enabled = !test_bit(MMF_DISABLE_THP, &mm->flags);
> + thp_enabled = !test_bit(MMF_DISABLE_THP_COMPLETELY, &mm->flags);
> seq_printf(m, "THP_enabled:\t%d\n", thp_enabled);
> }
>
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index 7748489fde1b..71db243a002e 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -318,16 +318,26 @@ struct thpsize {
> (transparent_hugepage_flags & \
> (1<<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG))
>
> +/*
> + * Check whether THPs are explicitly disabled for this VMA, for example,
> + * through madvise or prctl.
> + */
> static inline bool vma_thp_disabled(struct vm_area_struct *vma,
> vm_flags_t vm_flags)
> {
> + /* Are THPs disabled for this VMA? */
> + if (vm_flags & VM_NOHUGEPAGE)
> + return true;
> + /* Are THPs disabled for all VMAs in the whole process? */
> + if (test_bit(MMF_DISABLE_THP_COMPLETELY, &vma->vm_mm->flags))
> + return true;
> /*
> - * Explicitly disabled through madvise or prctl, or some
> - * architectures may disable THP for some mappings, for
> - * example, s390 kvm.
> + * Are THPs disabled only for VMAs where we didn't get an explicit
> + * advise to use them?
> */
> - return (vm_flags & VM_NOHUGEPAGE) ||
> - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags);
> + if (vm_flags & VM_HUGEPAGE)
> + return false;
Hm is this correct? This means that VM_NOHUGEPAGE no longer results in THP being
disabled here no?
> + return test_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, &vma->vm_mm->flags);
> }
>
> static inline bool thp_disabled_by_hw(void)
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 1ec273b06691..123fefaa4b98 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -1743,19 +1743,16 @@ enum {
> #define MMF_VM_MERGEABLE 16 /* KSM may merge identical pages */
> #define MMF_VM_HUGEPAGE 17 /* set when mm is available for khugepaged */
>
> -/*
> - * This one-shot flag is dropped due to necessity of changing exe once again
> - * on NFS restore
> - */
> -//#define MMF_EXE_FILE_CHANGED 18 /* see prctl_set_mm_exe_file() */
> +#define MMF_HUGE_ZERO_PAGE 18 /* mm has ever used the global huge zero page */
>
> #define MMF_HAS_UPROBES 19 /* has uprobes */
> #define MMF_RECALC_UPROBES 20 /* MMF_HAS_UPROBES can be wrong */
> #define MMF_OOM_SKIP 21 /* mm is of no interest for the OOM killer */
> #define MMF_UNSTABLE 22 /* mm is unstable for copy_from_user */
> -#define MMF_HUGE_ZERO_PAGE 23 /* mm has ever used the global huge zero page */
> -#define MMF_DISABLE_THP 24 /* disable THP for all VMAs */
> -#define MMF_DISABLE_THP_MASK (1 << MMF_DISABLE_THP)
> +#define MMF_DISABLE_THP_EXCEPT_ADVISED 23 /* no THP except when advised (e.g., VM_HUGEPAGE) */
> +#define MMF_DISABLE_THP_COMPLETELY 24 /* no THP for all VMAs */
It's almost a bit weird to have these as separate flags, since they're distinct
(of course, I don't think there's necessarily another way).
Though this makes me think maybe in future we can have a new mode where both
enabled == something else :P
But perhaps I've been infected with 'bit packing' disease.
Anyway as discussed in the THP meeting, I'm going to be (hopefully!) making
the mm flags a bitmap soon so we'll get more flags available.
> +#define MMF_DISABLE_THP_MASK ((1 << MMF_DISABLE_THP_COMPLETELY) |\
> + (1 << MMF_DISABLE_THP_EXCEPT_ADVISED))
> #define MMF_OOM_REAP_QUEUED 25 /* mm was queued for oom_reaper */
> #define MMF_MULTIPROCESS 26 /* mm is shared between processes */
> /*
> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> index 43dec6eed559..9c1d6e49b8a9 100644
> --- a/include/uapi/linux/prctl.h
> +++ b/include/uapi/linux/prctl.h
> @@ -177,7 +177,17 @@ struct prctl_mm_map {
>
> #define PR_GET_TID_ADDRESS 40
>
> +/*
> + * Flags for PR_SET_THP_DISABLE are only applicable when disabling. Bit 0
> + * is reserved, so PR_GET_THP_DISABLE can return "1 | flags", to effectively
> + * return "1" when no flags were specified for PR_SET_THP_DISABLE.
> + */
> #define PR_SET_THP_DISABLE 41
> +/*
> + * Don't disable THPs when explicitly advised (e.g., MADV_HUGEPAGE /
> + * VM_HUGEPAGE).
> + */
> +# define PR_THP_DISABLE_EXCEPT_ADVISED (1 << 1)
NO space after # please.
> #define PR_GET_THP_DISABLE 42
>
> /*
> diff --git a/kernel/sys.c b/kernel/sys.c
> index b153fb345ada..b87d0acaab0b 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2423,6 +2423,50 @@ static int prctl_get_auxv(void __user *addr, unsigned long len)
> return sizeof(mm->saved_auxv);
> }
>
> +static int prctl_get_thp_disable(unsigned long arg2, unsigned long arg3,
> + unsigned long arg4, unsigned long arg5)
> +{
> + unsigned long *mm_flags = ¤t->mm->flags;
> +
> + if (arg2 || arg3 || arg4 || arg5)
> + return -EINVAL;
> +
Can we have a comment here about what we're doing below re: the return
value?
> + if (test_bit(MMF_DISABLE_THP_COMPLETELY, mm_flags))
> + return 1;
> + else if (test_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, mm_flags))
> + return 1 | PR_THP_DISABLE_EXCEPT_ADVISED;
> + return 0;
> +}
> +
> +static int prctl_set_thp_disable(bool thp_disable, unsigned long flags,
> + unsigned long arg4, unsigned long arg5)
> +{
> + unsigned long *mm_flags = ¤t->mm->flags;
> +
> + if (arg4 || arg5)
> + return -EINVAL;
> +
> + /* Flags are only allowed when disabling. */
> + if ((!thp_disable && flags) || (flags & ~PR_THP_DISABLE_EXCEPT_ADVISED))
> + return -EINVAL;
> + if (mmap_write_lock_killable(current->mm))
> + return -EINTR;
> + if (thp_disable) {
> + if (flags & PR_THP_DISABLE_EXCEPT_ADVISED) {
> + clear_bit(MMF_DISABLE_THP_COMPLETELY, mm_flags);
> + set_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, mm_flags);
> + } else {
> + set_bit(MMF_DISABLE_THP_COMPLETELY, mm_flags);
> + clear_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, mm_flags);
> + }
> + } else {
> + clear_bit(MMF_DISABLE_THP_COMPLETELY, mm_flags);
> + clear_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, mm_flags);
> + }
> + mmap_write_unlock(current->mm);
> + return 0;
> +}
> +
> SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> unsigned long, arg4, unsigned long, arg5)
> {
> @@ -2596,20 +2640,10 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> return -EINVAL;
> return task_no_new_privs(current) ? 1 : 0;
> case PR_GET_THP_DISABLE:
> - if (arg2 || arg3 || arg4 || arg5)
> - return -EINVAL;
> - error = !!test_bit(MMF_DISABLE_THP, &me->mm->flags);
> + error = prctl_get_thp_disable(arg2, arg3, arg4, arg5);
> break;
> case PR_SET_THP_DISABLE:
> - if (arg3 || arg4 || arg5)
> - return -EINVAL;
> - if (mmap_write_lock_killable(me->mm))
> - return -EINTR;
> - if (arg2)
> - set_bit(MMF_DISABLE_THP, &me->mm->flags);
> - else
> - clear_bit(MMF_DISABLE_THP, &me->mm->flags);
> - mmap_write_unlock(me->mm);
> + error = prctl_set_thp_disable(arg2, arg3, arg4, arg5);
> break;
> case PR_MPX_ENABLE_MANAGEMENT:
> case PR_MPX_DISABLE_MANAGEMENT:
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 1ff0c7dd2be4..2c9008246785 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -410,7 +410,7 @@ static inline int hpage_collapse_test_exit(struct mm_struct *mm)
> static inline int hpage_collapse_test_exit_or_disable(struct mm_struct *mm)
> {
> return hpage_collapse_test_exit(mm) ||
> - test_bit(MMF_DISABLE_THP, &mm->flags);
> + test_bit(MMF_DISABLE_THP_COMPLETELY, &mm->flags);
> }
>
> static bool hugepage_pmd_enabled(void)
> --
> 2.47.3
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/5] prctl: extend PR_SET_THP_DISABLE to optionally exclude VM_HUGEPAGE
2025-07-30 19:31 ` Lorenzo Stoakes
@ 2025-07-30 19:42 ` Usama Arif
2025-07-31 8:29 ` Lorenzo Stoakes
0 siblings, 1 reply; 20+ messages in thread
From: Usama Arif @ 2025-07-30 19:42 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, david, linux-mm, linux-fsdevel, corbet, rppt,
surenb, mhocko, hannes, baohua, shakeel.butt, riel, ziy,
laoar.shao, dev.jain, baolin.wang, npache, Liam.Howlett,
ryan.roberts, vbabka, jannh, Arnd Bergmann, sj, linux-kernel,
linux-doc, kernel-team, Matthew Wilcox
On 30/07/2025 20:31, Lorenzo Stoakes wrote:
> An aside - we should update the man page for this - see
> https://man7.org/linux/man-pages/man2/PR_SET_THP_DISABLE.2const.html
>
> This has to be done separately from the series I think.
>
> On Fri, Jul 25, 2025 at 05:22:40PM +0100, Usama Arif wrote:
>> From: David Hildenbrand <david@redhat.com>
>>
>> People want to make use of more THPs, for example, moving from
>> the "never" system policy to "madvise", or from "madvise" to "always".
>>
>> While this is great news for every THP desperately waiting to get
>> allocated out there, apparently there are some workloads that require a
>> bit of care during that transition: individual processes may need to
>> opt-out from this behavior for various reasons, and this should be
>> permitted without needing to make all other workloads on the system
>> similarly opt-out.
>>
>> The following scenarios are imaginable:
>>
>> (1) Switch from "none" system policy to "madvise"/"always", but keep THPs
>> disabled for selected workloads.
>>
>> (2) Stay at "none" system policy, but enable THPs for selected
>> workloads, making only these workloads use the "madvise" or "always"
>> policy.
>>
>> (3) Switch from "madvise" system policy to "always", but keep the
>> "madvise" policy for selected workloads: allocate THPs only when
>> advised.
>>
>> (4) Stay at "madvise" system policy, but enable THPs even when not advised
>> for selected workloads -- "always" policy.
>>
>> Once can emulate (2) through (1), by setting the system policy to
>> "madvise"/"always" while disabling THPs for all processes that don't want
>> THPs. It requires configuring all workloads, but that is a user-space
>> problem to sort out.
>>
>> (4) can be emulated through (3) in a similar way.
>>
>> Back when (1) was relevant in the past, as people started enabling THPs,
>> we added PR_SET_THP_DISABLE, so relevant workloads that were not ready
>> yet (i.e., used by Redis) were able to just disable THPs completely. Redis
>> still implements the option to use this interface to disable THPs
>> completely.
>>
>> With PR_SET_THP_DISABLE, we added a way to force-disable THPs for a
>> workload -- a process, including fork+exec'ed process hierarchy.
>> That essentially made us support (1): simply disable THPs for all workloads
>> that are not ready for THPs yet, while still enabling THPs system-wide.
>>
>> The quest for handling (3) and (4) started, but current approaches
>> (completely new prctl, options to set other policies per process,
>> alternatives to prctl -- mctrl, cgroup handling) don't look particularly
>> promising. Likely, the future will use bpf or something similar to
>> implement better policies, in particular to also make better decisions
>> about THP sizes to use, but this will certainly take a while as that work
>> just started.
>>
>> Long story short: a simple enable/disable is not really suitable for the
>> future, so we're not willing to add completely new toggles.
>>
>> While we could emulate (3)+(4) through (1)+(2) by simply disabling THPs
>> completely for these processes, this is a step backwards, because these
>> processes can no longer allocate THPs in regions where THPs were
>> explicitly advised: regions flagged as VM_HUGEPAGE. Apparently, that
>> imposes a problem for relevant workloads, because "not THPs" is certainly
>> worse than "THPs only when advised".
>>
>> Could we simply relax PR_SET_THP_DISABLE, to "disable THPs unless not
>> explicitly advised by the app through MAD_HUGEPAGE"? *maybe*, but this
>> would change the documented semantics quite a bit, and the versatility
>> to use it for debugging purposes, so I am not 100% sure that is what we
>> want -- although it would certainly be much easier.
>>
>> So instead, as an easy way forward for (3) and (4), add an option to
>> make PR_SET_THP_DISABLE disable *less* THPs for a process.
>>
>> In essence, this patch:
>>
>> (A) Adds PR_THP_DISABLE_EXCEPT_ADVISED, to be used as a flag in arg3
>> of prctl(PR_SET_THP_DISABLE) when disabling THPs (arg2 != 0).
>>
>> prctl(PR_SET_THP_DISABLE, 1, PR_THP_DISABLE_EXCEPT_ADVISED).
>>
>> (B) Makes prctl(PR_GET_THP_DISABLE) return 3 if
>> PR_THP_DISABLE_EXCEPT_ADVISED was set while disabling.
>>
>> Previously, it would return 1 if THPs were disabled completely. Now
>> it returns the set flags as well: 3 if PR_THP_DISABLE_EXCEPT_ADVISED
>> was set.
>>
>> (C) Renames MMF_DISABLE_THP to MMF_DISABLE_THP_COMPLETELY, to express
>> the semantics clearly.
>>
>> Fortunately, there are only two instances outside of prctl() code.
>>
>> (D) Adds MMF_DISABLE_THP_EXCEPT_ADVISED to express "no THP except for VMAs
>> with VM_HUGEPAGE" -- essentially "thp=madvise" behavior
>>
>> Fortunately, we only have to extend vma_thp_disabled().
>>
>> (E) Indicates "THP_enabled: 0" in /proc/pid/status only if THPs are
>> disabled completely
>>
>> Only indicating that THPs are disabled when they are really disabled
>> completely, not only partially.
>>
>> For now, we don't add another interface to obtained whether THPs
>> are disabled partially (PR_THP_DISABLE_EXCEPT_ADVISED was set). If
>> ever required, we could add a new entry.
>>
>> The documented semantics in the man page for PR_SET_THP_DISABLE
>> "is inherited by a child created via fork(2) and is preserved across
>> execve(2)" is maintained. This behavior, for example, allows for
>> disabling THPs for a workload through the launching process (e.g.,
>> systemd where we fork() a helper process to then exec()).
>>
>> For now, MADV_COLLAPSE will *fail* in regions without VM_HUGEPAGE and
>> VM_NOHUGEPAGE. As MADV_COLLAPSE is a clear advise that user space
>> thinks a THP is a good idea, we'll enable that separately next
>> (requiring a bit of cleanup first).
>>
>> There is currently not way to prevent that a process will not issue
>> PR_SET_THP_DISABLE itself to re-enable THP. There are not really known
>> users for re-enabling it, and it's against the purpose of the original
>> interface. So if ever required, we could investigate just forbidding to
>> re-enable them, or make this somehow configurable.
>>
>> Acked-by: Usama Arif <usamaarif642@gmail.com>
>> Tested-by: Usama Arif <usamaarif642@gmail.com>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>> Cc: Zi Yan <ziy@nvidia.com>
>> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
>> Cc: "Liam R. Howlett" <Liam.Howlett@oracle.com>
>> Cc: Nico Pache <npache@redhat.com>
>> Cc: Ryan Roberts <ryan.roberts@arm.com>
>> Cc: Dev Jain <dev.jain@arm.com>
>> Cc: Barry Song <baohua@kernel.org>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Mike Rapoport <rppt@kernel.org>
>> Cc: Suren Baghdasaryan <surenb@google.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Usama Arif <usamaarif642@gmail.com>
>> Cc: SeongJae Park <sj@kernel.org>
>> Cc: Jann Horn <jannh@google.com>
>> Cc: Liam R. Howlett <Liam.Howlett@oracle.com>
>> Cc: Yafang Shao <laoar.shao@gmail.com>
>> Cc: Matthew Wilcox <willy@infradead.org>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>
>> ---
>>
>> At first, I thought of "why not simply relax PR_SET_THP_DISABLE", but I
>> think there might be real use cases where we want to disable any THPs --
>> in particular also around debugging THP-related problems, and
>> "never" not meaning ... "never" anymore ever since we add MADV_COLLAPSE.
>> PR_SET_THP_DISABLE will also block MADV_COLLAPSE, which can be very
>> helpful for debugging purposes. Of course, I thought of having a
>> system-wide config option to modify PR_SET_THP_DISABLE behavior, but
>> I just don't like the semantics.
>>
>> "prctl: allow overriding system THP policy to always"[1] proposed
>> "overriding policies to always", which is just the wrong way around: we
>> should not add mechanisms to "enable more" when we already have an
>> interface/mechanism to "disable" them (PR_SET_THP_DISABLE). It all gets
>> weird otherwise.
>>
>> "[PATCH 0/6] prctl: introduce PR_SET/GET_THP_POLICY"[2] proposed
>> setting the default of the VM_HUGEPAGE, which is similarly the wrong way
>> around I think now.
>>
>> The ideas explored by Lorenzo to extend process_madvise()[3] and mctrl()[4]
>> similarly were around the "default for VM_HUGEPAGE" idea, but after the
>> discussion, I think we should better leave VM_HUGEPAGE untouched.
>>
>> Happy to hear naming suggestions for "PR_THP_DISABLE_EXCEPT_ADVISED" where
>> we essentially want to say "leave advised regions alone" -- "keep THP
>> enabled for advised regions",
>>
>> The only thing I really dislike about this is using another MMF_* flag,
>> but well, no way around it -- and seems like we could easily support
>> more than 32 if we want to (most users already treat it like a proper
>> bitmap).
>>
>> I think this here (modifying an existing toggle) is the only prctl()
>> extension that we might be willing to accept. In general, I agree like
>> most others, that prctl() is a very bad interface for that -- but
>> PR_SET_THP_DISABLE is already there and is getting used.
>>
>> Long-term, I think the answer will be something based on bpf[5]. Maybe
>> in that context, I there could still be value in easily disabling THPs for
>> selected workloads (esp. debugging purposes).
>>
>> Jann raised valid concerns[6] about new flags that are persistent across
>> exec[6]. As this here is a relaxation to existing PR_SET_THP_DISABLE I
>> consider it having a similar security risk as our existing
>> PR_SET_THP_DISABLE, but devil is in the detail.
>>
>> [1] https://lore.kernel.org/r/20250507141132.2773275-1-usamaarif642@gmail.com
>> [2] https://lkml.kernel.org/r/20250515133519.2779639-2-usamaarif642@gmail.com
>> [3] https://lore.kernel.org/r/cover.1747686021.git.lorenzo.stoakes@oracle.com
>> [4] https://lkml.kernel.org/r/85778a76-7dc8-4ea8-8827-acb45f74ee05@lucifer.local
>> [5] https://lkml.kernel.org/r/20250608073516.22415-1-laoar.shao@gmail.com
>> [6] https://lore.kernel.org/r/CAG48ez3-7EnBVEjpdoW7z5K0hX41nLQN5Wb65Vg-1p8DdXRnjg@mail.gmail.com
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> Documentation/filesystems/proc.rst | 5 +--
>> fs/proc/array.c | 2 +-
>> include/linux/huge_mm.h | 20 ++++++++---
>> include/linux/mm_types.h | 13 +++----
>> include/uapi/linux/prctl.h | 10 ++++++
>> kernel/sys.c | 58 +++++++++++++++++++++++-------
>> mm/khugepaged.c | 2 +-
>> 7 files changed, 81 insertions(+), 29 deletions(-)
>>
>> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
>> index 2971551b7235..915a3e44bc12 100644
>> --- a/Documentation/filesystems/proc.rst
>> +++ b/Documentation/filesystems/proc.rst
>> @@ -291,8 +291,9 @@ It's slow but very precise.
>> HugetlbPages size of hugetlb memory portions
>> CoreDumping process's memory is currently being dumped
>> (killing the process may lead to a corrupted core)
>> - THP_enabled process is allowed to use THP (returns 0 when
>> - PR_SET_THP_DISABLE is set on the process
>> + THP_enabled process is allowed to use THP (returns 0 when
>> + PR_SET_THP_DISABLE is set on the process to disable
>> + THP completely, not just partially)
>> Threads number of threads
>> SigQ number of signals queued/max. number for queue
>> SigPnd bitmap of pending signals for the thread
>> diff --git a/fs/proc/array.c b/fs/proc/array.c
>> index d6a0369caa93..c4f91a784104 100644
>> --- a/fs/proc/array.c
>> +++ b/fs/proc/array.c
>> @@ -422,7 +422,7 @@ static inline void task_thp_status(struct seq_file *m, struct mm_struct *mm)
>> bool thp_enabled = IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE);
>>
>> if (thp_enabled)
>> - thp_enabled = !test_bit(MMF_DISABLE_THP, &mm->flags);
>> + thp_enabled = !test_bit(MMF_DISABLE_THP_COMPLETELY, &mm->flags);
>> seq_printf(m, "THP_enabled:\t%d\n", thp_enabled);
>> }
>>
>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>> index 7748489fde1b..71db243a002e 100644
>> --- a/include/linux/huge_mm.h
>> +++ b/include/linux/huge_mm.h
>> @@ -318,16 +318,26 @@ struct thpsize {
>> (transparent_hugepage_flags & \
>> (1<<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG))
>>
>> +/*
>> + * Check whether THPs are explicitly disabled for this VMA, for example,
>> + * through madvise or prctl.
>> + */
>> static inline bool vma_thp_disabled(struct vm_area_struct *vma,
>> vm_flags_t vm_flags)
>> {
>> + /* Are THPs disabled for this VMA? */
>> + if (vm_flags & VM_NOHUGEPAGE)
>> + return true;
VM_NOHUGEPAGE will cause the THP being disabled here.
>> + /* Are THPs disabled for all VMAs in the whole process? */
>> + if (test_bit(MMF_DISABLE_THP_COMPLETELY, &vma->vm_mm->flags))
>> + return true;
>> /*
>> - * Explicitly disabled through madvise or prctl, or some
>> - * architectures may disable THP for some mappings, for
>> - * example, s390 kvm.
>> + * Are THPs disabled only for VMAs where we didn't get an explicit
>> + * advise to use them?
>> */
>> - return (vm_flags & VM_NOHUGEPAGE) ||
>> - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags);
>> + if (vm_flags & VM_HUGEPAGE)
>> + return false;
>
> Hm is this correct? This means that VM_NOHUGEPAGE no longer results in THP being
> disabled here no?
Lorenzo, pointed to VM_NOHUGEPAGE case above.>
>> + return test_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, &vma->vm_mm->flags);
>> }
>>
>> static inline bool thp_disabled_by_hw(void)
>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>> index 1ec273b06691..123fefaa4b98 100644
>> --- a/include/linux/mm_types.h
>> +++ b/include/linux/mm_types.h
>> @@ -1743,19 +1743,16 @@ enum {
>> #define MMF_VM_MERGEABLE 16 /* KSM may merge identical pages */
>> #define MMF_VM_HUGEPAGE 17 /* set when mm is available for khugepaged */
>>
>> -/*
>> - * This one-shot flag is dropped due to necessity of changing exe once again
>> - * on NFS restore
>> - */
>> -//#define MMF_EXE_FILE_CHANGED 18 /* see prctl_set_mm_exe_file() */
>> +#define MMF_HUGE_ZERO_PAGE 18 /* mm has ever used the global huge zero page */
>>
>> #define MMF_HAS_UPROBES 19 /* has uprobes */
>> #define MMF_RECALC_UPROBES 20 /* MMF_HAS_UPROBES can be wrong */
>> #define MMF_OOM_SKIP 21 /* mm is of no interest for the OOM killer */
>> #define MMF_UNSTABLE 22 /* mm is unstable for copy_from_user */
>> -#define MMF_HUGE_ZERO_PAGE 23 /* mm has ever used the global huge zero page */
>> -#define MMF_DISABLE_THP 24 /* disable THP for all VMAs */
>> -#define MMF_DISABLE_THP_MASK (1 << MMF_DISABLE_THP)
>> +#define MMF_DISABLE_THP_EXCEPT_ADVISED 23 /* no THP except when advised (e.g., VM_HUGEPAGE) */
>> +#define MMF_DISABLE_THP_COMPLETELY 24 /* no THP for all VMAs */
>
> It's almost a bit weird to have these as separate flags, since they're distinct
> (of course, I don't think there's necessarily another way).
>
> Though this makes me think maybe in future we can have a new mode where both
> enabled == something else :P
>
> But perhaps I've been infected with 'bit packing' disease.
>
> Anyway as discussed in the THP meeting, I'm going to be (hopefully!) making
> the mm flags a bitmap soon so we'll get more flags available.
>
>> +#define MMF_DISABLE_THP_MASK ((1 << MMF_DISABLE_THP_COMPLETELY) |\
>> + (1 << MMF_DISABLE_THP_EXCEPT_ADVISED))
>> #define MMF_OOM_REAP_QUEUED 25 /* mm was queued for oom_reaper */
>> #define MMF_MULTIPROCESS 26 /* mm is shared between processes */
>> /*
>> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
>> index 43dec6eed559..9c1d6e49b8a9 100644
>> --- a/include/uapi/linux/prctl.h
>> +++ b/include/uapi/linux/prctl.h
>> @@ -177,7 +177,17 @@ struct prctl_mm_map {
>>
>> #define PR_GET_TID_ADDRESS 40
>>
>> +/*
>> + * Flags for PR_SET_THP_DISABLE are only applicable when disabling. Bit 0
>> + * is reserved, so PR_GET_THP_DISABLE can return "1 | flags", to effectively
>> + * return "1" when no flags were specified for PR_SET_THP_DISABLE.
>> + */
>> #define PR_SET_THP_DISABLE 41
>> +/*
>> + * Don't disable THPs when explicitly advised (e.g., MADV_HUGEPAGE /
>> + * VM_HUGEPAGE).
>> + */
>> +# define PR_THP_DISABLE_EXCEPT_ADVISED (1 << 1)
>
> NO space after # please.
>
I think this is following the file convention, the space is there in other flags
all over this file. I dont like the space as well.
>> #define PR_GET_THP_DISABLE 42
>>
>> /*
>> diff --git a/kernel/sys.c b/kernel/sys.c
>> index b153fb345ada..b87d0acaab0b 100644
>> --- a/kernel/sys.c
>> +++ b/kernel/sys.c
>> @@ -2423,6 +2423,50 @@ static int prctl_get_auxv(void __user *addr, unsigned long len)
>> return sizeof(mm->saved_auxv);
>> }
>>
>> +static int prctl_get_thp_disable(unsigned long arg2, unsigned long arg3,
>> + unsigned long arg4, unsigned long arg5)
>> +{
>> + unsigned long *mm_flags = ¤t->mm->flags;
>> +
>> + if (arg2 || arg3 || arg4 || arg5)
>> + return -EINVAL;
>> +
>
> Can we have a comment here about what we're doing below re: the return
> value?
>
Do you mean add returning 1 for MMF_DISABLE_THP_COMPLETELY and 3 for MMF_DISABLE_THP_EXCEPT_ADVISED?
I will add it in next revision.
>> + if (test_bit(MMF_DISABLE_THP_COMPLETELY, mm_flags))
>> + return 1;
>> + else if (test_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, mm_flags))
>> + return 1 | PR_THP_DISABLE_EXCEPT_ADVISED;
>> + return 0;
>> +}
>> +
>> +static int prctl_set_thp_disable(bool thp_disable, unsigned long flags,
>> + unsigned long arg4, unsigned long arg5)
>> +{
>> + unsigned long *mm_flags = ¤t->mm->flags;
>> +
>> + if (arg4 || arg5)
>> + return -EINVAL;
>> +
>> + /* Flags are only allowed when disabling. */
>> + if ((!thp_disable && flags) || (flags & ~PR_THP_DISABLE_EXCEPT_ADVISED))
>> + return -EINVAL;
>> + if (mmap_write_lock_killable(current->mm))
>> + return -EINTR;
>> + if (thp_disable) {
>> + if (flags & PR_THP_DISABLE_EXCEPT_ADVISED) {
>> + clear_bit(MMF_DISABLE_THP_COMPLETELY, mm_flags);
>> + set_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, mm_flags);
>> + } else {
>> + set_bit(MMF_DISABLE_THP_COMPLETELY, mm_flags);
>> + clear_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, mm_flags);
>> + }
>> + } else {
>> + clear_bit(MMF_DISABLE_THP_COMPLETELY, mm_flags);
>> + clear_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, mm_flags);
>> + }
>> + mmap_write_unlock(current->mm);
>> + return 0;
>> +}
>> +
>> SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>> unsigned long, arg4, unsigned long, arg5)
>> {
>> @@ -2596,20 +2640,10 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>> return -EINVAL;
>> return task_no_new_privs(current) ? 1 : 0;
>> case PR_GET_THP_DISABLE:
>> - if (arg2 || arg3 || arg4 || arg5)
>> - return -EINVAL;
>> - error = !!test_bit(MMF_DISABLE_THP, &me->mm->flags);
>> + error = prctl_get_thp_disable(arg2, arg3, arg4, arg5);
>> break;
>> case PR_SET_THP_DISABLE:
>> - if (arg3 || arg4 || arg5)
>> - return -EINVAL;
>> - if (mmap_write_lock_killable(me->mm))
>> - return -EINTR;
>> - if (arg2)
>> - set_bit(MMF_DISABLE_THP, &me->mm->flags);
>> - else
>> - clear_bit(MMF_DISABLE_THP, &me->mm->flags);
>> - mmap_write_unlock(me->mm);
>> + error = prctl_set_thp_disable(arg2, arg3, arg4, arg5);
>> break;
>> case PR_MPX_ENABLE_MANAGEMENT:
>> case PR_MPX_DISABLE_MANAGEMENT:
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 1ff0c7dd2be4..2c9008246785 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -410,7 +410,7 @@ static inline int hpage_collapse_test_exit(struct mm_struct *mm)
>> static inline int hpage_collapse_test_exit_or_disable(struct mm_struct *mm)
>> {
>> return hpage_collapse_test_exit(mm) ||
>> - test_bit(MMF_DISABLE_THP, &mm->flags);
>> + test_bit(MMF_DISABLE_THP_COMPLETELY, &mm->flags);
>> }
>>
>> static bool hugepage_pmd_enabled(void)
>> --
>> 2.47.3
>>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/5] prctl: extend PR_SET_THP_DISABLE to optionally exclude VM_HUGEPAGE
2025-07-30 19:42 ` Usama Arif
@ 2025-07-31 8:29 ` Lorenzo Stoakes
2025-07-31 8:38 ` David Hildenbrand
0 siblings, 1 reply; 20+ messages in thread
From: Lorenzo Stoakes @ 2025-07-31 8:29 UTC (permalink / raw)
To: Usama Arif
Cc: Andrew Morton, david, linux-mm, linux-fsdevel, corbet, rppt,
surenb, mhocko, hannes, baohua, shakeel.butt, riel, ziy,
laoar.shao, dev.jain, baolin.wang, npache, Liam.Howlett,
ryan.roberts, vbabka, jannh, Arnd Bergmann, sj, linux-kernel,
linux-doc, kernel-team, Matthew Wilcox
Just a ping on the man page stuff - you will do that right? :>)
Probably best at end of 6.18 cycle when this is about to go to Linus.
I can give you some details on how that all works if it'd be helpful.
On Wed, Jul 30, 2025 at 08:42:04PM +0100, Usama Arif wrote:
> >> Acked-by: Usama Arif <usamaarif642@gmail.com>
> >> Tested-by: Usama Arif <usamaarif642@gmail.com>
[snip]
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
All looks good aside from nits, so:
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> >> +/*
> >> + * Check whether THPs are explicitly disabled for this VMA, for example,
> >> + * through madvise or prctl.
> >> + */
> >> static inline bool vma_thp_disabled(struct vm_area_struct *vma,
> >> vm_flags_t vm_flags)
> >> {
> >> + /* Are THPs disabled for this VMA? */
> >> + if (vm_flags & VM_NOHUGEPAGE)
> >> + return true;
>
> VM_NOHUGEPAGE will cause the THP being disabled here.
>
> >> + /* Are THPs disabled for all VMAs in the whole process? */
> >> + if (test_bit(MMF_DISABLE_THP_COMPLETELY, &vma->vm_mm->flags))
> >> + return true;
> >> /*
> >> - * Explicitly disabled through madvise or prctl, or some
> >> - * architectures may disable THP for some mappings, for
> >> - * example, s390 kvm.
> >> + * Are THPs disabled only for VMAs where we didn't get an explicit
> >> + * advise to use them?
> >> */
> >> - return (vm_flags & VM_NOHUGEPAGE) ||
> >> - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags);
> >> + if (vm_flags & VM_HUGEPAGE)
> >> + return false;
> >
> > Hm is this correct? This means that VM_NOHUGEPAGE no longer results in THP being
> > disabled here no?
>
> Lorenzo, pointed to VM_NOHUGEPAGE case above.>
Haha doh! OK cool, obviously I did this review a little too late in the day
when Mr. Brain was not on best form :P
All good, thanks!
And of course this now enforces the 'except advised' logic below it.
> >> +/*
> >> + * Don't disable THPs when explicitly advised (e.g., MADV_HUGEPAGE /
> >> + * VM_HUGEPAGE).
> >> + */
> >> +# define PR_THP_DISABLE_EXCEPT_ADVISED (1 << 1)
> >
> > NO space after # please.
> >
>
> I think this is following the file convention, the space is there in other flags
> all over this file. I dont like the space as well.
Yeah yuck. It's not a big deal, but ideally I'd prefer us to be sane even
if the rest of the header is less so here.
>
> >> #define PR_GET_THP_DISABLE 42
> >>
> >> /*
> >> diff --git a/kernel/sys.c b/kernel/sys.c
> >> index b153fb345ada..b87d0acaab0b 100644
> >> --- a/kernel/sys.c
> >> +++ b/kernel/sys.c
> >> @@ -2423,6 +2423,50 @@ static int prctl_get_auxv(void __user *addr, unsigned long len)
> >> return sizeof(mm->saved_auxv);
> >> }
> >>
> >> +static int prctl_get_thp_disable(unsigned long arg2, unsigned long arg3,
> >> + unsigned long arg4, unsigned long arg5)
> >> +{
> >> + unsigned long *mm_flags = ¤t->mm->flags;
> >> +
> >> + if (arg2 || arg3 || arg4 || arg5)
> >> + return -EINVAL;
> >> +
> >
> > Can we have a comment here about what we're doing below re: the return
> > value?
> >
>
> Do you mean add returning 1 for MMF_DISABLE_THP_COMPLETELY and 3 for MMF_DISABLE_THP_EXCEPT_ADVISED?
Well more so something about we return essentially flags indicating what is
enabled or not, if bit 0 is set then it's disabled, if bit 1 is set then
it's that with the exception of VM_HUGEPAGE VMAs.
>
> I will add it in next revision.
Thanks!
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/5] prctl: extend PR_SET_THP_DISABLE to optionally exclude VM_HUGEPAGE
2025-07-31 8:29 ` Lorenzo Stoakes
@ 2025-07-31 8:38 ` David Hildenbrand
2025-07-31 9:09 ` Lorenzo Stoakes
2025-07-31 10:32 ` Usama Arif
0 siblings, 2 replies; 20+ messages in thread
From: David Hildenbrand @ 2025-07-31 8:38 UTC (permalink / raw)
To: Lorenzo Stoakes, Usama Arif
Cc: Andrew Morton, linux-mm, linux-fsdevel, corbet, rppt, surenb,
mhocko, hannes, baohua, shakeel.butt, riel, ziy, laoar.shao,
dev.jain, baolin.wang, npache, Liam.Howlett, ryan.roberts, vbabka,
jannh, Arnd Bergmann, sj, linux-kernel, linux-doc, kernel-team,
Matthew Wilcox
Thanks Lorenzo for the review, I'll leave handling all that to Usama
from this point :)
On 31.07.25 10:29, Lorenzo Stoakes wrote:
> Just a ping on the man page stuff - you will do that right? :>)
>
I'm hoping that Usama can take over that part. If not, I'll handle it
(had planned it for once it's in mm-stable / going upstream).
[ ... ]
>>>> +/*
>>>> + * Don't disable THPs when explicitly advised (e.g., MADV_HUGEPAGE /
>>>> + * VM_HUGEPAGE).
>>>> + */
>>>> +# define PR_THP_DISABLE_EXCEPT_ADVISED (1 << 1)
>>>
>>> NO space after # please.
>>>
>>
>> I think this is following the file convention, the space is there in other flags
>> all over this file. I dont like the space as well.
>
> Yeah yuck. It's not a big deal, but ideally I'd prefer us to be sane even
> if the rest of the header is less so here.
I'm afraid us doing something different here will not make prctl() any
better as a whole, so let's keep it consistent in this questionable file.
>
>>
>>>> #define PR_GET_THP_DISABLE 42
>>>>
>>>> /*
>>>> diff --git a/kernel/sys.c b/kernel/sys.c
>>>> index b153fb345ada..b87d0acaab0b 100644
>>>> --- a/kernel/sys.c
>>>> +++ b/kernel/sys.c
>>>> @@ -2423,6 +2423,50 @@ static int prctl_get_auxv(void __user *addr, unsigned long len)
>>>> return sizeof(mm->saved_auxv);
>>>> }
>>>>
>>>> +static int prctl_get_thp_disable(unsigned long arg2, unsigned long arg3,
>>>> + unsigned long arg4, unsigned long arg5)
>>>> +{
>>>> + unsigned long *mm_flags = ¤t->mm->flags;
>>>> +
>>>> + if (arg2 || arg3 || arg4 || arg5)
>>>> + return -EINVAL;
>>>> +
>>>
>>> Can we have a comment here about what we're doing below re: the return
>>> value?
>>>
>>
>> Do you mean add returning 1 for MMF_DISABLE_THP_COMPLETELY and 3 for MMF_DISABLE_THP_EXCEPT_ADVISED?
>
> Well more so something about we return essentially flags indicating what is
> enabled or not, if bit 0 is set then it's disabled, if bit 1 is set then
> it's that with the exception of VM_HUGEPAGE VMAs.
We have that documented above the defines for flags etc. Maybe simply here:
/* If disabled, we return "1 | flags", otherwise 0. */
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/5] prctl: extend PR_SET_THP_DISABLE to optionally exclude VM_HUGEPAGE
2025-07-31 8:38 ` David Hildenbrand
@ 2025-07-31 9:09 ` Lorenzo Stoakes
2025-07-31 10:32 ` Usama Arif
1 sibling, 0 replies; 20+ messages in thread
From: Lorenzo Stoakes @ 2025-07-31 9:09 UTC (permalink / raw)
To: David Hildenbrand
Cc: Usama Arif, Andrew Morton, linux-mm, linux-fsdevel, corbet, rppt,
surenb, mhocko, hannes, baohua, shakeel.butt, riel, ziy,
laoar.shao, dev.jain, baolin.wang, npache, Liam.Howlett,
ryan.roberts, vbabka, jannh, Arnd Bergmann, sj, linux-kernel,
linux-doc, kernel-team, Matthew Wilcox
On Thu, Jul 31, 2025 at 10:38:49AM +0200, David Hildenbrand wrote:
> Thanks Lorenzo for the review, I'll leave handling all that to Usama from
> this point :)
>
> On 31.07.25 10:29, Lorenzo Stoakes wrote:
> > Just a ping on the man page stuff - you will do that right? :>)
> >
>
> I'm hoping that Usama can take over that part. If not, I'll handle it (had
> planned it for once it's in mm-stable / going upstream).
>
> [ ... ]
Thanks
>
> > > > > +/*
> > > > > + * Don't disable THPs when explicitly advised (e.g., MADV_HUGEPAGE /
> > > > > + * VM_HUGEPAGE).
> > > > > + */
> > > > > +# define PR_THP_DISABLE_EXCEPT_ADVISED (1 << 1)
> > > >
> > > > NO space after # please.
> > > >
> > >
> > > I think this is following the file convention, the space is there in other flags
> > > all over this file. I dont like the space as well.
> >
> > Yeah yuck. It's not a big deal, but ideally I'd prefer us to be sane even
> > if the rest of the header is less so here.
>
> I'm afraid us doing something different here will not make prctl() any
> better as a whole, so let's keep it consistent in this questionable file.
Sure not a big deal.
>
> >
> > >
> > > > > #define PR_GET_THP_DISABLE 42
> > > > >
> > > > > /*
> > > > > diff --git a/kernel/sys.c b/kernel/sys.c
> > > > > index b153fb345ada..b87d0acaab0b 100644
> > > > > --- a/kernel/sys.c
> > > > > +++ b/kernel/sys.c
> > > > > @@ -2423,6 +2423,50 @@ static int prctl_get_auxv(void __user *addr, unsigned long len)
> > > > > return sizeof(mm->saved_auxv);
> > > > > }
> > > > >
> > > > > +static int prctl_get_thp_disable(unsigned long arg2, unsigned long arg3,
> > > > > + unsigned long arg4, unsigned long arg5)
> > > > > +{
> > > > > + unsigned long *mm_flags = ¤t->mm->flags;
> > > > > +
> > > > > + if (arg2 || arg3 || arg4 || arg5)
> > > > > + return -EINVAL;
> > > > > +
> > > >
> > > > Can we have a comment here about what we're doing below re: the return
> > > > value?
> > > >
> > >
> > > Do you mean add returning 1 for MMF_DISABLE_THP_COMPLETELY and 3 for MMF_DISABLE_THP_EXCEPT_ADVISED?
> >
> > Well more so something about we return essentially flags indicating what is
> > enabled or not, if bit 0 is set then it's disabled, if bit 1 is set then
> > it's that with the exception of VM_HUGEPAGE VMAs.
>
> We have that documented above the defines for flags etc. Maybe simply here:
>
> /* If disabled, we return "1 | flags", otherwise 0. */
Yup I know it was documented, but nice to have here as I'd definitely read this
code and think 'huh?'.
What you suggest is fine!
>
> --
> Cheers,
>
> David / dhildenb
>
Thanks, Lorenzo
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/5] prctl: extend PR_SET_THP_DISABLE to optionally exclude VM_HUGEPAGE
2025-07-31 8:38 ` David Hildenbrand
2025-07-31 9:09 ` Lorenzo Stoakes
@ 2025-07-31 10:32 ` Usama Arif
1 sibling, 0 replies; 20+ messages in thread
From: Usama Arif @ 2025-07-31 10:32 UTC (permalink / raw)
To: David Hildenbrand, Lorenzo Stoakes
Cc: Andrew Morton, linux-mm, linux-fsdevel, corbet, rppt, surenb,
mhocko, hannes, baohua, shakeel.butt, riel, ziy, laoar.shao,
dev.jain, baolin.wang, npache, Liam.Howlett, ryan.roberts, vbabka,
jannh, Arnd Bergmann, sj, linux-kernel, linux-doc, kernel-team,
Matthew Wilcox
On 31/07/2025 09:38, David Hildenbrand wrote:
> Thanks Lorenzo for the review, I'll leave handling all that to Usama from this point :)
>
> On 31.07.25 10:29, Lorenzo Stoakes wrote:
>> Just a ping on the man page stuff - you will do that right? :>)
>>
>
> I'm hoping that Usama can take over that part. If not, I'll handle it (had planned it for once it's in mm-stable / going upstream).
Yes, plan to take care of man page, systemd, etc once the patch makes it to mm-stable.
>
> [ ... ]
>
>>>>> +/*
>>>>> + * Don't disable THPs when explicitly advised (e.g., MADV_HUGEPAGE /
>>>>> + * VM_HUGEPAGE).
>>>>> + */
>>>>> +# define PR_THP_DISABLE_EXCEPT_ADVISED (1 << 1)
>>>>
>>>> NO space after # please.
>>>>
>>>
>>> I think this is following the file convention, the space is there in other flags
>>> all over this file. I dont like the space as well.
>>
>> Yeah yuck. It's not a big deal, but ideally I'd prefer us to be sane even
>> if the rest of the header is less so here.
>
> I'm afraid us doing something different here will not make prctl() any better as a whole, so let's keep it consistent in this questionable file.
>
>>
>>>
>>>>> #define PR_GET_THP_DISABLE 42
>>>>>
>>>>> /*
>>>>> diff --git a/kernel/sys.c b/kernel/sys.c
>>>>> index b153fb345ada..b87d0acaab0b 100644
>>>>> --- a/kernel/sys.c
>>>>> +++ b/kernel/sys.c
>>>>> @@ -2423,6 +2423,50 @@ static int prctl_get_auxv(void __user *addr, unsigned long len)
>>>>> return sizeof(mm->saved_auxv);
>>>>> }
>>>>>
>>>>> +static int prctl_get_thp_disable(unsigned long arg2, unsigned long arg3,
>>>>> + unsigned long arg4, unsigned long arg5)
>>>>> +{
>>>>> + unsigned long *mm_flags = ¤t->mm->flags;
>>>>> +
>>>>> + if (arg2 || arg3 || arg4 || arg5)
>>>>> + return -EINVAL;
>>>>> +
>>>>
>>>> Can we have a comment here about what we're doing below re: the return
>>>> value?
>>>>
>>>
>>> Do you mean add returning 1 for MMF_DISABLE_THP_COMPLETELY and 3 for MMF_DISABLE_THP_EXCEPT_ADVISED?
>>
>> Well more so something about we return essentially flags indicating what is
>> enabled or not, if bit 0 is set then it's disabled, if bit 1 is set then
>> it's that with the exception of VM_HUGEPAGE VMAs.
>
> We have that documented above the defines for flags etc. Maybe simply here:
>
> /* If disabled, we return "1 | flags", otherwise 0. */
>
Thanks, will add this to next revision.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/5] mm/huge_memory: convert "tva_flags" to "enum tva_type" for thp_vma_allowable_order*()
2025-07-25 16:22 [PATCH 0/5] prctl: extend PR_SET_THP_DISABLE to only provide THPs when advised Usama Arif
2025-07-25 16:22 ` [PATCH 1/5] prctl: extend PR_SET_THP_DISABLE to optionally exclude VM_HUGEPAGE Usama Arif
@ 2025-07-25 16:22 ` Usama Arif
2025-07-28 13:28 ` David Hildenbrand
2025-07-25 16:22 ` [PATCH 3/5] mm/huge_memory: treat MADV_COLLAPSE as an advise with PR_THP_DISABLE_EXCEPT_ADVISED Usama Arif
` (2 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Usama Arif @ 2025-07-25 16:22 UTC (permalink / raw)
To: Andrew Morton, david, linux-mm
Cc: linux-fsdevel, corbet, rppt, surenb, mhocko, hannes, baohua,
shakeel.butt, riel, ziy, laoar.shao, dev.jain, baolin.wang,
npache, lorenzo.stoakes, Liam.Howlett, ryan.roberts, vbabka,
jannh, Arnd Bergmann, sj, linux-kernel, linux-doc, kernel-team,
Usama Arif
From: David Hildenbrand <david@redhat.com>
Describing the context through a type is much clearer, and good enough
for our case.
We have:
* smaps handling for showing "THPeligible"
* Pagefault handling
* khugepaged handling
* Forced collapse handling: primarily MADV_COLLAPSE, but one other odd case
Really, we want to ignore sysfs only when we are forcing a collapse
through MADV_COLLAPSE, otherwise we want to enforce.
With this change, we immediately know if we are in the forced collapse
case, which will be valuable next.
Signed-off-by: David Hildenbrand <david@redhat.com>
Acked-by: Usama Arif <usamaarif642@gmail.com>
---
fs/proc/task_mmu.c | 4 ++--
include/linux/huge_mm.h | 30 ++++++++++++++++++------------
mm/huge_memory.c | 8 ++++----
mm/khugepaged.c | 18 +++++++++---------
mm/memory.c | 14 ++++++--------
5 files changed, 39 insertions(+), 35 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 3d6d8a9f13fc..d440df7b3d59 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1293,8 +1293,8 @@ static int show_smap(struct seq_file *m, void *v)
__show_smap(m, &mss, false);
seq_printf(m, "THPeligible: %8u\n",
- !!thp_vma_allowable_orders(vma, vma->vm_flags,
- TVA_SMAPS | TVA_ENFORCE_SYSFS, THP_ORDERS_ALL));
+ !!thp_vma_allowable_orders(vma, vma->vm_flags, TVA_SMAPS,
+ THP_ORDERS_ALL));
if (arch_pkeys_enabled())
seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma));
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index 71db243a002e..b0ff54eee81c 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -94,12 +94,15 @@ extern struct kobj_attribute thpsize_shmem_enabled_attr;
#define THP_ORDERS_ALL \
(THP_ORDERS_ALL_ANON | THP_ORDERS_ALL_SPECIAL | THP_ORDERS_ALL_FILE_DEFAULT)
-#define TVA_SMAPS (1 << 0) /* Will be used for procfs */
-#define TVA_IN_PF (1 << 1) /* Page fault handler */
-#define TVA_ENFORCE_SYSFS (1 << 2) /* Obey sysfs configuration */
+enum tva_type {
+ TVA_SMAPS, /* Exposing "THPeligible:" in smaps. */
+ TVA_PAGEFAULT, /* Serving a page fault. */
+ TVA_KHUGEPAGED, /* Khugepaged collapse. */
+ TVA_FORCED_COLLAPSE, /* Forced collapse (i.e., MADV_COLLAPSE). */
+};
-#define thp_vma_allowable_order(vma, vm_flags, tva_flags, order) \
- (!!thp_vma_allowable_orders(vma, vm_flags, tva_flags, BIT(order)))
+#define thp_vma_allowable_order(vma, vm_flags, type, order) \
+ (!!thp_vma_allowable_orders(vma, vm_flags, type, BIT(order)))
#define split_folio(f) split_folio_to_list(f, NULL)
@@ -264,14 +267,14 @@ static inline unsigned long thp_vma_suitable_orders(struct vm_area_struct *vma,
unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
vm_flags_t vm_flags,
- unsigned long tva_flags,
+ enum tva_type type,
unsigned long orders);
/**
* thp_vma_allowable_orders - determine hugepage orders that are allowed for vma
* @vma: the vm area to check
* @vm_flags: use these vm_flags instead of vma->vm_flags
- * @tva_flags: Which TVA flags to honour
+ * @type: TVA type
* @orders: bitfield of all orders to consider
*
* Calculates the intersection of the requested hugepage orders and the allowed
@@ -285,11 +288,14 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
static inline
unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
vm_flags_t vm_flags,
- unsigned long tva_flags,
+ enum tva_type type,
unsigned long orders)
{
- /* Optimization to check if required orders are enabled early. */
- if ((tva_flags & TVA_ENFORCE_SYSFS) && vma_is_anonymous(vma)) {
+ /*
+ * Optimization to check if required orders are enabled early. Only
+ * forced collapse ignores sysfs configs.
+ */
+ if (type != TVA_FORCED_COLLAPSE && vma_is_anonymous(vma)) {
unsigned long mask = READ_ONCE(huge_anon_orders_always);
if (vm_flags & VM_HUGEPAGE)
@@ -303,7 +309,7 @@ unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
return 0;
}
- return __thp_vma_allowable_orders(vma, vm_flags, tva_flags, orders);
+ return __thp_vma_allowable_orders(vma, vm_flags, type, orders);
}
struct thpsize {
@@ -536,7 +542,7 @@ static inline unsigned long thp_vma_suitable_orders(struct vm_area_struct *vma,
static inline unsigned long thp_vma_allowable_orders(struct vm_area_struct *vma,
vm_flags_t vm_flags,
- unsigned long tva_flags,
+ enum tva_type type,
unsigned long orders)
{
return 0;
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 2b4ea5a2ce7d..85252b468f80 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -99,12 +99,12 @@ static inline bool file_thp_enabled(struct vm_area_struct *vma)
unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
vm_flags_t vm_flags,
- unsigned long tva_flags,
+ enum tva_type type,
unsigned long orders)
{
- bool smaps = tva_flags & TVA_SMAPS;
- bool in_pf = tva_flags & TVA_IN_PF;
- bool enforce_sysfs = tva_flags & TVA_ENFORCE_SYSFS;
+ const bool smaps = type == TVA_SMAPS;
+ const bool in_pf = type == TVA_PAGEFAULT;
+ const bool enforce_sysfs = type != TVA_FORCED_COLLAPSE;
unsigned long supported_orders;
/* Check the intersection of requested and supported orders. */
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 2c9008246785..7a54b6f2a346 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -474,8 +474,7 @@ void khugepaged_enter_vma(struct vm_area_struct *vma,
{
if (!test_bit(MMF_VM_HUGEPAGE, &vma->vm_mm->flags) &&
hugepage_pmd_enabled()) {
- if (thp_vma_allowable_order(vma, vm_flags, TVA_ENFORCE_SYSFS,
- PMD_ORDER))
+ if (thp_vma_allowable_order(vma, vm_flags, TVA_KHUGEPAGED, PMD_ORDER))
__khugepaged_enter(vma->vm_mm);
}
}
@@ -921,7 +920,8 @@ static int hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address,
struct collapse_control *cc)
{
struct vm_area_struct *vma;
- unsigned long tva_flags = cc->is_khugepaged ? TVA_ENFORCE_SYSFS : 0;
+ enum tva_type tva_type = cc->is_khugepaged ? TVA_KHUGEPAGED :
+ TVA_FORCED_COLLAPSE;
if (unlikely(hpage_collapse_test_exit_or_disable(mm)))
return SCAN_ANY_PROCESS;
@@ -932,7 +932,7 @@ static int hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address,
if (!thp_vma_suitable_order(vma, address, PMD_ORDER))
return SCAN_ADDRESS_RANGE;
- if (!thp_vma_allowable_order(vma, vma->vm_flags, tva_flags, PMD_ORDER))
+ if (!thp_vma_allowable_order(vma, vma->vm_flags, tva_type, PMD_ORDER))
return SCAN_VMA_CHECK;
/*
* Anon VMA expected, the address may be unmapped then
@@ -1532,9 +1532,10 @@ int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr,
* in the page cache with a single hugepage. If a mm were to fault-in
* this memory (mapped by a suitably aligned VMA), we'd get the hugepage
* and map it by a PMD, regardless of sysfs THP settings. As such, let's
- * analogously elide sysfs THP settings here.
+ * analogously elide sysfs THP settings here and pretend we are
+ * collapsing.
*/
- if (!thp_vma_allowable_order(vma, vma->vm_flags, 0, PMD_ORDER))
+ if (!thp_vma_allowable_order(vma, vma->vm_flags, TVA_FORCED_COLLAPSE, PMD_ORDER))
return SCAN_VMA_CHECK;
/* Keep pmd pgtable for uffd-wp; see comment in retract_page_tables() */
@@ -2431,8 +2432,7 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, int *result,
progress++;
break;
}
- if (!thp_vma_allowable_order(vma, vma->vm_flags,
- TVA_ENFORCE_SYSFS, PMD_ORDER)) {
+ if (!thp_vma_allowable_order(vma, vma->vm_flags, TVA_KHUGEPAGED, PMD_ORDER)) {
skip:
progress++;
continue;
@@ -2766,7 +2766,7 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start,
BUG_ON(vma->vm_start > start);
BUG_ON(vma->vm_end < end);
- if (!thp_vma_allowable_order(vma, vma->vm_flags, 0, PMD_ORDER))
+ if (!thp_vma_allowable_order(vma, vma->vm_flags, TVA_FORCED_COLLAPSE, PMD_ORDER))
return -EINVAL;
cc = kmalloc(sizeof(*cc), GFP_KERNEL);
diff --git a/mm/memory.c b/mm/memory.c
index 92fd18a5d8d1..be761753f240 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4369,8 +4369,8 @@ static struct folio *alloc_swap_folio(struct vm_fault *vmf)
* Get a list of all the (large) orders below PMD_ORDER that are enabled
* and suitable for swapping THP.
*/
- orders = thp_vma_allowable_orders(vma, vma->vm_flags,
- TVA_IN_PF | TVA_ENFORCE_SYSFS, BIT(PMD_ORDER) - 1);
+ orders = thp_vma_allowable_orders(vma, vma->vm_flags, TVA_PAGEFAULT,
+ BIT(PMD_ORDER) - 1);
orders = thp_vma_suitable_orders(vma, vmf->address, orders);
orders = thp_swap_suitable_orders(swp_offset(entry),
vmf->address, orders);
@@ -4917,8 +4917,8 @@ static struct folio *alloc_anon_folio(struct vm_fault *vmf)
* for this vma. Then filter out the orders that can't be allocated over
* the faulting address and still be fully contained in the vma.
*/
- orders = thp_vma_allowable_orders(vma, vma->vm_flags,
- TVA_IN_PF | TVA_ENFORCE_SYSFS, BIT(PMD_ORDER) - 1);
+ orders = thp_vma_allowable_orders(vma, vma->vm_flags, TVA_PAGEFAULT,
+ BIT(PMD_ORDER) - 1);
orders = thp_vma_suitable_orders(vma, vmf->address, orders);
if (!orders)
@@ -6108,8 +6108,7 @@ static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
return VM_FAULT_OOM;
retry_pud:
if (pud_none(*vmf.pud) &&
- thp_vma_allowable_order(vma, vm_flags,
- TVA_IN_PF | TVA_ENFORCE_SYSFS, PUD_ORDER)) {
+ thp_vma_allowable_order(vma, vm_flags, TVA_PAGEFAULT, PUD_ORDER)) {
ret = create_huge_pud(&vmf);
if (!(ret & VM_FAULT_FALLBACK))
return ret;
@@ -6143,8 +6142,7 @@ static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
goto retry_pud;
if (pmd_none(*vmf.pmd) &&
- thp_vma_allowable_order(vma, vm_flags,
- TVA_IN_PF | TVA_ENFORCE_SYSFS, PMD_ORDER)) {
+ thp_vma_allowable_order(vma, vm_flags, TVA_PAGEFAULT, PMD_ORDER)) {
ret = create_huge_pmd(&vmf);
if (!(ret & VM_FAULT_FALLBACK))
return ret;
--
2.47.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/5] mm/huge_memory: convert "tva_flags" to "enum tva_type" for thp_vma_allowable_order*()
2025-07-25 16:22 ` [PATCH 2/5] mm/huge_memory: convert "tva_flags" to "enum tva_type" for thp_vma_allowable_order*() Usama Arif
@ 2025-07-28 13:28 ` David Hildenbrand
2025-07-28 14:09 ` Usama Arif
0 siblings, 1 reply; 20+ messages in thread
From: David Hildenbrand @ 2025-07-28 13:28 UTC (permalink / raw)
To: Usama Arif, Andrew Morton, linux-mm
Cc: linux-fsdevel, corbet, rppt, surenb, mhocko, hannes, baohua,
shakeel.butt, riel, ziy, laoar.shao, dev.jain, baolin.wang,
npache, lorenzo.stoakes, Liam.Howlett, ryan.roberts, vbabka,
jannh, Arnd Bergmann, sj, linux-kernel, linux-doc, kernel-team
On 25.07.25 18:22, Usama Arif wrote:
> From: David Hildenbrand <david@redhat.com>
>
> Describing the context through a type is much clearer, and good enough
> for our case.
>
> We have:
> * smaps handling for showing "THPeligible"
> * Pagefault handling
> * khugepaged handling
> * Forced collapse handling: primarily MADV_COLLAPSE, but one other odd case
>
> Really, we want to ignore sysfs only when we are forcing a collapse
> through MADV_COLLAPSE, otherwise we want to enforce.
>
> With this change, we immediately know if we are in the forced collapse
> case, which will be valuable next.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> Acked-by: Usama Arif <usamaarif642@gmail.com>
Nit: if you forward patches untouched, make sure to add your SOB as well.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] mm/huge_memory: convert "tva_flags" to "enum tva_type" for thp_vma_allowable_order*()
2025-07-28 13:28 ` David Hildenbrand
@ 2025-07-28 14:09 ` Usama Arif
0 siblings, 0 replies; 20+ messages in thread
From: Usama Arif @ 2025-07-28 14:09 UTC (permalink / raw)
To: David Hildenbrand, Andrew Morton, linux-mm
Cc: linux-fsdevel, corbet, rppt, surenb, mhocko, hannes, baohua,
shakeel.butt, riel, ziy, laoar.shao, dev.jain, baolin.wang,
npache, lorenzo.stoakes, Liam.Howlett, ryan.roberts, vbabka,
jannh, Arnd Bergmann, sj, linux-kernel, linux-doc, kernel-team
On 28/07/2025 14:28, David Hildenbrand wrote:
> On 25.07.25 18:22, Usama Arif wrote:
>> From: David Hildenbrand <david@redhat.com>
>>
>> Describing the context through a type is much clearer, and good enough
>> for our case.
>>
>> We have:
>> * smaps handling for showing "THPeligible"
>> * Pagefault handling
>> * khugepaged handling
>> * Forced collapse handling: primarily MADV_COLLAPSE, but one other odd case
>>
>> Really, we want to ignore sysfs only when we are forcing a collapse
>> through MADV_COLLAPSE, otherwise we want to enforce.
>>
>> With this change, we immediately know if we are in the forced collapse
>> case, which will be valuable next.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> Acked-by: Usama Arif <usamaarif642@gmail.com>
>
> Nit: if you forward patches untouched, make sure to add your SOB as well.
>
Thanks!
Signed-off-by: Usama Arif <usamaarif642@gmail.com>
Just adding above incase (hopefully) this revision gets into mm-new.
If it doesn't, I will add the sign off in the next revision.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/5] mm/huge_memory: treat MADV_COLLAPSE as an advise with PR_THP_DISABLE_EXCEPT_ADVISED
2025-07-25 16:22 [PATCH 0/5] prctl: extend PR_SET_THP_DISABLE to only provide THPs when advised Usama Arif
2025-07-25 16:22 ` [PATCH 1/5] prctl: extend PR_SET_THP_DISABLE to optionally exclude VM_HUGEPAGE Usama Arif
2025-07-25 16:22 ` [PATCH 2/5] mm/huge_memory: convert "tva_flags" to "enum tva_type" for thp_vma_allowable_order*() Usama Arif
@ 2025-07-25 16:22 ` Usama Arif
2025-07-25 16:22 ` [PATCH 4/5] selftests: prctl: introduce tests for disabling THPs completely Usama Arif
2025-07-25 16:22 ` [PATCH 5/5] selftests: prctl: introduce tests for disabling THPs except for madvise Usama Arif
4 siblings, 0 replies; 20+ messages in thread
From: Usama Arif @ 2025-07-25 16:22 UTC (permalink / raw)
To: Andrew Morton, david, linux-mm
Cc: linux-fsdevel, corbet, rppt, surenb, mhocko, hannes, baohua,
shakeel.butt, riel, ziy, laoar.shao, dev.jain, baolin.wang,
npache, lorenzo.stoakes, Liam.Howlett, ryan.roberts, vbabka,
jannh, Arnd Bergmann, sj, linux-kernel, linux-doc, kernel-team,
Usama Arif
From: David Hildenbrand <david@redhat.com>
Let's allow for making MADV_COLLAPSE succeed on areas that neither have
VM_HUGEPAGE nor VM_NOHUGEPAGE when we have THP disabled
unless explicitly advised (PR_THP_DISABLE_EXCEPT_ADVISED).
MADV_COLLAPSE is a clear advise that we want to collapse.
Note that we still respect the VM_NOHUGEPAGE flag, just like
MADV_COLLAPSE always does. So consequently, MADV_COLLAPSE is now only
refused on VM_NOHUGEPAGE with PR_THP_DISABLE_EXCEPT_ADVISED.
Co-developed-by: Usama Arif <usamaarif642@gmail.com>
Signed-off-by: Usama Arif <usamaarif642@gmail.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
include/linux/huge_mm.h | 8 +++++++-
include/uapi/linux/prctl.h | 2 +-
mm/huge_memory.c | 5 +++--
mm/memory.c | 6 ++++--
mm/shmem.c | 2 +-
5 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index b0ff54eee81c..aeaf93f8ac2e 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -329,7 +329,7 @@ struct thpsize {
* through madvise or prctl.
*/
static inline bool vma_thp_disabled(struct vm_area_struct *vma,
- vm_flags_t vm_flags)
+ vm_flags_t vm_flags, bool forced_collapse)
{
/* Are THPs disabled for this VMA? */
if (vm_flags & VM_NOHUGEPAGE)
@@ -343,6 +343,12 @@ static inline bool vma_thp_disabled(struct vm_area_struct *vma,
*/
if (vm_flags & VM_HUGEPAGE)
return false;
+ /*
+ * Forcing a collapse (e.g., madv_collapse), is a clear advise to
+ * use THPs.
+ */
+ if (forced_collapse)
+ return false;
return test_bit(MMF_DISABLE_THP_EXCEPT_ADVISED, &vma->vm_mm->flags);
}
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index 9c1d6e49b8a9..ee4165738779 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -185,7 +185,7 @@ struct prctl_mm_map {
#define PR_SET_THP_DISABLE 41
/*
* Don't disable THPs when explicitly advised (e.g., MADV_HUGEPAGE /
- * VM_HUGEPAGE).
+ * VM_HUGEPAGE / MADV_COLLAPSE).
*/
# define PR_THP_DISABLE_EXCEPT_ADVISED (1 << 1)
#define PR_GET_THP_DISABLE 42
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 85252b468f80..ef5ccb0ec5d5 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -104,7 +104,8 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
{
const bool smaps = type == TVA_SMAPS;
const bool in_pf = type == TVA_PAGEFAULT;
- const bool enforce_sysfs = type != TVA_FORCED_COLLAPSE;
+ const bool forced_collapse = type == TVA_FORCED_COLLAPSE;
+ const bool enforce_sysfs = !forced_collapse;
unsigned long supported_orders;
/* Check the intersection of requested and supported orders. */
@@ -122,7 +123,7 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
if (!vma->vm_mm) /* vdso */
return 0;
- if (thp_disabled_by_hw() || vma_thp_disabled(vma, vm_flags))
+ if (thp_disabled_by_hw() || vma_thp_disabled(vma, vm_flags, forced_collapse))
return 0;
/* khugepaged doesn't collapse DAX vma, but page fault is fine. */
diff --git a/mm/memory.c b/mm/memory.c
index be761753f240..bd04212d6f79 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5186,9 +5186,11 @@ vm_fault_t do_set_pmd(struct vm_fault *vmf, struct folio *folio, struct page *pa
* It is too late to allocate a small folio, we already have a large
* folio in the pagecache: especially s390 KVM cannot tolerate any
* PMD mappings, but PTE-mapped THP are fine. So let's simply refuse any
- * PMD mappings if THPs are disabled.
+ * PMD mappings if THPs are disabled. As we already have a THP ...
+ * behave as if we are forcing a collapse.
*/
- if (thp_disabled_by_hw() || vma_thp_disabled(vma, vma->vm_flags))
+ if (thp_disabled_by_hw() || vma_thp_disabled(vma, vma->vm_flags,
+ /* forced_collapse=*/ true))
return ret;
if (!thp_vma_suitable_order(vma, haddr, PMD_ORDER))
diff --git a/mm/shmem.c b/mm/shmem.c
index e6cdfda08aed..30609197a266 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1816,7 +1816,7 @@ unsigned long shmem_allowable_huge_orders(struct inode *inode,
vm_flags_t vm_flags = vma ? vma->vm_flags : 0;
unsigned int global_orders;
- if (thp_disabled_by_hw() || (vma && vma_thp_disabled(vma, vm_flags)))
+ if (thp_disabled_by_hw() || (vma && vma_thp_disabled(vma, vm_flags, shmem_huge_force)))
return 0;
global_orders = shmem_huge_global_enabled(inode, index, write_end,
--
2.47.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 4/5] selftests: prctl: introduce tests for disabling THPs completely
2025-07-25 16:22 [PATCH 0/5] prctl: extend PR_SET_THP_DISABLE to only provide THPs when advised Usama Arif
` (2 preceding siblings ...)
2025-07-25 16:22 ` [PATCH 3/5] mm/huge_memory: treat MADV_COLLAPSE as an advise with PR_THP_DISABLE_EXCEPT_ADVISED Usama Arif
@ 2025-07-25 16:22 ` Usama Arif
2025-07-28 15:06 ` David Hildenbrand
2025-07-25 16:22 ` [PATCH 5/5] selftests: prctl: introduce tests for disabling THPs except for madvise Usama Arif
4 siblings, 1 reply; 20+ messages in thread
From: Usama Arif @ 2025-07-25 16:22 UTC (permalink / raw)
To: Andrew Morton, david, linux-mm
Cc: linux-fsdevel, corbet, rppt, surenb, mhocko, hannes, baohua,
shakeel.butt, riel, ziy, laoar.shao, dev.jain, baolin.wang,
npache, lorenzo.stoakes, Liam.Howlett, ryan.roberts, vbabka,
jannh, Arnd Bergmann, sj, linux-kernel, linux-doc, kernel-team,
Usama Arif
The test will set the global system THP setting to madvise and
the 2M setting to inherit before it starts (and reset to original
at teardown)
This tests if the process can:
- successfully set and get the policy to disable THPs completely.
- never get a hugepage when the THPs are completely disabled,
including with MADV_HUGE and MADV_COLLAPSE.
- successfully reset the policy of the process.
- get hugepages only on MADV_HUGE and MADV_COLLAPSE after reset.
- repeat the above tests in a forked process to make sure
the policy is carried across forks.
Signed-off-by: Usama Arif <usamaarif642@gmail.com>
---
tools/testing/selftests/mm/.gitignore | 1 +
tools/testing/selftests/mm/Makefile | 1 +
.../testing/selftests/mm/prctl_thp_disable.c | 162 ++++++++++++++++++
3 files changed, 164 insertions(+)
create mode 100644 tools/testing/selftests/mm/prctl_thp_disable.c
diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
index e7b23a8a05fe..eb023ea857b3 100644
--- a/tools/testing/selftests/mm/.gitignore
+++ b/tools/testing/selftests/mm/.gitignore
@@ -58,3 +58,4 @@ pkey_sighandler_tests_32
pkey_sighandler_tests_64
guard-regions
merge
+prctl_thp_disable
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index d13b3cef2a2b..2bb8d3ebc17c 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -86,6 +86,7 @@ TEST_GEN_FILES += on-fault-limit
TEST_GEN_FILES += pagemap_ioctl
TEST_GEN_FILES += pfnmap
TEST_GEN_FILES += process_madv
+TEST_GEN_FILES += prctl_thp_disable
TEST_GEN_FILES += thuge-gen
TEST_GEN_FILES += transhuge-stress
TEST_GEN_FILES += uffd-stress
diff --git a/tools/testing/selftests/mm/prctl_thp_disable.c b/tools/testing/selftests/mm/prctl_thp_disable.c
new file mode 100644
index 000000000000..52f7e6659b1f
--- /dev/null
+++ b/tools/testing/selftests/mm/prctl_thp_disable.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Basic tests for PR_GET/SET_THP_DISABLE prctl calls
+ *
+ * Author(s): Usama Arif <usamaarif642@gmail.com>
+ */
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/mman.h>
+#include <sys/prctl.h>
+#include <sys/wait.h>
+
+#include "../kselftest_harness.h"
+#include "thp_settings.h"
+#include "vm_util.h"
+
+#ifndef PR_THP_DISABLE_EXCEPT_ADVISED
+#define PR_THP_DISABLE_EXCEPT_ADVISED (1 << 1)
+#endif
+
+#define NR_HUGEPAGES 6
+
+static int sz2ord(size_t size, size_t pagesize)
+{
+ return __builtin_ctzll(size / pagesize);
+}
+
+enum madvise_buffer {
+ NONE,
+ HUGE,
+ COLLAPSE
+};
+
+/*
+ * Function to mmap a buffer, fault it in, madvise it appropriately (before
+ * page fault for MADV_HUGE, and after for MADV_COLLAPSE), and check if the
+ * mmap region is huge.
+ * returns:
+ * 0 if test doesn't give hugepage
+ * 1 if test gives a hugepage
+ * -1 if mmap fails
+ */
+static int test_mmap_thp(enum madvise_buffer madvise_buf, size_t pmdsize)
+{
+ int ret;
+ int buf_size = NR_HUGEPAGES * pmdsize;
+
+ char *buffer = (char *)mmap(NULL, buf_size, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+ if (buffer == MAP_FAILED)
+ return -1;
+
+ if (madvise_buf == HUGE)
+ madvise(buffer, buf_size, MADV_HUGEPAGE);
+
+ /* Ensure memory is allocated */
+ memset(buffer, 1, buf_size);
+
+ if (madvise_buf == COLLAPSE)
+ madvise(buffer, buf_size, MADV_COLLAPSE);
+
+ ret = check_huge_anon(buffer, NR_HUGEPAGES, pmdsize);
+ munmap(buffer, buf_size);
+ return ret;
+}
+FIXTURE(prctl_thp_disable_completely)
+{
+ struct thp_settings settings;
+ size_t pmdsize;
+};
+
+FIXTURE_SETUP(prctl_thp_disable_completely)
+{
+ if (!thp_is_enabled())
+ SKIP(return, "Transparent Hugepages not available\n");
+
+ self->pmdsize = read_pmd_pagesize();
+ if (!self->pmdsize)
+ SKIP(return, "Unable to read PMD size\n");
+
+ thp_read_settings(&self->settings);
+ self->settings.thp_enabled = THP_MADVISE;
+ self->settings.hugepages[sz2ord(self->pmdsize, getpagesize())].enabled = THP_INHERIT;
+ thp_save_settings();
+ thp_push_settings(&self->settings);
+}
+
+FIXTURE_TEARDOWN(prctl_thp_disable_completely)
+{
+ thp_restore_settings();
+}
+
+/* prctl_thp_disable_except_madvise fixture sets system THP setting to madvise */
+static void prctl_thp_disable_completely(struct __test_metadata *const _metadata,
+ size_t pmdsize)
+{
+ int res = 0;
+
+ res = prctl(PR_GET_THP_DISABLE, NULL, NULL, NULL, NULL);
+ ASSERT_EQ(res, 1);
+
+ /* global = madvise, process = never, we shouldn't get HPs even with madvise */
+ res = test_mmap_thp(NONE, pmdsize);
+ ASSERT_EQ(res, 0);
+
+ res = test_mmap_thp(HUGE, pmdsize);
+ ASSERT_EQ(res, 0);
+
+ res = test_mmap_thp(COLLAPSE, pmdsize);
+ ASSERT_EQ(res, 0);
+
+ /* Reset to system policy */
+ res = prctl(PR_SET_THP_DISABLE, 0, NULL, NULL, NULL);
+ ASSERT_EQ(res, 0);
+
+ /* global = madvise */
+ res = test_mmap_thp(NONE, pmdsize);
+ ASSERT_EQ(res, 0);
+
+ res = test_mmap_thp(HUGE, pmdsize);
+ ASSERT_EQ(res, 1);
+
+ res = test_mmap_thp(COLLAPSE, pmdsize);
+ ASSERT_EQ(res, 1);
+}
+
+TEST_F(prctl_thp_disable_completely, nofork)
+{
+ int res = 0;
+
+ res = prctl(PR_SET_THP_DISABLE, 1, NULL, NULL, NULL);
+ ASSERT_EQ(res, 0);
+
+ prctl_thp_disable_completely(_metadata, self->pmdsize);
+}
+
+TEST_F(prctl_thp_disable_completely, fork)
+{
+ int res = 0, ret = 0;
+ pid_t pid;
+
+ res = prctl(PR_SET_THP_DISABLE, 1, NULL, NULL, NULL);
+ ASSERT_EQ(res, 0);
+
+ /* Make sure prctl changes are carried across fork */
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (!pid)
+ prctl_thp_disable_completely(_metadata, self->pmdsize);
+
+ wait(&ret);
+ if (WIFEXITED(ret))
+ ret = WEXITSTATUS(ret);
+ else
+ ret = -EINVAL;
+ ASSERT_EQ(ret, 0);
+}
+
+TEST_HARNESS_MAIN
--
2.47.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 4/5] selftests: prctl: introduce tests for disabling THPs completely
2025-07-25 16:22 ` [PATCH 4/5] selftests: prctl: introduce tests for disabling THPs completely Usama Arif
@ 2025-07-28 15:06 ` David Hildenbrand
2025-07-29 22:13 ` Usama Arif
0 siblings, 1 reply; 20+ messages in thread
From: David Hildenbrand @ 2025-07-28 15:06 UTC (permalink / raw)
To: Usama Arif, Andrew Morton, linux-mm
Cc: linux-fsdevel, corbet, rppt, surenb, mhocko, hannes, baohua,
shakeel.butt, riel, ziy, laoar.shao, dev.jain, baolin.wang,
npache, lorenzo.stoakes, Liam.Howlett, ryan.roberts, vbabka,
jannh, Arnd Bergmann, sj, linux-kernel, linux-doc, kernel-team
On 25.07.25 18:22, Usama Arif wrote:
> The test will set the global system THP setting to madvise and
> the 2M setting to inherit before it starts (and reset to original
> at teardown)
>
> This tests if the process can:
> - successfully set and get the policy to disable THPs completely.
> - never get a hugepage when the THPs are completely disabled,
> including with MADV_HUGE and MADV_COLLAPSE.
> - successfully reset the policy of the process.
> - get hugepages only on MADV_HUGE and MADV_COLLAPSE after reset.
> - repeat the above tests in a forked process to make sure
> the policy is carried across forks.
>
> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> ---
> tools/testing/selftests/mm/.gitignore | 1 +
> tools/testing/selftests/mm/Makefile | 1 +
> .../testing/selftests/mm/prctl_thp_disable.c | 162 ++++++++++++++++++
> 3 files changed, 164 insertions(+)
> create mode 100644 tools/testing/selftests/mm/prctl_thp_disable.c
>
> diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
> index e7b23a8a05fe..eb023ea857b3 100644
> --- a/tools/testing/selftests/mm/.gitignore
> +++ b/tools/testing/selftests/mm/.gitignore
> @@ -58,3 +58,4 @@ pkey_sighandler_tests_32
> pkey_sighandler_tests_64
> guard-regions
> merge
> +prctl_thp_disable
> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
> index d13b3cef2a2b..2bb8d3ebc17c 100644
> --- a/tools/testing/selftests/mm/Makefile
> +++ b/tools/testing/selftests/mm/Makefile
> @@ -86,6 +86,7 @@ TEST_GEN_FILES += on-fault-limit
> TEST_GEN_FILES += pagemap_ioctl
> TEST_GEN_FILES += pfnmap
> TEST_GEN_FILES += process_madv
> +TEST_GEN_FILES += prctl_thp_disable
> TEST_GEN_FILES += thuge-gen
> TEST_GEN_FILES += transhuge-stress
> TEST_GEN_FILES += uffd-stress
> diff --git a/tools/testing/selftests/mm/prctl_thp_disable.c b/tools/testing/selftests/mm/prctl_thp_disable.c
> new file mode 100644
> index 000000000000..52f7e6659b1f
> --- /dev/null
> +++ b/tools/testing/selftests/mm/prctl_thp_disable.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Basic tests for PR_GET/SET_THP_DISABLE prctl calls
> + *
> + * Author(s): Usama Arif <usamaarif642@gmail.com>
> + */
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/mman.h>
> +#include <sys/prctl.h>
> +#include <sys/wait.h>
> +
> +#include "../kselftest_harness.h"
> +#include "thp_settings.h"
> +#include "vm_util.h"
> +
> +#ifndef PR_THP_DISABLE_EXCEPT_ADVISED
> +#define PR_THP_DISABLE_EXCEPT_ADVISED (1 << 1)
> +#endif
Into patch #2 I guess.
> +
> +#define NR_HUGEPAGES 6
> +
> +static int sz2ord(size_t size, size_t pagesize)
> +{
> + return __builtin_ctzll(size / pagesize);
> +}
> +
> +enum madvise_buffer {
> + NONE,
> + HUGE,
> + COLLAPSE
> +};
Is that rather something like:
enum thp_collapse_type {
THP_COLLAPSE_NONE,
THP_COLLAPSE_MADV_HUGEPAGE, /* MADV_HUGEPAGE before access */
THP_COLLAPSE_MADV_COLLAPSE, /* MADV_COLLAPSE after access */
};
> +
> +/*
> + * Function to mmap a buffer, fault it in, madvise it appropriately (before
> + * page fault for MADV_HUGE, and after for MADV_COLLAPSE), and check if the
> + * mmap region is huge.
> + * returns:
> + * 0 if test doesn't give hugepage
> + * 1 if test gives a hugepage
> + * -1 if mmap fails
> + */
> +static int test_mmap_thp(enum madvise_buffer madvise_buf, size_t pmdsize)
> +{
> + int ret;
> + int buf_size = NR_HUGEPAGES * pmdsize;
> +
> + char *buffer = (char *)mmap(NULL, buf_size, PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
Can we get rid of NR_HUGEPAGES and just use a single one, aligning in a bigger area? This is similar to what we do in cow.c
/* For alignment purposes, we need twice the thp size. */
mmap_size = 2 * pmdsize;
mmap_area = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
if (mmap_area == MAP_FAILED)
return -errno; /* todo, document that above */
buffer = (char *)(((uintptr_t)mmap_area + pmdsize) & ~(pmdsize - 1));
...
ret = check_huge_anon(buffer, NR_HUGEPAGES, pmdsize);
...
munmap(mmap_area, mmap_size);
> + if (buffer == MAP_FAILED)
> + return -1;
> +
> + if (madvise_buf == HUGE)
> + madvise(buffer, buf_size, MADV_HUGEPAGE);
> +
> + /* Ensure memory is allocated */
> + memset(buffer, 1, buf_size);
> +
> + if (madvise_buf == COLLAPSE)
> + madvise(buffer, buf_size, MADV_COLLAPSE);
> +
> + ret = check_huge_anon(buffer, NR_HUGEPAGES, pmdsize);
> + munmap(buffer, buf_size);
> + return ret;
> +}
Empty line missing :)
> +FIXTURE(prctl_thp_disable_completely)
> +{
> + struct thp_settings settings;
> + size_t pmdsize;
> +};
> +
> +FIXTURE_SETUP(prctl_thp_disable_completely)
> +{
> + if (!thp_is_enabled())
> + SKIP(return, "Transparent Hugepages not available\n");
Heh, not completely correct. enabled != available.
Do we want a thp_available() that is essentially the first part of thp_is_enabled() ?
> +
> + self->pmdsize = read_pmd_pagesize();
> + if (!self->pmdsize)
> + SKIP(return, "Unable to read PMD size\n");
> +
> + thp_read_settings(&self->settings);
> + self->settings.thp_enabled = THP_MADVISE;
> + self->settings.hugepages[sz2ord(self->pmdsize, getpagesize())].enabled = THP_INHERIT;
> + thp_save_settings();
> + thp_push_settings(&self->settings);
push without pop, should that be alarming? :)
Can we just use thp_write_settings()? (not sure why that push/pop is required ... is it?)
> +}
> +
> +FIXTURE_TEARDOWN(prctl_thp_disable_completely)
> +{> + thp_restore_settings();
> +}
> +
> +/* prctl_thp_disable_except_madvise fixture sets system THP setting to madvise */
> +static void prctl_thp_disable_completely(struct __test_metadata *const _metadata,
> + size_t pmdsize)
> +{
> + int res = 0;
> +
> + res = prctl(PR_GET_THP_DISABLE, NULL, NULL, NULL, NULL);
> + ASSERT_EQ(res, 1);
> +
> + /* global = madvise, process = never, we shouldn't get HPs even with madvise */
s/HPs/THPs/
> + res = test_mmap_thp(NONE, pmdsize);
> + ASSERT_EQ(res, 0);
> +
> + res = test_mmap_thp(HUGE, pmdsize);
> + ASSERT_EQ(res, 0);
> +
> + res = test_mmap_thp(COLLAPSE, pmdsize);
> + ASSERT_EQ(res, 0);
> +
> + /* Reset to system policy */
> + res = prctl(PR_SET_THP_DISABLE, 0, NULL, NULL, NULL);
> + ASSERT_EQ(res, 0);
> +
> + /* global = madvise */
> + res = test_mmap_thp(NONE, pmdsize);
> + ASSERT_EQ(res, 0);
> +
> + res = test_mmap_thp(HUGE, pmdsize);
> + ASSERT_EQ(res, 1);
> +
> + res = test_mmap_thp(COLLAPSE, pmdsize);
> + ASSERT_EQ(res, 1);
Makes me wonder: should we test for global=always and global=always?
(or simply for all possible values, including global=never if easily possible?)
At least testing with global=always should exercise more possible paths
than global=always (esp., test_mmap_thp(NONE, pmdsize) which would
never apply in madvise mode).
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 4/5] selftests: prctl: introduce tests for disabling THPs completely
2025-07-28 15:06 ` David Hildenbrand
@ 2025-07-29 22:13 ` Usama Arif
2025-07-30 11:39 ` David Hildenbrand
0 siblings, 1 reply; 20+ messages in thread
From: Usama Arif @ 2025-07-29 22:13 UTC (permalink / raw)
To: David Hildenbrand, Andrew Morton, linux-mm
Cc: linux-fsdevel, corbet, rppt, surenb, mhocko, hannes, baohua,
shakeel.butt, riel, ziy, laoar.shao, dev.jain, baolin.wang,
npache, lorenzo.stoakes, Liam.Howlett, ryan.roberts, vbabka,
jannh, Arnd Bergmann, sj, linux-kernel, linux-doc, kernel-team
>> +
>> + self->pmdsize = read_pmd_pagesize();
>> + if (!self->pmdsize)
>> + SKIP(return, "Unable to read PMD size\n");
>> +
>> + thp_read_settings(&self->settings);
>> + self->settings.thp_enabled = THP_MADVISE;
>> + self->settings.hugepages[sz2ord(self->pmdsize, getpagesize())].enabled = THP_INHERIT;
>> + thp_save_settings();
>> + thp_push_settings(&self->settings);
>
> push without pop, should that be alarming? :)
>
> Can we just use thp_write_settings()? (not sure why that push/pop is required ... is it?)
>
Thanks for the reviews!
Ack on the previous comments, I have fixed them and will include in next revision.
Yes, I think we can replace thp_push_settings with thp_write_settings.
For this, I actually just copied what cow.c and uffd-wp-mremap.c are doing :)
You can see in these 2 files that we do [1]
- thp_read_settings / thp_save_settings
- thp_push_settings
Than we run the experiment
and at the end we do [2]
- thp_restore_settings
i.e. there is no pop.
I think we can change the thp_push_settings to thp_write_settings in [3] and [4] as well?
I can fix and test it if it makes sense. It should prevent people like me from making a
similar mistake when they just copy from it :)
[1] https://elixir.bootlin.com/linux/v6.16/source/tools/testing/selftests/mm/cow.c#L1884
[2] https://elixir.bootlin.com/linux/v6.16/source/tools/testing/selftests/mm/cow.c#L1911
[3] https://elixir.bootlin.com/linux/v6.16/source/tools/testing/selftests/mm/cow.c#L1886
[4] https://elixir.bootlin.com/linux/v6.16/source/tools/testing/selftests/mm/uffd-wp-mremap.c#L355
>> +}
>> +
>> +FIXTURE_TEARDOWN(prctl_thp_disable_completely)
>> +{> + thp_restore_settings();
>> +}
>> +
>> +/* prctl_thp_disable_except_madvise fixture sets system THP setting to madvise */
>> +static void prctl_thp_disable_completely(struct __test_metadata *const _metadata,
>> + size_t pmdsize)
>> +{
>> + int res = 0;
>> +
>> + res = prctl(PR_GET_THP_DISABLE, NULL, NULL, NULL, NULL);
>> + ASSERT_EQ(res, 1);
>> +
>> + /* global = madvise, process = never, we shouldn't get HPs even with madvise */
>
> s/HPs/THPs/
>
>> + res = test_mmap_thp(NONE, pmdsize);
>> + ASSERT_EQ(res, 0);
>> +
>> + res = test_mmap_thp(HUGE, pmdsize);
>> + ASSERT_EQ(res, 0);
>> +
>> + res = test_mmap_thp(COLLAPSE, pmdsize);
>> + ASSERT_EQ(res, 0);
>> +
>> + /* Reset to system policy */
>> + res = prctl(PR_SET_THP_DISABLE, 0, NULL, NULL, NULL);
>> + ASSERT_EQ(res, 0);
>> +
>> + /* global = madvise */
>> + res = test_mmap_thp(NONE, pmdsize);
>> + ASSERT_EQ(res, 0);
>> +
>> + res = test_mmap_thp(HUGE, pmdsize);
>> + ASSERT_EQ(res, 1);
>> +
>> + res = test_mmap_thp(COLLAPSE, pmdsize);
>> + ASSERT_EQ(res, 1);
>
>
> Makes me wonder: should we test for global=always and global=always?
Do you mean global=madvise and global=always?>
> (or simply for all possible values, including global=never if easily possible?)
>
> At least testing with global=always should exercise more possible paths
> than global=always (esp., test_mmap_thp(NONE, pmdsize) which would
> never apply in madvise mode).
>
lol I think over here as well you meant madvise in the 2nd instance.
I was just looking at other selftests and I saw FIXTURE_VARIANT_ADD, I think we can
use that to do it without replicating too much code. Let me see if I
can use that and do it for never, madvise and always. If it doesnt help
there might be some code replication, but that should be ok.
Thanks!
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 4/5] selftests: prctl: introduce tests for disabling THPs completely
2025-07-29 22:13 ` Usama Arif
@ 2025-07-30 11:39 ` David Hildenbrand
0 siblings, 0 replies; 20+ messages in thread
From: David Hildenbrand @ 2025-07-30 11:39 UTC (permalink / raw)
To: Usama Arif, Andrew Morton, linux-mm, ryan.roberts
Cc: linux-fsdevel, corbet, rppt, surenb, mhocko, hannes, baohua,
shakeel.butt, riel, ziy, laoar.shao, dev.jain, baolin.wang,
npache, lorenzo.stoakes, Liam.Howlett, vbabka, jannh,
Arnd Bergmann, sj, linux-kernel, linux-doc, kernel-team
On 30.07.25 00:13, Usama Arif wrote:
>>> +
>>> + self->pmdsize = read_pmd_pagesize();
>>> + if (!self->pmdsize)
>>> + SKIP(return, "Unable to read PMD size\n");
>>> +
>>> + thp_read_settings(&self->settings);
>>> + self->settings.thp_enabled = THP_MADVISE;
>>> + self->settings.hugepages[sz2ord(self->pmdsize, getpagesize())].enabled = THP_INHERIT;
>>> + thp_save_settings();
>>> + thp_push_settings(&self->settings);
>>
>> push without pop, should that be alarming? :)
>>
>> Can we just use thp_write_settings()? (not sure why that push/pop is required ... is it?)
>>
>
> Thanks for the reviews!
> Ack on the previous comments, I have fixed them and will include in next revision.
> Yes, I think we can replace thp_push_settings with thp_write_settings.
>
> For this, I actually just copied what cow.c and uffd-wp-mremap.c are doing :)
Right, I see push vs. pop in run_anon_test_case(), but push vs. restore
from main(). At least cow.c applies a configuration on top of another
one, so it needs the push+pop semantics.
In your case, we really only perform a single configuration. So
write+restore should be good enough I guess.
>
> You can see in these 2 files that we do [1]
> - thp_read_settings / thp_save_settings
> - thp_push_settings
>
> Than we run the experiment
>
> and at the end we do [2]
> - thp_restore_settings
>
> i.e. there is no pop.
>
> I think we can change the thp_push_settings to thp_write_settings in [3] and [4] as well?
I think we have to push there, so the following push+pop will do the
right thing (I think that was the whole idea of push+pop).
An alternative would have been to just have write+restore, whereby write
always returns the old state you save in a local variable.
I wonder if a final pop could be used instead of the restore somehow.
Anyhow, probably best to leave the other test cases alone for now,
unless you want to clean it up properly :)
[...]
>>> +}
>>> +
>>> +FIXTURE_TEARDOWN(prctl_thp_disable_completely)
>>> +{> + thp_restore_settings();
>>> +}
>>> +
>>> +/* prctl_thp_disable_except_madvise fixture sets system THP setting to madvise */
>>> +static void prctl_thp_disable_completely(struct __test_metadata *const _metadata,
>>> + size_t pmdsize)
>>> +{
>>> + int res = 0;
>>> +
>>> + res = prctl(PR_GET_THP_DISABLE, NULL, NULL, NULL, NULL);
>>> + ASSERT_EQ(res, 1);
>>> +
>>> + /* global = madvise, process = never, we shouldn't get HPs even with madvise */
>>
>> s/HPs/THPs/
>>
>>> + res = test_mmap_thp(NONE, pmdsize);
>>> + ASSERT_EQ(res, 0);
>>> +
>>> + res = test_mmap_thp(HUGE, pmdsize);
>>> + ASSERT_EQ(res, 0);
>>> +
>>> + res = test_mmap_thp(COLLAPSE, pmdsize);
>>> + ASSERT_EQ(res, 0);
>>> +
>>> + /* Reset to system policy */
>>> + res = prctl(PR_SET_THP_DISABLE, 0, NULL, NULL, NULL);
>>> + ASSERT_EQ(res, 0);
>>> +
>>> + /* global = madvise */
>>> + res = test_mmap_thp(NONE, pmdsize);
>>> + ASSERT_EQ(res, 0);
>>> +
>>> + res = test_mmap_thp(HUGE, pmdsize);
>>> + ASSERT_EQ(res, 1);
>>> +
>>> + res = test_mmap_thp(COLLAPSE, pmdsize);
>>> + ASSERT_EQ(res, 1);
>>
>>
>> Makes me wonder: should we test for global=always and global=always?
>
> Do you mean global=madvise and global=always?>
Yeah, rewrote it 3 times and then messed it up.
>> (or simply for all possible values, including global=never if easily possible?)
>>
>> At least testing with global=always should exercise more possible paths
>> than global=always (esp., test_mmap_thp(NONE, pmdsize) which would
>> never apply in madvise mode).
>>
>
> lol I think over here as well you meant madvise in the 2nd instance.
Yeah :)
>
> I was just looking at other selftests and I saw FIXTURE_VARIANT_ADD, I think we can
> use that to do it without replicating too much code. Let me see if I
> can use that and do it for never, madvise and always. If it doesnt help
> there might be some code replication, but that should be ok.
Yeah, some easy way without replicating would be very nice.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/5] selftests: prctl: introduce tests for disabling THPs except for madvise
2025-07-25 16:22 [PATCH 0/5] prctl: extend PR_SET_THP_DISABLE to only provide THPs when advised Usama Arif
` (3 preceding siblings ...)
2025-07-25 16:22 ` [PATCH 4/5] selftests: prctl: introduce tests for disabling THPs completely Usama Arif
@ 2025-07-25 16:22 ` Usama Arif
2025-07-28 16:55 ` SeongJae Park
4 siblings, 1 reply; 20+ messages in thread
From: Usama Arif @ 2025-07-25 16:22 UTC (permalink / raw)
To: Andrew Morton, david, linux-mm
Cc: linux-fsdevel, corbet, rppt, surenb, mhocko, hannes, baohua,
shakeel.butt, riel, ziy, laoar.shao, dev.jain, baolin.wang,
npache, lorenzo.stoakes, Liam.Howlett, ryan.roberts, vbabka,
jannh, Arnd Bergmann, sj, linux-kernel, linux-doc, kernel-team,
Usama Arif
The test will set the global system THP setting to always and
the 2M setting to inherit before it starts (and reset to original
at teardown)
This tests if the process can:
- successfully set and get the policy to disable THPs expect for madvise.
- get hugepages only on MADV_HUGE and MADV_COLLAPSE after policy is set.
- successfully reset the policy of the process.
- get hugepages always after reset.
- repeat the above tests in a forked process to make sure the policy is
carried across forks.
Signed-off-by: Usama Arif <usamaarif642@gmail.com>
---
.../testing/selftests/mm/prctl_thp_disable.c | 95 +++++++++++++++++++
1 file changed, 95 insertions(+)
diff --git a/tools/testing/selftests/mm/prctl_thp_disable.c b/tools/testing/selftests/mm/prctl_thp_disable.c
index 52f7e6659b1f..288d5ad6ffbb 100644
--- a/tools/testing/selftests/mm/prctl_thp_disable.c
+++ b/tools/testing/selftests/mm/prctl_thp_disable.c
@@ -65,6 +65,101 @@ static int test_mmap_thp(enum madvise_buffer madvise_buf, size_t pmdsize)
munmap(buffer, buf_size);
return ret;
}
+
+FIXTURE(prctl_thp_disable_except_madvise)
+{
+ struct thp_settings settings;
+ size_t pmdsize;
+};
+
+FIXTURE_SETUP(prctl_thp_disable_except_madvise)
+{
+ if (!thp_is_enabled())
+ SKIP(return, "Transparent Hugepages not available\n");
+
+ self->pmdsize = read_pmd_pagesize();
+ if (!self->pmdsize)
+ SKIP(return, "Unable to read PMD size\n");
+
+ thp_read_settings(&self->settings);
+ self->settings.thp_enabled = THP_ALWAYS;
+ self->settings.hugepages[sz2ord(self->pmdsize, getpagesize())].enabled = THP_INHERIT;
+ thp_save_settings();
+ thp_push_settings(&self->settings);
+
+}
+
+FIXTURE_TEARDOWN(prctl_thp_disable_except_madvise)
+{
+ thp_restore_settings();
+}
+
+/* prctl_thp_disable_except_madvise fixture sets system THP setting to always */
+static void prctl_thp_disable_except_madvise(struct __test_metadata *const _metadata,
+ size_t pmdsize)
+{
+ int res = 0;
+
+ res = prctl(PR_GET_THP_DISABLE, NULL, NULL, NULL, NULL);
+ ASSERT_EQ(res, 3);
+
+ /* global = always, process = madvise, we shouldn't get HPs without madvise */
+ res = test_mmap_thp(NONE, pmdsize);
+ ASSERT_EQ(res, 0);
+
+ res = test_mmap_thp(HUGE, pmdsize);
+ ASSERT_EQ(res, 1);
+
+ res = test_mmap_thp(COLLAPSE, pmdsize);
+ ASSERT_EQ(res, 1);
+
+ /* Reset to system policy */
+ res = prctl(PR_SET_THP_DISABLE, 0, NULL, NULL, NULL);
+ ASSERT_EQ(res, 0);
+
+ /* global = always, hence we should get HPs without madvise */
+ res = test_mmap_thp(NONE, pmdsize);
+ ASSERT_EQ(res, 1);
+
+ res = test_mmap_thp(HUGE, pmdsize);
+ ASSERT_EQ(res, 1);
+
+ res = test_mmap_thp(COLLAPSE, pmdsize);
+ ASSERT_EQ(res, 1);
+}
+
+TEST_F(prctl_thp_disable_except_madvise, nofork)
+{
+ int res = 0;
+
+ res = prctl(PR_SET_THP_DISABLE, 1, PR_THP_DISABLE_EXCEPT_ADVISED, NULL, NULL);
+ ASSERT_EQ(res, 0);
+ prctl_thp_disable_except_madvise(_metadata, self->pmdsize);
+}
+
+TEST_F(prctl_thp_disable_except_madvise, fork)
+{
+ int res = 0, ret = 0;
+ pid_t pid;
+
+ res = prctl(PR_SET_THP_DISABLE, 1, PR_THP_DISABLE_EXCEPT_ADVISED, NULL, NULL);
+ ASSERT_EQ(res, 0);
+
+ /* Make sure prctl changes are carried across fork */
+ pid = fork();
+ ASSERT_GE(pid, 0);
+
+ if (!pid)
+ prctl_thp_disable_except_madvise(_metadata, self->pmdsize);
+
+ wait(&ret);
+ if (WIFEXITED(ret))
+ ret = WEXITSTATUS(ret);
+ else
+ ret = -EINVAL;
+ ASSERT_EQ(ret, 0);
+}
+
FIXTURE(prctl_thp_disable_completely)
{
struct thp_settings settings;
--
2.47.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] selftests: prctl: introduce tests for disabling THPs except for madvise
2025-07-25 16:22 ` [PATCH 5/5] selftests: prctl: introduce tests for disabling THPs except for madvise Usama Arif
@ 2025-07-28 16:55 ` SeongJae Park
2025-07-29 22:17 ` Usama Arif
0 siblings, 1 reply; 20+ messages in thread
From: SeongJae Park @ 2025-07-28 16:55 UTC (permalink / raw)
To: Usama Arif
Cc: SeongJae Park, Andrew Morton, david, linux-mm, linux-fsdevel,
corbet, rppt, surenb, mhocko, hannes, baohua, shakeel.butt, riel,
ziy, laoar.shao, dev.jain, baolin.wang, npache, lorenzo.stoakes,
Liam.Howlett, ryan.roberts, vbabka, jannh, Arnd Bergmann,
linux-kernel, linux-doc, kernel-team
On Fri, 25 Jul 2025 17:22:44 +0100 Usama Arif <usamaarif642@gmail.com> wrote:
> The test will set the global system THP setting to always and
> the 2M setting to inherit before it starts (and reset to original
> at teardown)
>
> This tests if the process can:
> - successfully set and get the policy to disable THPs expect for madvise.
s/expect/except/
> - get hugepages only on MADV_HUGE and MADV_COLLAPSE after policy is set.
> - successfully reset the policy of the process.
> - get hugepages always after reset.
> - repeat the above tests in a forked process to make sure the policy is
> carried across forks.
>
> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> ---
> .../testing/selftests/mm/prctl_thp_disable.c | 95 +++++++++++++++++++
> 1 file changed, 95 insertions(+)
>
> diff --git a/tools/testing/selftests/mm/prctl_thp_disable.c b/tools/testing/selftests/mm/prctl_thp_disable.c
> index 52f7e6659b1f..288d5ad6ffbb 100644
> --- a/tools/testing/selftests/mm/prctl_thp_disable.c
> +++ b/tools/testing/selftests/mm/prctl_thp_disable.c
> @@ -65,6 +65,101 @@ static int test_mmap_thp(enum madvise_buffer madvise_buf, size_t pmdsize)
> munmap(buffer, buf_size);
> return ret;
> }
> +
> +FIXTURE(prctl_thp_disable_except_madvise)
> +{
> + struct thp_settings settings;
> + size_t pmdsize;
> +};
> +
> +FIXTURE_SETUP(prctl_thp_disable_except_madvise)
> +{
> + if (!thp_is_enabled())
> + SKIP(return, "Transparent Hugepages not available\n");
As David also pointed out on the other patch, the message and the function name
would better to be consistent.
> +
> + self->pmdsize = read_pmd_pagesize();
> + if (!self->pmdsize)
> + SKIP(return, "Unable to read PMD size\n");
> +
> + thp_read_settings(&self->settings);
> + self->settings.thp_enabled = THP_ALWAYS;
> + self->settings.hugepages[sz2ord(self->pmdsize, getpagesize())].enabled = THP_INHERIT;
> + thp_save_settings();
> + thp_push_settings(&self->settings);
> +
Unnecessary empty line?
> +}
> +
> +FIXTURE_TEARDOWN(prctl_thp_disable_except_madvise)
> +{
> + thp_restore_settings();
> +}
> +
> +/* prctl_thp_disable_except_madvise fixture sets system THP setting to always */
> +static void prctl_thp_disable_except_madvise(struct __test_metadata *const _metadata,
> + size_t pmdsize)
> +{
> + int res = 0;
> +
> + res = prctl(PR_GET_THP_DISABLE, NULL, NULL, NULL, NULL);
> + ASSERT_EQ(res, 3);
> +
> + /* global = always, process = madvise, we shouldn't get HPs without madvise */
> + res = test_mmap_thp(NONE, pmdsize);
> + ASSERT_EQ(res, 0);
> +
> + res = test_mmap_thp(HUGE, pmdsize);
> + ASSERT_EQ(res, 1);
> +
> + res = test_mmap_thp(COLLAPSE, pmdsize);
> + ASSERT_EQ(res, 1);
> +
> + /* Reset to system policy */
> + res = prctl(PR_SET_THP_DISABLE, 0, NULL, NULL, NULL);
> + ASSERT_EQ(res, 0);
> +
> + /* global = always, hence we should get HPs without madvise */
> + res = test_mmap_thp(NONE, pmdsize);
> + ASSERT_EQ(res, 1);
> +
> + res = test_mmap_thp(HUGE, pmdsize);
> + ASSERT_EQ(res, 1);
> +
> + res = test_mmap_thp(COLLAPSE, pmdsize);
> + ASSERT_EQ(res, 1);
Seems res is not being used other than saving the return value for assertions.
Why don't you do the assertion at once, e.g., ASSERT_EQ(test_mmap_thp(...), 1)?
No strong opinion, but I think that could make code shorter and easier to read.
> +}
> +
> +TEST_F(prctl_thp_disable_except_madvise, nofork)
> +{
> + int res = 0;
> +
> + res = prctl(PR_SET_THP_DISABLE, 1, PR_THP_DISABLE_EXCEPT_ADVISED, NULL, NULL);
> + ASSERT_EQ(res, 0);
Again, I think 'res' can be removed.
> + prctl_thp_disable_except_madvise(_metadata, self->pmdsize);
> +}
> +
> +TEST_F(prctl_thp_disable_except_madvise, fork)
> +{
> + int res = 0, ret = 0;
> + pid_t pid;
> +
> + res = prctl(PR_SET_THP_DISABLE, 1, PR_THP_DISABLE_EXCEPT_ADVISED, NULL, NULL);
> + ASSERT_EQ(res, 0);
Ditto.
> +
> + /* Make sure prctl changes are carried across fork */
> + pid = fork();
> + ASSERT_GE(pid, 0);
> +
> + if (!pid)
> + prctl_thp_disable_except_madvise(_metadata, self->pmdsize);
> +
> + wait(&ret);
> + if (WIFEXITED(ret))
> + ret = WEXITSTATUS(ret);
> + else
> + ret = -EINVAL;
> + ASSERT_EQ(ret, 0);
> +}
> +
> FIXTURE(prctl_thp_disable_completely)
> {
> struct thp_settings settings;
> --
> 2.47.3
Thanks,
SJ
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/5] selftests: prctl: introduce tests for disabling THPs except for madvise
2025-07-28 16:55 ` SeongJae Park
@ 2025-07-29 22:17 ` Usama Arif
0 siblings, 0 replies; 20+ messages in thread
From: Usama Arif @ 2025-07-29 22:17 UTC (permalink / raw)
To: SeongJae Park
Cc: Andrew Morton, david, linux-mm, linux-fsdevel, corbet, rppt,
surenb, mhocko, hannes, baohua, shakeel.butt, riel, ziy,
laoar.shao, dev.jain, baolin.wang, npache, lorenzo.stoakes,
Liam.Howlett, ryan.roberts, vbabka, jannh, Arnd Bergmann,
linux-kernel, linux-doc, kernel-team
>> + res = test_mmap_thp(COLLAPSE, pmdsize);
>> + ASSERT_EQ(res, 1);
>
> Seems res is not being used other than saving the return value for assertions.
> Why don't you do the assertion at once, e.g., ASSERT_EQ(test_mmap_thp(...), 1)?
> No strong opinion, but I think that could make code shorter and easier to read.
>
Yeah this is on purpose, I found it easier to evaluate what the output should be
if I wrote code like this. As compiler will likely just optimize it out (although
we dont really care about performance :) ), although others might find without res
better.
I think you had mentioned it in the earlier thread as well. I will remove it.>> +}
>> +
>> +TEST_F(prctl_thp_disable_except_madvise, nofork)
>> +{
>> + int res = 0;
>> +
>> + res = prctl(PR_SET_THP_DISABLE, 1, PR_THP_DISABLE_EXCEPT_ADVISED, NULL, NULL);
>> + ASSERT_EQ(res, 0);
>
> Again, I think 'res' can be removed.
>
>> + prctl_thp_disable_except_madvise(_metadata, self->pmdsize);
>> +}
>> +
>> +TEST_F(prctl_thp_disable_except_madvise, fork)
>> +{
>> + int res = 0, ret = 0;
>> + pid_t pid;
>> +
>> + res = prctl(PR_SET_THP_DISABLE, 1, PR_THP_DISABLE_EXCEPT_ADVISED, NULL, NULL);
>> + ASSERT_EQ(res, 0);
>
> Ditto.
>
>> +
>> + /* Make sure prctl changes are carried across fork */
>> + pid = fork();
>> + ASSERT_GE(pid, 0);
>> +
>> + if (!pid)
>> + prctl_thp_disable_except_madvise(_metadata, self->pmdsize);
>> +
>> + wait(&ret);
>> + if (WIFEXITED(ret))
>> + ret = WEXITSTATUS(ret);
>> + else
>> + ret = -EINVAL;
>> + ASSERT_EQ(ret, 0);
>> +}
>> +
>> FIXTURE(prctl_thp_disable_completely)
>> {
>> struct thp_settings settings;
>> --
>> 2.47.3
>
>
> Thanks,
> SJ
^ permalink raw reply [flat|nested] 20+ messages in thread