[PATCH 0/2] mm: Use pte marker for swapin errors

[PATCH 0/2] mm: Use pte marker for swapin errors

[Peter Xu]

rfc->v1:
- Rename SWAP_ERROR -> SWAPIN_ERROR [David, Miaohe]
- Added r-bs for Ying and Miaohe

Hi,

This series uses the pte marker to replace the swapin error swap entry,
then we save one more swap entry slot for swap devices.  A new pte marker
bit is defined.

One thing worth mentioning: we need the pte marker to be always built to
make sure it works like before, so I made an attempt in patch 1 to drop
CONFIG_PTE_MARKER and always compile pte marker in.  Since pte markers are
enabled in many distributions by default already, meanwhile the codeset is
really small I assume it's fine.  But still I'm tagging with RFC for this
initial version.

Smoke tested on anonymous mem on a fake swap failure.  Please have a look,
thanks.

Peter Xu (2):
  mm: Always compile in pte markers
  mm: Use pte markers for swap errors

 include/linux/swap.h    | 16 ++++----------
 include/linux/swapops.h | 49 +++++++++--------------------------------
 mm/Kconfig              |  7 ------
 mm/memory.c             | 13 ++++++-----
 mm/shmem.c              |  2 +-
 mm/swapfile.c           |  2 +-
 6 files changed, 23 insertions(+), 66 deletions(-)

-- 
2.37.3

[PATCH 1/2] mm: Always compile in pte markers

[Peter Xu]

The PTE markers code is tiny and now it's enabled for most of the
distributions.  It's fine to keep it as-is, but to make a broader use of
it (e.g. replacing read error swap entry) it needs to be there always
otherwise we need special code path to take care of !PTE_MARKER case.

It'll be easier just make pte marker always exist.  Use this chance to
extend its usage to anonymous too by simply touching up some of the old
comments, because it'll be used for anonymous pages in the follow up
patches.

Reviewed-by: Huang Ying <ying.huang@intel.com>
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/linux/swap.h    | 10 +++-------
 include/linux/swapops.h | 31 -------------------------------
 mm/Kconfig              |  7 -------
 mm/memory.c             |  7 +++----
 4 files changed, 6 insertions(+), 49 deletions(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index 369d7799205d..211aeca9bfa7 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -60,17 +60,13 @@ static inline int current_is_kswapd(void)
 			     SWP_MIGRATION_NUM + SWP_DEVICE_NUM + \
 			     SWP_PTE_MARKER_NUM)
 /*
- * PTE markers are used to persist information onto PTEs that are mapped with
- * file-backed memories.  As its name "PTE" hints, it should only be applied to
- * the leaves of pgtables.
+ * PTE markers are used to persist information onto PTEs that otherwise
+ * should be a none pte.  As its name "PTE" hints, it should only be
+ * applied to the leaves of pgtables.
  */
-#ifdef CONFIG_PTE_MARKER
 #define SWP_PTE_MARKER_NUM 1
 #define SWP_PTE_MARKER     (MAX_SWAPFILES + SWP_HWPOISON_NUM + \
 			    SWP_MIGRATION_NUM + SWP_DEVICE_NUM)
