Re: [PATCH 4/5] selftests: prctl: introduce tests for disabling THPs completely
From: David Hildenbrand
Date: Wed Jul 30 2025 - 07:40:01 EST
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