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

[Gerald Schaefer]

On Wed, 25 Jun 2025 09:58:31 +0530
Anshuman Khandual <anshuman.khandual@xxxxxxx> wrote:

[...]
> > 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 ?

Nothing is removed here. Only adjusted description, where David already
posted some improvement. And renamed __swp_to_pte/pmd_entry() to the
correct names __swp_entry_to_pte/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.

Yes, and it is. I just moved the swap entry creation logic from here to
init_args(), and instead use args->swp_entry here.

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

Sure, pte1/2 looks better. Same for pmd1/2 in pmd_swap_tests().

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

Right, should be possible, also in pmd_swap_tests().

[...]
> > @@ -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;
> }

Good question. I just moved this code here from pte_swap_exclusive_tests(),
see above, and did not think about that. Now I also wonder why
generic_max_swapfile_size() wasn't used before.

But it is not exactly the same thing, there is an extra "+ 1" there.
Maybe that is the reason, but I don't really understand the details /
difference, and therefore would not want to change it.

David, do you remember why you didn't use generic_max_swapfile_size()
in your pte_swap_exclusive_tests()?

> 
> > +	/* 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