-#else
-#define SWP_PTE_MARKER_NUM 0
-#endif
 
 /*
  * Unaddressable device memory support. See include/linux/hmm.h and
diff --git a/include/linux/swapops.h b/include/linux/swapops.h
index 3ba9bf56899d..35c1fe62d2e1 100644
--- a/include/linux/swapops.h
+++ b/include/linux/swapops.h
@@ -412,8 +412,6 @@ typedef unsigned long pte_marker;
 #define  PTE_MARKER_UFFD_WP  BIT(0)
 #define  PTE_MARKER_MASK     (PTE_MARKER_UFFD_WP)
 
-#ifdef CONFIG_PTE_MARKER
-
 static inline swp_entry_t make_pte_marker_entry(pte_marker marker)
 {
 	return swp_entry(SWP_PTE_MARKER, marker);
@@ -434,32 +432,6 @@ static inline bool is_pte_marker(pte_t pte)
 	return is_swap_pte(pte) && is_pte_marker_entry(pte_to_swp_entry(pte));
 }
 
-#else /* CONFIG_PTE_MARKER */
-
-static inline swp_entry_t make_pte_marker_entry(pte_marker marker)
-{
-	/* This should never be called if !CONFIG_PTE_MARKER */
-	WARN_ON_ONCE(1);
-	return swp_entry(0, 0);
-}
-
-static inline bool is_pte_marker_entry(swp_entry_t entry)
-{
-	return false;
-}
-
-static inline pte_marker pte_marker_get(swp_entry_t entry)
-{
-	return 0;
-}
-
-static inline bool is_pte_marker(pte_t pte)
-{
-	return false;
-}
-
-#endif /* CONFIG_PTE_MARKER */
-
 static inline pte_t make_pte_marker(pte_marker marker)
 {
 	return swp_entry_to_pte(make_pte_marker_entry(marker));
@@ -477,9 +449,6 @@ static inline pte_t make_pte_marker(pte_marker marker)
  * memory, kernel-only memory (including when the system is during-boot),
  * non-ram based generic file-system.  It's fine to be used even there, but the
  * extra pte marker check will be pure overhead.
- *
- * For systems configured with !CONFIG_PTE_MARKER this will be automatically
- * optimized to pte_none().
  */
 static inline int pte_none_mostly(pte_t pte)
 {
diff --git a/mm/Kconfig b/mm/Kconfig
index 57e1d8c5b505..4b28800d9be1 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -1107,17 +1107,10 @@ config HAVE_ARCH_USERFAULTFD_MINOR
 	help
 	  Arch has userfaultfd minor fault support
 
-config PTE_MARKER
-	bool
-
-	help
-	  Allows to create marker PTEs for file-backed memory.
-
 config PTE_MARKER_UFFD_WP
 	bool "Userfaultfd write protection support for shmem/hugetlbfs"
 	default y
 	depends on HAVE_ARCH_USERFAULTFD_WP
-	select PTE_MARKER
 
 	help
 	  Allows to create marker PTEs for userfaultfd write protection
diff --git a/mm/memory.c b/mm/memory.c
index c5599a9279b1..63718094163f 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3663,11 +3663,10 @@ static vm_fault_t handle_pte_marker(struct vm_fault *vmf)
 	unsigned long marker = pte_marker_get(entry);
 
 	/*
-	 * PTE markers should always be with file-backed memories, and the
-	 * marker should never be empty.  If anything weird happened, the best
-	 * thing to do is to kill the process along with its mm.
+	 * PTE markers should never be empty.  If anything weird happened,
+	 * the best thing to do is to kill the process along with its mm.
 	 */
-	if (WARN_ON_ONCE(vma_is_anonymous(vmf->vma) || !marker))
+	if (WARN_ON_ONCE(!marker))
 		return VM_FAULT_SIGBUS;
 
 	if (pte_marker_entry_uffd_wp(entry))
-- 
2.37.3

Re: [PATCH 1/2] mm: Always compile in pte markers

[David Hildenbrand]

On 30.10.22 22:41, Peter Xu wrote:
> The PTE markers code is tiny and now it's enabled for most of the
> distributions.  It's fine to keep it as-is, but to make a broader use of
> it (e.g. replacing read error swap entry) it needs to be there always
> otherwise we need special code path to take care of !PTE_MARKER case.
> 
> It'll be easier just make pte marker always exist.  Use this chance to
> extend its usage to anonymous too by simply touching up some of the old
> comments, because it'll be used for anonymous pages in the follow up
> patches.
> 
> Reviewed-by: Huang Ying <ying.huang@intel.com>
> Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>


Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH 2/2] mm: Use pte markers for swap errors

[Peter Xu]

PTE markers are ideal mechanism for things like SWP_SWAPIN_ERROR.  Using a
whole swap entry type for this purpose can be an overkill, especially if we
already have PTE markers.  Define a new bit for swapin error and replace it
with pte markers.  Then we can safely drop SWP_SWAPIN_ERROR and give one
device slot back to swap.

We used to have SWP_SWAPIN_ERROR taking the page pfn as part of the swap
entry, but it's never used.  Neither do I see how it can be useful because
normally the swapin failure should not be caused by a bad page but bad swap
device.  Drop it alongside.

Reviewed-by: Huang Ying <ying.huang@intel.com>
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/linux/swap.h    |  6 +-----
 include/linux/swapops.h | 26 ++++++++++++++------------
 mm/memory.c             |  6 ++++--
 mm/shmem.c              |  2 +-
 mm/swapfile.c           |  2 +-
 5 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index 211aeca9bfa7..fec6647a289a 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -55,10 +55,6 @@ static inline int current_is_kswapd(void)
  * actions on faults.
  */
 
-#define SWP_SWAPIN_ERROR_NUM 1
-#define SWP_SWAPIN_ERROR     (MAX_SWAPFILES + SWP_HWPOISON_NUM + \
-			     SWP_MIGRATION_NUM + SWP_DEVICE_NUM + \
-			     SWP_PTE_MARKER_NUM)
 /*
  * PTE markers are used to persist information onto PTEs that otherwise
  * should be a none pte.  As its name "PTE" hints, it should only be
@@ -121,7 +117,7 @@ static inline int current_is_kswapd(void)
 #define MAX_SWAPFILES \
 	((1 << MAX_SWAPFILES_SHIFT) - SWP_DEVICE_NUM - \
 	SWP_MIGRATION_NUM - SWP_HWPOISON_NUM - \
-	SWP_PTE_MARKER_NUM - SWP_SWAPIN_ERROR_NUM)
+	SWP_PTE_MARKER_NUM)
 
 /*
  * Magic header for a swap area. The first part of the union is
diff --git a/include/linux/swapops.h b/include/linux/swapops.h
index 35c1fe62d2e1..27ade4f22abb 100644
--- a/include/linux/swapops.h
+++ b/include/linux/swapops.h
@@ -162,16 +162,6 @@ static inline void *swp_to_radix_entry(swp_entry_t entry)
 	return xa_mk_value(entry.val);
 }
 
-static inline swp_entry_t make_swapin_error_entry(struct page *page)
-{
-	return swp_entry(SWP_SWAPIN_ERROR, page_to_pfn(page));
-}
-
-static inline int is_swapin_error_entry(swp_entry_t entry)
-{
-	return swp_type(entry) == SWP_SWAPIN_ERROR;
-}
-
 #if IS_ENABLED(CONFIG_DEVICE_PRIVATE)
 static inline swp_entry_t make_readable_device_private_entry(pgoff_t offset)
 {
@@ -409,8 +399,9 @@ static inline bool is_migration_entry_dirty(swp_entry_t entry)
 
 typedef unsigned long pte_marker;
 
-#define  PTE_MARKER_UFFD_WP  BIT(0)
-#define  PTE_MARKER_MASK     (PTE_MARKER_UFFD_WP)
+#define  PTE_MARKER_UFFD_WP			BIT(0)
+#define  PTE_MARKER_SWAPIN_ERROR		BIT(1)
+#define  PTE_MARKER_MASK			(BIT(2) - 1)
 
 static inline swp_entry_t make_pte_marker_entry(pte_marker marker)
 {
@@ -437,6 +428,17 @@ static inline pte_t make_pte_marker(pte_marker marker)
 	return swp_entry_to_pte(make_pte_marker_entry(marker));
 }
 
+static inline swp_entry_t make_swapin_error_entry(void)
+{
+	return make_pte_marker_entry(PTE_MARKER_SWAPIN_ERROR);
+}
+
+static inline int is_swapin_error_entry(swp_entry_t entry)
+{
+	return is_pte_marker_entry(entry) &&
+	    (pte_marker_get(entry) & PTE_MARKER_SWAPIN_ERROR);
+}
+
 /*
  * This is a special version to check pte_none() just to cover the case when
  * the pte is a pte marker.  It existed because in many cases the pte marker
diff --git a/mm/memory.c b/mm/memory.c
index 63718094163f..6090124b64f1 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3669,6 +3669,10 @@ static vm_fault_t handle_pte_marker(struct vm_fault *vmf)
 	if (WARN_ON_ONCE(!marker))
 		return VM_FAULT_SIGBUS;
 
+	/* Higher priority than uffd-wp when data corrupted */
+	if (marker & PTE_MARKER_SWAPIN_ERROR)
+		return VM_FAULT_SIGBUS;
+
 	if (pte_marker_entry_uffd_wp(entry))
 		return pte_marker_handle_uffd_wp(vmf);
 
