mm: Remember a/d bits for migration entries

Replace all the magic "5" with the macro.

Reviewed-by David Hildenbrand <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 arch/x86/include/asm/pgtable-3level.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

arch/x86/include/asm/pgtable-3level.h +4 -4

index e896ebef8c24..28421a887209 100644
@@ -256,10 +256,10 @@ static inline pud_t native_pudp_get_and_clear(pud_t *pudp)
 /* We always extract/encode the offset by shifting it all the way up, and then down again */
 #define SWP_OFFSET_SHIFT	(SWP_OFFSET_FIRST_BIT + SWP_TYPE_BITS)
 
-#define MAX_SWAPFILES_CHECK() BUILD_BUG_ON(MAX_SWAPFILES_SHIFT > 5)
-#define __swp_type(x)			(((x).val) & 0x1f)
-#define __swp_offset(x)			((x).val >> 5)
-#define __swp_entry(type, offset)	((swp_entry_t){(type) | (offset) << 5})
+#define MAX_SWAPFILES_CHECK() BUILD_BUG_ON(MAX_SWAPFILES_SHIFT > SWP_TYPE_BITS)
+#define __swp_type(x)			(((x).val) & ((1UL << SWP_TYPE_BITS) - 1))
+#define __swp_offset(x)			((x).val >> SWP_TYPE_BITS)
+#define __swp_entry(type, offset)	((swp_entry_t){(type) | (offset) << SWP_TYPE_BITS})
 
 /*
  * Normally, __swp_entry() converts from arch-independent swp_entry_t to

swapops.h contains quite a few layers of ifdef, some of the "else" and "endif" doesn't get proper comment on the macro so it's hard to follow on what are they referring to. Add the comments.

Suggested-by Nadav Amit <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 include/linux/swapops.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

include/linux/swapops.h +8 -8

index a3d435bf9f97..3a2901ff4f1e 100644
@@ -247,8 +247,8 @@ extern void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd,
 #ifdef CONFIG_HUGETLB_PAGE
 extern void __migration_entry_wait_huge(pte_t *ptep, spinlock_t *ptl);
 extern void migration_entry_wait_huge(struct vm_area_struct *vma, pte_t *pte);
-#endif
-#else
+#endif	/* CONFIG_HUGETLB_PAGE */
+#else  /* CONFIG_MIGRATION */
 static inline swp_entry_t make_readable_migration_entry(pgoff_t offset)
 {
 	return swp_entry(0, 0);

@@ -276,7 +276,7 @@ static inline void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd,
 #ifdef CONFIG_HUGETLB_PAGE
 static inline void __migration_entry_wait_huge(pte_t *ptep, spinlock_t *ptl) { }
 static inline void migration_entry_wait_huge(struct vm_area_struct *vma, pte_t *pte) { }
-#endif
+#endif	/* CONFIG_HUGETLB_PAGE */
 static inline int is_writable_migration_entry(swp_entry_t entry)
 {
 	return 0;

@@ -286,7 +286,7 @@ static inline int is_readable_migration_entry(swp_entry_t entry)
 	return 0;
 }
 
-#endif
+#endif	/* CONFIG_MIGRATION */
 
 typedef unsigned long pte_marker;
 

@@ -426,7 +426,7 @@ static inline int is_pmd_migration_entry(pmd_t pmd)
 {
 	return is_swap_pmd(pmd) && is_migration_entry(pmd_to_swp_entry(pmd));
 }
-#else
+#else  /* CONFIG_ARCH_ENABLE_THP_MIGRATION */
 static inline int set_pmd_migration_entry(struct page_vma_mapped_walk *pvmw,
 		struct page *page)
 {

@@ -455,7 +455,7 @@ static inline int is_pmd_migration_entry(pmd_t pmd)
 {
 	return 0;
 }
-#endif
+#endif  /* CONFIG_ARCH_ENABLE_THP_MIGRATION */
 
 #ifdef CONFIG_MEMORY_FAILURE
 

@@ -495,7 +495,7 @@ static inline void num_poisoned_pages_sub(long i)
 	atomic_long_sub(i, &num_poisoned_pages);
 }
 
-#else
+#else  /* CONFIG_MEMORY_FAILURE */
 
 static inline swp_entry_t make_hwpoison_entry(struct page *page)
 {

@@ -514,7 +514,7 @@ static inline void num_poisoned_pages_inc(void)
 static inline void num_poisoned_pages_sub(long i)
 {
 }
-#endif
+#endif  /* CONFIG_MEMORY_FAILURE */
 
 static inline int non_swap_entry(swp_entry_t entry)
 {

We've got a bunch of special swap entries that stores PFN inside the swap offset fields. To fetch the PFN, normally the user just calls swp_offset() assuming that'll be the PFN.

Add a helper swp_offset_pfn() to fetch the PFN instead, fetching only the max possible length of a PFN on the host, meanwhile doing proper check with MAX_PHYSMEM_BITS to make sure the swap offsets can actually store the PFNs properly always using the BUILD_BUG_ON() in is_pfn_swap_entry().

One reason to do so is we never tried to sanitize whether swap offset can really fit for storing PFN. At the meantime, this patch also prepares us with the future possibility to store more information inside the swp offset field, so assuming "swp_offset(entry)" to be the PFN will not stand any more very soon.

Replace many of the swp_offset() callers to use swp_offset_pfn() where proper. Note that many of the existing users are not candidates for the replacement, e.g.:

(1) When the swap entry is not a pfn swap entry at all, or,
(2) when we wanna keep the whole swp_offset but only change the swp type.

For the latter, it can happen when fork() triggered on a write-migration swap entry pte, we may want to only change the migration type from write->read but keep the rest, so it's not "fetching PFN" but "changing swap type only". They're left aside so that when there're more information within the swp offset they'll be carried over naturally in those cases.

Since at it, dropping hwpoison_entry_to_pfn() because that's exactly what the new swp_offset_pfn() is about.

Signed-off-by Peter Xu <[email protected]>

Quoted 28 lines · Peter Xu

We've got a bunch of special swap entries that stores PFN inside the swap offset fields. To fetch the PFN, normally the user just calls swp_offset() assuming that'll be the PFN.

Add a helper swp_offset_pfn() to fetch the PFN instead, fetching only the max possible length of a PFN on the host, meanwhile doing proper check with MAX_PHYSMEM_BITS to make sure the swap offsets can actually store the PFNs properly always using the BUILD_BUG_ON() in is_pfn_swap_entry().

One reason to do so is we never tried to sanitize whether swap offset can really fit for storing PFN. At the meantime, this patch also prepares us with the future possibility to store more information inside the swp offset field, so assuming "swp_offset(entry)" to be the PFN will not stand any more very soon.

Replace many of the swp_offset() callers to use swp_offset_pfn() where proper. Note that many of the existing users are not candidates for the replacement, e.g.:

(1) When the swap entry is not a pfn swap entry at all, or,
(2) when we wanna keep the whole swp_offset but only change the swp type.

For the latter, it can happen when fork() triggered on a write-migration swap entry pte, we may want to only change the migration type from write->read but keep the rest, so it's not "fetching PFN" but "changing swap type only". They're left aside so that when there're more information within the swp offset they'll be carried over naturally in those cases.

Quoted 4 lines

Since at it, dropping hwpoison_entry_to_pfn() because that's exactly what the new swp_offset_pfn() is about.

Signed-off-by Peter Xu <[email protected]>

The patch itself looks good. But I searched swp_entry() in kernel source code, and found that we need to do more.

For example, in pte_to_pagemap_entry()

			frame = swp_type(entry) |
				(swp_offset(entry) << MAX_SWAPFILES_SHIFT);

If it's a migration entry, we need

			frame = swp_type(entry) |
				(swp_offset_pfn(entry) << MAX_SWAPFILES_SHIFT);

So I think you need to search all swp_offset() calling in the kernel source and check whether they need to be changed.

Carry over the dirty bit from pmd to pte when a huge pmd splits. It shouldn't be a correctness issue since when pmd_dirty() we'll have the page marked dirty anyway, however having dirty bit carried over helps the next initial writes of split ptes on some archs like x86.

Signed-off-by Peter Xu <[email protected]>

---
 mm/huge_memory.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/huge_memory.c +1 -1

index 0611b2fd145a..e8e78d1bac5f 100644
@@ -2005,7 +2005,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 	pgtable_t pgtable;
 	pmd_t old_pmd, _pmd;
 	bool young, write, soft_dirty, pmd_migration = false, uffd_wp = false;
-	bool anon_exclusive = false;
+	bool anon_exclusive = false, dirty = false;
 	unsigned long addr;
 	int i;
 

mm/huge_memory.c +3 -0

 		soft_dirty = pmd_soft_dirty(old_pmd);
 		uffd_wp = pmd_uffd_wp(old_pmd);
 

@@ -2161,6 +2162,9 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 				entry = pte_wrprotect(entry);
 			if (!young)
 				entry = pte_mkold(entry);
+			/* NOTE: this may set soft-dirty too on some archs */
+			if (dirty)
+				entry = pte_mkdirty(entry);
 			if (soft_dirty)
 				entry = pte_mksoft_dirty(entry);
 			if (uffd_wp)

Quoted 24 lines · Peter Xu

Carry over the dirty bit from pmd to pte when a huge pmd splits. It shouldn't be a correctness issue since when pmd_dirty() we'll have the page marked dirty anyway, however having dirty bit carried over helps the next initial writes of split ptes on some archs like x86.

Signed-off-by Peter Xu <[email protected]>

---
 mm/huge_memory.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/huge_memory.c +1 -1

index 0611b2fd145a..e8e78d1bac5f 100644
@@ -2005,7 +2005,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 	pgtable_t pgtable;
 	pmd_t old_pmd, _pmd;
 	bool young, write, soft_dirty, pmd_migration = false, uffd_wp = false;
-	bool anon_exclusive = false;
+	bool anon_exclusive = false, dirty = false;
 	unsigned long addr;
 	int i;
 

mm/huge_memory.c static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, · line 2101

Quoted 4 lines

mm/huge_memory.c +1 -0

@@ -2098,6 +2098,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 			SetPageDirty(page);
 		write = pmd_write(old_pmd);
 		young = pmd_young(old_pmd);
+		dirty = pmd_dirty(old_pmd);

Nitpick: This can be put under

		if (pmd_dirty(old_pmd))
			SetPageDirty(page);

Not a big deal.

Reviewed-by "Huang, Ying" <[email protected]>

When page migration happens, we always ignore the young/dirty bit settings in the old pgtable, and marking the page as old in the new page table using either pte_mkold() or pmd_mkold(), and keeping the pte clean.

That's fine from functional-wise, but that's not friendly to page reclaim because the moving page can be actively accessed within the procedure. Not to mention hardware setting the young bit can bring quite some overhead on some systems, e.g. x86_64 needs a few hundreds nanoseconds to set the bit. The same slowdown problem to dirty bits when the memory is first written after page migration happened.

Actually we can easily remember the A/D bit configuration and recover the information after the page is migrated. To achieve it, define a new set of bits in the migration swap offset field to cache the A/D bits for old pte. Then when removing/recovering the migration entry, we can recover the A/D bits even if the page changed.

One thing to mention is that here we used max_swapfile_size() to detect how many swp offset bits we have, and we'll only enable this feature if we know the swp offset can be big enough to store both the PFN value and the young

bit. Otherwise the A/D bits are dropped like before.

Signed-off-by Peter Xu <[email protected]>

---
 include/linux/swapops.h | 99 +++++++++++++++++++++++++++++++++++++++++
 mm/huge_memory.c        | 18 +++++++-
 mm/migrate.c            |  6 ++-
 mm/migrate_device.c     |  4 ++
 mm/rmap.c               |  5 ++-
 5 files changed, 128 insertions(+), 4 deletions(-)

include/linux/swapops.h +0 -0

index e1accbcd1136..0e9579b90659 100644

Quoted 17 lines · Peter Xu

When page migration happens, we always ignore the young/dirty bit settings in the old pgtable, and marking the page as old in the new page table using either pte_mkold() or pmd_mkold(), and keeping the pte clean.

That's fine from functional-wise, but that's not friendly to page reclaim because the moving page can be actively accessed within the procedure. Not to mention hardware setting the young bit can bring quite some overhead on some systems, e.g. x86_64 needs a few hundreds nanoseconds to set the bit. The same slowdown problem to dirty bits when the memory is first written after page migration happened.

Actually we can easily remember the A/D bit configuration and recover the information after the page is migrated. To achieve it, define a new set of bits in the migration swap offset field to cache the A/D bits for old pte. Then when removing/recovering the migration entry, we can recover the A/D bits even if the page changed.

Quoted 3 lines

One thing to mention is that here we used max_swapfile_size() to detect how many swp offset bits we have, and we'll only enable this feature if we know the swp offset can be big enough to store both the PFN value and the young

                                                                       ~~~~~

Nitpick: A/D

Quoted 18 lines

bit. Otherwise the A/D bits are dropped like before.

Signed-off-by Peter Xu <[email protected]>

---
 include/linux/swapops.h | 99 +++++++++++++++++++++++++++++++++++++++++
 mm/huge_memory.c        | 18 +++++++-
 mm/migrate.c            |  6 ++-
 mm/migrate_device.c     |  4 ++
 mm/rmap.c               |  5 ++-
 5 files changed, 128 insertions(+), 4 deletions(-)

include/linux/swapops.h +0 -0

index e1accbcd1136..0e9579b90659 100644
@@ -8,6 +8,10 @@
 

include/linux/swapops.h line 13

Quoted 5 lines

include/linux/swapops.h +3 -0

 #ifdef CONFIG_MMU
 
+#ifdef CONFIG_SWAP
+#include <linux/swapfile.h>
+#endif	/* CONFIG_SWAP */

I don't think we need the comment here. The #ifdef is too near. But this isn't a big deal.

mm/migrate_device.c +0 -0

index 27fb37d65476..699f821b8443 100644

Quoted 5 lines · Peter Xu

mm/migrate_device.c +0 -0

index 27fb37d65476..699f821b8443 100644

mm/migrate_device.c static int migrate_vma_collect_pmd(pmd_t *pmdp, · line 230

Quoted 10 lines

mm/migrate_device.c +4 -0

@@ -221,6 +221,10 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 			else
 				entry = make_readable_migration_entry(
 							page_to_pfn(page));
+			if (pte_young(pte))
+				entry = make_migration_entry_young(entry);
+			if (pte_dirty(pte))
+				entry = make_migration_entry_dirty(entry);
 			swp_pte = swp_entry_to_pte(entry);
 			if (pte_present(pte)) {
 				if (pte_soft_dirty(pte))

This change needs to be wrapped with pte_present() at least..

I also just noticed that this change probably won't help anyway because:

(1) When ram->device, the pte will finally be replaced with a device private entry, and device private entry does not yet support A/D, it means A/D will be dropped again,

(2) When device->ram, we are missing information on either A/D bits, or even if device private entries start to suport A/D, it's still not clear whether we should take device read/write into considerations too on the page A/D bits to be accurate.

I think I'll probably keep the code there for completeness, but I think it won't really help much until more things are done.

Quoted 30 lines · Peter Xu

Quoted 5 lines · Peter Xu

mm/migrate_device.c +0 -0

index 27fb37d65476..699f821b8443 100644

mm/migrate_device.c static int migrate_vma_collect_pmd(pmd_t *pmdp, · line 230

Quoted 10 lines

mm/migrate_device.c +4 -0

@@ -221,6 +221,10 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 			else
 				entry = make_readable_migration_entry(
 							page_to_pfn(page));
+			if (pte_young(pte))
+				entry = make_migration_entry_young(entry);
+			if (pte_dirty(pte))
+				entry = make_migration_entry_dirty(entry);
 			swp_pte = swp_entry_to_pte(entry);
 			if (pte_present(pte)) {
 				if (pte_soft_dirty(pte))

This change needs to be wrapped with pte_present() at least..

I also just noticed that this change probably won't help anyway because:

(1) When ram->device, the pte will finally be replaced with a device private entry, and device private entry does not yet support A/D, it means A/D will be dropped again,

(2) When device->ram, we are missing information on either A/D bits, or even if device private entries start to suport A/D, it's still not clear whether we should take device read/write into considerations too on the page A/D bits to be accurate.

Quoted 2 lines

I think I'll probably keep the code there for completeness, but I think it won't really help much until more things are done.

It appears that there are more issues. Between "pte = *ptep" and pte clear, CPU may set A/D bit in PTE, so we may need to update pte when clearing PTE.

And I don't find the TLB is flushed in some cases after PTE is cleared.

Quoted 32 lines · Ying

Quoted 30 lines · Peter Xu

Quoted 5 lines · Peter Xu

mm/migrate_device.c +0 -0

index 27fb37d65476..699f821b8443 100644

mm/migrate_device.c static int migrate_vma_collect_pmd(pmd_t *pmdp, · line 230

Quoted 10 lines

mm/migrate_device.c +4 -0

@@ -221,6 +221,10 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 			else
 				entry = make_readable_migration_entry(
 							page_to_pfn(page));
+			if (pte_young(pte))
+				entry = make_migration_entry_young(entry);
+			if (pte_dirty(pte))
+				entry = make_migration_entry_dirty(entry);
 			swp_pte = swp_entry_to_pte(entry);
 			if (pte_present(pte)) {
 				if (pte_soft_dirty(pte))

This change needs to be wrapped with pte_present() at least..

I also just noticed that this change probably won't help anyway because:

(1) When ram->device, the pte will finally be replaced with a device private entry, and device private entry does not yet support A/D, it means A/D will be dropped again,

(2) When device->ram, we are missing information on either A/D bits, or even if device private entries start to suport A/D, it's still not clear whether we should take device read/write into considerations too on the page A/D bits to be accurate.

Quoted 6 lines

Quoted 2 lines

I think I'll probably keep the code there for completeness, but I think it won't really help much until more things are done.

It appears that there are more issues. Between "pte = *ptep" and pte clear, CPU may set A/D bit in PTE, so we may need to update pte when clearing PTE.

Agreed, I didn't see it a huge problem with current code, but it should be better in that way.

Quoted 1 line

And I don't find the TLB is flushed in some cases after PTE is cleared.

I think it's okay to not flush tlb if pte not present. But maybe you're talking about something else?

Quoted 40 lines · Peter Xu

Quoted 32 lines · Ying

Quoted 30 lines · Peter Xu

Quoted 5 lines · Peter Xu

mm/migrate_device.c +0 -0

index 27fb37d65476..699f821b8443 100644

mm/migrate_device.c static int migrate_vma_collect_pmd(pmd_t *pmdp, · line 230

Quoted 10 lines

mm/migrate_device.c +4 -0

@@ -221,6 +221,10 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 			else
 				entry = make_readable_migration_entry(
 							page_to_pfn(page));
+			if (pte_young(pte))
+				entry = make_migration_entry_young(entry);
+			if (pte_dirty(pte))
+				entry = make_migration_entry_dirty(entry);
 			swp_pte = swp_entry_to_pte(entry);
 			if (pte_present(pte)) {
 				if (pte_soft_dirty(pte))

This change needs to be wrapped with pte_present() at least..

I also just noticed that this change probably won't help anyway because:

(1) When ram->device, the pte will finally be replaced with a device private entry, and device private entry does not yet support A/D, it means A/D will be dropped again,

(2) When device->ram, we are missing information on either A/D bits, or even if device private entries start to suport A/D, it's still not clear whether we should take device read/write into considerations too on the page A/D bits to be accurate.

Quoted 6 lines

Quoted 2 lines

I think I'll probably keep the code there for completeness, but I think it won't really help much until more things are done.

It appears that there are more issues. Between "pte = *ptep" and pte clear, CPU may set A/D bit in PTE, so we may need to update pte when clearing PTE.

Quoted 7 lines

Agreed, I didn't see it a huge problem with current code, but it should be better in that way.

Quoted 1 line

And I don't find the TLB is flushed in some cases after PTE is cleared.

I think it's okay to not flush tlb if pte not present. But maybe you're talking about something else?

I think Huang refers to situation in which the PTE is cleared, still not flushed, and then A/D is being set by the hardware.

At least on x86, the hardware is not supposed to do so. The only case I remember (and sometimes misremembers) is with KNL erratum, which perhaps needs to be considered:

https://lore.kernel.org/all/[email protected]/

Quoted 6 lines

At least on x86, the hardware is not supposed to do so. The only case I remember (and sometimes misremembers) is with KNL erratum, which perhaps needs to be considered:

https://lore.kernel.org/all/[email protected]/

I keep not remembering this erratum correctly. IIRC, the erratum says that the access/dirty might be set, but it does not mean that a write is possible after the PTE is cleared (i.e., the dirty/access might be set on the non-present PTE, but the access itself would fail). So it is not an issue in this case - losing A/D would not impact correctness since the access should fail.

Dave Hansen hates when I get confused with this one, but I cc him if he wants to confirm.

Quoted 43 lines · Peter Xu

Quoted 32 lines · Ying

Quoted 30 lines · Peter Xu

Quoted 5 lines · Peter Xu

mm/migrate_device.c +0 -0

index 27fb37d65476..699f821b8443 100644

mm/migrate_device.c static int migrate_vma_collect_pmd(pmd_t *pmdp, · line 230

Quoted 10 lines

mm/migrate_device.c +4 -0

@@ -221,6 +221,10 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 			else
 				entry = make_readable_migration_entry(
 							page_to_pfn(page));
+			if (pte_young(pte))
+				entry = make_migration_entry_young(entry);
+			if (pte_dirty(pte))
+				entry = make_migration_entry_dirty(entry);
 			swp_pte = swp_entry_to_pte(entry);
 			if (pte_present(pte)) {
 				if (pte_soft_dirty(pte))

This change needs to be wrapped with pte_present() at least..

I also just noticed that this change probably won't help anyway because:

(1) When ram->device, the pte will finally be replaced with a device private entry, and device private entry does not yet support A/D, it means A/D will be dropped again,

(2) When device->ram, we are missing information on either A/D bits, or even if device private entries start to suport A/D, it's still not clear whether we should take device read/write into considerations too on the page A/D bits to be accurate.

Quoted 6 lines

Quoted 2 lines

I think I'll probably keep the code there for completeness, but I think it won't really help much until more things are done.

It appears that there are more issues. Between "pte = *ptep" and pte clear, CPU may set A/D bit in PTE, so we may need to update pte when clearing PTE.

Agreed, I didn't see it a huge problem with current code, but it should be better in that way.

Quoted 4 lines

Quoted 1 line

And I don't find the TLB is flushed in some cases after PTE is cleared.

I think it's okay to not flush tlb if pte not present. But maybe you're talking about something else?

I think Huang refers to situation in which the PTE is cleared, still not flushed, and then A/D is being set by the hardware.

We used to have swapfile_maximum_size() fetching a maximum value of swapfile size per-arch.

As the caller of max_swapfile_size() grows, this patch introduce a variable "swapfile_maximum_size" and cache the value of old max_swapfile_size(), so that we don't need to calculate the value every time.

Caching the value in swapfile_init() is safe because when reaching the phase we should have initialized all the relevant information. Here the major arch to look after is x86, which defines the max size based on L1TF mitigation.

Here both X86_BUG_L1TF or l1tf_mitigation should have been setup properly when reaching swapfile_init(). As a reference, the code path looks like this for x86:

- start_kernel
- setup_arch
- early_cpu_init
- early_identify_cpu --> setup X86_BUG_L1TF
- parse_early_param
- l1tf_cmdline --> set l1tf_mitigation
- check_bugs
- l1tf_select_mitigation --> set l1tf_mitigation
- arch_call_rest_init
- rest_init
- kernel_init
- kernel_init_freeable
- do_basic_setup
- do_initcalls --> calls swapfile_init() (initcall level 4)

The swapfile size only depends on swp pte format on non-x86 archs, so caching it is safe too.

Since at it, rename max_swapfile_size() to arch_max_swapfile_size() because arch can define its own function, so it's more straightforward to have "arch_" as its prefix. At the meantime, keep the swapfile_maximum_size() function to fetch the value from the cache initialized in swapfile_init().

Signed-off-by Peter Xu <[email protected]>

---
 arch/x86/mm/init.c |  2 +-
 mm/swapfile.c      | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

arch/x86/mm/init.c +1 -1

index 82a042c03824..9121bc1b9453 100644
@@ -1054,7 +1054,7 @@ void update_cache_mode_entry(unsigned entry, enum page_cache_mode cache)
 }
 
 #ifdef CONFIG_SWAP
-unsigned long max_swapfile_size(void)
+unsigned long arch_max_swapfile_size(void)
 {
 	unsigned long pages;
 

mm/swapfile.c +1 -1

index 1fdccd2f1422..794fa37bd0c3 100644
@@ -63,6 +63,7 @@ EXPORT_SYMBOL_GPL(nr_swap_pages);
 /* protected with swap_lock. reading in vm_swap_full() doesn't need lock */
 long total_swap_pages;
 static int least_priority = -1;
+static unsigned long swapfile_maximum_size;
 
 static const char Bad_file[] = "Bad swap file entry ";
 static const char Unused_file[] = "Unused swap file entry ";

@@ -2816,11 +2817,16 @@ unsigned long generic_max_swapfile_size(void)
 }
 
 /* Can be overridden by an architecture for additional checks. */
-__weak unsigned long max_swapfile_size(void)

mm/swapfile.c +2 -0

 static unsigned long read_swap_header(struct swap_info_struct *p,
 					union swap_header *swap_header,
 					struct inode *inode)

@@ -3677,6 +3683,8 @@ static int __init swapfile_init(void)
 	for_each_node(nid)
 		plist_head_init(&swap_avail_heads[nid]);
 
+	swapfile_maximum_size = arch_max_swapfile_size();
+
 	return 0;
 }
 subsys_initcall(swapfile_init);

Quoted 80 lines · Peter Xu

We used to have swapfile_maximum_size() fetching a maximum value of swapfile size per-arch.

As the caller of max_swapfile_size() grows, this patch introduce a variable "swapfile_maximum_size" and cache the value of old max_swapfile_size(), so that we don't need to calculate the value every time.

Caching the value in swapfile_init() is safe because when reaching the phase we should have initialized all the relevant information. Here the major arch to look after is x86, which defines the max size based on L1TF mitigation.

Here both X86_BUG_L1TF or l1tf_mitigation should have been setup properly when reaching swapfile_init(). As a reference, the code path looks like this for x86:

- start_kernel
- setup_arch
- early_cpu_init
- early_identify_cpu --> setup X86_BUG_L1TF
- parse_early_param
- l1tf_cmdline --> set l1tf_mitigation
- check_bugs
- l1tf_select_mitigation --> set l1tf_mitigation
- arch_call_rest_init
- rest_init
- kernel_init
- kernel_init_freeable
- do_basic_setup
- do_initcalls --> calls swapfile_init() (initcall level 4)

The swapfile size only depends on swp pte format on non-x86 archs, so caching it is safe too.

Since at it, rename max_swapfile_size() to arch_max_swapfile_size() because arch can define its own function, so it's more straightforward to have "arch_" as its prefix. At the meantime, keep the swapfile_maximum_size() function to fetch the value from the cache initialized in swapfile_init().

Signed-off-by Peter Xu <[email protected]>

---
 arch/x86/mm/init.c |  2 +-
 mm/swapfile.c      | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

arch/x86/mm/init.c +1 -1

index 82a042c03824..9121bc1b9453 100644
@@ -1054,7 +1054,7 @@ void update_cache_mode_entry(unsigned entry, enum page_cache_mode cache)
 }
 
 #ifdef CONFIG_SWAP
-unsigned long max_swapfile_size(void)
+unsigned long arch_max_swapfile_size(void)
 {
 	unsigned long pages;
 

mm/swapfile.c +2 -1

index 1fdccd2f1422..794fa37bd0c3 100644
@@ -63,6 +63,7 @@ EXPORT_SYMBOL_GPL(nr_swap_pages);
 /* protected with swap_lock. reading in vm_swap_full() doesn't need lock */
 long total_swap_pages;
 static int least_priority = -1;
+static unsigned long swapfile_maximum_size;
 
 static const char Bad_file[] = "Bad swap file entry ";
 static const char Unused_file[] = "Unused swap file entry ";

@@ -2816,11 +2817,16 @@ unsigned long generic_max_swapfile_size(void)
 }
 
 /* Can be overridden by an architecture for additional checks. */
-__weak unsigned long max_swapfile_size(void)
+__weak unsigned long arch_max_swapfile_size(void)
 {
 	return generic_max_swapfile_size();
 }
 

mm/swapfile.c unsigned long generic_max_swapfile_size(void) · line 2829

Quoted 5 lines

mm/swapfile.c +5 -0

+unsigned long max_swapfile_size(void)
+{
+	return swapfile_maximum_size;
+}
+

It appears unnecessary to hide a variable with a function. Why not just use the variable directly.

Introduce a variable swap_migration_ad_supported to cache whether the arch supports swap migration A/D bits.

Here one thing to mention is that SWP_MIG_TOTAL_BITS will internally reference the other macro MAX_PHYSMEM_BITS, which is a function call on x86 (constant on all the rest of archs).

It's safe to reference it in swapfile_init() because when reaching here we're already during initcalls level 4 so we must have initialized 5-level pgtable for x86_64 (right after early_identify_cpu() finishes).

- start_kernel
- setup_arch
- early_cpu_init
- get_cpu_cap --> fetch from CPUID (including X86_FEATURE_LA57)
- early_identify_cpu --> clear X86_FEATURE_LA57 (if early lvl5 not enabled (USE_EARLY_PGTABLE_L5))
- arch_call_rest_init
- rest_init
- kernel_init
- kernel_init_freeable
- do_basic_setup
- do_initcalls --> calls swapfile_init() (initcall level 4)

This should slightly speed up the migration swap entry handlings.

Signed-off-by Peter Xu <[email protected]>

---
 include/linux/swapfile.h | 1 +
 include/linux/swapops.h  | 7 +------
 mm/swapfile.c            | 8 ++++++++
 3 files changed, 10 insertions(+), 6 deletions(-)

include/linux/swapfile.h +1 -0

index 54078542134c..87ec5e2cdb02 100644
@@ -9,5 +9,6 @@
 extern struct swap_info_struct *swap_info[];
 extern unsigned long generic_max_swapfile_size(void);
 extern unsigned long max_swapfile_size(void);
+extern bool swap_migration_ad_supported;
 
 #endif /* _LINUX_SWAPFILE_H */

include/linux/swapops.h +1 -6

index 0e9579b90659..e6afc77c51ad 100644
@@ -301,13 +301,8 @@ static inline swp_entry_t make_writable_migration_entry(pgoff_t offset)
  */
 static inline bool migration_entry_supports_ad(void)
 {
-	/*
-	 * max_swapfile_size() returns the max supported swp-offset plus 1.
-	 * We can support the migration A/D bits iff the pfn swap entry has
-	 * the offset large enough to cover all of them (PFN, A & D bits).
-	 */
 #ifdef CONFIG_SWAP
-	return max_swapfile_size() >= (1UL << SWP_MIG_TOTAL_BITS);
+	return swap_migration_ad_supported;
 #else  /* CONFIG_SWAP */
 	return false;
 #endif	/* CONFIG_SWAP */

mm/swapfile.c +3 -0

index 794fa37bd0c3..c49cf25f0d08 100644
@@ -64,6 +64,9 @@ EXPORT_SYMBOL_GPL(nr_swap_pages);
 long total_swap_pages;
 static int least_priority = -1;
 static unsigned long swapfile_maximum_size;
+#ifdef CONFIG_MIGRATION
+bool swap_migration_ad_supported;
+#endif	/* CONFIG_MIGRATION */
 
 static const char Bad_file[] = "Bad swap file entry ";
 static const char Unused_file[] = "Unused swap file entry ";

@@ -3685,6 +3688,11 @@ static int __init swapfile_init(void)
 

Quoted 80 lines · Peter Xu

Introduce a variable swap_migration_ad_supported to cache whether the arch supports swap migration A/D bits.

Here one thing to mention is that SWP_MIG_TOTAL_BITS will internally reference the other macro MAX_PHYSMEM_BITS, which is a function call on x86 (constant on all the rest of archs).

It's safe to reference it in swapfile_init() because when reaching here we're already during initcalls level 4 so we must have initialized 5-level pgtable for x86_64 (right after early_identify_cpu() finishes).

- start_kernel
- setup_arch
- early_cpu_init
- get_cpu_cap --> fetch from CPUID (including X86_FEATURE_LA57)
- early_identify_cpu --> clear X86_FEATURE_LA57 (if early lvl5 not enabled (USE_EARLY_PGTABLE_L5))
- arch_call_rest_init
- rest_init
- kernel_init
- kernel_init_freeable
- do_basic_setup
- do_initcalls --> calls swapfile_init() (initcall level 4)

This should slightly speed up the migration swap entry handlings.

Signed-off-by Peter Xu <[email protected]>

---
 include/linux/swapfile.h | 1 +
 include/linux/swapops.h  | 7 +------
 mm/swapfile.c            | 8 ++++++++
 3 files changed, 10 insertions(+), 6 deletions(-)

include/linux/swapfile.h +1 -0

index 54078542134c..87ec5e2cdb02 100644
@@ -9,5 +9,6 @@
 extern struct swap_info_struct *swap_info[];
 extern unsigned long generic_max_swapfile_size(void);
 extern unsigned long max_swapfile_size(void);
+extern bool swap_migration_ad_supported;
 
 #endif /* _LINUX_SWAPFILE_H */

include/linux/swapops.h +1 -6

index 0e9579b90659..e6afc77c51ad 100644
@@ -301,13 +301,8 @@ static inline swp_entry_t make_writable_migration_entry(pgoff_t offset)
  */
 static inline bool migration_entry_supports_ad(void)
 {
-	/*
-	 * max_swapfile_size() returns the max supported swp-offset plus 1.
-	 * We can support the migration A/D bits iff the pfn swap entry has
-	 * the offset large enough to cover all of them (PFN, A & D bits).
-	 */
 #ifdef CONFIG_SWAP
-	return max_swapfile_size() >= (1UL << SWP_MIG_TOTAL_BITS);
+	return swap_migration_ad_supported;
 #else  /* CONFIG_SWAP */
 	return false;
 #endif	/* CONFIG_SWAP */

mm/swapfile.c +3 -0

index 794fa37bd0c3..c49cf25f0d08 100644
@@ -64,6 +64,9 @@ EXPORT_SYMBOL_GPL(nr_swap_pages);
 long total_swap_pages;
 static int least_priority = -1;
 static unsigned long swapfile_maximum_size;
+#ifdef CONFIG_MIGRATION
+bool swap_migration_ad_supported;
+#endif	/* CONFIG_MIGRATION */
 
 static const char Bad_file[] = "Bad swap file entry ";
 static const char Unused_file[] = "Unused swap file entry ";

@@ -3685,6 +3688,11 @@ static int __init swapfile_init(void)
 
 	swapfile_maximum_size = arch_max_swapfile_size();
 

mm/swapfile.c static int __init swapfile_init(void) · line 3698

Quoted 8 lines

mm/swapfile.c +5 -0

+#ifdef CONFIG_MIGRATION
+	if (swapfile_maximum_size >= (1UL << SWP_MIG_TOTAL_BITS))
+		swap_migration_ad_supported = true;
+#endif	/* CONFIG_MIGRATION */
+
 	return 0;
 }
 subsys_initcall(swapfile_init);

I don't think it's necessary to add a variable for such a simple function and it's not a super hot path. But I don't have strong opinions here.