Re: [PATCH v3 03/16] mm: avoid unnecessary uses of is_swap_pte()

[Zi Yan <ziy@nvidia.com>]

On 10 Nov 2025, at 17:21, Lorenzo Stoakes wrote:

> There's an established convention in the kernel that we treat PTEs as
> containing swap entries (and the unfortunately named non-swap swap entries)
> should they be neither empty (i.e. pte_none() evaluating true) nor present
> (i.e. pte_present() evaluating true).
>
> However, there is some inconsistency in how this is applied, as we also
> have the is_swap_pte() helper which explicitly performs this check:
>
> 	/* check whether a pte points to a swap entry */
> 	static inline int is_swap_pte(pte_t pte)
> 	{
> 		return !pte_none(pte) && !pte_present(pte);
> 	}
>
> As this represents a predicate, and it's logical to assume that in order to
> establish that a PTE entry can correctly be manipulated as a swap/non-swap
> entry, this predicate seems as if it must first be checked.
>
> But we instead, we far more often utilise the established convention of
> checking pte_none() / pte_present() before operating on entries as if they
> were swap/non-swap.
>
> This patch works towards correcting this inconsistency by removing all uses
> of is_swap_pte() where we are already in a position where we perform
> pte_none()/pte_present() checks anyway or otherwise it is clearly logical
> to do so.
>
> We also take advantage of the fact that pte_swp_uffd_wp() is only set on
> swap entries.
>
> Additionally, update comments referencing to is_swap_pte() and
> non_swap_entry().
>
> No functional change intended.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>  fs/proc/task_mmu.c            | 49 ++++++++++++++++++++++++-----------
>  include/linux/userfaultfd_k.h |  3 +--
>  mm/hugetlb.c                  |  6 ++---
>  mm/internal.h                 |  6 ++---
>  mm/khugepaged.c               | 29 +++++++++++----------
>  mm/migrate.c                  |  2 +-
>  mm/mprotect.c                 | 43 ++++++++++++++----------------
>  mm/mremap.c                   |  7 +++--
>  mm/page_table_check.c         | 13 ++++++----
>  mm/page_vma_mapped.c          | 31 +++++++++++-----------
>  10 files changed, 104 insertions(+), 85 deletions(-)
>

<snip>

> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
> index be20468fb5a9..a4e23818f37f 100644
> --- a/mm/page_vma_mapped.c
> +++ b/mm/page_vma_mapped.c
> @@ -16,6 +16,7 @@ static inline bool not_found(struct page_vma_mapped_walk *pvmw)
>  static bool map_pte(struct page_vma_mapped_walk *pvmw, pmd_t *pmdvalp,
>  		    spinlock_t **ptlp)
>  {
> +	bool is_migration;
>  	pte_t ptent;
>
>  	if (pvmw->flags & PVMW_SYNC) {
> @@ -26,6 +27,7 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw, pmd_t *pmdvalp,
>  		return !!pvmw->pte;
>  	}
>
> +	is_migration = pvmw->flags & PVMW_MIGRATION;
>  again:
>  	/*
>  	 * It is important to return the ptl corresponding to pte,
> @@ -41,11 +43,14 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw, pmd_t *pmdvalp,
>
>  	ptent = ptep_get(pvmw->pte);
>
> -	if (pvmw->flags & PVMW_MIGRATION) {
> -		if (!is_swap_pte(ptent))

Here, is_migration = true and either pte_none() or pte_present()
would return false, and ...

> +	if (pte_none(ptent)) {
> +		return false;
> +	} else if (pte_present(ptent)) {
> +		if (is_migration)
>  			return false;
> -	} else if (is_swap_pte(ptent)) {
> +	} else if (!is_migration) {
>  		swp_entry_t entry;
> +
>  		/*
>  		 * Handle un-addressable ZONE_DEVICE memory.
>  		 *
> @@ -66,8 +71,6 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw, pmd_t *pmdvalp,
>  		if (!is_device_private_entry(entry) &&
>  		    !is_device_exclusive_entry(entry))
>  			return false;
> -	} else if (!pte_present(ptent)) {
> -		return false;

... is_migration = false and !pte_present() is actually pte_none(),
because of the is_swap_pte() above the added !is_migration check.
So pte_none() should return false regardless of is_migration.

This is a nice cleanup. Thanks.

>  	}
>  	spin_lock(*ptlp);
>  	if (unlikely(!pmd_same(*pmdvalp, pmdp_get_lockless(pvmw->pmd)))) {
> @@ -113,21 +116,17 @@ static bool check_pte(struct page_vma_mapped_walk *pvmw, unsigned long pte_nr)
>  			return false;
>
>  		pfn = softleaf_to_pfn(entry);
> -	} else if (is_swap_pte(ptent)) {
> -		swp_entry_t entry;
> +	} else if (pte_present(ptent)) {
> +		pfn = pte_pfn(ptent);
> +	} else {
> +		const softleaf_t entry = softleaf_from_pte(ptent);
>
>  		/* Handle un-addressable ZONE_DEVICE memory */
> -		entry = pte_to_swp_entry(ptent);
> -		if (!is_device_private_entry(entry) &&
> -		    !is_device_exclusive_entry(entry))
> -			return false;
> -
> -		pfn = swp_offset_pfn(entry);
> -	} else {
> -		if (!pte_present(ptent))

This !pte_present() is pte_none(). It seems that there should be

} else if (pte_none(ptent)) {
	return false;
}

before the above "} else {".

> +		if (!softleaf_is_device_private(entry) &&
> +		    !softleaf_is_device_exclusive(entry))
>  			return false;
>
> -		pfn = pte_pfn(ptent);
> +		pfn = softleaf_to_pfn(entry);
>  	}
>
>  	if ((pfn + pte_nr - 1) < pvmw->pfn)
> -- 
> 2.51.0

Otherwise, LGTM. With the above issue addressed, feel free to
add Reviewed-by: Zi Yan <ziy@nvidia.com>

--
Best Regards,
Yan, Zi