Re: [PATCH v10 6/6] selftests/mm: add a GUP selftest

[Sarthak Sharma <sarthak.sharma@arm.com>]

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) <david@kernel.org>

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.