Hi,

currently working on enabling THP_SWAP and THP_MIGRATION support for s390,
and stumbling over the WARN_ON(args->fixed_pmd_pfn != pmd_pfn(pmd)) in
debug_vm_pgtable pmd_swap_tests(). The problem is that pmd_pfn() on s390
will use different shift values for leaf (large) and non-leaf PMDs. And
when used on swapped PMDs, for which pmd_leaf() will always return false
because !pmd_present(), the result is not really well defined.

I think that pmd_pfn() is not safe or ever meant to be called on swapped
PMD entries, and it doesn't seem to be used in that way anywhere else but
debug_vm_pgtable. Also, the whole logic to test the various swap helpers
on normal PTE/PMD entries seems wrong to me. It just works by chance,
because e.g. __pmd_to_swp_entry() and __swp_entry_to_pmd() are just no-ops
on other architectures (also on s390, but only for PTEs), and also
pmd_pfn() does not have any dependency on leaf/non-leaf entries there.

So, I started with a small patch to make pmd_swap_tests() use a proper
swapped PMD entry as input value, similar to how it is already done in
pte_swap_exclusive_tests(), and not use pmd_pfn() for compare but rather
compare the whole entries, again similar to pte_swap_exclusive_tests().

But then I noticed that such a change would probably also make sense for
the other swap tests, and also a small inconsistency in Documentation,
where it says e.g.

__pte_to_swp_entry        | Creates a swapped entry (arch) from a mapped PTE

I think this is wrong, those helpers should never operate on present and
mapped PTEs, and they certainly don't create any swapped entry from a
mapped entry, given that they are just no-ops on most architectures.
Instead, in this example, it just returns the arch-dependent
representation of a swp_entry_t, which happens to be just the entry
itself on most architectures. See also pte_to_swp_entry() /
swp_entry_to_pte() in include/linux/swapops.h.

Now it became a larger clean-up, and I hope it makes sense. This is all
rather new common code for me, so maybe I got things wrong, feedback is
welcome.

Gerald Schaefer (1):
  mm/debug_vm_pgtable: Use a swp_entry_t input value for swap tests

 Documentation/mm/arch_pgtable_helpers.rst |  8 ++--
 mm/debug_vm_pgtable.c                     | 55 ++++++++++++++---------
 2 files changed, 38 insertions(+), 25 deletions(-)

-- 
2.48.1

Hello Gerald,

On 24/06/25 12:13 AM, Gerald Schaefer wrote:
> Hi,
> 
> currently working on enabling THP_SWAP and THP_MIGRATION support for s390,
> and stumbling over the WARN_ON(args->fixed_pmd_pfn != pmd_pfn(pmd)) in
> debug_vm_pgtable pmd_swap_tests(). The problem is that pmd_pfn() on s390
> will use different shift values for leaf (large) and non-leaf PMDs. And
> when used on swapped PMDs, for which pmd_leaf() will always return false
> because !pmd_present(), the result is not really well defined.

Just curious - pmd_pfn() would have otherwise worked on leaf PMD entries ?
Because the PMD swap entries are not leaf entries as pmd_present() returns
negative, pmd_pfn() does not work on those ?

> 
> I think that pmd_pfn() is not safe or ever meant to be called on swapped
> PMD entries, and it doesn't seem to be used in that way anywhere else but
> debug_vm_pgtable. Also, the whole logic to test the various swap helpers

But is not the pmd_pfn() called on pmd which is derived from the swap entry
first.

	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));

> on normal PTE/PMD entries seems wrong to me. It just works by chance,
> because e.g. __pmd_to_swp_entry() and __swp_entry_to_pmd() are just no-ops
> on other architectures (also on s390, but only for PTEs), and also

Hmm, basically it just tests pfn_pmd() and pmd_pfn() conversions ?

> pmd_pfn() does not have any dependency on leaf/non-leaf entries there.Could you please elaborate on that ? 

> 
> So, I started with a small patch to make pmd_swap_tests() use a proper
> swapped PMD entry as input value, similar to how it is already done in
> pte_swap_exclusive_tests(), and not use pmd_pfn() for compare but rather
> compare the whole entries, again similar to pte_swap_exclusive_tests().

Agreed, that will make sense as well.

