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

[David Hildenbrand <david@redhat.com>]

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