@@ -3728,8 +3732,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 			put_page(vmf->page);
 		} else if (is_hwpoison_entry(entry)) {
 			ret = VM_FAULT_HWPOISON;
-		} else if (is_swapin_error_entry(entry)) {
-			ret = VM_FAULT_SIGBUS;
 		} else if (is_pte_marker_entry(entry)) {
 			ret = handle_pte_marker(vmf);
 		} else {
diff --git a/mm/shmem.c b/mm/shmem.c
index 0a7c4a748811..7428ae3fa4b9 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1682,7 +1682,7 @@ static void shmem_set_folio_swapin_error(struct inode *inode, pgoff_t index,
 	swp_entry_t swapin_error;
 	void *old;
 
-	swapin_error = make_swapin_error_entry(&folio->page);
+	swapin_error = make_swapin_error_entry();
 	old = xa_cmpxchg_irq(&mapping->i_pages, index,
 			     swp_to_radix_entry(swap),
 			     swp_to_radix_entry(swapin_error), 0);
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 5fc1237a9f21..13f2569a6719 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1781,7 +1781,7 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
 		pte_t pteval;
 
 		dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
-		pteval = swp_entry_to_pte(make_swapin_error_entry(page));
+		pteval = swp_entry_to_pte(make_swapin_error_entry());
 		set_pte_at(vma->vm_mm, addr, pte, pteval);
 		swap_free(entry);
 		ret = 0;
-- 
2.37.3

Re: [PATCH 2/2] mm: Use pte markers for swap errors

[David Hildenbrand]

On 30.10.22 22:41, Peter Xu wrote:
> PTE markers are ideal mechanism for things like SWP_SWAPIN_ERROR.  Using a
> whole swap entry type for this purpose can be an overkill, especially if we
> already have PTE markers.  Define a new bit for swapin error and replace it
> with pte markers.  Then we can safely drop SWP_SWAPIN_ERROR and give one
> device slot back to swap.
> 
> We used to have SWP_SWAPIN_ERROR taking the page pfn as part of the swap
> entry, but it's never used.  Neither do I see how it can be useful because
> normally the swapin failure should not be caused by a bad page but bad swap
> device.  Drop it alongside.
> 
> Reviewed-by: Huang Ying <ying.huang@intel.com>
> Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---


Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 0/2] mm: Use pte marker for swapin errors

[Huang, Ying]

Peter Xu <peterx@redhat.com> writes:

> rfc->v1:
> - Rename SWAP_ERROR -> SWAPIN_ERROR [David, Miaohe]
> - Added r-bs for Ying and Miaohe
>
> Hi,
>
> This series uses the pte marker to replace the swapin error swap entry,
> then we save one more swap entry slot for swap devices.  A new pte marker
> bit is defined.

Per my understanding, "swap entry" describes one swap page (4KB on x86)
in one swap device.  So, the original swapin error implementation uses
one swap file (or swap device) instead of one swap entry.  So your patch
saves one swap file (or swap device).

Best Regards,
Huang, Ying

> One thing worth mentioning: we need the pte marker to be always built to
> make sure it works like before, so I made an attempt in patch 1 to drop
> CONFIG_PTE_MARKER and always compile pte marker in.  Since pte markers are
> enabled in many distributions by default already, meanwhile the codeset is
> really small I assume it's fine.  But still I'm tagging with RFC for this
> initial version.
>
> Smoke tested on anonymous mem on a fake swap failure.  Please have a look,
> thanks.
>
> Peter Xu (2):
>   mm: Always compile in pte markers
>   mm: Use pte markers for swap errors
>
>  include/linux/swap.h    | 16 ++++----------
>  include/linux/swapops.h | 49 +++++++++--------------------------------
>  mm/Kconfig              |  7 ------
>  mm/memory.c             | 13 ++++++-----
>  mm/shmem.c              |  2 +-
>  mm/swapfile.c           |  2 +-
>  6 files changed, 23 insertions(+), 66 deletions(-)

Re: [PATCH 0/2] mm: Use pte marker for swapin errors

[Peter Xu]

On Mon, Oct 31, 2022 at 09:40:15AM +0800, Huang, Ying wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > rfc->v1:
> > - Rename SWAP_ERROR -> SWAPIN_ERROR [David, Miaohe]
> > - Added r-bs for Ying and Miaohe
> >
> > Hi,
> >
> > This series uses the pte marker to replace the swapin error swap entry,
> > then we save one more swap entry slot for swap devices.  A new pte marker
> > bit is defined.
> 
> Per my understanding, "swap entry" describes one swap page (4KB on x86)
> in one swap device.  So, the original swapin error implementation uses
> one swap file (or swap device) instead of one swap entry.  So your patch
> saves one swap file (or swap device).

Yeah I was definitely not accurate enough, I wanted to use "slot" to mean
"type" here but obviously I didn't make it, sorry.  Hopefully the whole
patchset is still clear enough anyway so no reader will get confused by it.

-- 
Peter Xu

Re: [PATCH 0/2] mm: Use pte marker for swapin errors

[Huang, Ying]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Oct 31, 2022 at 09:40:15AM +0800, Huang, Ying wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > rfc->v1:
>> > - Rename SWAP_ERROR -> SWAPIN_ERROR [David, Miaohe]
>> > - Added r-bs for Ying and Miaohe
>> >
>> > Hi,
>> >
>> > This series uses the pte marker to replace the swapin error swap entry,
>> > then we save one more swap entry slot for swap devices.  A new pte marker
>> > bit is defined.
>> 
>> Per my understanding, "swap entry" describes one swap page (4KB on x86)
>> in one swap device.  So, the original swapin error implementation uses
>> one swap file (or swap device) instead of one swap entry.  So your patch
>> saves one swap file (or swap device).
>
> Yeah I was definitely not accurate enough, I wanted to use "slot" to mean
> "type" here but obviously I didn't make it, sorry.  Hopefully the whole
> patchset is still clear enough anyway so no reader will get confused by it.

The terminology used in swap code is a little complex.  The swap slot is
used to describe swap entry (swap page) too in swap slots cache
(swap_slots.c/h).

Best Regards,
Huang, Ying