> 
> But then I noticed that such a change would probably also make sense for
> the other swap tests, and also a small inconsistency in Documentation,
> where it says e.g.
> 
> __pte_to_swp_entry        | Creates a swapped entry (arch) from a mapped PTE
> 
> I think this is wrong, those helpers should never operate on present and
> mapped PTEs, and they certainly don't create any swapped entry from a
> mapped entry, given that they are just no-ops on most architectures.
> Instead, in this example, it just returns the arch-dependent
> representation of a swp_entry_t, which happens to be just the entry
> itself on most architectures. See also pte_to_swp_entry() /
> swp_entry_to_pte() in include/linux/swapops.h.

Alright.

> 
> Now it became a larger clean-up, and I hope it makes sense. This is all
> rather new common code for me, so maybe I got things wrong, feedback is
> welcome.

A quick ran on arm64 looks just fine, will keep looking into this.

> 
> Gerald Schaefer (1):
>   mm/debug_vm_pgtable: Use a swp_entry_t input value for swap tests
> 
>  Documentation/mm/arch_pgtable_helpers.rst |  8 ++--
>  mm/debug_vm_pgtable.c                     | 55 ++++++++++++++---------
>  2 files changed, 38 insertions(+), 25 deletions(-)
>

On Tue, 24 Jun 2025 13:20:42 +0530
Anshuman Khandual <anshuman.khandual@arm.com> wrote:

> Hello Gerald,
> 
> On 24/06/25 12:13 AM, Gerald Schaefer wrote:
> > Hi,
> > 
> > currently working on enabling THP_SWAP and THP_MIGRATION support for s390,
> > and stumbling over the WARN_ON(args->fixed_pmd_pfn != pmd_pfn(pmd)) in
> > debug_vm_pgtable pmd_swap_tests(). The problem is that pmd_pfn() on s390
> > will use different shift values for leaf (large) and non-leaf PMDs. And
> > when used on swapped PMDs, for which pmd_leaf() will always return false
> > because !pmd_present(), the result is not really well defined.  
> 
> Just curious - pmd_pfn() would have otherwise worked on leaf PMD entries ?
> Because the PMD swap entries are not leaf entries as pmd_present() returns
> negative, pmd_pfn() does not work on those ?

Yes, but there are actually two problems with this. The initial pmd that
is created with pfn_pmd() is already not leaf/large, but present, so
pmd_pfn() would already not work correctly on s390.

Later, after the __pmd_to_swp_entry() / __swp_entry_to_pmd() cycle, the
present bit got removed because of how those helpers will be implemented
for s390. Now it is neither large nor present, and pmd_pfn() will be
extra confused.

IOW, even if we could implement those helpers as simple no-ops similar
to other archs, the check would still not work, even though the PMD would
have the present bit set, but it still wouldn't be leaf/large.

I guess my description was a bit confusing, since the !pmd_present()
case would only show on s390, but it is not the only problem here.
I think the point is that those helpers should only be used on "proper"
swap PTE/PMD entries, which already cannot be present. And of course
that pte/pmd_pfn() is not meant to be used on such entries at all, as
David explained.

> 
> > 
> > I think that pmd_pfn() is not safe or ever meant to be called on swapped
> > PMD entries, and it doesn't seem to be used in that way anywhere else but
> > debug_vm_pgtable. Also, the whole logic to test the various swap helpers  
> 
> But is not the pmd_pfn() called on pmd which is derived from the swap entry
> first.
> 
> 	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));

Yes, but this logic is not really testing swap entries. It only works
because on other archs the __pmd_to_swp_entry() / __swp_entry_to_pmd() are
no-ops, and because pmd_pfn() does not care about leaf/large.

> 
> > on normal PTE/PMD entries seems wrong to me. It just works by chance,
> > because e.g. __pmd_to_swp_entry() and __swp_entry_to_pmd() are just no-ops
> > on other architectures (also on s390, but only for PTEs), and also  
> 
> Hmm, basically it just tests pfn_pmd() and pmd_pfn() conversions ?

Correct, but with the extra quirk that the initial PMD created by pfn_pmd()
is not leaf/large, which is apparently not a problem on other archs for
the pmd_pfn() conversion.

