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