[PATCH] mm: Remove stub for non_swap_entry()

[PATCH] mm: Remove stub for non_swap_entry()

[Peter Xu]

The stub for non_swap_entry() may not help much, because MAX_SWAPFILES has
already contained all the information to decide whether a swap entry is
real swap entry of pesudo ones (migrations, ...).

There can be some performance influences on non_swap_entry() with below
conditions all met:

  !CONFIG_MIGRATION && !CONFIG_MEMORY_FAILURE && !CONFIG_DEVICE_PRIVATE

But that's definitely not the major config most machines will use, at the
meantime it's already in a slow path of swap entry (being parsed from a
swap pte), so IMHO it shouldn't be a major issue.  Also according to the
analysis from Alistair, somehow the stub didn't do the job right [1].

To make the code cleaner, let's drop the stub.

[1] https://lore.kernel.org/lkml/8735ihbw6g.fsf@nvdebian.thelocal/

Cc: Alistair Popple <apopple@nvidia.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---

Note: the uffd-wp shmem & hugetlbfs series will need this patch to make
sure swap entries work as expected with below config as spotted by
Alistair:

  !CONFIG_MIGRATION &&
  !CONFIG_MEMORY_FAILURE &&
  !CONFIG_DEVICE_PRIVATE &&
  CONFIG_PTE_MARKER

(PS: this config should mostly never gonna happen, though, afaict..)

Quoting the same thread [1] as above.

Please review, thanks.
---
 include/linux/swapops.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/include/linux/swapops.h b/include/linux/swapops.h
index fffbba0036f6..a291f210e7f8 100644
--- a/include/linux/swapops.h
+++ b/include/linux/swapops.h
@@ -493,18 +493,10 @@ static inline void num_poisoned_pages_inc(void)
 }
 #endif
 
-#if defined(CONFIG_MEMORY_FAILURE) || defined(CONFIG_MIGRATION) || \
-    defined(CONFIG_DEVICE_PRIVATE)
 static inline int non_swap_entry(swp_entry_t entry)
 {
 	return swp_type(entry) >= MAX_SWAPFILES;
 }
-#else
-static inline int non_swap_entry(swp_entry_t entry)
-{
-	return 0;
-}
-#endif
 
 #endif /* CONFIG_MMU */
 #endif /* _LINUX_SWAPOPS_H */
-- 
2.32.0

Re: [PATCH] mm: Remove stub for non_swap_entry()

[Alistair Popple]

[-- Attachment #1: Type: text/plain, Size: 2434 bytes --]

Peter Xu <peterx@redhat.com> writes:

> The stub for non_swap_entry() may not help much, because MAX_SWAPFILES has
> already contained all the information to decide whether a swap entry is
> real swap entry of pesudo ones (migrations, ...).
>
> There can be some performance influences on non_swap_entry() with below
> conditions all met:
>
>   !CONFIG_MIGRATION && !CONFIG_MEMORY_FAILURE && !CONFIG_DEVICE_PRIVATE
>
> But that's definitely not the major config most machines will use, at the
> meantime it's already in a slow path of swap entry (being parsed from a
> swap pte), so IMHO it shouldn't be a major issue.  Also according to the
> analysis from Alistair, somehow the stub didn't do the job right [1].

I wasn't so much concerned about execution speed given it's on the slow path
anyway but overall code size, which is one reason all those config options might
be disabled. However in practice it made little to no difference as those config
options already remove most of the extra code so I agree we can drop the stub.

Reviewed-by: Alistair Popple <apopple@nvidia.com>

> To make the code cleaner, let's drop the stub.
>
> [1] <https://lore.kernel.org/lkml/8735ihbw6g.fsf@nvdebian.thelocal/>
>
> Cc: Alistair Popple <apopple@nvidia.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>
> Note: the uffd-wp shmem & hugetlbfs series will need this patch to make
> sure swap entries work as expected with below config as spotted by
> Alistair:
>
>   !CONFIG_MIGRATION &&
>   !CONFIG_MEMORY_FAILURE &&
>   !CONFIG_DEVICE_PRIVATE &&
>   CONFIG_PTE_MARKER
>
> (PS: this config should mostly never gonna happen, though, afaict..)
>
> Quoting the same thread [1] as above.
>
> Please review, thanks.
> ---
>  include/linux/swapops.h | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> index fffbba0036f6..a291f210e7f8 100644
> --- a/include/linux/swapops.h
> +++ b/include/linux/swapops.h
> @@ -493,18 +493,10 @@ static inline void num_poisoned_pages_inc(void)
>  }
>  #endif
>
> -#if defined(CONFIG_MEMORY_FAILURE) || defined(CONFIG_MIGRATION) || \
> -    defined(CONFIG_DEVICE_PRIVATE)
>  static inline int non_swap_entry(swp_entry_t entry)
>  {
>  	return swp_type(entry) >= MAX_SWAPFILES;
>  }
> -#else
> -static inline int non_swap_entry(swp_entry_t entry)
> -{
> -	return 0;
> -}
> -#endif
>
>  #endif /* CONFIG_MMU */
>  #endif /* _LINUX_SWAPOPS_H */

Re: [PATCH] mm: Remove stub for non_swap_entry()

[Peter Xu]

On Thu, Apr 14, 2022 at 03:48:33PM +1000, Alistair Popple wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > The stub for non_swap_entry() may not help much, because MAX_SWAPFILES has
> > already contained all the information to decide whether a swap entry is
> > real swap entry of pesudo ones (migrations, ...).
> >
> > There can be some performance influences on non_swap_entry() with below
> > conditions all met:
> >
> >   !CONFIG_MIGRATION && !CONFIG_MEMORY_FAILURE && !CONFIG_DEVICE_PRIVATE
> >
> > But that's definitely not the major config most machines will use, at the
> > meantime it's already in a slow path of swap entry (being parsed from a
> > swap pte), so IMHO it shouldn't be a major issue.  Also according to the
> > analysis from Alistair, somehow the stub didn't do the job right [1].
> 
> I wasn't so much concerned about execution speed given it's on the slow path
> anyway but overall code size, which is one reason all those config options might
> be disabled. However in practice it made little to no difference as those config
> options already remove most of the extra code so I agree we can drop the stub.

I see, yeah that's a good point.

I'd wildly guess a minumum set of Linux build could still like that, but
not strongly, as I'd first think about not having CONFIG_SWAP at all if so.

> 
> Reviewed-by: Alistair Popple <apopple@nvidia.com>

Thanks!

-- 
Peter Xu