Actually, I now wonder why pfn_pmd() would not implicitly mark it as
leaf/large already, as it seems that this should only be used for leaf
PMDs. But maybe there are some special cases where it could also be
used for non-leaf PMDs.

> 
> > pmd_pfn() does not have any dependency on leaf/non-leaf entries there.
> Could you please elaborate on that ?

As explained above, the initial PMD created by pfn_pmd() is not leaf/large.
Well, conceptually it is more or less, but it is not marked as such. This
would lead to incorrect pmd_pfn() result (only) on s390.

> 
> > 
> > So, I started with a small patch to make pmd_swap_tests() use a proper
> > swapped PMD entry as input value, similar to how it is already done in
> > pte_swap_exclusive_tests(), and not use pmd_pfn() for compare but rather
> > compare the whole entries, again similar to pte_swap_exclusive_tests().  
> 
> Agreed, that will make sense as well.
> 
> > 
> > But then I noticed that such a change would probably also make sense for
> > the other swap tests, and also a small inconsistency in Documentation,
> > where it says e.g.
> > 
> > __pte_to_swp_entry        | Creates a swapped entry (arch) from a mapped PTE
> > 
> > I think this is wrong, those helpers should never operate on present and
> > mapped PTEs, and they certainly don't create any swapped entry from a
> > mapped entry, given that they are just no-ops on most architectures.
> > Instead, in this example, it just returns the arch-dependent
> > representation of a swp_entry_t, which happens to be just the entry
> > itself on most architectures. See also pte_to_swp_entry() /
> > swp_entry_to_pte() in include/linux/swapops.h.  
> 
> Alright.
> 
> > 
> > Now it became a larger clean-up, and I hope it makes sense. This is all
> > rather new common code for me, so maybe I got things wrong, feedback is
> > welcome.  
> 
> A quick ran on arm64 looks just fine, will keep looking into this.

Thanks!

On 23.06.25 20:43, Gerald Schaefer wrote:
> Hi,
> 
> currently working on enabling THP_SWAP and THP_MIGRATION support for s390,
> and stumbling over the WARN_ON(args->fixed_pmd_pfn != pmd_pfn(pmd)) in
> debug_vm_pgtable pmd_swap_tests(). The problem is that pmd_pfn() on s390
> will use different shift values for leaf (large) and non-leaf PMDs. And
> when used on swapped PMDs, for which pmd_leaf() will always return false
> because !pmd_present(), the result is not really well defined.
> 
> I think that pmd_pfn() is not safe or ever meant to be called on swapped
> PMD entries,

Exactly that. Just like pte_pfn() on a swap entry is bogus.

Instead, we can test for is_pfn_swap_entry() and then use 
swp_offset_pfn/pfn_swap_entry_to_page/pfn_swap_entry_folio.

Code in task_mmu.c uses something like

	swp_entry_t entry = pmd_to_swp_entry(*pmd);

	if (is_pfn_swap_entry(entry))
		page = pfn_swap_entry_to_page(entry);

and it doesn't seem to be used in that way anywhere else but
> debug_vm_pgtable. Also, the whole logic to test the various swap helpers
> on normal PTE/PMD entries seems wrong to me. It just works by chance,
> because e.g. __pmd_to_swp_entry() and __swp_entry_to_pmd() are just no-ops
> on other architectures (also on s390, but only for PTEs), and also
> pmd_pfn() does not have any dependency on leaf/non-leaf entries there.
> 
> So, I started with a small patch to make pmd_swap_tests() use a proper
> swapped PMD entry as input value, similar to how it is already done in
> pte_swap_exclusive_tests(), and not use pmd_pfn() for compare but rather
> compare the whole entries, again similar to pte_swap_exclusive_tests().
> 
> But then I noticed that such a change would probably also make sense for
> the other swap tests, and also a small inconsistency in Documentation,
> where it says e.g.
> 
> __pte_to_swp_entry        | Creates a swapped entry (arch) from a mapped PTE
> 
> I think this is wrong, those helpers should never operate on present and
> mapped PTEs, and they certainly don't create any swapped entry from a
> mapped entry, given that they are just no-ops on most architectures.

"mapped" is probably misleading. Probably "mapped" as in "this PTE is in 
the page tables", not "mapped" as in "maps a present page".

In any case, it should be clarified.

-- 
Cheers,

David / dhildenb