Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fa6009d4-643e-97ec-5317-a57a535e0495@redhat.com>
Date:   Thu, 1 Jun 2023 10:16:41 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Content-Language: en-US
To:     Lorenzo Stoakes <lstoakes@gmail.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        linux-kselftest@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Shuah Khan <shuah@kernel.org>, Jens Axboe <axboe@kernel.dk>,
        Peter Xu <peterx@redhat.com>, Jason Gunthorpe <jgg@nvidia.com>,
        John Hubbard <jhubbard@nvidia.com>, Jan Kara <jack@suse.cz>
References: <20230519102723.185721-1-david@redhat.com>
 <20230519102723.185721-3-david@redhat.com>
 <be2346e4-e8c0-4470-9bf4-59eb864063a8@lucifer.local>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH v1 2/3] selftests/mm: gup_longterm: new functional test
 for FOLL_LONGTERM
In-Reply-To: <be2346e4-e8c0-4470-9bf4-59eb864063a8@lucifer.local>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 28.05.23 17:03, Lorenzo Stoakes wrote:
> On Fri, May 19, 2023 at 12:27:22PM +0200, David Hildenbrand wrote:
>> Let's add a new test for checking whether GUP long-term page pinning
>> works as expected (R/O vs. R/W, MAP_PRIVATE vs. MAP_SHARED, GUP vs.
>> GUP-fast). Note that COW handling with long-term R/O pinning in private
>> mappings, and pinning of anonymous memory in general, is tested by the
>> COW selftest. This test, therefore, focuses on page pinning in
>> file mappings.
>>
>> The most interesting case is probably the "local tmpfile" case, as that
>> will likely end up on a "real" filesystem such as ext4 or xfs, not on a
>> virtual one like tmpfs or hugetlb where any long-term page pinning is
>> always expected to succeed.
>>
>> For now, only add tests that use the "/sys/kernel/debug/gup_test"
>> interface. We'll add tests based on liburing separately next.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---

[...]

>> +static void do_test(int fd, size_t size, enum test_type type, bool shared)
>> +{
>> +	__fsword_t fs_type = get_fs_type(fd);
>> +	bool should_work;
>> +	char *mem;
>> +	int ret;
>> +
>> +	if (ftruncate(fd, size)) {
>> +		ksft_test_result_fail("ftruncate() failed\n");
>> +		return;
>> +	}
>> +
>> +	if (fallocate(fd, 0, 0, size)) {
>> +		if (size == pagesize)
>> +			ksft_test_result_fail("fallocate() failed\n");
>> +		else
>> +			ksft_test_result_skip("need more free huge pages\n");
>> +		return;
>> +	}
>> +
>> +	mem = mmap(NULL, size, PROT_READ | PROT_WRITE,
>> +		   shared ? MAP_SHARED : MAP_PRIVATE, fd, 0);
>> +	if (mem == MAP_FAILED) {
>> +		if (size == pagesize || shared)
>> +			ksft_test_result_fail("mmap() failed\n");
>> +		else
>> +			ksft_test_result_skip("need more free huge pages\n");
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * Fault in the page writable such that GUP-fast can eventually pin
>> +	 * it immediately.
>> +	 */
>> +	memset(mem, 0, size);
> 

For shared mappings, MAP_POPULATE will not fault-in the pages writable. 
See mm/gup.c:populate_vma_page_range().

[There is also the case that mmap() doesn't fail if populate fails, but 
that's only a side note regarding weird semantics of MAP_POPULATE]

[...]

>> +	int flags = MFD_HUGETLB;
>> +	int fd;
>> +
>> +	ksft_print_msg("[RUN] %s ... with memfd hugetlb (%zu kB)\n", desc,
>> +		       hugetlbsize / 1024);
>> +
>> +	flags |= __builtin_ctzll(hugetlbsize) << MFD_HUGE_SHIFT;
> 
> Hm this feels a little cute :)

It's a weird interfacing, having to specify the desired size via flags 
... see the man page of memfd_create, which links to the man page of 
mmap: "the desired huge page size can be configured by encoding the 
base-2 logarithm of the desired page size in the six bits at the offset 
MAP_HUGE_SHIFT".

FWIW, we're using the same approach in cow.c already [and other memfd 
users like QEMU do it just like that, using ctz].

[...]

>> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
>> index 4893eb60d96d..b6b1eb6a8a6b 100644
>> --- a/tools/testing/selftests/mm/run_vmtests.sh
>> +++ b/tools/testing/selftests/mm/run_vmtests.sh
>> @@ -24,7 +24,7 @@ separated by spaces:
>>   - mmap
>>   	tests for mmap(2)
>>   - gup_test
>> -	tests for gup using gup_test interface
>> +	tests for gup
> 
> Super nitty again, but I'm guessing this means the CONFIG_GUP_TEST
> interface, perhaps worth keeping?

With this patch, agreed. But not longer with the next patch -- guess I 
simplified when splitting it up. If there are no strong feelings I'll 
leave it in this patch.

[...]

>>
> 
> OK this patch is really nice + well implemented, I can only point out a
> couple EXTREMELY nitty comments :) Thanks very much for adding a test for
> this, it's super useful!
> 
> Therefore,
> 
> Reviewed-by: Lorenzo Stoakes <lstoakes@gmail.com>
> 

Thanks for the review! My selftest patches rarely get that much 
attention, so highly appreciated :)

-- 
Thanks,

David / dhildenb