On 9/11/26 9:18 PM, David Hildenbrand (Arm) wrote:
[...]
>> +
>> +/* Just the flags we need, copied from the kernel internals. */
>> +#define FOLL_WRITE 0x01 /* check pte is writable */
>
> BTW, it's odd that we support passing GUP-flags ... we should probably switch
> at
> some point simpler attributes (bool write) or custom flags (but we don't seem
> to
> need many ...).
Agreed
>
>> +
>> +/* Page counts exercising single, THP-batch, partial, and full-mapping GUP.
>> */
>> +static const int nr_pages_list[] = { 1, 512, 123, -1 };
>
>
> Would we want to calculate 512 dynamically at runtime using the PMD pagesize?
> Could be something for a follow-up patch.
Yeah this can be done.
>
>> +
>> +#define GUP_TEST_FILE "/sys/kernel/debug/gup_test"
>> +#define NR_HUGE_PAGES 2
>
> I'd call this "NR_HUGETLB_PAGES".
Okay.
[...]
>> +static void run_gup_cmd(struct __test_metadata *_metadata,
>> + FIXTURE_DATA(gup_test) *self,
>> + const FIXTURE_VARIANT(gup_test) *variant,
>> + unsigned long command)
>
> We prefer two tab indents.
Okay.
>
>> +{
>> + int i;
>> +
>> + for (i = 0; i < (int)ARRAY_SIZE(nr_pages_list); i++) {
>> + struct gup_test gup = {
>> + .addr = (unsigned long)self->addr,
>> + .size = self->size,
>> + .nr_pages_per_call = nr_pages_list[i] < 0 ?
>> + self->size / psize() : nr_pages_list[i],
>> + .gup_flags = variant->write ? FOLL_WRITE : 0,
>> + };
>> +
>> + TH_LOG("nr_pages_per_call=%u", gup.nr_pages_per_call);
>> + ASSERT_EQ(ioctl(self->gup_fd, command, &gup), 0);
>> + ASSERT_EQ(gup.size, self->size);
>> + }
>> +}
>> +
>> +TEST_F(gup_test, get_user_pages)
>> +{
>> + run_gup_cmd(_metadata, self, variant, GUP_BASIC_TEST);
>> +}
>> +
>> +TEST_F(gup_test, pin_user_pages)
>> +{
>> + run_gup_cmd(_metadata, self, variant, PIN_BASIC_TEST);
>> +}
>> +
>> +TEST_F(gup_test, get_user_pages_fast)
>> +{
>> + run_gup_cmd(_metadata, self, variant, GUP_FAST_BENCHMARK);
>> +}
>> +
>> +TEST_F(gup_test, pin_user_pages_fast)
>> +{
>> + run_gup_cmd(_metadata, self, variant, PIN_FAST_BENCHMARK);
>> +}
>> +
>> +TEST_F(gup_test, pin_user_pages_longterm)
>> +{
>> + run_gup_cmd(_metadata, self, variant, PIN_LONGTERM_BENCHMARK);
>> +}
>
> Heh, is there actually a reason why these kernel things are called _BENCHMARK?
>
> I think they are really just tests that can be used for benchmarking ... the
> measurement logic is entirely in user space.
>
> We could consider cleaning that up as a follow-up.
Yeah this can be worked on as well.
>
>> +
>> +int main(int argc, char **argv)
>> +{
>> + int fd;
>> +
>> + fd = open(GUP_TEST_FILE, O_RDWR);
>
> Could do
>
> const int fd = open(GUP_TEST_FILE, O_RDWR);
Okay.
>
>
> Thanks for doing that!
>
> Acked-by: David Hildenbrand (Arm) <[email protected]>
Thank you David!
>
>
> I think reasonable extensions will be to execute tests on all available mTHP
> sizes and all available hugetlb sizes, similar to what cow.c already does.
>
> Can you look into that as part of some follow-up work? mTHP support will be
> interesting for testing some of the patches Rik has been working on.
Yup, I'll take that up.
Apart from the indentation, const int fd and the NR_HUGE_PAGES rename,
is there anything else I should incorporate in a respin? I think other
changes would require a separate series. If you want me to fold any
other change in a respin, please let me know.
