Re: [PATCH 4/5] selftests: prctl: introduce tests for disabling THPs completely

[Usama Arif]

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