Re: [RFC PATCH 1/1] mm/debug_vm_pgtable: Use a swp_entry_t input value for swap tests

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 24/06/25 12:13 AM, Gerald Schaefer wrote:
> The various __pte/pmd_to_swp_entry and __swp_entry_to_pte/pmd helper
> functions are expected to operate on swapped PTE/PMD entries, not on
> present and mapped entries.
> 
> Reflect this in the swap tests by using a swp_entry_t as input value,
> similar to how it is already done in pte_swap_exclusive_tests().
> Move the swap entry creation to init_args() and store it in args, so
> it can also be used in other functions.

Makes sense.

> 
> The pte/pmd_swap_tests() are also changed to compare entries instead of
> pfn values, because pte/pmd_pfn() helpers are not expected to operate on

Swap entries compare is also happening now in pte_swap_exclusive_tests().

> swapped entries. E.g. on s390, pmd_pfn() needs different shifts for leaf
> (large) and non-leaf PMDs.
> 
> Also update documentation, to reflect that the helpers operate on
> swapped and not mapped entries, and use correct names, i.e.
> __swp_to_pte/pmd_entry -> __swp_entry_to_pte/pmd.
> 
> For consistency, also change pte/pmd_swap_soft_dirty_tests() to use
> args->swp_entry instead of a present and mapped PTE/PMD.

Makes sense.

> 
> Signed-off-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
> ---
>  Documentation/mm/arch_pgtable_helpers.rst |  8 ++--
>  mm/debug_vm_pgtable.c                     | 55 ++++++++++++++---------
>  2 files changed, 38 insertions(+), 25 deletions(-)
> 
> diff --git a/Documentation/mm/arch_pgtable_helpers.rst b/Documentation/mm/arch_pgtable_helpers.rst
> index af245161d8e7..e2ac76202a85 100644
> --- a/Documentation/mm/arch_pgtable_helpers.rst
> +++ b/Documentation/mm/arch_pgtable_helpers.rst
> @@ -242,13 +242,13 @@ SWAP Page Table Helpers
>  ========================
>  
>  +---------------------------+--------------------------------------------------+
> -| __pte_to_swp_entry        | Creates a swapped entry (arch) from a mapped PTE |
> +| __pte_to_swp_entry        | Creates a swap entry (arch) from a swapped PTE   |
>  +---------------------------+--------------------------------------------------+
> -| __swp_to_pte_entry        | Creates a mapped PTE from a swapped entry (arch) |
> +| __swp_entry_to_pte        | Creates a swapped PTE from a swap entry (arch)   |
>  +---------------------------+--------------------------------------------------+
> -| __pmd_to_swp_entry        | Creates a swapped entry (arch) from a mapped PMD |
> +| __pmd_to_swp_entry        | Creates a swap entry (arch) from a swapped PMD   |
>  +---------------------------+--------------------------------------------------+
> -| __swp_to_pmd_entry        | Creates a mapped PMD from a swapped entry (arch) |
> +| __swp_entry_to_pmd        | Creates a swapped PMD from a swap entry (arch)   |
>  +---------------------------+--------------------------------------------------+
>  | is_migration_entry        | Tests a migration (read or write) swapped entry  |
>  +-------------------------------+----------------------------------------------+

__pte_to_swp_entry() and __pmd_to_swp_entry() are still being used (and tested)
even after applying this patch. Should not their entries be preserved ?

> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index 7731b238b534..3b0f83ed6c2e 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -73,6 +73,8 @@ struct pgtable_debug_args {
>  	unsigned long		fixed_pud_pfn;
>  	unsigned long		fixed_pmd_pfn;
>  	unsigned long		fixed_pte_pfn;
> +
> +	swp_entry_t		swp_entry;
>  };
>  
>  static void __init pte_basic_tests(struct pgtable_debug_args *args, int idx)
> @@ -754,12 +756,15 @@ static void __init pte_soft_dirty_tests(struct pgtable_debug_args *args)
>  
>  static void __init pte_swap_soft_dirty_tests(struct pgtable_debug_args *args)
>  {
> -	pte_t pte = pfn_pte(args->fixed_pte_pfn, args->page_prot);
> +	pte_t pte;
>  
>  	if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
>  		return;
>  
>  	pr_debug("Validating PTE swap soft dirty\n");
> +	pte = swp_entry_to_pte(args->swp_entry);
> +	WARN_ON(!is_swap_pte(pte));
> +
>  	WARN_ON(!pte_swp_soft_dirty(pte_swp_mksoft_dirty(pte)));
>  	WARN_ON(pte_swp_soft_dirty(pte_swp_clear_soft_dirty(pte)));
>  }
> @@ -793,7 +798,9 @@ static void __init pmd_swap_soft_dirty_tests(struct pgtable_debug_args *args)
>  		return;
>  
>  	pr_debug("Validating PMD swap soft dirty\n");
> -	pmd = pfn_pmd(args->fixed_pmd_pfn, args->page_prot);
> +	pmd = swp_entry_to_pmd(args->swp_entry);
> +	WARN_ON(!is_swap_pmd(pmd));
> +
>  	WARN_ON(!pmd_swp_soft_dirty(pmd_swp_mksoft_dirty(pmd)));
>  	WARN_ON(pmd_swp_soft_dirty(pmd_swp_clear_soft_dirty(pmd)));
>  }
> @@ -804,17 +811,11 @@ static void __init pmd_swap_soft_dirty_tests(struct pgtable_debug_args *args) {
>  
>  static void __init pte_swap_exclusive_tests(struct pgtable_debug_args *args)
>  {
> -	unsigned long max_swap_offset;
>  	swp_entry_t entry, entry2;
>  	pte_t pte;
>  
>  	pr_debug("Validating PTE swap exclusive\n");
> -
> -	/* See generic_max_swapfile_size(): probe the maximum offset */
> -	max_swap_offset = swp_offset(pte_to_swp_entry(swp_entry_to_pte(swp_entry(0, ~0UL))));
> -
> -	/* Create a swp entry with all possible bits set */
> -	entry = swp_entry((1 << MAX_SWAPFILES_SHIFT) - 1, max_swap_offset);
> +	entry = args->swp_entry;
args->swp_entry should be reused here as well.

>  
>  	pte = swp_entry_to_pte(entry);
>  	WARN_ON(pte_swp_exclusive(pte));
> @@ -838,30 +839,36 @@ static void __init pte_swap_exclusive_tests(struct pgtable_debug_args *args)
>  
>  static void __init pte_swap_tests(struct pgtable_debug_args *args)
>  {
> -	swp_entry_t swp;
> -	pte_t pte;
> +	swp_entry_t entry, arch_entry;
> +	pte_t pte, pte2;
A very small nit - s/pte2/pte as the first one is pmd not pte or
make it pte1, pte2 if preferred.

>  
>  	pr_debug("Validating PTE swap\n");
> -	pte = pfn_pte(args->fixed_pte_pfn, args->page_prot);
> -	swp = __pte_to_swp_entry(pte);
> -	pte = __swp_entry_to_pte(swp);
> -	WARN_ON(args->fixed_pte_pfn != pte_pfn(pte));
> +	entry = args->swp_entry;

Should args->swp_entry be used directly here and 'entry' local variable
be dropped ?

> +
> +	pte = swp_entry_to_pte(entry);
> +	WARN_ON(!is_swap_pte(pte));
> +	arch_entry = __pte_to_swp_entry(pte);
> +	pte2 = __swp_entry_to_pte(arch_entry);
> +	WARN_ON(memcmp(&pte, &pte2, sizeof(pte)));
>  }
>  
>  #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
>  static void __init pmd_swap_tests(struct pgtable_debug_args *args)
>  {
> -	swp_entry_t swp;
> -	pmd_t pmd;
> +	swp_entry_t entry, arch_entry;
> +	pmd_t pmd, pmd2;

A very small nit - s/pmd2/pmd1 as the first one is pmd not pmd1 or
just make it pmd1, pmd2 if preferred.

>  
>  	if (!has_transparent_hugepage())
>  		return;
>  
>  	pr_debug("Validating PMD swap\n");
> -	pmd = pfn_pmd(args->fixed_pmd_pfn, args->page_prot);
> -	swp = __pmd_to_swp_entry(pmd);
> -	pmd = __swp_entry_to_pmd(swp);
> -	WARN_ON(args->fixed_pmd_pfn != pmd_pfn(pmd));
> +	entry = args->swp_entry;
> +	pmd = swp_entry_to_pmd(entry);
> +	WARN_ON(!is_swap_pmd(pmd));

Good to have the WARN_ON() to test whether it is a swap PMD or not.

> +
> +	arch_entry = __pmd_to_swp_entry(pmd);
> +	pmd2 = __swp_entry_to_pmd(arch_entry);
> +	WARN_ON(memcmp(&pmd, &pmd2, sizeof(pmd)));

Sounds good.

>  }
>  #else  /* !CONFIG_ARCH_ENABLE_THP_MIGRATION */
>  static void __init pmd_swap_tests(struct pgtable_debug_args *args) { }
> @@ -1166,6 +1173,7 @@ static void __init init_fixed_pfns(struct pgtable_debug_args *args)
>  
>  static int __init init_args(struct pgtable_debug_args *args)
>  {
> +	unsigned long max_swap_offset;
>  	struct page *page = NULL;
>  	int ret = 0;
>  
> @@ -1248,6 +1256,11 @@ static int __init init_args(struct pgtable_debug_args *args)
>  
>  	init_fixed_pfns(args);
>  
> +	/* See generic_max_swapfile_size(): probe the maximum offset */
> +	max_swap_offset = swp_offset(pte_to_swp_entry(swp_entry_to_pte(swp_entry(0, ~0UL))));
Why not directly use generic_max_swapfile_size() which is doing exact same thing.

unsigned long generic_max_swapfile_size(void)
{
	return swp_offset(pte_to_swp_entry(
			swp_entry_to_pte(swp_entry(0, ~0UL)))) + 1;
}

> +	/* Create a swp entry with all possible bits set */
> +	args->swp_entry = swp_entry((1 << MAX_SWAPFILES_SHIFT) - 1, max_swap_offset);
> +

Makes sense to use maximum possible bits while creating the swap entry for testing.

>  	/*
>  	 * Allocate (huge) pages because some of the tests need to access
>  	 * the data in the pages. The corresponding tests will be skipped