Re: [PATCH v4 3/5] selftests/mm: reimplement is_backed_by_thp() with more precise check

[Zi Yan <ziy@nvidia.com>]

On 18 Aug 2025, at 4:33, David Hildenbrand wrote:

> On 15.08.25 04:39, Zi Yan wrote:
>> and rename it to is_backed_by_folio().
>>
>> is_backed_by_folio() checks if the given vaddr is backed a folio with
>> a given order. It does so by:
>> 1. getting the pfn of the vaddr;
>> 2. checking kpageflags of the pfn;
>>
>> if order is greater than 0:
>> 3. checking kpageflags of the head pfn;
>> 4. checking kpageflags of all tail pfns.
>>
>> pmd_order is added to split_huge_page_test.c and replaces max_order.
>>
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>> ---
>>   .../selftests/mm/split_huge_page_test.c       | 90 ++++++++++++++-----
>>   tools/testing/selftests/mm/vm_util.c          | 13 +++
>>   tools/testing/selftests/mm/vm_util.h          |  4 +
>>   3 files changed, 84 insertions(+), 23 deletions(-)
>>
>> diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
>> index 89d3dc08fe4c..80f718ca21c7 100644
>> --- a/tools/testing/selftests/mm/split_huge_page_test.c
>> +++ b/tools/testing/selftests/mm/split_huge_page_test.c
>> @@ -25,6 +25,7 @@
>>   uint64_t pagesize;
>>   unsigned int pageshift;
>>   uint64_t pmd_pagesize;
>> +unsigned int pmd_order;
>>    #define SPLIT_DEBUGFS "/sys/kernel/debug/split_huge_pages"
>>   #define SMAP_PATH "/proc/self/smaps"
>> @@ -34,27 +35,71 @@ uint64_t pmd_pagesize;
>>   #define PID_FMT_OFFSET "%d,0x%lx,0x%lx,%d,%d"
>>   #define PATH_FMT "%s,0x%lx,0x%lx,%d"
>>  -#define PFN_MASK     ((1UL<<55)-1)
>> -#define KPF_THP      (1UL<<22)
>>   #define GET_ORDER(nr_pages)    (31 - __builtin_clz(nr_pages))
>>  -static int is_backed_by_thp(char *vaddr, int pagemap_file, int kpageflags_file)
>> +static int is_backed_by_folio(char *vaddr, int order, int pagemap_fd,
>> +		int kpageflags_fd)
>
> Could we convert this into a bool and simply return "false" on error instead of "-"? These tristate returns for a "is_*" function are a bit unfortunate.

OK.

>
>>   {
>> -	uint64_t paddr;
>> -	uint64_t page_flags;
>> +	unsigned long pfn_head;
>> +	uint64_t pfn_flags;
>> +	unsigned long pfn;
>> +	unsigned long i;
>>  -	if (pagemap_file) {
>> -		pread(pagemap_file, &paddr, sizeof(paddr),
>> -			((long)vaddr >> pageshift) * sizeof(paddr));
>> +	if (pagemap_fd == -1 || kpageflags_fd == -1)
>> +		goto fail;
>
> Should we rather expect that callers make sure these are valid? In particular, because split_pte_mapped_thp() seems to ksft_exit_fail_msg() already.

Sure.

>
>>  -		if (kpageflags_file) {
>> -			pread(kpageflags_file, &page_flags, sizeof(page_flags),
>> -				(paddr & PFN_MASK) * sizeof(page_flags));
>> +	pfn = pagemap_get_pfn(pagemap_fd, vaddr);
>
> Hm, if it's swapped out we would get intermittent errors, but that just seems hard to avoid. The caller could mock to avoid swapout.
>
> Memory migration is another possible problem ...
>
> But this is nothing new regarding your patch, so no need to worry for now.

Right. The function is only used by PTE-mapped THP split and I assume swapping
and migration would not happen. If this function is used more broadly, it
will need to take care of cases.

>
> [...]
>
>>  +int get_pfn_flags(unsigned long pfn, int kpageflags_fd, uint64_t *flags)
>> +{
>> +	size_t count;
>> +
>> +	count = pread(kpageflags_fd, flags, sizeof(*flags),
>> +		      pfn * sizeof(*flags));
>> +
>> +	if (count != sizeof(*flags))
>> +		return -1;
>> +
>> +	return 0;
>> +}
>
> I would have called this function "pageflags_get()" to resemble "pagemap_get"

OK. Will rename it.

Thanks.

--
Best Regards,
Yan, Zi