Now that the rmap overhaul[1] is upstream that provides a clean interface
for rmap batching, let's implement PTE batching during fork when processing
PTE-mapped THPs.
This series is partially based on Ryan's previous work[2] to implement
cont-pte support on arm64, but its a complete rewrite based on [1] to
optimize all architectures independent of any such PTE bits, and to
use the new rmap batching functions that simplify the code and prepare
for further rmap accounting changes.
We collect consecutive PTEs that map consecutive pages of the same large
folio, making sure that the other PTE bits are compatible, and (a) adjust
the refcount only once per batch, (b) call rmap handling functions only
once per batch and (c) perform batch PTE setting/updates.
While this series should be beneficial for adding cont-pte support on
ARM64[2], it's one of the requirements for maintaining a total mapcount[3]
for large folios with minimal added overhead and further changes[4] that
build up on top of the total mapcount.
Independent of all that, this series results in a speedup during fork with
PTE-mapped THP, which is the default with THPs that are smaller than a PMD
(for example, 16KiB to 1024KiB mTHPs for anonymous memory[5]).
On an Intel Xeon Silver 4210R CPU, fork'ing with 1GiB of PTE-mapped folios
of the same size (stddev < 1%) results in the following runtimes
for fork() (shorter is better):
Folio Size | v6.8-rc1 | New | Change
------------------------------------------
4KiB | 0.014328 | 0.014035 | - 2%
16KiB | 0.014263 | 0.01196 | -16%
32KiB | 0.014334 | 0.01094 | -24%
64KiB | 0.014046 | 0.010444 | -26%
128KiB | 0.014011 | 0.010063 | -28%
256KiB | 0.013993 | 0.009938 | -29%
512KiB | 0.013983 | 0.00985 | -30%
1024KiB | 0.013986 | 0.00982 | -30%
2048KiB | 0.014305 | 0.010076 | -30%
Note that these numbers are even better than the ones from v1 (verified
over multiple reboots), even though there were only minimal code changes.
Well, I removed a pte_mkclean() call for anon folios, maybe that also
plays a role.
But my experience is that fork() is extremely sensitive to code size,
inlining, ... so I suspect we'll see on other architectures rather a change
of -20% instead of -30%, and it will be easy to "lose" some of that speedup
in the future by subtle code changes.
Next up is PTE batching when unmapping. Only tested on x86-64.
Compile-tested on most other architectures.
v2 -> v3:
* Rebased on mm-unstable
* Picked up RB's
* Updated documentation of wrprotect_ptes().
v1 -> v2:
* "arm64/mm: Make set_ptes() robust when OAs cross 48-bit boundary"
-> Added patch from Ryan
* "arm/pgtable: define PFN_PTE_SHIFT"
-> Removed the arm64 bits
* "mm/pgtable: make pte_next_pfn() independent of set_ptes()"
* "arm/mm: use pte_next_pfn() in set_ptes()"
* "powerpc/mm: use pte_next_pfn() in set_ptes()"
-> Added to use pte_next_pfn() in some arch set_ptes() implementations
I tried to make use of pte_next_pfn() also in the others, but it's
not trivial because the other archs implement set_ptes() in their
asm/pgtable.h. Future work.
* "mm/memory: factor out copying the actual PTE in copy_present_pte()"
-> Move common folio_get() out of if/else
* "mm/memory: optimize fork() with PTE-mapped THP"
-> Add doc for wrprotect_ptes
-> Extend description to mention handling of pinned folios
-> Move common folio_ref_add() out of if/else
* "mm/memory: ignore dirty/accessed/soft-dirty bits in folio_pte_batch()"
-> Be more conservative with dirt/soft-dirty, let the caller specify
using flags
[1] https://lkml.kernel.org/r/[email protected]
[2] https://lkml.kernel.org/r/[email protected]
[3] https://lkml.kernel.org/r/[email protected]
[4] https://lkml.kernel.org/r/[email protected]
[5] https://lkml.kernel.org/r/[email protected]
Cc: Andrew Morton <[email protected]>
Cc: Matthew Wilcox (Oracle) <[email protected]>
Cc: Ryan Roberts <[email protected]>
Cc: Russell King <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Dinh Nguyen <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Nicholas Piggin <[email protected]>
Cc: Christophe Leroy <[email protected]>
Cc: "Aneesh Kumar K.V" <[email protected]>
Cc: "Naveen N. Rao" <[email protected]>
Cc: Paul Walmsley <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Albert Ou <[email protected]>
Cc: Alexander Gordeev <[email protected]>
Cc: Gerald Schaefer <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Vasily Gorbik <[email protected]>
Cc: Christian Borntraeger <[email protected]>
Cc: Sven Schnelle <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
Andrew asked for a resend based on latest mm-unstable. I am sending this
out earlier than I would usually have sent out the next version, so we can
pull it into mm-unstable again now that v1 was dropped.
David Hildenbrand (14):
arm/pgtable: define PFN_PTE_SHIFT
nios2/pgtable: define PFN_PTE_SHIFT
powerpc/pgtable: define PFN_PTE_SHIFT
riscv/pgtable: define PFN_PTE_SHIFT
s390/pgtable: define PFN_PTE_SHIFT
sparc/pgtable: define PFN_PTE_SHIFT
mm/pgtable: make pte_next_pfn() independent of set_ptes()
arm/mm: use pte_next_pfn() in set_ptes()
powerpc/mm: use pte_next_pfn() in set_ptes()
mm/memory: factor out copying the actual PTE in copy_present_pte()
mm/memory: pass PTE to copy_present_pte()
mm/memory: optimize fork() with PTE-mapped THP
mm/memory: ignore dirty/accessed/soft-dirty bits in folio_pte_batch()
mm/memory: ignore writable bit in folio_pte_batch()
Ryan Roberts (1):
arm64/mm: Make set_ptes() robust when OAs cross 48-bit boundary
arch/arm/include/asm/pgtable.h | 2 +
arch/arm/mm/mmu.c | 2 +-
arch/arm64/include/asm/pgtable.h | 28 ++--
arch/nios2/include/asm/pgtable.h | 2 +
arch/powerpc/include/asm/pgtable.h | 2 +
arch/powerpc/mm/pgtable.c | 5 +-
arch/riscv/include/asm/pgtable.h | 2 +
arch/s390/include/asm/pgtable.h | 2 +
arch/sparc/include/asm/pgtable_64.h | 2 +
include/linux/pgtable.h | 33 ++++-
mm/memory.c | 212 ++++++++++++++++++++++------
11 files changed, 229 insertions(+), 63 deletions(-)
base-commit: d162e170f1181b4305494843e1976584ddf2b72e
--
2.43.0
From: Ryan Roberts <[email protected]>
Since the high bits [51:48] of an OA are not stored contiguously in the
PTE, there is a theoretical bug in set_ptes(), which just adds PAGE_SIZE
to the pte to get the pte with the next pfn. This works until the pfn
crosses the 48-bit boundary, at which point we overflow into the upper
attributes.
Of course one could argue (and Matthew Wilcox has :) that we will never
see a folio cross this boundary because we only allow naturally aligned
power-of-2 allocation, so this would require a half-petabyte folio. So
its only a theoretical bug. But its better that the code is robust
regardless.
I've implemented pte_next_pfn() as part of the fix, which is an opt-in
core-mm interface. So that is now available to the core-mm, which will
be needed shortly to support forthcoming fork()-batching optimizations.
Link: https://lkml.kernel.org/r/[email protected]
Fixes: 4a169d61c2ed ("arm64: implement the new page table range API")
Closes: https://lore.kernel.org/linux-mm/[email protected]/
Signed-off-by: Ryan Roberts <[email protected]>
Reviewed-by: Catalin Marinas <[email protected]>
Reviewed-by: David Hildenbrand <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
arch/arm64/include/asm/pgtable.h | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index b50270107e2f..9428801c1040 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -341,6 +341,22 @@ static inline void __sync_cache_and_tags(pte_t pte, unsigned int nr_pages)
mte_sync_tags(pte, nr_pages);
}
+/*
+ * Select all bits except the pfn
+ */
+static inline pgprot_t pte_pgprot(pte_t pte)
+{
+ unsigned long pfn = pte_pfn(pte);
+
+ return __pgprot(pte_val(pfn_pte(pfn, __pgprot(0))) ^ pte_val(pte));
+}
+
+#define pte_next_pfn pte_next_pfn
+static inline pte_t pte_next_pfn(pte_t pte)
+{
+ return pfn_pte(pte_pfn(pte) + 1, pte_pgprot(pte));
+}
+
static inline void set_ptes(struct mm_struct *mm,
unsigned long __always_unused addr,
pte_t *ptep, pte_t pte, unsigned int nr)
@@ -354,7 +370,7 @@ static inline void set_ptes(struct mm_struct *mm,
if (--nr == 0)
break;
ptep++;
- pte_val(pte) += PAGE_SIZE;
+ pte = pte_next_pfn(pte);
}
}
#define set_ptes set_ptes
@@ -433,16 +449,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
return clear_pte_bit(pte, __pgprot(PTE_SWP_EXCLUSIVE));
}
-/*
- * Select all bits except the pfn
- */
-static inline pgprot_t pte_pgprot(pte_t pte)
-{
- unsigned long pfn = pte_pfn(pte);
-
- return __pgprot(pte_val(pfn_pte(pfn, __pgprot(0))) ^ pte_val(pte));
-}
-
#ifdef CONFIG_NUMA_BALANCING
/*
* See the comment in include/linux/pgtable.h
--
2.43.0
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simply define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <[email protected]>
---
arch/arm/include/asm/pgtable.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index d657b84b6bf7..be91e376df79 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -209,6 +209,8 @@ static inline void __sync_icache_dcache(pte_t pteval)
extern void __sync_icache_dcache(pte_t pteval);
#endif
+#define PFN_PTE_SHIFT PAGE_SHIFT
+
void set_ptes(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, pte_t pteval, unsigned int nr);
#define set_ptes set_ptes
--
2.43.0
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simply define PFN_PTE_SHIFT, required by pte_next_pfn().
Reviewed-by: Christophe Leroy <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
arch/powerpc/include/asm/pgtable.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
index 9224f23065ff..7a1ba8889aea 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -41,6 +41,8 @@ struct mm_struct;
#ifndef __ASSEMBLY__
+#define PFN_PTE_SHIFT PTE_RPN_SHIFT
+
void set_ptes(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
pte_t pte, unsigned int nr);
#define set_ptes set_ptes
--
2.43.0
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simply define PFN_PTE_SHIFT, required by pte_next_pfn().
Reviewed-by: Alexandre Ghiti <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
arch/riscv/include/asm/pgtable.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index 0c94260b5d0c..add5cd30ab34 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -523,6 +523,8 @@ static inline void __set_pte_at(pte_t *ptep, pte_t pteval)
set_pte(ptep, pteval);
}
+#define PFN_PTE_SHIFT _PAGE_PFN_SHIFT
+
static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, pte_t pteval, unsigned int nr)
{
--
2.43.0
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simply define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <[email protected]>
---
arch/nios2/include/asm/pgtable.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/nios2/include/asm/pgtable.h b/arch/nios2/include/asm/pgtable.h
index 5144506dfa69..d052dfcbe8d3 100644
--- a/arch/nios2/include/asm/pgtable.h
+++ b/arch/nios2/include/asm/pgtable.h
@@ -178,6 +178,8 @@ static inline void set_pte(pte_t *ptep, pte_t pteval)
*ptep = pteval;
}
+#define PFN_PTE_SHIFT 0
+
static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, pte_t pte, unsigned int nr)
{
--
2.43.0
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simply define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <[email protected]>
---
arch/s390/include/asm/pgtable.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
index 1299b56e43f6..4b91e65c85d9 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -1316,6 +1316,8 @@ pgprot_t pgprot_writecombine(pgprot_t prot);
#define pgprot_writethrough pgprot_writethrough
pgprot_t pgprot_writethrough(pgprot_t prot);
+#define PFN_PTE_SHIFT PAGE_SHIFT
+
/*
* Set multiple PTEs to consecutive pages with a single call. All PTEs
* are within the same folio, PMD and VMA.
--
2.43.0
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simply define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <[email protected]>
---
arch/sparc/include/asm/pgtable_64.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/sparc/include/asm/pgtable_64.h b/arch/sparc/include/asm/pgtable_64.h
index a8c871b7d786..652af9d63fa2 100644
--- a/arch/sparc/include/asm/pgtable_64.h
+++ b/arch/sparc/include/asm/pgtable_64.h
@@ -929,6 +929,8 @@ static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
maybe_tlb_batch_add(mm, addr, ptep, orig, fullmm, PAGE_SHIFT);
}
+#define PFN_PTE_SHIFT PAGE_SHIFT
+
static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, pte_t pte, unsigned int nr)
{
--
2.43.0
Let's provide pte_next_pfn(), independently of set_ptes(). This allows for
using the generic pte_next_pfn() version in some arch-specific set_ptes()
implementations, and prepares for reusing pte_next_pfn() in other context.
Reviewed-by: Christophe Leroy <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
include/linux/pgtable.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index f6d0e3513948..351cd9dc7194 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -212,7 +212,6 @@ static inline int pmd_dirty(pmd_t pmd)
#define arch_flush_lazy_mmu_mode() do {} while (0)
#endif
-#ifndef set_ptes
#ifndef pte_next_pfn
static inline pte_t pte_next_pfn(pte_t pte)
@@ -221,6 +220,7 @@ static inline pte_t pte_next_pfn(pte_t pte)
}
#endif
+#ifndef set_ptes
/**
* set_ptes - Map consecutive pages to a contiguous range of addresses.
* @mm: Address space to map the pages into.
--
2.43.0
Let's use our handy helper now that it's available on all archs.
Signed-off-by: David Hildenbrand <[email protected]>
---
arch/arm/mm/mmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 674ed71573a8..c24e29c0b9a4 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1814,6 +1814,6 @@ void set_ptes(struct mm_struct *mm, unsigned long addr,
if (--nr == 0)
break;
ptep++;
- pte_val(pteval) += PAGE_SIZE;
+ pteval = pte_next_pfn(pteval);
}
}
--
2.43.0
Let's use our handy new helper. Note that the implementation is slightly
different, but shouldn't really make a difference in practice.
Reviewed-by: Christophe Leroy <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
arch/powerpc/mm/pgtable.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index a04ae4449a02..549a440ed7f6 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -220,10 +220,7 @@ void set_ptes(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
break;
ptep++;
addr += PAGE_SIZE;
- /*
- * increment the pfn.
- */
- pte = pfn_pte(pte_pfn(pte) + 1, pte_pgprot((pte)));
+ pte = pte_next_pfn(pte);
}
}
--
2.43.0
Let's prepare for further changes.
Reviewed-by: Ryan Roberts <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
mm/memory.c | 63 ++++++++++++++++++++++++++++-------------------------
1 file changed, 33 insertions(+), 30 deletions(-)
diff --git a/mm/memory.c b/mm/memory.c
index 8d14ba440929..a3bdb25f4c8d 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -930,6 +930,29 @@ copy_present_page(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
return 0;
}
+static inline void __copy_present_pte(struct vm_area_struct *dst_vma,
+ struct vm_area_struct *src_vma, pte_t *dst_pte, pte_t *src_pte,
+ pte_t pte, unsigned long addr)
+{
+ struct mm_struct *src_mm = src_vma->vm_mm;
+
+ /* If it's a COW mapping, write protect it both processes. */
+ if (is_cow_mapping(src_vma->vm_flags) && pte_write(pte)) {
+ ptep_set_wrprotect(src_mm, addr, src_pte);
+ pte = pte_wrprotect(pte);
+ }
+
+ /* If it's a shared mapping, mark it clean in the child. */
+ if (src_vma->vm_flags & VM_SHARED)
+ pte = pte_mkclean(pte);
+ pte = pte_mkold(pte);
+
+ if (!userfaultfd_wp(dst_vma))
+ pte = pte_clear_uffd_wp(pte);
+
+ set_pte_at(dst_vma->vm_mm, addr, dst_pte, pte);
+}
+
/*
* Copy one pte. Returns 0 if succeeded, or -EAGAIN if one preallocated page
* is required to copy this pte.
@@ -939,23 +962,23 @@ copy_present_pte(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
pte_t *dst_pte, pte_t *src_pte, unsigned long addr, int *rss,
struct folio **prealloc)
{
- struct mm_struct *src_mm = src_vma->vm_mm;
- unsigned long vm_flags = src_vma->vm_flags;
pte_t pte = ptep_get(src_pte);
struct page *page;
struct folio *folio;
page = vm_normal_page(src_vma, addr, pte);
- if (page)
- folio = page_folio(page);
- if (page && folio_test_anon(folio)) {
+ if (unlikely(!page))
+ goto copy_pte;
+
+ folio = page_folio(page);
+ folio_get(folio);
+ if (folio_test_anon(folio)) {
/*
* If this page may have been pinned by the parent process,
* copy the page immediately for the child so that we'll always
* guarantee the pinned page won't be randomly replaced in the
* future.
*/
- folio_get(folio);
if (unlikely(folio_try_dup_anon_rmap_pte(folio, page, src_vma))) {
/* Page may be pinned, we have to copy. */
folio_put(folio);
@@ -963,34 +986,14 @@ copy_present_pte(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
addr, rss, prealloc, page);
}
rss[MM_ANONPAGES]++;
- } else if (page) {
- folio_get(folio);
+ VM_WARN_ON_FOLIO(PageAnonExclusive(page), folio);
+ } else {
folio_dup_file_rmap_pte(folio, page);
rss[mm_counter_file(folio)]++;
}
- /*
- * If it's a COW mapping, write protect it both
- * in the parent and the child
- */
- if (is_cow_mapping(vm_flags) && pte_write(pte)) {
- ptep_set_wrprotect(src_mm, addr, src_pte);
- pte = pte_wrprotect(pte);
- }
- VM_BUG_ON(page && folio_test_anon(folio) && PageAnonExclusive(page));
-
- /*
- * If it's a shared mapping, mark it clean in
- * the child
- */
- if (vm_flags & VM_SHARED)
- pte = pte_mkclean(pte);
- pte = pte_mkold(pte);
-
- if (!userfaultfd_wp(dst_vma))
- pte = pte_clear_uffd_wp(pte);
-
- set_pte_at(dst_vma->vm_mm, addr, dst_pte, pte);
+copy_pte:
+ __copy_present_pte(dst_vma, src_vma, dst_pte, src_pte, pte, addr);
return 0;
}
--
2.43.0
We already read it, let's just forward it.
This patch is based on work by Ryan Roberts.
Reviewed-by: Ryan Roberts <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
mm/memory.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/mm/memory.c b/mm/memory.c
index a3bdb25f4c8d..41b24da5be38 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -959,10 +959,9 @@ static inline void __copy_present_pte(struct vm_area_struct *dst_vma,
*/
static inline int
copy_present_pte(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
- pte_t *dst_pte, pte_t *src_pte, unsigned long addr, int *rss,
- struct folio **prealloc)
+ pte_t *dst_pte, pte_t *src_pte, pte_t pte, unsigned long addr,
+ int *rss, struct folio **prealloc)
{
- pte_t pte = ptep_get(src_pte);
struct page *page;
struct folio *folio;
@@ -1103,7 +1102,7 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
}
/* copy_present_pte() will clear `*prealloc' if consumed */
ret = copy_present_pte(dst_vma, src_vma, dst_pte, src_pte,
- addr, rss, &prealloc);
+ ptent, addr, rss, &prealloc);
/*
* If we need a pre-allocated page for this pte, drop the
* locks, allocate, and try again.
--
2.43.0
Let's implement PTE batching when consecutive (present) PTEs map
consecutive pages of the same large folio, and all other PTE bits besides
the PFNs are equal.
We will optimize folio_pte_batch() separately, to ignore selected
PTE bits. This patch is based on work by Ryan Roberts.
Use __always_inline for __copy_present_ptes() and keep the handling for
single PTEs completely separate from the multi-PTE case: we really want
the compiler to optimize for the single-PTE case with small folios, to
not degrade performance.
Note that PTE batching will never exceed a single page table and will
always stay within VMA boundaries.
Further, processing PTE-mapped THP that maybe pinned and have
PageAnonExclusive set on at least one subpage should work as expected,
but there is room for improvement: We will repeatedly (1) detect a PTE
batch (2) detect that we have to copy a page (3) fall back and allocate a
single page to copy a single page. For now we won't care as pinned pages
are a corner case, and we should rather look into maintaining only a
single PageAnonExclusive bit for large folios.
Reviewed-by: Ryan Roberts <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
include/linux/pgtable.h | 31 +++++++++++
mm/memory.c | 112 +++++++++++++++++++++++++++++++++-------
2 files changed, 124 insertions(+), 19 deletions(-)
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index 351cd9dc7194..aab227e12493 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -650,6 +650,37 @@ static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addres
}
#endif
+#ifndef wrprotect_ptes
+/**
+ * wrprotect_ptes - Write-protect PTEs that map consecutive pages of the same
+ * folio.
+ * @mm: Address space the pages are mapped into.
+ * @addr: Address the first page is mapped at.
+ * @ptep: Page table pointer for the first entry.
+ * @nr: Number of entries to write-protect.
+ *
+ * May be overridden by the architecture; otherwise, implemented as a simple
+ * loop over ptep_set_wrprotect().
+ *
+ * Note that PTE bits in the PTE range besides the PFN can differ. For example,
+ * some PTEs might be write-protected.
+ *
+ * Context: The caller holds the page table lock. The PTEs map consecutive
+ * pages that belong to the same folio. The PTEs are all in the same PMD.
+ */
+static inline void wrprotect_ptes(struct mm_struct *mm, unsigned long addr,
+ pte_t *ptep, unsigned int nr)
+{
+ for (;;) {
+ ptep_set_wrprotect(mm, addr, ptep);
+ if (--nr == 0)
+ break;
+ ptep++;
+ addr += PAGE_SIZE;
+ }
+}
+#endif
+
/*
* On some architectures hardware does not set page access bit when accessing
* memory page, it is responsibility of software setting this bit. It brings
diff --git a/mm/memory.c b/mm/memory.c
index 41b24da5be38..86f8a0021c8e 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -930,15 +930,15 @@ copy_present_page(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
return 0;
}
-static inline void __copy_present_pte(struct vm_area_struct *dst_vma,
+static __always_inline void __copy_present_ptes(struct vm_area_struct *dst_vma,
struct vm_area_struct *src_vma, pte_t *dst_pte, pte_t *src_pte,
- pte_t pte, unsigned long addr)
+ pte_t pte, unsigned long addr, int nr)
{
struct mm_struct *src_mm = src_vma->vm_mm;
/* If it's a COW mapping, write protect it both processes. */
if (is_cow_mapping(src_vma->vm_flags) && pte_write(pte)) {
- ptep_set_wrprotect(src_mm, addr, src_pte);
+ wrprotect_ptes(src_mm, addr, src_pte, nr);
pte = pte_wrprotect(pte);
}
@@ -950,26 +950,93 @@ static inline void __copy_present_pte(struct vm_area_struct *dst_vma,
if (!userfaultfd_wp(dst_vma))
pte = pte_clear_uffd_wp(pte);
- set_pte_at(dst_vma->vm_mm, addr, dst_pte, pte);
+ set_ptes(dst_vma->vm_mm, addr, dst_pte, pte, nr);
+}
+
+/*
+ * Detect a PTE batch: consecutive (present) PTEs that map consecutive
+ * pages of the same folio.
+ *
+ * All PTEs inside a PTE batch have the same PTE bits set, excluding the PFN.
+ */
+static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
+ pte_t *start_ptep, pte_t pte, int max_nr)
+{
+ unsigned long folio_end_pfn = folio_pfn(folio) + folio_nr_pages(folio);
+ const pte_t *end_ptep = start_ptep + max_nr;
+ pte_t expected_pte = pte_next_pfn(pte);
+ pte_t *ptep = start_ptep + 1;
+
+ VM_WARN_ON_FOLIO(!pte_present(pte), folio);
+
+ while (ptep != end_ptep) {
+ pte = ptep_get(ptep);
+
+ if (!pte_same(pte, expected_pte))
+ break;
+
+ /*
+ * Stop immediately once we reached the end of the folio. In
+ * corner cases the next PFN might fall into a different
+ * folio.
+ */
+ if (pte_pfn(pte) == folio_end_pfn)
+ break;
+
+ expected_pte = pte_next_pfn(expected_pte);
+ ptep++;
+ }
+
+ return ptep - start_ptep;
}
/*
- * Copy one pte. Returns 0 if succeeded, or -EAGAIN if one preallocated page
- * is required to copy this pte.
+ * Copy one present PTE, trying to batch-process subsequent PTEs that map
+ * consecutive pages of the same folio by copying them as well.
+ *
+ * Returns -EAGAIN if one preallocated page is required to copy the next PTE.
+ * Otherwise, returns the number of copied PTEs (at least 1).
*/
static inline int
-copy_present_pte(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
+copy_present_ptes(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
pte_t *dst_pte, pte_t *src_pte, pte_t pte, unsigned long addr,
- int *rss, struct folio **prealloc)
+ int max_nr, int *rss, struct folio **prealloc)
{
struct page *page;
struct folio *folio;
+ int err, nr;
page = vm_normal_page(src_vma, addr, pte);
if (unlikely(!page))
goto copy_pte;
folio = page_folio(page);
+
+ /*
+ * If we likely have to copy, just don't bother with batching. Make
+ * sure that the common "small folio" case is as fast as possible
+ * by keeping the batching logic separate.
+ */
+ if (unlikely(!*prealloc && folio_test_large(folio) && max_nr != 1)) {
+ nr = folio_pte_batch(folio, addr, src_pte, pte, max_nr);
+ folio_ref_add(folio, nr);
+ if (folio_test_anon(folio)) {
+ if (unlikely(folio_try_dup_anon_rmap_ptes(folio, page,
+ nr, src_vma))) {
+ folio_ref_sub(folio, nr);
+ return -EAGAIN;
+ }
+ rss[MM_ANONPAGES] += nr;
+ VM_WARN_ON_FOLIO(PageAnonExclusive(page), folio);
+ } else {
+ folio_dup_file_rmap_ptes(folio, page, nr);
+ rss[mm_counter_file(folio)] += nr;
+ }
+ __copy_present_ptes(dst_vma, src_vma, dst_pte, src_pte, pte,
+ addr, nr);
+ return nr;
+ }
+
folio_get(folio);
if (folio_test_anon(folio)) {
/*
@@ -981,8 +1048,9 @@ copy_present_pte(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
if (unlikely(folio_try_dup_anon_rmap_pte(folio, page, src_vma))) {
/* Page may be pinned, we have to copy. */
folio_put(folio);
- return copy_present_page(dst_vma, src_vma, dst_pte, src_pte,
- addr, rss, prealloc, page);
+ err = copy_present_page(dst_vma, src_vma, dst_pte, src_pte,
+ addr, rss, prealloc, page);
+ return err ? err : 1;
}
rss[MM_ANONPAGES]++;
VM_WARN_ON_FOLIO(PageAnonExclusive(page), folio);
@@ -992,8 +1060,8 @@ copy_present_pte(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
}
copy_pte:
- __copy_present_pte(dst_vma, src_vma, dst_pte, src_pte, pte, addr);
- return 0;
+ __copy_present_ptes(dst_vma, src_vma, dst_pte, src_pte, pte, addr, 1);
+ return 1;
}
static inline struct folio *folio_prealloc(struct mm_struct *src_mm,
@@ -1030,10 +1098,11 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
pte_t *src_pte, *dst_pte;
pte_t ptent;
spinlock_t *src_ptl, *dst_ptl;
- int progress, ret = 0;
+ int progress, max_nr, ret = 0;
int rss[NR_MM_COUNTERS];
swp_entry_t entry = (swp_entry_t){0};
struct folio *prealloc = NULL;
+ int nr;
again:
progress = 0;
@@ -1064,6 +1133,8 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
arch_enter_lazy_mmu_mode();
do {
+ nr = 1;
+
/*
* We are holding two locks at this point - either of them
* could generate latencies in another task on another CPU.
@@ -1100,9 +1171,10 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
*/
WARN_ON_ONCE(ret != -ENOENT);
}
- /* copy_present_pte() will clear `*prealloc' if consumed */
- ret = copy_present_pte(dst_vma, src_vma, dst_pte, src_pte,
- ptent, addr, rss, &prealloc);
+ /* copy_present_ptes() will clear `*prealloc' if consumed */
+ max_nr = (end - addr) / PAGE_SIZE;
+ ret = copy_present_ptes(dst_vma, src_vma, dst_pte, src_pte,
+ ptent, addr, max_nr, rss, &prealloc);
/*
* If we need a pre-allocated page for this pte, drop the
* locks, allocate, and try again.
@@ -1119,8 +1191,10 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
folio_put(prealloc);
prealloc = NULL;
}
- progress += 8;
- } while (dst_pte++, src_pte++, addr += PAGE_SIZE, addr != end);
+ nr = ret;
+ progress += 8 * nr;
+ } while (dst_pte += nr, src_pte += nr, addr += PAGE_SIZE * nr,
+ addr != end);
arch_leave_lazy_mmu_mode();
pte_unmap_unlock(orig_src_pte, src_ptl);
@@ -1141,7 +1215,7 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
prealloc = folio_prealloc(src_mm, src_vma, addr, false);
if (!prealloc)
return -ENOMEM;
- } else if (ret) {
+ } else if (ret < 0) {
VM_WARN_ON_ONCE(1);
}
--
2.43.0
Let's always ignore the accessed/young bit: we'll always mark the PTE
as old in our child process during fork, and upcoming users will
similarly not care.
Ignore the dirty bit only if we don't want to duplicate the dirty bit
into the child process during fork. Maybe, we could just set all PTEs
in the child dirty if any PTE is dirty. For now, let's keep the behavior
unchanged, this can be optimized later if required.
Ignore the soft-dirty bit only if the bit doesn't have any meaning in
the src vma, and similarly won't have any in the copied dst vma.
For now, we won't bother with the uffd-wp bit.
Reviewed-by: Ryan Roberts <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
mm/memory.c | 36 +++++++++++++++++++++++++++++++-----
1 file changed, 31 insertions(+), 5 deletions(-)
diff --git a/mm/memory.c b/mm/memory.c
index 86f8a0021c8e..b2ec2b6b54c7 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -953,24 +953,44 @@ static __always_inline void __copy_present_ptes(struct vm_area_struct *dst_vma,
set_ptes(dst_vma->vm_mm, addr, dst_pte, pte, nr);
}
+/* Flags for folio_pte_batch(). */
+typedef int __bitwise fpb_t;
+
+/* Compare PTEs after pte_mkclean(), ignoring the dirty bit. */
+#define FPB_IGNORE_DIRTY ((__force fpb_t)BIT(0))
+
+/* Compare PTEs after pte_clear_soft_dirty(), ignoring the soft-dirty bit. */
+#define FPB_IGNORE_SOFT_DIRTY ((__force fpb_t)BIT(1))
+
+static inline pte_t __pte_batch_clear_ignored(pte_t pte, fpb_t flags)
+{
+ if (flags & FPB_IGNORE_DIRTY)
+ pte = pte_mkclean(pte);
+ if (likely(flags & FPB_IGNORE_SOFT_DIRTY))
+ pte = pte_clear_soft_dirty(pte);
+ return pte_mkold(pte);
+}
+
/*
* Detect a PTE batch: consecutive (present) PTEs that map consecutive
* pages of the same folio.
*
- * All PTEs inside a PTE batch have the same PTE bits set, excluding the PFN.
+ * All PTEs inside a PTE batch have the same PTE bits set, excluding the PFN,
+ * the accessed bit, dirty bit (with FPB_IGNORE_DIRTY) and soft-dirty bit
+ * (with FPB_IGNORE_SOFT_DIRTY).
*/
static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
- pte_t *start_ptep, pte_t pte, int max_nr)
+ pte_t *start_ptep, pte_t pte, int max_nr, fpb_t flags)
{
unsigned long folio_end_pfn = folio_pfn(folio) + folio_nr_pages(folio);
const pte_t *end_ptep = start_ptep + max_nr;
- pte_t expected_pte = pte_next_pfn(pte);
+ pte_t expected_pte = __pte_batch_clear_ignored(pte_next_pfn(pte), flags);
pte_t *ptep = start_ptep + 1;
VM_WARN_ON_FOLIO(!pte_present(pte), folio);
while (ptep != end_ptep) {
- pte = ptep_get(ptep);
+ pte = __pte_batch_clear_ignored(ptep_get(ptep), flags);
if (!pte_same(pte, expected_pte))
break;
@@ -1004,6 +1024,7 @@ copy_present_ptes(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
{
struct page *page;
struct folio *folio;
+ fpb_t flags = 0;
int err, nr;
page = vm_normal_page(src_vma, addr, pte);
@@ -1018,7 +1039,12 @@ copy_present_ptes(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
* by keeping the batching logic separate.
*/
if (unlikely(!*prealloc && folio_test_large(folio) && max_nr != 1)) {
- nr = folio_pte_batch(folio, addr, src_pte, pte, max_nr);
+ if (src_vma->vm_flags & VM_SHARED)
+ flags |= FPB_IGNORE_DIRTY;
+ if (!vma_soft_dirty_enabled(src_vma))
+ flags |= FPB_IGNORE_SOFT_DIRTY;
+
+ nr = folio_pte_batch(folio, addr, src_pte, pte, max_nr, flags);
folio_ref_add(folio, nr);
if (folio_test_anon(folio)) {
if (unlikely(folio_try_dup_anon_rmap_ptes(folio, page,
--
2.43.0
.. and conditionally return to the caller if any PTE except the first one
is writable. fork() has to make sure to properly write-protect in case any
PTE is writable. Other users (e.g., page unmaping) are expected to not
care.
Reviewed-by: Ryan Roberts <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
mm/memory.c | 30 ++++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/mm/memory.c b/mm/memory.c
index b2ec2b6b54c7..b05fd28dbce1 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -968,7 +968,7 @@ static inline pte_t __pte_batch_clear_ignored(pte_t pte, fpb_t flags)
pte = pte_mkclean(pte);
if (likely(flags & FPB_IGNORE_SOFT_DIRTY))
pte = pte_clear_soft_dirty(pte);
- return pte_mkold(pte);
+ return pte_wrprotect(pte_mkold(pte));
}
/*
@@ -976,21 +976,32 @@ static inline pte_t __pte_batch_clear_ignored(pte_t pte, fpb_t flags)
* pages of the same folio.
*
* All PTEs inside a PTE batch have the same PTE bits set, excluding the PFN,
- * the accessed bit, dirty bit (with FPB_IGNORE_DIRTY) and soft-dirty bit
- * (with FPB_IGNORE_SOFT_DIRTY).
+ * the accessed bit, writable bit, dirty bit (with FPB_IGNORE_DIRTY) and
+ * soft-dirty bit (with FPB_IGNORE_SOFT_DIRTY).
+ *
+ * If "any_writable" is set, it will indicate if any other PTE besides the
+ * first (given) PTE is writable.
*/
static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
- pte_t *start_ptep, pte_t pte, int max_nr, fpb_t flags)
+ pte_t *start_ptep, pte_t pte, int max_nr, fpb_t flags,
+ bool *any_writable)
{
unsigned long folio_end_pfn = folio_pfn(folio) + folio_nr_pages(folio);
const pte_t *end_ptep = start_ptep + max_nr;
pte_t expected_pte = __pte_batch_clear_ignored(pte_next_pfn(pte), flags);
pte_t *ptep = start_ptep + 1;
+ bool writable;
+
+ if (any_writable)
+ *any_writable = false;
VM_WARN_ON_FOLIO(!pte_present(pte), folio);
while (ptep != end_ptep) {
- pte = __pte_batch_clear_ignored(ptep_get(ptep), flags);
+ pte = ptep_get(ptep);
+ if (any_writable)
+ writable = !!pte_write(pte);
+ pte = __pte_batch_clear_ignored(pte, flags);
if (!pte_same(pte, expected_pte))
break;
@@ -1003,6 +1014,9 @@ static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
if (pte_pfn(pte) == folio_end_pfn)
break;
+ if (any_writable)
+ *any_writable |= writable;
+
expected_pte = pte_next_pfn(expected_pte);
ptep++;
}
@@ -1024,6 +1038,7 @@ copy_present_ptes(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
{
struct page *page;
struct folio *folio;
+ bool any_writable;
fpb_t flags = 0;
int err, nr;
@@ -1044,7 +1059,8 @@ copy_present_ptes(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
if (!vma_soft_dirty_enabled(src_vma))
flags |= FPB_IGNORE_SOFT_DIRTY;
- nr = folio_pte_batch(folio, addr, src_pte, pte, max_nr, flags);
+ nr = folio_pte_batch(folio, addr, src_pte, pte, max_nr, flags,
+ &any_writable);
folio_ref_add(folio, nr);
if (folio_test_anon(folio)) {
if (unlikely(folio_try_dup_anon_rmap_ptes(folio, page,
@@ -1058,6 +1074,8 @@ copy_present_ptes(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
folio_dup_file_rmap_ptes(folio, page, nr);
rss[mm_counter_file(folio)] += nr;
}
+ if (any_writable)
+ pte = pte_mkwrite(pte, src_vma);
__copy_present_ptes(dst_vma, src_vma, dst_pte, src_pte, pte,
addr, nr);
return nr;
--
2.43.0
On 29/01/2024 12:46, David Hildenbrand wrote:
> Now that the rmap overhaul[1] is upstream that provides a clean interface
> for rmap batching, let's implement PTE batching during fork when processing
> PTE-mapped THPs.
>
> This series is partially based on Ryan's previous work[2] to implement
> cont-pte support on arm64, but its a complete rewrite based on [1] to
> optimize all architectures independent of any such PTE bits, and to
> use the new rmap batching functions that simplify the code and prepare
> for further rmap accounting changes.
>
> We collect consecutive PTEs that map consecutive pages of the same large
> folio, making sure that the other PTE bits are compatible, and (a) adjust
> the refcount only once per batch, (b) call rmap handling functions only
> once per batch and (c) perform batch PTE setting/updates.
>
> While this series should be beneficial for adding cont-pte support on
> ARM64[2], it's one of the requirements for maintaining a total mapcount[3]
> for large folios with minimal added overhead and further changes[4] that
> build up on top of the total mapcount.
>
> Independent of all that, this series results in a speedup during fork with
> PTE-mapped THP, which is the default with THPs that are smaller than a PMD
> (for example, 16KiB to 1024KiB mTHPs for anonymous memory[5]).
>
> On an Intel Xeon Silver 4210R CPU, fork'ing with 1GiB of PTE-mapped folios
> of the same size (stddev < 1%) results in the following runtimes
> for fork() (shorter is better):
>
> Folio Size | v6.8-rc1 | New | Change
> ------------------------------------------
> 4KiB | 0.014328 | 0.014035 | - 2%
> 16KiB | 0.014263 | 0.01196 | -16%
> 32KiB | 0.014334 | 0.01094 | -24%
> 64KiB | 0.014046 | 0.010444 | -26%
> 128KiB | 0.014011 | 0.010063 | -28%
> 256KiB | 0.013993 | 0.009938 | -29%
> 512KiB | 0.013983 | 0.00985 | -30%
> 1024KiB | 0.013986 | 0.00982 | -30%
> 2048KiB | 0.014305 | 0.010076 | -30%
Just a heads up that I'm seeing some strange results on Apple M2. Fork for
order-0 is seemingly costing ~17% more. I'm using GCC 13.2 and was pretty sure I
didn't see this problem with version 1; although that was on a different
baseline and I've thrown the numbers away so will rerun and try to debug this.
| kernel | mean_rel | std_rel |
|:------------|-----------:|----------:|
| mm-unstable | 0.0% | 1.1% |
| patch 1 | -2.3% | 1.3% |
| patch 10 | -2.9% | 2.7% |
| patch 11 | 13.5% | 0.5% |
| patch 12 | 15.2% | 1.2% |
| patch 13 | 18.2% | 0.7% |
| patch 14 | 20.5% | 1.0% |
| patch 15 | 17.1% | 1.6% |
| patch 15 | 16.7% | 0.8% |
fork for order-9 is looking good (-20%), and for the zap series, munmap is
looking good, but dontneed is looking poor for both order-0 and 9. But one thing
at a time... let's concentrate on fork order-0 first.
Note that I'm still using the "old" benchmark code. Could you resend me the link
to the new code? Although I don't think there should be any effect for order-0
anyway, if I understood your changes correctly?
>
> Note that these numbers are even better than the ones from v1 (verified
> over multiple reboots), even though there were only minimal code changes.
> Well, I removed a pte_mkclean() call for anon folios, maybe that also
> plays a role.
>
> But my experience is that fork() is extremely sensitive to code size,
> inlining, ... so I suspect we'll see on other architectures rather a change
> of -20% instead of -30%, and it will be easy to "lose" some of that speedup
> in the future by subtle code changes.
>
> Next up is PTE batching when unmapping. Only tested on x86-64.
> Compile-tested on most other architectures.
>
> v2 -> v3:
> * Rebased on mm-unstable
> * Picked up RB's
> * Updated documentation of wrprotect_ptes().
>
> v1 -> v2:
> * "arm64/mm: Make set_ptes() robust when OAs cross 48-bit boundary"
> -> Added patch from Ryan
> * "arm/pgtable: define PFN_PTE_SHIFT"
> -> Removed the arm64 bits
> * "mm/pgtable: make pte_next_pfn() independent of set_ptes()"
> * "arm/mm: use pte_next_pfn() in set_ptes()"
> * "powerpc/mm: use pte_next_pfn() in set_ptes()"
> -> Added to use pte_next_pfn() in some arch set_ptes() implementations
> I tried to make use of pte_next_pfn() also in the others, but it's
> not trivial because the other archs implement set_ptes() in their
> asm/pgtable.h. Future work.
> * "mm/memory: factor out copying the actual PTE in copy_present_pte()"
> -> Move common folio_get() out of if/else
> * "mm/memory: optimize fork() with PTE-mapped THP"
> -> Add doc for wrprotect_ptes
> -> Extend description to mention handling of pinned folios
> -> Move common folio_ref_add() out of if/else
> * "mm/memory: ignore dirty/accessed/soft-dirty bits in folio_pte_batch()"
> -> Be more conservative with dirt/soft-dirty, let the caller specify
> using flags
>
> [1] https://lkml.kernel.org/r/[email protected]
> [2] https://lkml.kernel.org/r/[email protected]
> [3] https://lkml.kernel.org/r/[email protected]
> [4] https://lkml.kernel.org/r/[email protected]
> [5] https://lkml.kernel.org/r/[email protected]
>
> Cc: Andrew Morton <[email protected]>
> Cc: Matthew Wilcox (Oracle) <[email protected]>
> Cc: Ryan Roberts <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Dinh Nguyen <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Nicholas Piggin <[email protected]>
> Cc: Christophe Leroy <[email protected]>
> Cc: "Aneesh Kumar K.V" <[email protected]>
> Cc: "Naveen N. Rao" <[email protected]>
> Cc: Paul Walmsley <[email protected]>
> Cc: Palmer Dabbelt <[email protected]>
> Cc: Albert Ou <[email protected]>
> Cc: Alexander Gordeev <[email protected]>
> Cc: Gerald Schaefer <[email protected]>
> Cc: Heiko Carstens <[email protected]>
> Cc: Vasily Gorbik <[email protected]>
> Cc: Christian Borntraeger <[email protected]>
> Cc: Sven Schnelle <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> ---
>
> Andrew asked for a resend based on latest mm-unstable. I am sending this
> out earlier than I would usually have sent out the next version, so we can
> pull it into mm-unstable again now that v1 was dropped.
>
> David Hildenbrand (14):
> arm/pgtable: define PFN_PTE_SHIFT
> nios2/pgtable: define PFN_PTE_SHIFT
> powerpc/pgtable: define PFN_PTE_SHIFT
> riscv/pgtable: define PFN_PTE_SHIFT
> s390/pgtable: define PFN_PTE_SHIFT
> sparc/pgtable: define PFN_PTE_SHIFT
> mm/pgtable: make pte_next_pfn() independent of set_ptes()
> arm/mm: use pte_next_pfn() in set_ptes()
> powerpc/mm: use pte_next_pfn() in set_ptes()
> mm/memory: factor out copying the actual PTE in copy_present_pte()
> mm/memory: pass PTE to copy_present_pte()
> mm/memory: optimize fork() with PTE-mapped THP
> mm/memory: ignore dirty/accessed/soft-dirty bits in folio_pte_batch()
> mm/memory: ignore writable bit in folio_pte_batch()
>
> Ryan Roberts (1):
> arm64/mm: Make set_ptes() robust when OAs cross 48-bit boundary
>
> arch/arm/include/asm/pgtable.h | 2 +
> arch/arm/mm/mmu.c | 2 +-
> arch/arm64/include/asm/pgtable.h | 28 ++--
> arch/nios2/include/asm/pgtable.h | 2 +
> arch/powerpc/include/asm/pgtable.h | 2 +
> arch/powerpc/mm/pgtable.c | 5 +-
> arch/riscv/include/asm/pgtable.h | 2 +
> arch/s390/include/asm/pgtable.h | 2 +
> arch/sparc/include/asm/pgtable_64.h | 2 +
> include/linux/pgtable.h | 33 ++++-
> mm/memory.c | 212 ++++++++++++++++++++++------
> 11 files changed, 229 insertions(+), 63 deletions(-)
>
>
> base-commit: d162e170f1181b4305494843e1976584ddf2b72e
On 31.01.24 11:43, Ryan Roberts wrote:
> On 29/01/2024 12:46, David Hildenbrand wrote:
>> Now that the rmap overhaul[1] is upstream that provides a clean interface
>> for rmap batching, let's implement PTE batching during fork when processing
>> PTE-mapped THPs.
>>
>> This series is partially based on Ryan's previous work[2] to implement
>> cont-pte support on arm64, but its a complete rewrite based on [1] to
>> optimize all architectures independent of any such PTE bits, and to
>> use the new rmap batching functions that simplify the code and prepare
>> for further rmap accounting changes.
>>
>> We collect consecutive PTEs that map consecutive pages of the same large
>> folio, making sure that the other PTE bits are compatible, and (a) adjust
>> the refcount only once per batch, (b) call rmap handling functions only
>> once per batch and (c) perform batch PTE setting/updates.
>>
>> While this series should be beneficial for adding cont-pte support on
>> ARM64[2], it's one of the requirements for maintaining a total mapcount[3]
>> for large folios with minimal added overhead and further changes[4] that
>> build up on top of the total mapcount.
>>
>> Independent of all that, this series results in a speedup during fork with
>> PTE-mapped THP, which is the default with THPs that are smaller than a PMD
>> (for example, 16KiB to 1024KiB mTHPs for anonymous memory[5]).
>>
>> On an Intel Xeon Silver 4210R CPU, fork'ing with 1GiB of PTE-mapped folios
>> of the same size (stddev < 1%) results in the following runtimes
>> for fork() (shorter is better):
>>
>> Folio Size | v6.8-rc1 | New | Change
>> ------------------------------------------
>> 4KiB | 0.014328 | 0.014035 | - 2%
>> 16KiB | 0.014263 | 0.01196 | -16%
>> 32KiB | 0.014334 | 0.01094 | -24%
>> 64KiB | 0.014046 | 0.010444 | -26%
>> 128KiB | 0.014011 | 0.010063 | -28%
>> 256KiB | 0.013993 | 0.009938 | -29%
>> 512KiB | 0.013983 | 0.00985 | -30%
>> 1024KiB | 0.013986 | 0.00982 | -30%
>> 2048KiB | 0.014305 | 0.010076 | -30%
>
> Just a heads up that I'm seeing some strange results on Apple M2. Fork for
> order-0 is seemingly costing ~17% more. I'm using GCC 13.2 and was pretty sure I
> didn't see this problem with version 1; although that was on a different
> baseline and I've thrown the numbers away so will rerun and try to debug this.
>
So far, on my x86 tests (Intel, AMD EPYC), I was not able to observe
this. fork() for order-0 was consistently effectively unchanged. Do you
observe that on other ARM systems as well?
> | kernel | mean_rel | std_rel |
> |:------------|-----------:|----------:|
> | mm-unstable | 0.0% | 1.1% |
> | patch 1 | -2.3% | 1.3% |
> | patch 10 | -2.9% | 2.7% |
> | patch 11 | 13.5% | 0.5% |
> | patch 12 | 15.2% | 1.2% |
> | patch 13 | 18.2% | 0.7% |
> | patch 14 | 20.5% | 1.0% |
> | patch 15 | 17.1% | 1.6% |
> | patch 15 | 16.7% | 0.8% |
>
> fork for order-9 is looking good (-20%), and for the zap series, munmap is
> looking good, but dontneed is looking poor for both order-0 and 9. But one thing
> at a time... let's concentrate on fork order-0 first.
munmap and dontneed end up calling the exact same call paths. So a big
performance difference is rather surprising and might indicate something
else.
(I think I told you that I was running in some kind of VMA merging
problem where one would suddenly get with my benchmark 1 VMA per page.
The new benchmark below works around that, but I am not sure if that was
fixed in the meantime)
VMA merging can of course explain a big difference in fork and munmap
vs. dontneed times, especially when comparing different code base where
that VMA merging behavior was different.
>
> Note that I'm still using the "old" benchmark code. Could you resend me the link
> to the new code? Although I don't think there should be any effect for order-0
> anyway, if I understood your changes correctly?
This is the combined one (small and large PTEs):
https://gitlab.com/davidhildenbrand/scratchspace/-/raw/main/pte-mapped-folio-benchmarks.c?inline=false
--
Cheers,
David / dhildenb
On 31.01.24 12:16, Ryan Roberts wrote:
> On 31/01/2024 11:06, David Hildenbrand wrote:
>> On 31.01.24 11:43, Ryan Roberts wrote:
>>> On 29/01/2024 12:46, David Hildenbrand wrote:
>>>> Now that the rmap overhaul[1] is upstream that provides a clean interface
>>>> for rmap batching, let's implement PTE batching during fork when processing
>>>> PTE-mapped THPs.
>>>>
>>>> This series is partially based on Ryan's previous work[2] to implement
>>>> cont-pte support on arm64, but its a complete rewrite based on [1] to
>>>> optimize all architectures independent of any such PTE bits, and to
>>>> use the new rmap batching functions that simplify the code and prepare
>>>> for further rmap accounting changes.
>>>>
>>>> We collect consecutive PTEs that map consecutive pages of the same large
>>>> folio, making sure that the other PTE bits are compatible, and (a) adjust
>>>> the refcount only once per batch, (b) call rmap handling functions only
>>>> once per batch and (c) perform batch PTE setting/updates.
>>>>
>>>> While this series should be beneficial for adding cont-pte support on
>>>> ARM64[2], it's one of the requirements for maintaining a total mapcount[3]
>>>> for large folios with minimal added overhead and further changes[4] that
>>>> build up on top of the total mapcount.
>>>>
>>>> Independent of all that, this series results in a speedup during fork with
>>>> PTE-mapped THP, which is the default with THPs that are smaller than a PMD
>>>> (for example, 16KiB to 1024KiB mTHPs for anonymous memory[5]).
>>>>
>>>> On an Intel Xeon Silver 4210R CPU, fork'ing with 1GiB of PTE-mapped folios
>>>> of the same size (stddev < 1%) results in the following runtimes
>>>> for fork() (shorter is better):
>>>>
>>>> Folio Size | v6.8-rc1 |Â Â Â Â Â New | Change
>>>> ------------------------------------------
>>>> Â Â Â Â Â Â 4KiB | 0.014328 | 0.014035 |Â Â - 2%
>>>> Â Â Â Â Â 16KiB | 0.014263 | 0.01196Â |Â Â -16%
>>>> Â Â Â Â Â 32KiB | 0.014334 | 0.01094Â |Â Â -24%
>>>> Â Â Â Â Â 64KiB | 0.014046 | 0.010444 |Â Â -26%
>>>> Â Â Â Â 128KiB | 0.014011 | 0.010063 |Â Â -28%
>>>> Â Â Â Â 256KiB | 0.013993 | 0.009938 |Â Â -29%
>>>> Â Â Â Â 512KiB | 0.013983 | 0.00985Â |Â Â -30%
>>>> Â Â Â 1024KiB | 0.013986 | 0.00982Â |Â Â -30%
>>>> Â Â Â 2048KiB | 0.014305 | 0.010076 |Â Â -30%
>>>
>>> Just a heads up that I'm seeing some strange results on Apple M2. Fork for
>>> order-0 is seemingly costing ~17% more. I'm using GCC 13.2 and was pretty sure I
>>> didn't see this problem with version 1; although that was on a different
>>> baseline and I've thrown the numbers away so will rerun and try to debug this.
>>>
>>
>> So far, on my x86 tests (Intel, AMD EPYC), I was not able to observe this.
>> fork() for order-0 was consistently effectively unchanged. Do you observe that
>> on other ARM systems as well?
>
> Nope; running the exact same kernel binary and user space on Altra, I see
> sensible numbers;
>
> fork order-0: -1.3%
> fork order-9: -7.6%
> dontneed order-0: -0.5%
> dontneed order-9: 0.1%
> munmap order-0: 0.0%
> munmap order-9: -67.9%
>
> So I guess some pipelining issue that causes the M2 to stall more?
With one effective added folio_test_large(), it could only be a code
layout problem? Or the compiler does something stupid, but you say that
you run the exact same kernel binary, so that doesn't make sense.
I'm also surprised about the dontneed vs. munmap numbers. Doesn't make
any sense (again, there was this VMA merging problem but it would still
allow for batching within a single VMA that spans exactly one large folio).
What are you using as baseline? Really just mm-unstable vs.
mm-unstable+patches?
Let's see if the new test changes the numbers you measure.
--
Cheers,
David / dhildenb
On 31/01/2024 11:06, David Hildenbrand wrote:
> On 31.01.24 11:43, Ryan Roberts wrote:
>> On 29/01/2024 12:46, David Hildenbrand wrote:
>>> Now that the rmap overhaul[1] is upstream that provides a clean interface
>>> for rmap batching, let's implement PTE batching during fork when processing
>>> PTE-mapped THPs.
>>>
>>> This series is partially based on Ryan's previous work[2] to implement
>>> cont-pte support on arm64, but its a complete rewrite based on [1] to
>>> optimize all architectures independent of any such PTE bits, and to
>>> use the new rmap batching functions that simplify the code and prepare
>>> for further rmap accounting changes.
>>>
>>> We collect consecutive PTEs that map consecutive pages of the same large
>>> folio, making sure that the other PTE bits are compatible, and (a) adjust
>>> the refcount only once per batch, (b) call rmap handling functions only
>>> once per batch and (c) perform batch PTE setting/updates.
>>>
>>> While this series should be beneficial for adding cont-pte support on
>>> ARM64[2], it's one of the requirements for maintaining a total mapcount[3]
>>> for large folios with minimal added overhead and further changes[4] that
>>> build up on top of the total mapcount.
>>>
>>> Independent of all that, this series results in a speedup during fork with
>>> PTE-mapped THP, which is the default with THPs that are smaller than a PMD
>>> (for example, 16KiB to 1024KiB mTHPs for anonymous memory[5]).
>>>
>>> On an Intel Xeon Silver 4210R CPU, fork'ing with 1GiB of PTE-mapped folios
>>> of the same size (stddev < 1%) results in the following runtimes
>>> for fork() (shorter is better):
>>>
>>> Folio Size | v6.8-rc1 |Â Â Â Â Â New | Change
>>> ------------------------------------------
>>> Â Â Â Â Â Â 4KiB | 0.014328 | 0.014035 |Â Â - 2%
>>> Â Â Â Â Â 16KiB | 0.014263 | 0.01196Â |Â Â -16%
>>> Â Â Â Â Â 32KiB | 0.014334 | 0.01094Â |Â Â -24%
>>> Â Â Â Â Â 64KiB | 0.014046 | 0.010444 |Â Â -26%
>>> Â Â Â Â 128KiB | 0.014011 | 0.010063 |Â Â -28%
>>> Â Â Â Â 256KiB | 0.013993 | 0.009938 |Â Â -29%
>>> Â Â Â Â 512KiB | 0.013983 | 0.00985Â |Â Â -30%
>>> Â Â Â 1024KiB | 0.013986 | 0.00982Â |Â Â -30%
>>> Â Â Â 2048KiB | 0.014305 | 0.010076 |Â Â -30%
>>
>> Just a heads up that I'm seeing some strange results on Apple M2. Fork for
>> order-0 is seemingly costing ~17% more. I'm using GCC 13.2 and was pretty sure I
>> didn't see this problem with version 1; although that was on a different
>> baseline and I've thrown the numbers away so will rerun and try to debug this.
>>
>
> So far, on my x86 tests (Intel, AMD EPYC), I was not able to observe this.
> fork() for order-0 was consistently effectively unchanged. Do you observe that
> on other ARM systems as well?
Nope; running the exact same kernel binary and user space on Altra, I see
sensible numbers;
fork order-0: -1.3%
fork order-9: -7.6%
dontneed order-0: -0.5%
dontneed order-9: 0.1%
munmap order-0: 0.0%
munmap order-9: -67.9%
So I guess some pipelining issue that causes the M2 to stall more?
>
>
>> | kernel     |  mean_rel |  std_rel |
>> |:------------|-----------:|----------:|
>> | mm-unstable |Â Â Â Â Â Â 0.0% |Â Â Â Â Â 1.1% |
>> | patch 1Â Â Â Â |Â Â Â Â Â -2.3% |Â Â Â Â Â 1.3% |
>> | patch 10Â Â Â |Â Â Â Â Â -2.9% |Â Â Â Â Â 2.7% |
>> | patch 11Â Â Â |Â Â Â Â Â 13.5% |Â Â Â Â Â 0.5% |
>> | patch 12Â Â Â |Â Â Â Â Â 15.2% |Â Â Â Â Â 1.2% |
>> | patch 13Â Â Â |Â Â Â Â Â 18.2% |Â Â Â Â Â 0.7% |
>> | patch 14Â Â Â |Â Â Â Â Â 20.5% |Â Â Â Â Â 1.0% |
>> | patch 15Â Â Â |Â Â Â Â Â 17.1% |Â Â Â Â Â 1.6% |
>> | patch 15Â Â Â |Â Â Â Â Â 16.7% |Â Â Â Â Â 0.8% |
>>
>> fork for order-9 is looking good (-20%), and for the zap series, munmap is
>> looking good, but dontneed is looking poor for both order-0 and 9. But one thing
>> at a time... let's concentrate on fork order-0 first.
>
> munmap and dontneed end up calling the exact same call paths. So a big
> performance difference is rather surprising and might indicate something else.
>
> (I think I told you that I was running in some kind of VMA merging problem where
> one would suddenly get with my benchmark 1 VMA per page. The new benchmark below
> works around that, but I am not sure if that was fixed in the meantime)
>
> VMA merging can of course explain a big difference in fork and munmap vs.
> dontneed times, especially when comparing different code base where that VMA
> merging behavior was different.
>
>>
>> Note that I'm still using the "old" benchmark code. Could you resend me the link
>> to the new code? Although I don't think there should be any effect for order-0
>> anyway, if I understood your changes correctly?
>
> This is the combined one (small and large PTEs):
>
> https://gitlab.com/davidhildenbrand/scratchspace/-/raw/main/pte-mapped-folio-benchmarks.c?inline=false
I'll have a go with this.
>
On 31/01/2024 11:28, David Hildenbrand wrote:
> On 31.01.24 12:16, Ryan Roberts wrote:
>> On 31/01/2024 11:06, David Hildenbrand wrote:
>>> On 31.01.24 11:43, Ryan Roberts wrote:
>>>> On 29/01/2024 12:46, David Hildenbrand wrote:
>>>>> Now that the rmap overhaul[1] is upstream that provides a clean interface
>>>>> for rmap batching, let's implement PTE batching during fork when processing
>>>>> PTE-mapped THPs.
>>>>>
>>>>> This series is partially based on Ryan's previous work[2] to implement
>>>>> cont-pte support on arm64, but its a complete rewrite based on [1] to
>>>>> optimize all architectures independent of any such PTE bits, and to
>>>>> use the new rmap batching functions that simplify the code and prepare
>>>>> for further rmap accounting changes.
>>>>>
>>>>> We collect consecutive PTEs that map consecutive pages of the same large
>>>>> folio, making sure that the other PTE bits are compatible, and (a) adjust
>>>>> the refcount only once per batch, (b) call rmap handling functions only
>>>>> once per batch and (c) perform batch PTE setting/updates.
>>>>>
>>>>> While this series should be beneficial for adding cont-pte support on
>>>>> ARM64[2], it's one of the requirements for maintaining a total mapcount[3]
>>>>> for large folios with minimal added overhead and further changes[4] that
>>>>> build up on top of the total mapcount.
>>>>>
>>>>> Independent of all that, this series results in a speedup during fork with
>>>>> PTE-mapped THP, which is the default with THPs that are smaller than a PMD
>>>>> (for example, 16KiB to 1024KiB mTHPs for anonymous memory[5]).
>>>>>
>>>>> On an Intel Xeon Silver 4210R CPU, fork'ing with 1GiB of PTE-mapped folios
>>>>> of the same size (stddev < 1%) results in the following runtimes
>>>>> for fork() (shorter is better):
>>>>>
>>>>> Folio Size | v6.8-rc1 |Â Â Â Â Â New | Change
>>>>> ------------------------------------------
>>>>> Â Â Â Â Â Â Â 4KiB | 0.014328 | 0.014035 |Â Â - 2%
>>>>> Â Â Â Â Â Â 16KiB | 0.014263 | 0.01196Â |Â Â -16%
>>>>> Â Â Â Â Â Â 32KiB | 0.014334 | 0.01094Â |Â Â -24%
>>>>> Â Â Â Â Â Â 64KiB | 0.014046 | 0.010444 |Â Â -26%
>>>>> Â Â Â Â Â 128KiB | 0.014011 | 0.010063 |Â Â -28%
>>>>> Â Â Â Â Â 256KiB | 0.013993 | 0.009938 |Â Â -29%
>>>>> Â Â Â Â Â 512KiB | 0.013983 | 0.00985Â |Â Â -30%
>>>>> Â Â Â Â 1024KiB | 0.013986 | 0.00982Â |Â Â -30%
>>>>> Â Â Â Â 2048KiB | 0.014305 | 0.010076 |Â Â -30%
>>>>
>>>> Just a heads up that I'm seeing some strange results on Apple M2. Fork for
>>>> order-0 is seemingly costing ~17% more. I'm using GCC 13.2 and was pretty
>>>> sure I
>>>> didn't see this problem with version 1; although that was on a different
>>>> baseline and I've thrown the numbers away so will rerun and try to debug this.
Numbers for v1 of the series, both on top of 6.8-rc1 and rebased to the same
mm-unstable base as v3 of the series (first 2 rows are from what I just posted
for context):
| kernel | mean_rel | std_rel |
|:-------------------|-----------:|----------:|
| mm-unstabe (base) | 0.0% | 1.1% |
| mm-unstable + v3 | 16.7% | 0.8% |
| mm-unstable + v1 | -2.5% | 1.7% |
| v6.8-rc1 + v1 | -6.6% | 1.1% |
So all looks good with v1. And seems to suggest mm-unstable has regressed by ~4%
vs v6.8-rc1. Is this really a useful benchmark? Does the raw performance of
fork() syscall really matter? Evidence suggests its moving all over the place -
breath on the code and it changes - not a great place to be when using the test
for gating purposes!
Still with the old tests - I'll move to the new ones now.
>>>>
>>>
>>> So far, on my x86 tests (Intel, AMD EPYC), I was not able to observe this.
>>> fork() for order-0 was consistently effectively unchanged. Do you observe that
>>> on other ARM systems as well?
>>
>> Nope; running the exact same kernel binary and user space on Altra, I see
>> sensible numbers;
>>
>> fork order-0: -1.3%
>> fork order-9: -7.6%
>> dontneed order-0: -0.5%
>> dontneed order-9: 0.1%
>> munmap order-0: 0.0%
>> munmap order-9: -67.9%
>>
>> So I guess some pipelining issue that causes the M2 to stall more?
>
> With one effective added folio_test_large(), it could only be a code layout
> problem? Or the compiler does something stupid, but you say that you run the
> exact same kernel binary, so that doesn't make sense.
Yup, same binary. We know this code is very sensitive - 1 cycle makes a big
difference. So could easily be code layout, branch prediction, etc...
>
> I'm also surprised about the dontneed vs. munmap numbers.
You mean the ones for Altra that I posted? (I didn't post any for M2). The altra
numbers look ok to me; dontneed has no change, and munmap has no change for
order-0 and is massively improved for order-9.
Doesn't make any sense
> (again, there was this VMA merging problem but it would still allow for batching
> within a single VMA that spans exactly one large folio).
>
> What are you using as baseline? Really just mm-unstable vs. mm-unstable+patches?
yes. except for "v6.8-rc1 + v1" above.
>
> Let's see if the new test changes the numbers you measure.
>
On 31.01.24 13:37, Ryan Roberts wrote:
> On 31/01/2024 11:49, Ryan Roberts wrote:
>> On 31/01/2024 11:28, David Hildenbrand wrote:
>>> On 31.01.24 12:16, Ryan Roberts wrote:
>>>> On 31/01/2024 11:06, David Hildenbrand wrote:
>>>>> On 31.01.24 11:43, Ryan Roberts wrote:
>>>>>> On 29/01/2024 12:46, David Hildenbrand wrote:
>>>>>>> Now that the rmap overhaul[1] is upstream that provides a clean interface
>>>>>>> for rmap batching, let's implement PTE batching during fork when processing
>>>>>>> PTE-mapped THPs.
>>>>>>>
>>>>>>> This series is partially based on Ryan's previous work[2] to implement
>>>>>>> cont-pte support on arm64, but its a complete rewrite based on [1] to
>>>>>>> optimize all architectures independent of any such PTE bits, and to
>>>>>>> use the new rmap batching functions that simplify the code and prepare
>>>>>>> for further rmap accounting changes.
>>>>>>>
>>>>>>> We collect consecutive PTEs that map consecutive pages of the same large
>>>>>>> folio, making sure that the other PTE bits are compatible, and (a) adjust
>>>>>>> the refcount only once per batch, (b) call rmap handling functions only
>>>>>>> once per batch and (c) perform batch PTE setting/updates.
>>>>>>>
>>>>>>> While this series should be beneficial for adding cont-pte support on
>>>>>>> ARM64[2], it's one of the requirements for maintaining a total mapcount[3]
>>>>>>> for large folios with minimal added overhead and further changes[4] that
>>>>>>> build up on top of the total mapcount.
>>>>>>>
>>>>>>> Independent of all that, this series results in a speedup during fork with
>>>>>>> PTE-mapped THP, which is the default with THPs that are smaller than a PMD
>>>>>>> (for example, 16KiB to 1024KiB mTHPs for anonymous memory[5]).
>>>>>>>
>>>>>>> On an Intel Xeon Silver 4210R CPU, fork'ing with 1GiB of PTE-mapped folios
>>>>>>> of the same size (stddev < 1%) results in the following runtimes
>>>>>>> for fork() (shorter is better):
>>>>>>>
>>>>>>> Folio Size | v6.8-rc1 |Â Â Â Â Â New | Change
>>>>>>> ------------------------------------------
>>>>>>> Â Â Â Â Â Â Â 4KiB | 0.014328 | 0.014035 |Â Â - 2%
>>>>>>> Â Â Â Â Â Â 16KiB | 0.014263 | 0.01196Â |Â Â -16%
>>>>>>> Â Â Â Â Â Â 32KiB | 0.014334 | 0.01094Â |Â Â -24%
>>>>>>> Â Â Â Â Â Â 64KiB | 0.014046 | 0.010444 |Â Â -26%
>>>>>>> Â Â Â Â Â 128KiB | 0.014011 | 0.010063 |Â Â -28%
>>>>>>> Â Â Â Â Â 256KiB | 0.013993 | 0.009938 |Â Â -29%
>>>>>>> Â Â Â Â Â 512KiB | 0.013983 | 0.00985Â |Â Â -30%
>>>>>>> Â Â Â Â 1024KiB | 0.013986 | 0.00982Â |Â Â -30%
>>>>>>> Â Â Â Â 2048KiB | 0.014305 | 0.010076 |Â Â -30%
>>>>>>
>>>>>> Just a heads up that I'm seeing some strange results on Apple M2. Fork for
>>>>>> order-0 is seemingly costing ~17% more. I'm using GCC 13.2 and was pretty
>>>>>> sure I
>>>>>> didn't see this problem with version 1; although that was on a different
>>>>>> baseline and I've thrown the numbers away so will rerun and try to debug this.
>>
>> Numbers for v1 of the series, both on top of 6.8-rc1 and rebased to the same
>> mm-unstable base as v3 of the series (first 2 rows are from what I just posted
>> for context):
>>
>> | kernel | mean_rel | std_rel |
>> |:-------------------|-----------:|----------:|
>> | mm-unstabe (base) | 0.0% | 1.1% |
>> | mm-unstable + v3 | 16.7% | 0.8% |
>> | mm-unstable + v1 | -2.5% | 1.7% |
>> | v6.8-rc1 + v1 | -6.6% | 1.1% |
>>
>> So all looks good with v1. And seems to suggest mm-unstable has regressed by ~4%
>> vs v6.8-rc1. Is this really a useful benchmark? Does the raw performance of
>> fork() syscall really matter? Evidence suggests its moving all over the place -
>> breath on the code and it changes - not a great place to be when using the test
>> for gating purposes!
>>
>> Still with the old tests - I'll move to the new ones now.
>>
>>
>>>>>>
>>>>>
>>>>> So far, on my x86 tests (Intel, AMD EPYC), I was not able to observe this.
>>>>> fork() for order-0 was consistently effectively unchanged. Do you observe that
>>>>> on other ARM systems as well?
>>>>
>>>> Nope; running the exact same kernel binary and user space on Altra, I see
>>>> sensible numbers;
>>>>
>>>> fork order-0: -1.3%
>>>> fork order-9: -7.6%
>>>> dontneed order-0: -0.5%
>>>> dontneed order-9: 0.1%
>>>> munmap order-0: 0.0%
>>>> munmap order-9: -67.9%
>>>>
>>>> So I guess some pipelining issue that causes the M2 to stall more?
>>>
>>> With one effective added folio_test_large(), it could only be a code layout
>>> problem? Or the compiler does something stupid, but you say that you run the
>>> exact same kernel binary, so that doesn't make sense.
>>
>> Yup, same binary. We know this code is very sensitive - 1 cycle makes a big
>> difference. So could easily be code layout, branch prediction, etc...
>>
>>>
>>> I'm also surprised about the dontneed vs. munmap numbers.
>>
>> You mean the ones for Altra that I posted? (I didn't post any for M2). The altra
>> numbers look ok to me; dontneed has no change, and munmap has no change for
>> order-0 and is massively improved for order-9.
>>
>> Doesn't make any sense
>>> (again, there was this VMA merging problem but it would still allow for batching
>>> within a single VMA that spans exactly one large folio).
>>>
>>> What are you using as baseline? Really just mm-unstable vs. mm-unstable+patches?
>>
>> yes. except for "v6.8-rc1 + v1" above.
>>
>>>
>>> Let's see if the new test changes the numbers you measure.
>
> Nope: looks the same. I've taken my test harness out of the picture and done
> everything manually from the ground up, with the old tests and the new. Headline
> is that I see similar numbers from both.
I took me a while to get really reproducible numbers on Intel. Most
importantly:
* Set a fixed CPU frequency, disabling any boost and avoiding any
thermal throttling.
* Pin the test to CPUs and set a nice level.
Another thing is, to avoid systems where you can have NUMA effects
within a single socket. Otherwise, memory access latency is just random
and depends on what the buddy enjoys giving you.
But you seem to get the same +17 even after reboots, so that indicates
that the CPU is not happy about the code for some reason. And the weird
thing is, that nothing significantly changed for order-0 folios between
v1 and v3 that could explain any of this.
I'm not worried about 5% or so, nobody cares. But it would be good to
have at least an explanation why only that system shows +17%.
>
> Some details:
> - I'm running for 10 seconds then averaging the output
Same here.
> - test is bimodal; first run (of 10 seconds) after boot is a bit faster on
> average (up to 10%) than the rest; I could guess this is due to the memory
> being allocated more contiguously the first few times through, so struct
> pages have better locality, but that's a guess.
I think it also has to do with the PCP lists, and the high-pcp auto
tuning (I played with disabling that). Running on a freshly booted
system gave me reproducible results.
But yes: I was observing something similar on AMD EPYC, where you get
consecutive pages from the buddy, but once you allocate from the PCP it
might no longer be consecutive.
> - test is 5-10% slower when output is printed to terminal vs when redirected to
> file. I've always effectively been redirecting. Not sure if this overhead
> could start to dominate the regression and that's why you don't see it?
That's weird, because we don't print while measuring? Anyhow, 5/10%
variance on some system is not the end of the world.
>
> I'm inclined to run this test for the last N kernel releases and if the number
> moves around significantly, conclude that these tests don't really matter.
> Otherwise its an exercise in randomly refactoring code until it works well, but
> that's just overfitting to the compiler and hw. What do you think?
Personally, I wouldn't lose sleep if you see weird, unexplainable
behavior on some system (not even architecture!). Trying to optimize for
that would indeed be random refactorings.
But I would not be so fast to say that "these tests don't really matter"
and then go wild and degrade them as much as you want. There are use
cases that care about fork performance especially with order-0 pages --
such as Redis.
--
Cheers,
David / dhildenb
>>
>> I'm also surprised about the dontneed vs. munmap numbers.
>
> You mean the ones for Altra that I posted? (I didn't post any for M2). The altra
> numbers look ok to me; dontneed has no change, and munmap has no change for
> order-0 and is massively improved for order-9.
I would expect that dontneed would similarly benefit -- same code path.
But I focused on munmap measurements for now, I'll try finding time to
confirm that it's the same on Intel.
--
Cheers,
David / dhildenb
On 31/01/2024 11:49, Ryan Roberts wrote:
> On 31/01/2024 11:28, David Hildenbrand wrote:
>> On 31.01.24 12:16, Ryan Roberts wrote:
>>> On 31/01/2024 11:06, David Hildenbrand wrote:
>>>> On 31.01.24 11:43, Ryan Roberts wrote:
>>>>> On 29/01/2024 12:46, David Hildenbrand wrote:
>>>>>> Now that the rmap overhaul[1] is upstream that provides a clean interface
>>>>>> for rmap batching, let's implement PTE batching during fork when processing
>>>>>> PTE-mapped THPs.
>>>>>>
>>>>>> This series is partially based on Ryan's previous work[2] to implement
>>>>>> cont-pte support on arm64, but its a complete rewrite based on [1] to
>>>>>> optimize all architectures independent of any such PTE bits, and to
>>>>>> use the new rmap batching functions that simplify the code and prepare
>>>>>> for further rmap accounting changes.
>>>>>>
>>>>>> We collect consecutive PTEs that map consecutive pages of the same large
>>>>>> folio, making sure that the other PTE bits are compatible, and (a) adjust
>>>>>> the refcount only once per batch, (b) call rmap handling functions only
>>>>>> once per batch and (c) perform batch PTE setting/updates.
>>>>>>
>>>>>> While this series should be beneficial for adding cont-pte support on
>>>>>> ARM64[2], it's one of the requirements for maintaining a total mapcount[3]
>>>>>> for large folios with minimal added overhead and further changes[4] that
>>>>>> build up on top of the total mapcount.
>>>>>>
>>>>>> Independent of all that, this series results in a speedup during fork with
>>>>>> PTE-mapped THP, which is the default with THPs that are smaller than a PMD
>>>>>> (for example, 16KiB to 1024KiB mTHPs for anonymous memory[5]).
>>>>>>
>>>>>> On an Intel Xeon Silver 4210R CPU, fork'ing with 1GiB of PTE-mapped folios
>>>>>> of the same size (stddev < 1%) results in the following runtimes
>>>>>> for fork() (shorter is better):
>>>>>>
>>>>>> Folio Size | v6.8-rc1 |Â Â Â Â Â New | Change
>>>>>> ------------------------------------------
>>>>>> Â Â Â Â Â Â Â 4KiB | 0.014328 | 0.014035 |Â Â - 2%
>>>>>> Â Â Â Â Â Â 16KiB | 0.014263 | 0.01196Â |Â Â -16%
>>>>>> Â Â Â Â Â Â 32KiB | 0.014334 | 0.01094Â |Â Â -24%
>>>>>> Â Â Â Â Â Â 64KiB | 0.014046 | 0.010444 |Â Â -26%
>>>>>> Â Â Â Â Â 128KiB | 0.014011 | 0.010063 |Â Â -28%
>>>>>> Â Â Â Â Â 256KiB | 0.013993 | 0.009938 |Â Â -29%
>>>>>> Â Â Â Â Â 512KiB | 0.013983 | 0.00985Â |Â Â -30%
>>>>>> Â Â Â Â 1024KiB | 0.013986 | 0.00982Â |Â Â -30%
>>>>>> Â Â Â Â 2048KiB | 0.014305 | 0.010076 |Â Â -30%
>>>>>
>>>>> Just a heads up that I'm seeing some strange results on Apple M2. Fork for
>>>>> order-0 is seemingly costing ~17% more. I'm using GCC 13.2 and was pretty
>>>>> sure I
>>>>> didn't see this problem with version 1; although that was on a different
>>>>> baseline and I've thrown the numbers away so will rerun and try to debug this.
>
> Numbers for v1 of the series, both on top of 6.8-rc1 and rebased to the same
> mm-unstable base as v3 of the series (first 2 rows are from what I just posted
> for context):
>
> | kernel | mean_rel | std_rel |
> |:-------------------|-----------:|----------:|
> | mm-unstabe (base) | 0.0% | 1.1% |
> | mm-unstable + v3 | 16.7% | 0.8% |
> | mm-unstable + v1 | -2.5% | 1.7% |
> | v6.8-rc1 + v1 | -6.6% | 1.1% |
>
> So all looks good with v1. And seems to suggest mm-unstable has regressed by ~4%
> vs v6.8-rc1. Is this really a useful benchmark? Does the raw performance of
> fork() syscall really matter? Evidence suggests its moving all over the place -
> breath on the code and it changes - not a great place to be when using the test
> for gating purposes!
>
> Still with the old tests - I'll move to the new ones now.
>
>
>>>>>
>>>>
>>>> So far, on my x86 tests (Intel, AMD EPYC), I was not able to observe this.
>>>> fork() for order-0 was consistently effectively unchanged. Do you observe that
>>>> on other ARM systems as well?
>>>
>>> Nope; running the exact same kernel binary and user space on Altra, I see
>>> sensible numbers;
>>>
>>> fork order-0: -1.3%
>>> fork order-9: -7.6%
>>> dontneed order-0: -0.5%
>>> dontneed order-9: 0.1%
>>> munmap order-0: 0.0%
>>> munmap order-9: -67.9%
>>>
>>> So I guess some pipelining issue that causes the M2 to stall more?
>>
>> With one effective added folio_test_large(), it could only be a code layout
>> problem? Or the compiler does something stupid, but you say that you run the
>> exact same kernel binary, so that doesn't make sense.
>
> Yup, same binary. We know this code is very sensitive - 1 cycle makes a big
> difference. So could easily be code layout, branch prediction, etc...
>
>>
>> I'm also surprised about the dontneed vs. munmap numbers.
>
> You mean the ones for Altra that I posted? (I didn't post any for M2). The altra
> numbers look ok to me; dontneed has no change, and munmap has no change for
> order-0 and is massively improved for order-9.
>
> Doesn't make any sense
>> (again, there was this VMA merging problem but it would still allow for batching
>> within a single VMA that spans exactly one large folio).
>>
>> What are you using as baseline? Really just mm-unstable vs. mm-unstable+patches?
>
> yes. except for "v6.8-rc1 + v1" above.
>
>>
>> Let's see if the new test changes the numbers you measure.
Nope: looks the same. I've taken my test harness out of the picture and done
everything manually from the ground up, with the old tests and the new. Headline
is that I see similar numbers from both.
Some details:
- I'm running for 10 seconds then averaging the output
- test is bimodal; first run (of 10 seconds) after boot is a bit faster on
average (up to 10%) than the rest; I could guess this is due to the memory
being allocated more contiguously the first few times through, so struct
pages have better locality, but that's a guess.
- test is 5-10% slower when output is printed to terminal vs when redirected to
file. I've always effectively been redirecting. Not sure if this overhead
could start to dominate the regression and that's why you don't see it?
I'm inclined to run this test for the last N kernel releases and if the number
moves around significantly, conclude that these tests don't really matter.
Otherwise its an exercise in randomly refactoring code until it works well, but
that's just overfitting to the compiler and hw. What do you think?
Thanks,
Ryan
On 31/01/2024 12:56, David Hildenbrand wrote:
> On 31.01.24 13:37, Ryan Roberts wrote:
>> On 31/01/2024 11:49, Ryan Roberts wrote:
>>> On 31/01/2024 11:28, David Hildenbrand wrote:
>>>> On 31.01.24 12:16, Ryan Roberts wrote:
>>>>> On 31/01/2024 11:06, David Hildenbrand wrote:
>>>>>> On 31.01.24 11:43, Ryan Roberts wrote:
>>>>>>> On 29/01/2024 12:46, David Hildenbrand wrote:
>>>>>>>> Now that the rmap overhaul[1] is upstream that provides a clean interface
>>>>>>>> for rmap batching, let's implement PTE batching during fork when processing
>>>>>>>> PTE-mapped THPs.
>>>>>>>>
>>>>>>>> This series is partially based on Ryan's previous work[2] to implement
>>>>>>>> cont-pte support on arm64, but its a complete rewrite based on [1] to
>>>>>>>> optimize all architectures independent of any such PTE bits, and to
>>>>>>>> use the new rmap batching functions that simplify the code and prepare
>>>>>>>> for further rmap accounting changes.
>>>>>>>>
>>>>>>>> We collect consecutive PTEs that map consecutive pages of the same large
>>>>>>>> folio, making sure that the other PTE bits are compatible, and (a) adjust
>>>>>>>> the refcount only once per batch, (b) call rmap handling functions only
>>>>>>>> once per batch and (c) perform batch PTE setting/updates.
>>>>>>>>
>>>>>>>> While this series should be beneficial for adding cont-pte support on
>>>>>>>> ARM64[2], it's one of the requirements for maintaining a total mapcount[3]
>>>>>>>> for large folios with minimal added overhead and further changes[4] that
>>>>>>>> build up on top of the total mapcount.
>>>>>>>>
>>>>>>>> Independent of all that, this series results in a speedup during fork with
>>>>>>>> PTE-mapped THP, which is the default with THPs that are smaller than a PMD
>>>>>>>> (for example, 16KiB to 1024KiB mTHPs for anonymous memory[5]).
>>>>>>>>
>>>>>>>> On an Intel Xeon Silver 4210R CPU, fork'ing with 1GiB of PTE-mapped folios
>>>>>>>> of the same size (stddev < 1%) results in the following runtimes
>>>>>>>> for fork() (shorter is better):
>>>>>>>>
>>>>>>>> Folio Size | v6.8-rc1 |Â Â Â Â Â New | Change
>>>>>>>> ------------------------------------------
>>>>>>>> Â Â Â Â Â Â Â Â 4KiB | 0.014328 | 0.014035 |Â Â - 2%
>>>>>>>> Â Â Â Â Â Â Â 16KiB | 0.014263 | 0.01196Â |Â Â -16%
>>>>>>>> Â Â Â Â Â Â Â 32KiB | 0.014334 | 0.01094Â |Â Â -24%
>>>>>>>> Â Â Â Â Â Â Â 64KiB | 0.014046 | 0.010444 |Â Â -26%
>>>>>>>> Â Â Â Â Â Â 128KiB | 0.014011 | 0.010063 |Â Â -28%
>>>>>>>> Â Â Â Â Â Â 256KiB | 0.013993 | 0.009938 |Â Â -29%
>>>>>>>> Â Â Â Â Â Â 512KiB | 0.013983 | 0.00985Â |Â Â -30%
>>>>>>>> Â Â Â Â Â 1024KiB | 0.013986 | 0.00982Â |Â Â -30%
>>>>>>>> Â Â Â Â Â 2048KiB | 0.014305 | 0.010076 |Â Â -30%
>>>>>>>
>>>>>>> Just a heads up that I'm seeing some strange results on Apple M2. Fork for
>>>>>>> order-0 is seemingly costing ~17% more. I'm using GCC 13.2 and was pretty
>>>>>>> sure I
>>>>>>> didn't see this problem with version 1; although that was on a different
>>>>>>> baseline and I've thrown the numbers away so will rerun and try to debug
>>>>>>> this.
>>>
>>> Numbers for v1 of the series, both on top of 6.8-rc1 and rebased to the same
>>> mm-unstable base as v3 of the series (first 2 rows are from what I just posted
>>> for context):
>>>
>>> | kernel            |  mean_rel |  std_rel |
>>> |:-------------------|-----------:|----------:|
>>> | mm-unstabe (base)Â |Â Â Â Â Â Â 0.0% |Â Â Â Â Â 1.1% |
>>> | mm-unstable + v3Â Â |Â Â Â Â Â 16.7% |Â Â Â Â Â 0.8% |
>>> | mm-unstable + v1Â Â |Â Â Â Â Â -2.5% |Â Â Â Â Â 1.7% |
>>> | v6.8-rc1 + v1Â Â Â Â Â |Â Â Â Â Â -6.6% |Â Â Â Â Â 1.1% |
>>>
>>> So all looks good with v1. And seems to suggest mm-unstable has regressed by ~4%
>>> vs v6.8-rc1. Is this really a useful benchmark? Does the raw performance of
>>> fork() syscall really matter? Evidence suggests its moving all over the place -
>>> breath on the code and it changes - not a great place to be when using the test
>>> for gating purposes!
>>>
>>> Still with the old tests - I'll move to the new ones now.
>>>
>>>
>>>>>>>
>>>>>>
>>>>>> So far, on my x86 tests (Intel, AMD EPYC), I was not able to observe this.
>>>>>> fork() for order-0 was consistently effectively unchanged. Do you observe
>>>>>> that
>>>>>> on other ARM systems as well?
>>>>>
>>>>> Nope; running the exact same kernel binary and user space on Altra, I see
>>>>> sensible numbers;
>>>>>
>>>>> fork order-0: -1.3%
>>>>> fork order-9: -7.6%
>>>>> dontneed order-0: -0.5%
>>>>> dontneed order-9: 0.1%
>>>>> munmap order-0: 0.0%
>>>>> munmap order-9: -67.9%
>>>>>
>>>>> So I guess some pipelining issue that causes the M2 to stall more?
>>>>
>>>> With one effective added folio_test_large(), it could only be a code layout
>>>> problem? Or the compiler does something stupid, but you say that you run the
>>>> exact same kernel binary, so that doesn't make sense.
>>>
>>> Yup, same binary. We know this code is very sensitive - 1 cycle makes a big
>>> difference. So could easily be code layout, branch prediction, etc...
>>>
>>>>
>>>> I'm also surprised about the dontneed vs. munmap numbers.
>>>
>>> You mean the ones for Altra that I posted? (I didn't post any for M2). The altra
>>> numbers look ok to me; dontneed has no change, and munmap has no change for
>>> order-0 and is massively improved for order-9.
>>>
>>> Â Doesn't make any sense
>>>> (again, there was this VMA merging problem but it would still allow for
>>>> batching
>>>> within a single VMA that spans exactly one large folio).
>>>>
>>>> What are you using as baseline? Really just mm-unstable vs.
>>>> mm-unstable+patches?
>>>
>>> yes. except for "v6.8-rc1 + v1" above.
>>>
>>>>
>>>> Let's see if the new test changes the numbers you measure.
>>
>> Nope: looks the same. I've taken my test harness out of the picture and done
>> everything manually from the ground up, with the old tests and the new. Headline
>> is that I see similar numbers from both.
>
> I took me a while to get really reproducible numbers on Intel. Most importantly:
> * Set a fixed CPU frequency, disabling any boost and avoiding any
> Â thermal throttling.
> * Pin the test to CPUs and set a nice level.
I'm already pinning the test to cpu 0. But for M2, at least, I'm running in a VM
on top of macos, and I don't have a mechanism to pin the QEMU threads to the
physical CPUs. Anyway, I don't think these are problems because for a given
kernel build I can accurately repro numbers.
>
> Another thing is, to avoid systems where you can have NUMA effects within a
> single socket. Otherwise, memory access latency is just random and depends on
> what the buddy enjoys giving you.
Yep; same. M2 is 1 NUMA node. On Altra, I'm disabling the second NUMA node to
remove those effects.
>
> But you seem to get the same +17 even after reboots, so that indicates that the
> CPU is not happy about the code for some reason. And the weird thing is, that
> nothing significantly changed for order-0 folios between v1 and v3 that could
> explain any of this.
>
> I'm not worried about 5% or so, nobody cares. But it would be good to have at
> least an explanation why only that system shows +17%.
Yep understood.
>
>>
>> Some details:
>> Â - I'm running for 10 seconds then averaging the output
>
> Same here.
>
>> Â - test is bimodal; first run (of 10 seconds) after boot is a bit faster on
>> Â Â Â average (up to 10%) than the rest; I could guess this is due to the memory
>> Â Â Â being allocated more contiguously the first few times through, so struct
>> Â Â Â pages have better locality, but that's a guess.
>
> I think it also has to do with the PCP lists, and the high-pcp auto tuning (I
> played with disabling that). Running on a freshly booted system gave me
> reproducible results.
>
> But yes: I was observing something similar on AMD EPYC, where you get
> consecutive pages from the buddy, but once you allocate from the PCP it might no
> longer be consecutive.
>
>> Â - test is 5-10% slower when output is printed to terminal vs when redirected to
>> Â Â Â file. I've always effectively been redirecting. Not sure if this overhead
>> Â Â Â could start to dominate the regression and that's why you don't see it?
>
> That's weird, because we don't print while measuring? Anyhow, 5/10% variance on
> some system is not the end of the world.
I imagine its cache effects? More work to do to print the output could be
evicting some code that's in the benchmark path?
>
>>
>> I'm inclined to run this test for the last N kernel releases and if the number
>> moves around significantly, conclude that these tests don't really matter.
>> Otherwise its an exercise in randomly refactoring code until it works well, but
>> that's just overfitting to the compiler and hw. What do you think?
>
> Personally, I wouldn't lose sleep if you see weird, unexplainable behavior on
> some system (not even architecture!). Trying to optimize for that would indeed
> be random refactorings.
>
> But I would not be so fast to say that "these tests don't really matter" and
> then go wild and degrade them as much as you want. There are use cases that care
> about fork performance especially with order-0 pages -- such as Redis.
Indeed. But also remember that my fork baseline time is ~2.5ms, and I think you
said yours was 14ms :)
I'll continue to mess around with it until the end of the day. But I'm not
making any headway, then I'll change tack; I'll just measure the performance of
my contpte changes using your fork/zap stuff as the baseline and post based on that.
On 31/01/2024 13:38, David Hildenbrand wrote:
>>>> Nope: looks the same. I've taken my test harness out of the picture and done
>>>> everything manually from the ground up, with the old tests and the new.
>>>> Headline
>>>> is that I see similar numbers from both.
>>>
>>> I took me a while to get really reproducible numbers on Intel. Most importantly:
>>> * Set a fixed CPU frequency, disabling any boost and avoiding any
>>> Â Â thermal throttling.
>>> * Pin the test to CPUs and set a nice level.
>>
>> I'm already pinning the test to cpu 0. But for M2, at least, I'm running in a VM
>> on top of macos, and I don't have a mechanism to pin the QEMU threads to the
>> physical CPUs. Anyway, I don't think these are problems because for a given
>> kernel build I can accurately repro numbers.
>
> Oh, you do have a layer of virtualization in there. I *suspect* that might
> amplify some odd things regarding code layout, caching effects, etc.
>
> I guess especially the fork() benchmark is too sensible (fast) for things like
> that, so I would just focus on bare metal results where you can control the
> environment completely.
Yeah, maybe. OK I'll park M2 for now.
>
> Note that regarding NUMA effects, I mean when some memory access within the same
> socket is faster/slower even with only a single node. On AMD EPYC that's
> possible, depending on which core you are running and on which memory controller
> the memory you want to access is located. If both are in different quadrants
> IIUC, the access latency will be different.
I've configured the NUMA to only bring the RAM and CPUs for a single socket
online, so I shouldn't be seeing any of these effects. Anyway, I've been using
the Altra as a secondary because its so much slower than the M2. Let me move
over to it and see if everything looks more straightforward there.
>
>>> But yes: I was observing something similar on AMD EPYC, where you get
>>> consecutive pages from the buddy, but once you allocate from the PCP it might no
>>> longer be consecutive.
>>>
>>>> Â Â - test is 5-10% slower when output is printed to terminal vs when
>>>> redirected to
>>>> Â Â Â Â file. I've always effectively been redirecting. Not sure if this overhead
>>>> Â Â Â Â could start to dominate the regression and that's why you don't see it?
>>>
>>> That's weird, because we don't print while measuring? Anyhow, 5/10% variance on
>>> some system is not the end of the world.
>>
>> I imagine its cache effects? More work to do to print the output could be
>> evicting some code that's in the benchmark path?
>
> Maybe. Do you also see these oddities on the bare metal system?
>
>>
>>>
>>>>
>>>> I'm inclined to run this test for the last N kernel releases and if the number
>>>> moves around significantly, conclude that these tests don't really matter.
>>>> Otherwise its an exercise in randomly refactoring code until it works well, but
>>>> that's just overfitting to the compiler and hw. What do you think?
>>>
>>> Personally, I wouldn't lose sleep if you see weird, unexplainable behavior on
>>> some system (not even architecture!). Trying to optimize for that would indeed
>>> be random refactorings.
>>>
>>> But I would not be so fast to say that "these tests don't really matter" and
>>> then go wild and degrade them as much as you want. There are use cases that care
>>> about fork performance especially with order-0 pages -- such as Redis.
>>
>> Indeed. But also remember that my fork baseline time is ~2.5ms, and I think you
>> said yours was 14ms :)
>
> Yes, no idea why M2 is that fast (BTW, which page size? 4k or 16k? ) :)
The guest kernel is using 4K pages. I'm not quite sure what is happening at
stage2; QEMU doesn't expose any options to explicitly request huge pages for
macos AFAICT.
>
>>
>> I'll continue to mess around with it until the end of the day. But I'm not
>> making any headway, then I'll change tack; I'll just measure the performance of
>> my contpte changes using your fork/zap stuff as the baseline and post based on
>> that.
>
> You should likely not focus on M2 results. Just pick a representative bare metal
> machine where you get consistent, explainable results.
>
> Nothing in the code is fine-tuned for a particular architecture so far, only
> order-0 handling is kept separate.
>
> BTW: I see the exact same speedups for dontneed that I see for munmap. For
> example, for order-9, it goes from 0.023412s -> 0.009785, so -58%. So I'm
> curious why you see a speedup for munmap but not for dontneed.
Ugh... ok, coming up.
>>> Nope: looks the same. I've taken my test harness out of the picture and done
>>> everything manually from the ground up, with the old tests and the new. Headline
>>> is that I see similar numbers from both.
>>
>> I took me a while to get really reproducible numbers on Intel. Most importantly:
>> * Set a fixed CPU frequency, disabling any boost and avoiding any
>> Â thermal throttling.
>> * Pin the test to CPUs and set a nice level.
>
> I'm already pinning the test to cpu 0. But for M2, at least, I'm running in a VM
> on top of macos, and I don't have a mechanism to pin the QEMU threads to the
> physical CPUs. Anyway, I don't think these are problems because for a given
> kernel build I can accurately repro numbers.
Oh, you do have a layer of virtualization in there. I *suspect* that
might amplify some odd things regarding code layout, caching effects, etc.
I guess especially the fork() benchmark is too sensible (fast) for
things like that, so I would just focus on bare metal results where you
can control the environment completely.
Note that regarding NUMA effects, I mean when some memory access within
the same socket is faster/slower even with only a single node. On AMD
EPYC that's possible, depending on which core you are running and on
which memory controller the memory you want to access is located. If
both are in different quadrants IIUC, the access latency will be different.
>> But yes: I was observing something similar on AMD EPYC, where you get
>> consecutive pages from the buddy, but once you allocate from the PCP it might no
>> longer be consecutive.
>>
>>> Â - test is 5-10% slower when output is printed to terminal vs when redirected to
>>> Â Â Â file. I've always effectively been redirecting. Not sure if this overhead
>>> Â Â Â could start to dominate the regression and that's why you don't see it?
>>
>> That's weird, because we don't print while measuring? Anyhow, 5/10% variance on
>> some system is not the end of the world.
>
> I imagine its cache effects? More work to do to print the output could be
> evicting some code that's in the benchmark path?
Maybe. Do you also see these oddities on the bare metal system?
>
>>
>>>
>>> I'm inclined to run this test for the last N kernel releases and if the number
>>> moves around significantly, conclude that these tests don't really matter.
>>> Otherwise its an exercise in randomly refactoring code until it works well, but
>>> that's just overfitting to the compiler and hw. What do you think?
>>
>> Personally, I wouldn't lose sleep if you see weird, unexplainable behavior on
>> some system (not even architecture!). Trying to optimize for that would indeed
>> be random refactorings.
>>
>> But I would not be so fast to say that "these tests don't really matter" and
>> then go wild and degrade them as much as you want. There are use cases that care
>> about fork performance especially with order-0 pages -- such as Redis.
>
> Indeed. But also remember that my fork baseline time is ~2.5ms, and I think you
> said yours was 14ms :)
Yes, no idea why M2 is that fast (BTW, which page size? 4k or 16k? ) :)
>
> I'll continue to mess around with it until the end of the day. But I'm not
> making any headway, then I'll change tack; I'll just measure the performance of
> my contpte changes using your fork/zap stuff as the baseline and post based on that.
You should likely not focus on M2 results. Just pick a representative
bare metal machine where you get consistent, explainable results.
Nothing in the code is fine-tuned for a particular architecture so far,
only order-0 handling is kept separate.
BTW: I see the exact same speedups for dontneed that I see for munmap.
For example, for order-9, it goes from 0.023412s -> 0.009785, so -58%.
So I'm curious why you see a speedup for munmap but not for dontneed.
--
Cheers,
David / dhildenb
>> Note that regarding NUMA effects, I mean when some memory access within the same
>> socket is faster/slower even with only a single node. On AMD EPYC that's
>> possible, depending on which core you are running and on which memory controller
>> the memory you want to access is located. If both are in different quadrants
>> IIUC, the access latency will be different.
>
> I've configured the NUMA to only bring the RAM and CPUs for a single socket
> online, so I shouldn't be seeing any of these effects. Anyway, I've been using
> the Altra as a secondary because its so much slower than the M2. Let me move
> over to it and see if everything looks more straightforward there.
Better use a system where people will actually run Linux production
workloads on, even if it is slower :)
[...]
>>>
>>> I'll continue to mess around with it until the end of the day. But I'm not
>>> making any headway, then I'll change tack; I'll just measure the performance of
>>> my contpte changes using your fork/zap stuff as the baseline and post based on
>>> that.
>>
>> You should likely not focus on M2 results. Just pick a representative bare metal
>> machine where you get consistent, explainable results.
>>
>> Nothing in the code is fine-tuned for a particular architecture so far, only
>> order-0 handling is kept separate.
>>
>> BTW: I see the exact same speedups for dontneed that I see for munmap. For
>> example, for order-9, it goes from 0.023412s -> 0.009785, so -58%. So I'm
>> curious why you see a speedup for munmap but not for dontneed.
>
> Ugh... ok, coming up.
Hopefully you were just staring at the wrong numbers (e.g., only with
fork patches). Because both (munmap/pte-dontneed) are using the exact
same code path.
--
Cheers,
David / dhildenb
On 31/01/2024 15:05, David Hildenbrand wrote:
> On 31.01.24 16:02, Ryan Roberts wrote:
>> On 31/01/2024 14:29, David Hildenbrand wrote:
>>>>> Note that regarding NUMA effects, I mean when some memory access within the
>>>>> same
>>>>> socket is faster/slower even with only a single node. On AMD EPYC that's
>>>>> possible, depending on which core you are running and on which memory
>>>>> controller
>>>>> the memory you want to access is located. If both are in different quadrants
>>>>> IIUC, the access latency will be different.
>>>>
>>>> I've configured the NUMA to only bring the RAM and CPUs for a single socket
>>>> online, so I shouldn't be seeing any of these effects. Anyway, I've been using
>>>> the Altra as a secondary because its so much slower than the M2. Let me move
>>>> over to it and see if everything looks more straightforward there.
>>>
>>> Better use a system where people will actually run Linux production workloads
>>> on, even if it is slower :)
>>>
>>> [...]
>>>
>>>>>>
>>>>>> I'll continue to mess around with it until the end of the day. But I'm not
>>>>>> making any headway, then I'll change tack; I'll just measure the
>>>>>> performance of
>>>>>> my contpte changes using your fork/zap stuff as the baseline and post
>>>>>> based on
>>>>>> that.
>>>>>
>>>>> You should likely not focus on M2 results. Just pick a representative bare
>>>>> metal
>>>>> machine where you get consistent, explainable results.
>>>>>
>>>>> Nothing in the code is fine-tuned for a particular architecture so far, only
>>>>> order-0 handling is kept separate.
>>>>>
>>>>> BTW: I see the exact same speedups for dontneed that I see for munmap. For
>>>>> example, for order-9, it goes from 0.023412s -> 0.009785, so -58%. So I'm
>>>>> curious why you see a speedup for munmap but not for dontneed.
>>>>
>>>> Ugh... ok, coming up.
>>>
>>> Hopefully you were just staring at the wrong numbers (e.g., only with fork
>>> patches). Because both (munmap/pte-dontneed) are using the exact same code path.
>>>
>>
>> Ahh... I'm doing pte-dontneed, which is the only option in your original
>> benchmark - it does MADV_DONTNEED one page at a time. It looks like your new
>> benchmark has an additional "dontneed" option that does it in one shot. Which
>> option are you running? Assuming the latter, I think that explains it.
>
> I temporarily removed that option and then re-added it. Guess you got a wrong
> snapshot of the benchmark :D
>
> pte-dontneed not observing any change is great (no batching possible).
indeed.
>
> dontneed should hopefully/likely see a speedup.
Yes, but that's almost exactly the same path as munmap, so I'm sure it really
adds much for this particular series. Anyway, on Altra at least, I'm seeing no
regressions, so:
Tested-by: Ryan Roberts <[email protected]>
>
> Great!
>
>> dontneed should hopefully/likely see a speedup.
>
> Yes, but that's almost exactly the same path as munmap, so I'm sure it really
> adds much for this particular series.
Right, that's why I'm not including these measurements. dontneed vs.
munmap is more about measuring the overhead of VMA modifications + page
table reclaim.
> Anyway, on Altra at least, I'm seeing no
> regressions, so:
>
> Tested-by: Ryan Roberts <[email protected]>
>
Thanks!
--
Cheers,
David / dhildenb
On 31/01/2024 14:29, David Hildenbrand wrote:
>>> Note that regarding NUMA effects, I mean when some memory access within the same
>>> socket is faster/slower even with only a single node. On AMD EPYC that's
>>> possible, depending on which core you are running and on which memory controller
>>> the memory you want to access is located. If both are in different quadrants
>>> IIUC, the access latency will be different.
>>
>> I've configured the NUMA to only bring the RAM and CPUs for a single socket
>> online, so I shouldn't be seeing any of these effects. Anyway, I've been using
>> the Altra as a secondary because its so much slower than the M2. Let me move
>> over to it and see if everything looks more straightforward there.
>
> Better use a system where people will actually run Linux production workloads
> on, even if it is slower :)
>
> [...]
>
>>>>
>>>> I'll continue to mess around with it until the end of the day. But I'm not
>>>> making any headway, then I'll change tack; I'll just measure the performance of
>>>> my contpte changes using your fork/zap stuff as the baseline and post based on
>>>> that.
>>>
>>> You should likely not focus on M2 results. Just pick a representative bare metal
>>> machine where you get consistent, explainable results.
>>>
>>> Nothing in the code is fine-tuned for a particular architecture so far, only
>>> order-0 handling is kept separate.
>>>
>>> BTW: I see the exact same speedups for dontneed that I see for munmap. For
>>> example, for order-9, it goes from 0.023412s -> 0.009785, so -58%. So I'm
>>> curious why you see a speedup for munmap but not for dontneed.
>>
>> Ugh... ok, coming up.
>
> Hopefully you were just staring at the wrong numbers (e.g., only with fork
> patches). Because both (munmap/pte-dontneed) are using the exact same code path.
>
Ahh... I'm doing pte-dontneed, which is the only option in your original
benchmark - it does MADV_DONTNEED one page at a time. It looks like your new
benchmark has an additional "dontneed" option that does it in one shot. Which
option are you running? Assuming the latter, I think that explains it.
On 31.01.24 16:02, Ryan Roberts wrote:
> On 31/01/2024 14:29, David Hildenbrand wrote:
>>>> Note that regarding NUMA effects, I mean when some memory access within the same
>>>> socket is faster/slower even with only a single node. On AMD EPYC that's
>>>> possible, depending on which core you are running and on which memory controller
>>>> the memory you want to access is located. If both are in different quadrants
>>>> IIUC, the access latency will be different.
>>>
>>> I've configured the NUMA to only bring the RAM and CPUs for a single socket
>>> online, so I shouldn't be seeing any of these effects. Anyway, I've been using
>>> the Altra as a secondary because its so much slower than the M2. Let me move
>>> over to it and see if everything looks more straightforward there.
>>
>> Better use a system where people will actually run Linux production workloads
>> on, even if it is slower :)
>>
>> [...]
>>
>>>>>
>>>>> I'll continue to mess around with it until the end of the day. But I'm not
>>>>> making any headway, then I'll change tack; I'll just measure the performance of
>>>>> my contpte changes using your fork/zap stuff as the baseline and post based on
>>>>> that.
>>>>
>>>> You should likely not focus on M2 results. Just pick a representative bare metal
>>>> machine where you get consistent, explainable results.
>>>>
>>>> Nothing in the code is fine-tuned for a particular architecture so far, only
>>>> order-0 handling is kept separate.
>>>>
>>>> BTW: I see the exact same speedups for dontneed that I see for munmap. For
>>>> example, for order-9, it goes from 0.023412s -> 0.009785, so -58%. So I'm
>>>> curious why you see a speedup for munmap but not for dontneed.
>>>
>>> Ugh... ok, coming up.
>>
>> Hopefully you were just staring at the wrong numbers (e.g., only with fork
>> patches). Because both (munmap/pte-dontneed) are using the exact same code path.
>>
>
> Ahh... I'm doing pte-dontneed, which is the only option in your original
> benchmark - it does MADV_DONTNEED one page at a time. It looks like your new
> benchmark has an additional "dontneed" option that does it in one shot. Which
> option are you running? Assuming the latter, I think that explains it.
I temporarily removed that option and then re-added it. Guess you got a
wrong snapshot of the benchmark :D
pte-dontneed not observing any change is great (no batching possible).
dontneed should hopefully/likely see a speedup.
Great!
--
Cheers,
David / dhildenb
On Mon, Jan 29, 2024 at 01:46:35PM +0100, David Hildenbrand wrote:
> From: Ryan Roberts <[email protected]>
>
> Since the high bits [51:48] of an OA are not stored contiguously in the
> PTE, there is a theoretical bug in set_ptes(), which just adds PAGE_SIZE
> to the pte to get the pte with the next pfn. This works until the pfn
> crosses the 48-bit boundary, at which point we overflow into the upper
> attributes.
>
> Of course one could argue (and Matthew Wilcox has :) that we will never
> see a folio cross this boundary because we only allow naturally aligned
> power-of-2 allocation, so this would require a half-petabyte folio. So
> its only a theoretical bug. But its better that the code is robust
> regardless.
>
> I've implemented pte_next_pfn() as part of the fix, which is an opt-in
> core-mm interface. So that is now available to the core-mm, which will
> be needed shortly to support forthcoming fork()-batching optimizations.
>
> Link: https://lkml.kernel.org/r/[email protected]
> Fixes: 4a169d61c2ed ("arm64: implement the new page table range API")
> Closes: https://lore.kernel.org/linux-mm/[email protected]/
> Signed-off-by: Ryan Roberts <[email protected]>
> Reviewed-by: Catalin Marinas <[email protected]>
> Reviewed-by: David Hildenbrand <[email protected]>
> Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
> ---
> arch/arm64/include/asm/pgtable.h | 28 +++++++++++++++++-----------
> 1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index b50270107e2f..9428801c1040 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -341,6 +341,22 @@ static inline void __sync_cache_and_tags(pte_t pte, unsigned int nr_pages)
> mte_sync_tags(pte, nr_pages);
> }
>
> +/*
> + * Select all bits except the pfn
> + */
> +static inline pgprot_t pte_pgprot(pte_t pte)
> +{
> + unsigned long pfn = pte_pfn(pte);
> +
> + return __pgprot(pte_val(pfn_pte(pfn, __pgprot(0))) ^ pte_val(pte));
> +}
> +
> +#define pte_next_pfn pte_next_pfn
> +static inline pte_t pte_next_pfn(pte_t pte)
> +{
> + return pfn_pte(pte_pfn(pte) + 1, pte_pgprot(pte));
> +}
> +
> static inline void set_ptes(struct mm_struct *mm,
> unsigned long __always_unused addr,
> pte_t *ptep, pte_t pte, unsigned int nr)
> @@ -354,7 +370,7 @@ static inline void set_ptes(struct mm_struct *mm,
> if (--nr == 0)
> break;
> ptep++;
> - pte_val(pte) += PAGE_SIZE;
> + pte = pte_next_pfn(pte);
> }
> }
> #define set_ptes set_ptes
> @@ -433,16 +449,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> return clear_pte_bit(pte, __pgprot(PTE_SWP_EXCLUSIVE));
> }
>
> -/*
> - * Select all bits except the pfn
> - */
> -static inline pgprot_t pte_pgprot(pte_t pte)
> -{
> - unsigned long pfn = pte_pfn(pte);
> -
> - return __pgprot(pte_val(pfn_pte(pfn, __pgprot(0))) ^ pte_val(pte));
> -}
> -
> #ifdef CONFIG_NUMA_BALANCING
> /*
> * See the comment in include/linux/pgtable.h
> --
> 2.43.0
>
>
--
Sincerely yours,
Mike.
On Mon, Jan 29, 2024 at 01:46:36PM +0100, David Hildenbrand wrote:
> We want to make use of pte_next_pfn() outside of set_ptes(). Let's
> simply define PFN_PTE_SHIFT, required by pte_next_pfn().
>
> Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
> ---
> arch/arm/include/asm/pgtable.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> index d657b84b6bf7..be91e376df79 100644
> --- a/arch/arm/include/asm/pgtable.h
> +++ b/arch/arm/include/asm/pgtable.h
> @@ -209,6 +209,8 @@ static inline void __sync_icache_dcache(pte_t pteval)
> extern void __sync_icache_dcache(pte_t pteval);
> #endif
>
> +#define PFN_PTE_SHIFT PAGE_SHIFT
> +
> void set_ptes(struct mm_struct *mm, unsigned long addr,
> pte_t *ptep, pte_t pteval, unsigned int nr);
> #define set_ptes set_ptes
> --
> 2.43.0
>
>
--
Sincerely yours,
Mike.
On Mon, Jan 29, 2024 at 01:46:37PM +0100, David Hildenbrand wrote:
> We want to make use of pte_next_pfn() outside of set_ptes(). Let's
> simply define PFN_PTE_SHIFT, required by pte_next_pfn().
>
> Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
> ---
> arch/nios2/include/asm/pgtable.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/nios2/include/asm/pgtable.h b/arch/nios2/include/asm/pgtable.h
> index 5144506dfa69..d052dfcbe8d3 100644
> --- a/arch/nios2/include/asm/pgtable.h
> +++ b/arch/nios2/include/asm/pgtable.h
> @@ -178,6 +178,8 @@ static inline void set_pte(pte_t *ptep, pte_t pteval)
> *ptep = pteval;
> }
>
> +#define PFN_PTE_SHIFT 0
> +
> static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
> pte_t *ptep, pte_t pte, unsigned int nr)
> {
> --
> 2.43.0
>
>
--
Sincerely yours,
Mike.
On Mon, Jan 29, 2024 at 01:46:38PM +0100, David Hildenbrand wrote:
> We want to make use of pte_next_pfn() outside of set_ptes(). Let's
> simply define PFN_PTE_SHIFT, required by pte_next_pfn().
>
> Reviewed-by: Christophe Leroy <[email protected]>
> Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
> ---
> arch/powerpc/include/asm/pgtable.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
> index 9224f23065ff..7a1ba8889aea 100644
> --- a/arch/powerpc/include/asm/pgtable.h
> +++ b/arch/powerpc/include/asm/pgtable.h
> @@ -41,6 +41,8 @@ struct mm_struct;
>
> #ifndef __ASSEMBLY__
>
> +#define PFN_PTE_SHIFT PTE_RPN_SHIFT
> +
> void set_ptes(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
> pte_t pte, unsigned int nr);
> #define set_ptes set_ptes
> --
> 2.43.0
>
>
--
Sincerely yours,
Mike.
On Mon, Jan 29, 2024 at 01:46:39PM +0100, David Hildenbrand wrote:
> We want to make use of pte_next_pfn() outside of set_ptes(). Let's
> simply define PFN_PTE_SHIFT, required by pte_next_pfn().
>
> Reviewed-by: Alexandre Ghiti <[email protected]>
> Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
> ---
> arch/riscv/include/asm/pgtable.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index 0c94260b5d0c..add5cd30ab34 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -523,6 +523,8 @@ static inline void __set_pte_at(pte_t *ptep, pte_t pteval)
> set_pte(ptep, pteval);
> }
>
> +#define PFN_PTE_SHIFT _PAGE_PFN_SHIFT
> +
> static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
> pte_t *ptep, pte_t pteval, unsigned int nr)
> {
> --
> 2.43.0
>
>
--
Sincerely yours,
Mike.
On Mon, Jan 29, 2024 at 01:46:40PM +0100, David Hildenbrand wrote:
> We want to make use of pte_next_pfn() outside of set_ptes(). Let's
> simply define PFN_PTE_SHIFT, required by pte_next_pfn().
>
> Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
> ---
> arch/s390/include/asm/pgtable.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
> index 1299b56e43f6..4b91e65c85d9 100644
> --- a/arch/s390/include/asm/pgtable.h
> +++ b/arch/s390/include/asm/pgtable.h
> @@ -1316,6 +1316,8 @@ pgprot_t pgprot_writecombine(pgprot_t prot);
> #define pgprot_writethrough pgprot_writethrough
> pgprot_t pgprot_writethrough(pgprot_t prot);
>
> +#define PFN_PTE_SHIFT PAGE_SHIFT
> +
> /*
> * Set multiple PTEs to consecutive pages with a single call. All PTEs
> * are within the same folio, PMD and VMA.
> --
> 2.43.0
>
>
--
Sincerely yours,
Mike.
On Mon, Jan 29, 2024 at 01:46:41PM +0100, David Hildenbrand wrote:
> We want to make use of pte_next_pfn() outside of set_ptes(). Let's
> simply define PFN_PTE_SHIFT, required by pte_next_pfn().
>
> Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
> ---
> arch/sparc/include/asm/pgtable_64.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/sparc/include/asm/pgtable_64.h b/arch/sparc/include/asm/pgtable_64.h
> index a8c871b7d786..652af9d63fa2 100644
> --- a/arch/sparc/include/asm/pgtable_64.h
> +++ b/arch/sparc/include/asm/pgtable_64.h
> @@ -929,6 +929,8 @@ static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
> maybe_tlb_batch_add(mm, addr, ptep, orig, fullmm, PAGE_SHIFT);
> }
>
> +#define PFN_PTE_SHIFT PAGE_SHIFT
> +
> static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
> pte_t *ptep, pte_t pte, unsigned int nr)
> {
> --
> 2.43.0
>
>
--
Sincerely yours,
Mike.
On Mon, Jan 29, 2024 at 01:46:42PM +0100, David Hildenbrand wrote:
> Let's provide pte_next_pfn(), independently of set_ptes(). This allows for
> using the generic pte_next_pfn() version in some arch-specific set_ptes()
> implementations, and prepares for reusing pte_next_pfn() in other context.
>
> Reviewed-by: Christophe Leroy <[email protected]>
> Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
> ---
> include/linux/pgtable.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index f6d0e3513948..351cd9dc7194 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -212,7 +212,6 @@ static inline int pmd_dirty(pmd_t pmd)
> #define arch_flush_lazy_mmu_mode() do {} while (0)
> #endif
>
> -#ifndef set_ptes
>
> #ifndef pte_next_pfn
> static inline pte_t pte_next_pfn(pte_t pte)
> @@ -221,6 +220,7 @@ static inline pte_t pte_next_pfn(pte_t pte)
> }
> #endif
>
> +#ifndef set_ptes
> /**
> * set_ptes - Map consecutive pages to a contiguous range of addresses.
> * @mm: Address space to map the pages into.
> --
> 2.43.0
>
>
--
Sincerely yours,
Mike.
On Mon, Jan 29, 2024 at 01:46:44PM +0100, David Hildenbrand wrote:
> Let's use our handy new helper. Note that the implementation is slightly
> different, but shouldn't really make a difference in practice.
>
> Reviewed-by: Christophe Leroy <[email protected]>
> Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
> ---
> arch/powerpc/mm/pgtable.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
> index a04ae4449a02..549a440ed7f6 100644
> --- a/arch/powerpc/mm/pgtable.c
> +++ b/arch/powerpc/mm/pgtable.c
> @@ -220,10 +220,7 @@ void set_ptes(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
> break;
> ptep++;
> addr += PAGE_SIZE;
> - /*
> - * increment the pfn.
> - */
> - pte = pfn_pte(pte_pfn(pte) + 1, pte_pgprot((pte)));
> + pte = pte_next_pfn(pte);
> }
> }
>
> --
> 2.43.0
>
>
--
Sincerely yours,
Mike.
On Mon, Jan 29, 2024 at 01:46:43PM +0100, David Hildenbrand wrote:
> Let's use our handy helper now that it's available on all archs.
>
> Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
> ---
> arch/arm/mm/mmu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 674ed71573a8..c24e29c0b9a4 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -1814,6 +1814,6 @@ void set_ptes(struct mm_struct *mm, unsigned long addr,
> if (--nr == 0)
> break;
> ptep++;
> - pte_val(pteval) += PAGE_SIZE;
> + pteval = pte_next_pfn(pteval);
> }
> }
> --
> 2.43.0
>
>
--
Sincerely yours,
Mike.
On Mon, Jan 29, 2024 at 01:46:45PM +0100, David Hildenbrand wrote:
> Let's prepare for further changes.
>
> Reviewed-by: Ryan Roberts <[email protected]>
> Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
> ---
> mm/memory.c | 63 ++++++++++++++++++++++++++++-------------------------
> 1 file changed, 33 insertions(+), 30 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 8d14ba440929..a3bdb25f4c8d 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -930,6 +930,29 @@ copy_present_page(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
> return 0;
> }
>
> +static inline void __copy_present_pte(struct vm_area_struct *dst_vma,
> + struct vm_area_struct *src_vma, pte_t *dst_pte, pte_t *src_pte,
> + pte_t pte, unsigned long addr)
> +{
> + struct mm_struct *src_mm = src_vma->vm_mm;
> +
> + /* If it's a COW mapping, write protect it both processes. */
> + if (is_cow_mapping(src_vma->vm_flags) && pte_write(pte)) {
> + ptep_set_wrprotect(src_mm, addr, src_pte);
> + pte = pte_wrprotect(pte);
> + }
> +
> + /* If it's a shared mapping, mark it clean in the child. */
> + if (src_vma->vm_flags & VM_SHARED)
> + pte = pte_mkclean(pte);
> + pte = pte_mkold(pte);
> +
> + if (!userfaultfd_wp(dst_vma))
> + pte = pte_clear_uffd_wp(pte);
> +
> + set_pte_at(dst_vma->vm_mm, addr, dst_pte, pte);
> +}
> +
> /*
> * Copy one pte. Returns 0 if succeeded, or -EAGAIN if one preallocated page
> * is required to copy this pte.
> @@ -939,23 +962,23 @@ copy_present_pte(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> pte_t *dst_pte, pte_t *src_pte, unsigned long addr, int *rss,
> struct folio **prealloc)
> {
> - struct mm_struct *src_mm = src_vma->vm_mm;
> - unsigned long vm_flags = src_vma->vm_flags;
> pte_t pte = ptep_get(src_pte);
> struct page *page;
> struct folio *folio;
>
> page = vm_normal_page(src_vma, addr, pte);
> - if (page)
> - folio = page_folio(page);
> - if (page && folio_test_anon(folio)) {
> + if (unlikely(!page))
> + goto copy_pte;
> +
> + folio = page_folio(page);
> + folio_get(folio);
> + if (folio_test_anon(folio)) {
> /*
> * If this page may have been pinned by the parent process,
> * copy the page immediately for the child so that we'll always
> * guarantee the pinned page won't be randomly replaced in the
> * future.
> */
> - folio_get(folio);
> if (unlikely(folio_try_dup_anon_rmap_pte(folio, page, src_vma))) {
> /* Page may be pinned, we have to copy. */
> folio_put(folio);
> @@ -963,34 +986,14 @@ copy_present_pte(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> addr, rss, prealloc, page);
> }
> rss[MM_ANONPAGES]++;
> - } else if (page) {
> - folio_get(folio);
> + VM_WARN_ON_FOLIO(PageAnonExclusive(page), folio);
> + } else {
> folio_dup_file_rmap_pte(folio, page);
> rss[mm_counter_file(folio)]++;
> }
>
> - /*
> - * If it's a COW mapping, write protect it both
> - * in the parent and the child
> - */
> - if (is_cow_mapping(vm_flags) && pte_write(pte)) {
> - ptep_set_wrprotect(src_mm, addr, src_pte);
> - pte = pte_wrprotect(pte);
> - }
> - VM_BUG_ON(page && folio_test_anon(folio) && PageAnonExclusive(page));
> -
> - /*
> - * If it's a shared mapping, mark it clean in
> - * the child
> - */
> - if (vm_flags & VM_SHARED)
> - pte = pte_mkclean(pte);
> - pte = pte_mkold(pte);
> -
> - if (!userfaultfd_wp(dst_vma))
> - pte = pte_clear_uffd_wp(pte);
> -
> - set_pte_at(dst_vma->vm_mm, addr, dst_pte, pte);
> +copy_pte:
> + __copy_present_pte(dst_vma, src_vma, dst_pte, src_pte, pte, addr);
> return 0;
> }
>
> --
> 2.43.0
>
>
--
Sincerely yours,
Mike.
On Mon, Jan 29, 2024 at 01:46:46PM +0100, David Hildenbrand wrote:
> We already read it, let's just forward it.
>
> This patch is based on work by Ryan Roberts.
>
> Reviewed-by: Ryan Roberts <[email protected]>
> Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
> ---
> mm/memory.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index a3bdb25f4c8d..41b24da5be38 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -959,10 +959,9 @@ static inline void __copy_present_pte(struct vm_area_struct *dst_vma,
> */
> static inline int
> copy_present_pte(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> - pte_t *dst_pte, pte_t *src_pte, unsigned long addr, int *rss,
> - struct folio **prealloc)
> + pte_t *dst_pte, pte_t *src_pte, pte_t pte, unsigned long addr,
> + int *rss, struct folio **prealloc)
> {
> - pte_t pte = ptep_get(src_pte);
> struct page *page;
> struct folio *folio;
>
> @@ -1103,7 +1102,7 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> }
> /* copy_present_pte() will clear `*prealloc' if consumed */
> ret = copy_present_pte(dst_vma, src_vma, dst_pte, src_pte,
> - addr, rss, &prealloc);
> + ptent, addr, rss, &prealloc);
> /*
> * If we need a pre-allocated page for this pte, drop the
> * locks, allocate, and try again.
> --
> 2.43.0
>
>
--
Sincerely yours,
Mike.
On Mon, Jan 29, 2024 at 01:46:47PM +0100, David Hildenbrand wrote:
> Let's implement PTE batching when consecutive (present) PTEs map
> consecutive pages of the same large folio, and all other PTE bits besides
> the PFNs are equal.
>
> We will optimize folio_pte_batch() separately, to ignore selected
> PTE bits. This patch is based on work by Ryan Roberts.
>
> Use __always_inline for __copy_present_ptes() and keep the handling for
> single PTEs completely separate from the multi-PTE case: we really want
> the compiler to optimize for the single-PTE case with small folios, to
> not degrade performance.
>
> Note that PTE batching will never exceed a single page table and will
> always stay within VMA boundaries.
>
> Further, processing PTE-mapped THP that maybe pinned and have
> PageAnonExclusive set on at least one subpage should work as expected,
> but there is room for improvement: We will repeatedly (1) detect a PTE
> batch (2) detect that we have to copy a page (3) fall back and allocate a
> single page to copy a single page. For now we won't care as pinned pages
> are a corner case, and we should rather look into maintaining only a
> single PageAnonExclusive bit for large folios.
>
> Reviewed-by: Ryan Roberts <[email protected]>
> Signed-off-by: David Hildenbrand <[email protected]>
Reviewed-by: Mike Rapoport (IBM) <[email protected]>
> ---
> include/linux/pgtable.h | 31 +++++++++++
> mm/memory.c | 112 +++++++++++++++++++++++++++++++++-------
> 2 files changed, 124 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index 351cd9dc7194..aab227e12493 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -650,6 +650,37 @@ static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addres
> }
> #endif
>
> +#ifndef wrprotect_ptes
> +/**
> + * wrprotect_ptes - Write-protect PTEs that map consecutive pages of the same
> + * folio.
> + * @mm: Address space the pages are mapped into.
> + * @addr: Address the first page is mapped at.
> + * @ptep: Page table pointer for the first entry.
> + * @nr: Number of entries to write-protect.
> + *
> + * May be overridden by the architecture; otherwise, implemented as a simple
> + * loop over ptep_set_wrprotect().
> + *
> + * Note that PTE bits in the PTE range besides the PFN can differ. For example,
> + * some PTEs might be write-protected.
> + *
> + * Context: The caller holds the page table lock. The PTEs map consecutive
> + * pages that belong to the same folio. The PTEs are all in the same PMD.
> + */
> +static inline void wrprotect_ptes(struct mm_struct *mm, unsigned long addr,
> + pte_t *ptep, unsigned int nr)
> +{
> + for (;;) {
> + ptep_set_wrprotect(mm, addr, ptep);
> + if (--nr == 0)
> + break;
> + ptep++;
> + addr += PAGE_SIZE;
> + }
> +}
> +#endif
> +
> /*
> * On some architectures hardware does not set page access bit when accessing
> * memory page, it is responsibility of software setting this bit. It brings
> diff --git a/mm/memory.c b/mm/memory.c
> index 41b24da5be38..86f8a0021c8e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -930,15 +930,15 @@ copy_present_page(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
> return 0;
> }
>
> -static inline void __copy_present_pte(struct vm_area_struct *dst_vma,
> +static __always_inline void __copy_present_ptes(struct vm_area_struct *dst_vma,
> struct vm_area_struct *src_vma, pte_t *dst_pte, pte_t *src_pte,
> - pte_t pte, unsigned long addr)
> + pte_t pte, unsigned long addr, int nr)
> {
> struct mm_struct *src_mm = src_vma->vm_mm;
>
> /* If it's a COW mapping, write protect it both processes. */
> if (is_cow_mapping(src_vma->vm_flags) && pte_write(pte)) {
> - ptep_set_wrprotect(src_mm, addr, src_pte);
> + wrprotect_ptes(src_mm, addr, src_pte, nr);
> pte = pte_wrprotect(pte);
> }
>
> @@ -950,26 +950,93 @@ static inline void __copy_present_pte(struct vm_area_struct *dst_vma,
> if (!userfaultfd_wp(dst_vma))
> pte = pte_clear_uffd_wp(pte);
>
> - set_pte_at(dst_vma->vm_mm, addr, dst_pte, pte);
> + set_ptes(dst_vma->vm_mm, addr, dst_pte, pte, nr);
> +}
> +
> +/*
> + * Detect a PTE batch: consecutive (present) PTEs that map consecutive
> + * pages of the same folio.
> + *
> + * All PTEs inside a PTE batch have the same PTE bits set, excluding the PFN.
> + */
> +static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
> + pte_t *start_ptep, pte_t pte, int max_nr)
> +{
> + unsigned long folio_end_pfn = folio_pfn(folio) + folio_nr_pages(folio);
> + const pte_t *end_ptep = start_ptep + max_nr;
> + pte_t expected_pte = pte_next_pfn(pte);
> + pte_t *ptep = start_ptep + 1;
> +
> + VM_WARN_ON_FOLIO(!pte_present(pte), folio);
> +
> + while (ptep != end_ptep) {
> + pte = ptep_get(ptep);
> +
> + if (!pte_same(pte, expected_pte))
> + break;
> +
> + /*
> + * Stop immediately once we reached the end of the folio. In
> + * corner cases the next PFN might fall into a different
> + * folio.
> + */
> + if (pte_pfn(pte) == folio_end_pfn)
> + break;
> +
> + expected_pte = pte_next_pfn(expected_pte);
> + ptep++;
> + }
> +
> + return ptep - start_ptep;
> }
>
> /*
> - * Copy one pte. Returns 0 if succeeded, or -EAGAIN if one preallocated page
> - * is required to copy this pte.
> + * Copy one present PTE, trying to batch-process subsequent PTEs that map
> + * consecutive pages of the same folio by copying them as well.
> + *
> + * Returns -EAGAIN if one preallocated page is required to copy the next PTE.
> + * Otherwise, returns the number of copied PTEs (at least 1).
> */
> static inline int
> -copy_present_pte(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> +copy_present_ptes(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> pte_t *dst_pte, pte_t *src_pte, pte_t pte, unsigned long addr,
> - int *rss, struct folio **prealloc)
> + int max_nr, int *rss, struct folio **prealloc)
> {
> struct page *page;
> struct folio *folio;
> + int err, nr;
>
> page = vm_normal_page(src_vma, addr, pte);
> if (unlikely(!page))
> goto copy_pte;
>
> folio = page_folio(page);
> +
> + /*
> + * If we likely have to copy, just don't bother with batching. Make
> + * sure that the common "small folio" case is as fast as possible
> + * by keeping the batching logic separate.
> + */
> + if (unlikely(!*prealloc && folio_test_large(folio) && max_nr != 1)) {
> + nr = folio_pte_batch(folio, addr, src_pte, pte, max_nr);
> + folio_ref_add(folio, nr);
> + if (folio_test_anon(folio)) {
> + if (unlikely(folio_try_dup_anon_rmap_ptes(folio, page,
> + nr, src_vma))) {
> + folio_ref_sub(folio, nr);
> + return -EAGAIN;
> + }
> + rss[MM_ANONPAGES] += nr;
> + VM_WARN_ON_FOLIO(PageAnonExclusive(page), folio);
> + } else {
> + folio_dup_file_rmap_ptes(folio, page, nr);
> + rss[mm_counter_file(folio)] += nr;
> + }
> + __copy_present_ptes(dst_vma, src_vma, dst_pte, src_pte, pte,
> + addr, nr);
> + return nr;
> + }
> +
> folio_get(folio);
> if (folio_test_anon(folio)) {
> /*
> @@ -981,8 +1048,9 @@ copy_present_pte(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> if (unlikely(folio_try_dup_anon_rmap_pte(folio, page, src_vma))) {
> /* Page may be pinned, we have to copy. */
> folio_put(folio);
> - return copy_present_page(dst_vma, src_vma, dst_pte, src_pte,
> - addr, rss, prealloc, page);
> + err = copy_present_page(dst_vma, src_vma, dst_pte, src_pte,
> + addr, rss, prealloc, page);
> + return err ? err : 1;
> }
> rss[MM_ANONPAGES]++;
> VM_WARN_ON_FOLIO(PageAnonExclusive(page), folio);
> @@ -992,8 +1060,8 @@ copy_present_pte(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> }
>
> copy_pte:
> - __copy_present_pte(dst_vma, src_vma, dst_pte, src_pte, pte, addr);
> - return 0;
> + __copy_present_ptes(dst_vma, src_vma, dst_pte, src_pte, pte, addr, 1);
> + return 1;
> }
>
> static inline struct folio *folio_prealloc(struct mm_struct *src_mm,
> @@ -1030,10 +1098,11 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> pte_t *src_pte, *dst_pte;
> pte_t ptent;
> spinlock_t *src_ptl, *dst_ptl;
> - int progress, ret = 0;
> + int progress, max_nr, ret = 0;
> int rss[NR_MM_COUNTERS];
> swp_entry_t entry = (swp_entry_t){0};
> struct folio *prealloc = NULL;
> + int nr;
>
> again:
> progress = 0;
> @@ -1064,6 +1133,8 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> arch_enter_lazy_mmu_mode();
>
> do {
> + nr = 1;
> +
> /*
> * We are holding two locks at this point - either of them
> * could generate latencies in another task on another CPU.
> @@ -1100,9 +1171,10 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> */
> WARN_ON_ONCE(ret != -ENOENT);
> }
> - /* copy_present_pte() will clear `*prealloc' if consumed */
> - ret = copy_present_pte(dst_vma, src_vma, dst_pte, src_pte,
> - ptent, addr, rss, &prealloc);
> + /* copy_present_ptes() will clear `*prealloc' if consumed */
> + max_nr = (end - addr) / PAGE_SIZE;
> + ret = copy_present_ptes(dst_vma, src_vma, dst_pte, src_pte,
> + ptent, addr, max_nr, rss, &prealloc);
> /*
> * If we need a pre-allocated page for this pte, drop the
> * locks, allocate, and try again.
> @@ -1119,8 +1191,10 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> folio_put(prealloc);
> prealloc = NULL;
> }
> - progress += 8;
> - } while (dst_pte++, src_pte++, addr += PAGE_SIZE, addr != end);
> + nr = ret;
> + progress += 8 * nr;
> + } while (dst_pte += nr, src_pte += nr, addr += PAGE_SIZE * nr,
> + addr != end);
>
> arch_leave_lazy_mmu_mode();
> pte_unmap_unlock(orig_src_pte, src_ptl);
> @@ -1141,7 +1215,7 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> prealloc = folio_prealloc(src_mm, src_vma, addr, false);
> if (!prealloc)
> return -ENOMEM;
> - } else if (ret) {
> + } else if (ret < 0) {
> VM_WARN_ON_ONCE(1);
> }
>
> --
> 2.43.0
>
>
--
Sincerely yours,
Mike.
On 08.02.24 07:10, Mike Rapoport wrote:
> On Mon, Jan 29, 2024 at 01:46:35PM +0100, David Hildenbrand wrote:
>> From: Ryan Roberts <[email protected]>
>>
>> Since the high bits [51:48] of an OA are not stored contiguously in the
>> PTE, there is a theoretical bug in set_ptes(), which just adds PAGE_SIZE
>> to the pte to get the pte with the next pfn. This works until the pfn
>> crosses the 48-bit boundary, at which point we overflow into the upper
>> attributes.
>>
>> Of course one could argue (and Matthew Wilcox has :) that we will never
>> see a folio cross this boundary because we only allow naturally aligned
>> power-of-2 allocation, so this would require a half-petabyte folio. So
>> its only a theoretical bug. But its better that the code is robust
>> regardless.
>>
>> I've implemented pte_next_pfn() as part of the fix, which is an opt-in
>> core-mm interface. So that is now available to the core-mm, which will
>> be needed shortly to support forthcoming fork()-batching optimizations.
>>
>> Link: https://lkml.kernel.org/r/[email protected]
>> Fixes: 4a169d61c2ed ("arm64: implement the new page table range API")
>> Closes: https://lore.kernel.org/linux-mm/[email protected]/
>> Signed-off-by: Ryan Roberts <[email protected]>
>> Reviewed-by: Catalin Marinas <[email protected]>
>> Reviewed-by: David Hildenbrand <[email protected]>
>> Signed-off-by: David Hildenbrand <[email protected]>
>
> Reviewed-by: Mike Rapoport (IBM) <[email protected]>
Thanks for the review Mike, appreciated!
--
Cheers,
David / dhildenb
On 29.01.24 13:46, David Hildenbrand wrote:
> We already read it, let's just forward it.
>
> This patch is based on work by Ryan Roberts.
>
> Reviewed-by: Ryan Roberts <[email protected]>
> Signed-off-by: David Hildenbrand <[email protected]>
> ---
> mm/memory.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index a3bdb25f4c8d..41b24da5be38 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -959,10 +959,9 @@ static inline void __copy_present_pte(struct vm_area_struct *dst_vma,
> */
> static inline int
> copy_present_pte(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> - pte_t *dst_pte, pte_t *src_pte, unsigned long addr, int *rss,
> - struct folio **prealloc)
> + pte_t *dst_pte, pte_t *src_pte, pte_t pte, unsigned long addr,
> + int *rss, struct folio **prealloc)
> {
> - pte_t pte = ptep_get(src_pte);
> struct page *page;
> struct folio *folio;
>
> @@ -1103,7 +1102,7 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
> }
> /* copy_present_pte() will clear `*prealloc' if consumed */
> ret = copy_present_pte(dst_vma, src_vma, dst_pte, src_pte,
> - addr, rss, &prealloc);
> + ptent, addr, rss, &prealloc);
> /*
> * If we need a pre-allocated page for this pte, drop the
> * locks, allocate, and try again.
The following fixup for that device-exclusive thingy on top (fixing a hmm
selftest I just discovered to be broken).
From 8f9e44f25087dc71890b8d9bd680375691232e85 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <[email protected]>
Date: Wed, 14 Feb 2024 23:09:29 +0100
Subject: [PATCH] fixup: mm/memory: pass PTE to copy_present_pte()
For device-exclusive nonswp entries (is_device_exclusive_entry()),
copy_nonpresent_pte() can turn the PTEs into actual present PTEs while
holding the page table lock.
We hae to re-read the PTE after that operation, such that we won't be
working on the stale non-present PTE, assuming it would be present.
This fixes the hmm "exclusive_cow" selftest.
./run_vmtests.sh -t hmm
# # RUN hmm.hmm_device_private.exclusive_cow ...
# # OK hmm.hmm_device_private.exclusive_cow
# ok 23 hmm.hmm_device_private.exclusive_cow
Signed-off-by: David Hildenbrand <[email protected]>
---
mm/memory.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/memory.c b/mm/memory.c
index 3b8e56eb08a3..29a75f38df7c 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1208,6 +1208,8 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
progress += 8;
continue;
}
+ ptent = ptep_get(src_pte);
+ VM_WARN_ON_ONCE(!pte_present(ptent));
/*
* Device exclusive entry restored, continue by copying
--
2.43.0
--
Cheers,
David / dhildenb
Hello:
This series was applied to riscv/linux.git (fixes)
by Andrew Morton <[email protected]>:
On Mon, 29 Jan 2024 13:46:34 +0100 you wrote:
> Now that the rmap overhaul[1] is upstream that provides a clean interface
> for rmap batching, let's implement PTE batching during fork when processing
> PTE-mapped THPs.
>
> This series is partially based on Ryan's previous work[2] to implement
> cont-pte support on arm64, but its a complete rewrite based on [1] to
> optimize all architectures independent of any such PTE bits, and to
> use the new rmap batching functions that simplify the code and prepare
> for further rmap accounting changes.
>
> [...]
Here is the summary with links:
- [v3,01/15] arm64/mm: Make set_ptes() robust when OAs cross 48-bit boundary
(no matching commit)
- [v3,02/15] arm/pgtable: define PFN_PTE_SHIFT
(no matching commit)
- [v3,03/15] nios2/pgtable: define PFN_PTE_SHIFT
(no matching commit)
- [v3,04/15] powerpc/pgtable: define PFN_PTE_SHIFT
(no matching commit)
- [v3,05/15] riscv/pgtable: define PFN_PTE_SHIFT
https://git.kernel.org/riscv/c/57c254b2fb31
- [v3,06/15] s390/pgtable: define PFN_PTE_SHIFT
(no matching commit)
- [v3,07/15] sparc/pgtable: define PFN_PTE_SHIFT
(no matching commit)
- [v3,08/15] mm/pgtable: make pte_next_pfn() independent of set_ptes()
(no matching commit)
- [v3,09/15] arm/mm: use pte_next_pfn() in set_ptes()
(no matching commit)
- [v3,10/15] powerpc/mm: use pte_next_pfn() in set_ptes()
(no matching commit)
- [v3,11/15] mm/memory: factor out copying the actual PTE in copy_present_pte()
(no matching commit)
- [v3,12/15] mm/memory: pass PTE to copy_present_pte()
(no matching commit)
- [v3,13/15] mm/memory: optimize fork() with PTE-mapped THP
(no matching commit)
- [v3,14/15] mm/memory: ignore dirty/accessed/soft-dirty bits in folio_pte_batch()
(no matching commit)
- [v3,15/15] mm/memory: ignore writable bit in folio_pte_batch()
(no matching commit)
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html