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

[David Hildenbrand <david@redhat.com>]

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.

>   {
> -	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.

>   
> -		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.

[...]

>   
> +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"

-- 
Cheers

David / dhildenb