From: David Hildenbrand <hidden> Date: 2024-01-22 19:42:15
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.014265 | 0%
16KiB | 0.014263 | 0.013293 | - 7%
32KiB | 0.014334 | 0.012355 | -14%
64KiB | 0.014046 | 0.011837 | -16%
128KiB | 0.014011 | 0.011536 | -18%
256KiB | 0.013993 | 0.01134 | -19%
512KiB | 0.013983 | 0.011311 | -19%
1024KiB | 0.013986 | 0.011282 | -19%
2048KiB | 0.014305 | 0.011496 | -20%
Next up is PTE batching when unmapping, that I'll probably send out
based on this series this/next week.
Only tested on x86-64. Compile-tested on most other architectures. Will
do more testing and double-check the arch changes while this is getting
some review.
[1] https://lkml.kernel.org/r/20231220224504.646757-1-david@redhat.com
[2] https://lkml.kernel.org/r/20231218105100.172635-1-ryan.roberts@arm.com
[3] https://lkml.kernel.org/r/20230809083256.699513-1-david@redhat.com
[4] https://lkml.kernel.org/r/20231124132626.235350-1-david@redhat.com
[5] https://lkml.kernel.org/r/20231207161211.2374093-1-ryan.roberts@arm.com
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Ryan Roberts <ryan.roberts@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Dinh Nguyen <dinguyen@kernel.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Christophe Leroy <redacted>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>
Cc: "Naveen N. Rao" <redacted>
Cc: Paul Walmsley <redacted>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Albert Ou <aou@eecs.berkeley.edu>
Cc: Alexander Gordeev <agordeev@linux.ibm.com>
Cc: Gerald Schaefer <gerald.schaefer@linux.ibm.com>
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-riscv@lists.infradead.org
Cc: linux-s390@vger.kernel.org
Cc: sparclinux@vger.kernel.org
David Hildenbrand (11):
arm/pgtable: define PFN_PTE_SHIFT on arm and arm64
nios2/pgtable: define PFN_PTE_SHIFT
powerpc/pgtable: define PFN_PTE_SHIFT
risc: pgtable: define PFN_PTE_SHIFT
s390/pgtable: define PFN_PTE_SHIFT
sparc/pgtable: define PFN_PTE_SHIFT
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()
arch/arm/include/asm/pgtable.h | 2 +
arch/arm64/include/asm/pgtable.h | 2 +
arch/nios2/include/asm/pgtable.h | 2 +
arch/powerpc/include/asm/pgtable.h | 2 +
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 | 17 ++-
mm/memory.c | 188 +++++++++++++++++++++-------
9 files changed, 173 insertions(+), 46 deletions(-)
base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
--
2.43.0
From: David Hildenbrand <hidden> Date: 2024-01-22 19:42:23
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/arm/include/asm/pgtable.h | 2 ++
arch/arm64/include/asm/pgtable.h | 2 ++
2 files changed, 4 insertions(+)
From: David Hildenbrand <hidden> Date: 2024-01-22 19:42:28
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/nios2/include/asm/pgtable.h | 2 ++
1 file changed, 2 insertions(+)
From: David Hildenbrand <hidden> Date: 2024-01-22 19:42:33
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/powerpc/include/asm/pgtable.h | 2 ++
1 file changed, 2 insertions(+)
From: David Hildenbrand <hidden> Date: 2024-01-22 19:42:40
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/riscv/include/asm/pgtable.h | 2 ++
1 file changed, 2 insertions(+)
From: David Hildenbrand <hidden> Date: 2024-01-22 19:42:41
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/s390/include/asm/pgtable.h | 2 ++
1 file changed, 2 insertions(+)
From: David Hildenbrand <hidden> Date: 2024-01-22 19:42:47
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/sparc/include/asm/pgtable_64.h | 2 ++
1 file changed, 2 insertions(+)
@@ -930,6 +930,29 @@ copy_present_page(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vmareturn0;}+staticinlinevoid__copy_present_pte(structvm_area_struct*dst_vma,+structvm_area_struct*src_vma,pte_t*dst_pte,pte_t*src_pte,+pte_tpte,unsignedlongaddr)+{+structmm_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);+}+/**Copyonepte.Returns0ifsucceeded,or-EAGAINifonepreallocatedpage*isrequiredtocopythispte.
From: David Hildenbrand <hidden> Date: 2024-01-22 19:42:58
We already read it, let's just forward it.
This patch is based on work by Ryan Roberts.
Signed-off-by: David Hildenbrand <redacted>
---
mm/memory.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
From: David Hildenbrand <hidden> Date: 2024-01-22 19:43:03
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 some other
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.
Signed-off-by: David Hildenbrand <redacted>
---
include/linux/pgtable.h | 17 +++++-
mm/memory.c | 113 +++++++++++++++++++++++++++++++++-------
2 files changed, 109 insertions(+), 21 deletions(-)
@@ -981,8 +1049,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);-returncopy_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);+returnerr?err:1;}rss[MM_ANONPAGES]++;VM_WARN_ON_FOLIO(PageAnonExclusive(page),folio);
From: David Hildenbrand <hidden> Date: 2024-01-22 19:43:14
Let's ignore these bits: they are irrelevant for fork, and will likely
be irrelevant for upcoming users such as page unmapping.
Signed-off-by: David Hildenbrand <redacted>
---
mm/memory.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
From: David Hildenbrand <hidden> Date: 2024-01-22 19:43:24
... 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) won't care.
Signed-off-by: David Hildenbrand <redacted>
---
mm/memory.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
Hi David,
On 22/01/2024 20:41, David Hildenbrand wrote:
quoted hunk
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/riscv/include/asm/pgtable.h | 2 ++
1 file changed, 2 insertions(+)
From: David Hildenbrand <hidden> Date: 2024-01-22 20:08:33
On 22.01.24 21:03, Alexandre Ghiti wrote:
Hi David,
On 22/01/2024 20:41, David Hildenbrand wrote:
quoted
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/riscv/include/asm/pgtable.h | 2 ++
1 file changed, 2 insertions(+)
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 10:34:29
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted hunk
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/arm/include/asm/pgtable.h | 2 ++
arch/arm64/include/asm/pgtable.h | 2 ++
2 files changed, 4 insertions(+)
I think this is buggy. And so is the arm64 implementation of set_ptes(). It
works fine for 48-bit output address, but for 52-bit OAs, the high bits are not
kept contigously, so if you happen to be setting a mapping for which the
physical memory block straddles bit 48, this won't work.
Today, only the 64K base page config can support 52 bits, and for this,
OA[51:48] are stored in PTE[15:12]. But 52 bits for 4K and 16K base pages is
coming (hopefully v6.9) and in this case OA[51:50] are stored in PTE[9:8].
Fortunately we already have helpers in arm64 to abstract this.
So I think arm64 will want to define its own pte_next_pfn():
#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));
}
I'll do a separate patch to fix the already broken arm64 set_ptes() implementation.
I'm not sure if this type of problem might also apply to other arches?
+
static inline void set_ptes(struct mm_struct *mm,
unsigned long __always_unused addr,
pte_t *ptep, pte_t pte, unsigned int nr)
@@ -930,6 +930,29 @@ copy_present_page(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vmareturn0;}+staticinlinevoid__copy_present_pte(structvm_area_struct*dst_vma,+structvm_area_struct*src_vma,pte_t*dst_pte,pte_t*src_pte,+pte_tpte,unsignedlongaddr)+{+structmm_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);+}+/**Copyonepte.Returns0ifsucceeded,or-EAGAINifonepreallocatedpage*isrequiredtocopythispte.
From: David Hildenbrand <hidden> Date: 2024-01-23 10:48:37
On 23.01.24 11:34, Ryan Roberts wrote:
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/arm/include/asm/pgtable.h | 2 ++
arch/arm64/include/asm/pgtable.h | 2 ++
2 files changed, 4 insertions(+)
I think this is buggy. And so is the arm64 implementation of set_ptes(). It
works fine for 48-bit output address, but for 52-bit OAs, the high bits are not
kept contigously, so if you happen to be setting a mapping for which the
physical memory block straddles bit 48, this won't work.
Right, as soon as the PTE bits are not contiguous, this stops working,
just like set_ptes() would, which I used as orientation.
Today, only the 64K base page config can support 52 bits, and for this,
OA[51:48] are stored in PTE[15:12]. But 52 bits for 4K and 16K base pages is
coming (hopefully v6.9) and in this case OA[51:50] are stored in PTE[9:8].
Fortunately we already have helpers in arm64 to abstract this.
So I think arm64 will want to define its own pte_next_pfn():
#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));
}
I'll do a separate patch to fix the already broken arm64 set_ptes() implementation.
Make sense.
I'm not sure if this type of problem might also apply to other arches?
I saw similar handling in the PPC implementation of set_ptes, but was
not able to convince me that it is actually required there.
pte_pfn on ppc does:
static inline unsigned long pte_pfn(pte_t pte)
{
return (pte_val(pte) & PTE_RPN_MASK) >> PTE_RPN_SHIFT;
}
But that means that the PFNs *are* contiguous. If high bits are used for
something else, then we might produce a garbage PTE on overflow, but
that shouldn't really matter I concluded for folio_pte_batch() purposes,
we'd not detect "belongs to this folio batch" either way.
Maybe it's likely cleaner to also have a custom pte_next_pfn() on ppc, I
just hope that we don't lose any other arbitrary PTE bits by doing the
pte_pgprot().
I guess pte_pfn() implementations should tell us if anything special
needs to happen.
--
Cheers,
David / dhildenb
From: David Hildenbrand <hidden> Date: 2024-01-23 11:02:27
On 23.01.24 11:48, David Hildenbrand wrote:
On 23.01.24 11:34, Ryan Roberts wrote:
quoted
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/arm/include/asm/pgtable.h | 2 ++
arch/arm64/include/asm/pgtable.h | 2 ++
2 files changed, 4 insertions(+)
I think this is buggy. And so is the arm64 implementation of set_ptes(). It
works fine for 48-bit output address, but for 52-bit OAs, the high bits are not
kept contigously, so if you happen to be setting a mapping for which the
physical memory block straddles bit 48, this won't work.
Right, as soon as the PTE bits are not contiguous, this stops working,
just like set_ptes() would, which I used as orientation.
quoted
Today, only the 64K base page config can support 52 bits, and for this,
OA[51:48] are stored in PTE[15:12]. But 52 bits for 4K and 16K base pages is
coming (hopefully v6.9) and in this case OA[51:50] are stored in PTE[9:8].
Fortunately we already have helpers in arm64 to abstract this.
So I think arm64 will want to define its own pte_next_pfn():
#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));
}
Digging into the details, on arm64 we have:
#define pte_pfn(pte) (__pte_to_phys(pte) >> PAGE_SHIFT)
and
#define __pte_to_phys(pte) (pte_val(pte) & PTE_ADDR_MASK)
But that implies, that upstream the PFN is always contiguous, no?
--
Cheers,
David / dhildenb
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 11:08:43
On 23/01/2024 10:48, David Hildenbrand wrote:
On 23.01.24 11:34, Ryan Roberts wrote:
quoted
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/arm/include/asm/pgtable.h | 2 ++
arch/arm64/include/asm/pgtable.h | 2 ++
2 files changed, 4 insertions(+)
unsigned int nr_pages)
mte_sync_tags(pte, nr_pages);
}
+#define PFN_PTE_SHIFT PAGE_SHIFT
I think this is buggy. And so is the arm64 implementation of set_ptes(). It
works fine for 48-bit output address, but for 52-bit OAs, the high bits are not
kept contigously, so if you happen to be setting a mapping for which the
physical memory block straddles bit 48, this won't work.
Right, as soon as the PTE bits are not contiguous, this stops working, just like
set_ptes() would, which I used as orientation.
quoted
Today, only the 64K base page config can support 52 bits, and for this,
OA[51:48] are stored in PTE[15:12]. But 52 bits for 4K and 16K base pages is
coming (hopefully v6.9) and in this case OA[51:50] are stored in PTE[9:8].
Fortunately we already have helpers in arm64 to abstract this.
So I think arm64 will want to define its own pte_next_pfn():
#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));
}
I'll do a separate patch to fix the already broken arm64 set_ptes()
implementation.
Make sense.
quoted
I'm not sure if this type of problem might also apply to other arches?
I saw similar handling in the PPC implementation of set_ptes, but was not able
to convince me that it is actually required there.
pte_pfn on ppc does:
static inline unsigned long pte_pfn(pte_t pte)
{
return (pte_val(pte) & PTE_RPN_MASK) >> PTE_RPN_SHIFT;
}
But that means that the PFNs *are* contiguous.
all the ppc pfn_pte() implementations also only shift the pfn, so I think ppc is
safe to just define PFN_PTE_SHIFT. Although 2 of the 3 implementations shift by
PTE_RPN_SHIFT and the other shifts by PAGE_SIZE, so you might want to define
PFN_PTE_SHIFT separately for all 3 configs?
If high bits are used for
something else, then we might produce a garbage PTE on overflow, but that
shouldn't really matter I concluded for folio_pte_batch() purposes, we'd not
detect "belongs to this folio batch" either way.
Exactly.
Maybe it's likely cleaner to also have a custom pte_next_pfn() on ppc, I just
hope that we don't lose any other arbitrary PTE bits by doing the pte_pgprot().
I don't see the need for ppc to implement pte_next_pfn().
pte_pgprot() is not a "proper" arch interface (its only required by the core-mm
if the arch implements a certain Kconfig IIRC). For arm64, all bits that are not
pfn are pgprot, so there are no bits lost.
I guess pte_pfn() implementations should tell us if anything special needs to
happen.
Le 23/01/2024 à 11:48, David Hildenbrand a écrit :
On 23.01.24 11:34, Ryan Roberts wrote:
quoted
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/arm/include/asm/pgtable.h | 2 ++
arch/arm64/include/asm/pgtable.h | 2 ++
2 files changed, 4 insertions(+)
diff --git a/arch/arm/include/asm/pgtable.h
b/arch/arm/include/asm/pgtable.h
index d657b84b6bf70..be91e376df79e 100644
pte, unsigned int nr_pages)
mte_sync_tags(pte, nr_pages);
}
+#define PFN_PTE_SHIFT PAGE_SHIFT
I think this is buggy. And so is the arm64 implementation of
set_ptes(). It
works fine for 48-bit output address, but for 52-bit OAs, the high
bits are not
kept contigously, so if you happen to be setting a mapping for which the
physical memory block straddles bit 48, this won't work.
Right, as soon as the PTE bits are not contiguous, this stops working,
just like set_ptes() would, which I used as orientation.
quoted
Today, only the 64K base page config can support 52 bits, and for this,
OA[51:48] are stored in PTE[15:12]. But 52 bits for 4K and 16K base
pages is
coming (hopefully v6.9) and in this case OA[51:50] are stored in
PTE[9:8].
Fortunately we already have helpers in arm64 to abstract this.
So I think arm64 will want to define its own pte_next_pfn():
#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));
}
I'll do a separate patch to fix the already broken arm64 set_ptes()
implementation.
Make sense.
quoted
I'm not sure if this type of problem might also apply to other arches?
I saw similar handling in the PPC implementation of set_ptes, but was
not able to convince me that it is actually required there.
pte_pfn on ppc does:
static inline unsigned long pte_pfn(pte_t pte)
{
return (pte_val(pte) & PTE_RPN_MASK) >> PTE_RPN_SHIFT;
}
But that means that the PFNs *are* contiguous. If high bits are used for
something else, then we might produce a garbage PTE on overflow, but
that shouldn't really matter I concluded for folio_pte_batch() purposes,
we'd not detect "belongs to this folio batch" either way.
Maybe it's likely cleaner to also have a custom pte_next_pfn() on ppc, I
just hope that we don't lose any other arbitrary PTE bits by doing the
pte_pgprot().
I guess pte_pfn() implementations should tell us if anything special
needs to happen.
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/arm/include/asm/pgtable.h | 2 ++
arch/arm64/include/asm/pgtable.h | 2 ++
2 files changed, 4 insertions(+)
unsigned int nr_pages)
mte_sync_tags(pte, nr_pages);
}
+#define PFN_PTE_SHIFT PAGE_SHIFT
I think this is buggy. And so is the arm64 implementation of set_ptes(). It
works fine for 48-bit output address, but for 52-bit OAs, the high bits are not
kept contigously, so if you happen to be setting a mapping for which the
physical memory block straddles bit 48, this won't work.
Right, as soon as the PTE bits are not contiguous, this stops working, just like
set_ptes() would, which I used as orientation.
quoted
Today, only the 64K base page config can support 52 bits, and for this,
OA[51:48] are stored in PTE[15:12]. But 52 bits for 4K and 16K base pages is
coming (hopefully v6.9) and in this case OA[51:50] are stored in PTE[9:8].
Fortunately we already have helpers in arm64 to abstract this.
So I think arm64 will want to define its own pte_next_pfn():
#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));
}
I'll do a separate patch to fix the already broken arm64 set_ptes()
implementation.
Make sense.
quoted
I'm not sure if this type of problem might also apply to other arches?
I saw similar handling in the PPC implementation of set_ptes, but was not able
to convince me that it is actually required there.
pte_pfn on ppc does:
static inline unsigned long pte_pfn(pte_t pte)
{
return (pte_val(pte) & PTE_RPN_MASK) >> PTE_RPN_SHIFT;
}
But that means that the PFNs *are* contiguous.
all the ppc pfn_pte() implementations also only shift the pfn, so I think ppc is
safe to just define PFN_PTE_SHIFT. Although 2 of the 3 implementations shift by
PTE_RPN_SHIFT and the other shifts by PAGE_SIZE, so you might want to define
PFN_PTE_SHIFT separately for all 3 configs?
We have PTE_RPN_SHIFT defined for all 4 implementations, for some of
them you are right it is defined as PAGE_SHIFT, but I see no reason to
define PFN_PTE_SHIFT separately.
quoted
If high bits are used for
something else, then we might produce a garbage PTE on overflow, but that
shouldn't really matter I concluded for folio_pte_batch() purposes, we'd not
detect "belongs to this folio batch" either way.
Exactly.
quoted
Maybe it's likely cleaner to also have a custom pte_next_pfn() on ppc, I just
hope that we don't lose any other arbitrary PTE bits by doing the pte_pgprot().
I don't see the need for ppc to implement pte_next_pfn().
Agreed.
pte_pgprot() is not a "proper" arch interface (its only required by the core-mm
if the arch implements a certain Kconfig IIRC). For arm64, all bits that are not
pfn are pgprot, so there are no bits lost.
quoted
I guess pte_pfn() implementations should tell us if anything special needs to
happen.
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 11:17:13
On 23/01/2024 11:02, David Hildenbrand wrote:
On 23.01.24 11:48, David Hildenbrand wrote:
quoted
On 23.01.24 11:34, Ryan Roberts wrote:
quoted
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/arm/include/asm/pgtable.h | 2 ++
arch/arm64/include/asm/pgtable.h | 2 ++
2 files changed, 4 insertions(+)
unsigned int nr_pages)
mte_sync_tags(pte, nr_pages);
}
+#define PFN_PTE_SHIFT PAGE_SHIFT
I think this is buggy. And so is the arm64 implementation of set_ptes(). It
works fine for 48-bit output address, but for 52-bit OAs, the high bits are not
kept contigously, so if you happen to be setting a mapping for which the
physical memory block straddles bit 48, this won't work.
Right, as soon as the PTE bits are not contiguous, this stops working,
just like set_ptes() would, which I used as orientation.
quoted
Today, only the 64K base page config can support 52 bits, and for this,
OA[51:48] are stored in PTE[15:12]. But 52 bits for 4K and 16K base pages is
coming (hopefully v6.9) and in this case OA[51:50] are stored in PTE[9:8].
Fortunately we already have helpers in arm64 to abstract this.
So I think arm64 will want to define its own pte_next_pfn():
#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));
}
Digging into the details, on arm64 we have:
#define pte_pfn(pte) (__pte_to_phys(pte) >> PAGE_SHIFT)
and
#define __pte_to_phys(pte) (pte_val(pte) & PTE_ADDR_MASK)
But that implies, that upstream the PFN is always contiguous, no?
But __pte_to_phys() and __phys_to_pte_val() depend on a Kconfig. If PA bits is
52, the bits are not all contiguous:
#ifdef CONFIG_ARM64_PA_BITS_52
static inline phys_addr_t __pte_to_phys(pte_t pte)
{
return (pte_val(pte) & PTE_ADDR_LOW) |
((pte_val(pte) & PTE_ADDR_HIGH) << PTE_ADDR_HIGH_SHIFT);
}
static inline pteval_t __phys_to_pte_val(phys_addr_t phys)
{
return (phys | (phys >> PTE_ADDR_HIGH_SHIFT)) & PTE_ADDR_MASK;
}
#else
#define __pte_to_phys(pte) (pte_val(pte) & PTE_ADDR_MASK)
#define __phys_to_pte_val(phys) (phys)
#endif
From: David Hildenbrand <hidden> Date: 2024-01-23 11:31:49
quoted
quoted
If high bits are used for
something else, then we might produce a garbage PTE on overflow, but that
shouldn't really matter I concluded for folio_pte_batch() purposes, we'd not
detect "belongs to this folio batch" either way.
Exactly.
quoted
Maybe it's likely cleaner to also have a custom pte_next_pfn() on ppc, I just
hope that we don't lose any other arbitrary PTE bits by doing the pte_pgprot().
I don't see the need for ppc to implement pte_next_pfn().
Agreed.
So likely we should then do on top for powerpc (whitespace damage):
From: David Hildenbrand <hidden> Date: 2024-01-23 11:33:47
On 23.01.24 12:17, Ryan Roberts wrote:
On 23/01/2024 11:02, David Hildenbrand wrote:
quoted
On 23.01.24 11:48, David Hildenbrand wrote:
quoted
On 23.01.24 11:34, Ryan Roberts wrote:
quoted
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/arm/include/asm/pgtable.h | 2 ++
arch/arm64/include/asm/pgtable.h | 2 ++
2 files changed, 4 insertions(+)
unsigned int nr_pages)
mte_sync_tags(pte, nr_pages);
}
+#define PFN_PTE_SHIFT PAGE_SHIFT
I think this is buggy. And so is the arm64 implementation of set_ptes(). It
works fine for 48-bit output address, but for 52-bit OAs, the high bits are not
kept contigously, so if you happen to be setting a mapping for which the
physical memory block straddles bit 48, this won't work.
Right, as soon as the PTE bits are not contiguous, this stops working,
just like set_ptes() would, which I used as orientation.
quoted
Today, only the 64K base page config can support 52 bits, and for this,
OA[51:48] are stored in PTE[15:12]. But 52 bits for 4K and 16K base pages is
coming (hopefully v6.9) and in this case OA[51:50] are stored in PTE[9:8].
Fortunately we already have helpers in arm64 to abstract this.
So I think arm64 will want to define its own pte_next_pfn():
#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));
}
Digging into the details, on arm64 we have:
#define pte_pfn(pte) (__pte_to_phys(pte) >> PAGE_SHIFT)
and
#define __pte_to_phys(pte) (pte_val(pte) & PTE_ADDR_MASK)
But that implies, that upstream the PFN is always contiguous, no?
But __pte_to_phys() and __phys_to_pte_val() depend on a Kconfig. If PA bits is
52, the bits are not all contiguous:
#ifdef CONFIG_ARM64_PA_BITS_52
static inline phys_addr_t __pte_to_phys(pte_t pte)
{
return (pte_val(pte) & PTE_ADDR_LOW) |
((pte_val(pte) & PTE_ADDR_HIGH) << PTE_ADDR_HIGH_SHIFT);
}
static inline pteval_t __phys_to_pte_val(phys_addr_t phys)
{
return (phys | (phys >> PTE_ADDR_HIGH_SHIFT)) & PTE_ADDR_MASK;
}
#else
#define __pte_to_phys(pte) (pte_val(pte) & PTE_ADDR_MASK)
#define __phys_to_pte_val(phys) (phys)
#endif
Ah, how could I've missed that. Agreed, set_ptes() and this patch are
broken.
Do you want to send a patch to implement pte_next_pfn() on arm64, and
then use pte_next_pfn() in set_ptes()? Then I can drop this patch here
completely from this series.
--
Cheers,
David / dhildenb
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 11:38:37
On 23/01/2024 11:31, David Hildenbrand wrote:
quoted hunk
quoted
quoted
quoted
If high bits are used for
something else, then we might produce a garbage PTE on overflow, but that
shouldn't really matter I concluded for folio_pte_batch() purposes, we'd not
detect "belongs to this folio batch" either way.
Exactly.
quoted
Maybe it's likely cleaner to also have a custom pte_next_pfn() on ppc, I just
hope that we don't lose any other arbitrary PTE bits by doing the pte_pgprot().
I don't see the need for ppc to implement pte_next_pfn().
Agreed.
So likely we should then do on top for powerpc (whitespace damage):
Looks like commit 47b8def9358c ("powerpc/mm: Avoid calling
arch_enter/leave_lazy_mmu() in set_ptes") changed from doing the simple
increment to this more complex approach, but the log doesn't say why.
From: David Hildenbrand <hidden> Date: 2024-01-23 11:40:30
On 23.01.24 12:38, Ryan Roberts wrote:
On 23/01/2024 11:31, David Hildenbrand wrote:
quoted
quoted
quoted
quoted
If high bits are used for
something else, then we might produce a garbage PTE on overflow, but that
shouldn't really matter I concluded for folio_pte_batch() purposes, we'd not
detect "belongs to this folio batch" either way.
Exactly.
quoted
Maybe it's likely cleaner to also have a custom pte_next_pfn() on ppc, I just
hope that we don't lose any other arbitrary PTE bits by doing the pte_pgprot().
I don't see the need for ppc to implement pte_next_pfn().
Agreed.
So likely we should then do on top for powerpc (whitespace damage):
Looks like commit 47b8def9358c ("powerpc/mm: Avoid calling
arch_enter/leave_lazy_mmu() in set_ptes") changed from doing the simple
increment to this more complex approach, but the log doesn't say why.
@Aneesh, was that change on purpose?
--
Cheers,
David / dhildenb
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 11:44:41
On 23/01/2024 11:33, David Hildenbrand wrote:
On 23.01.24 12:17, Ryan Roberts wrote:
quoted
On 23/01/2024 11:02, David Hildenbrand wrote:
quoted
On 23.01.24 11:48, David Hildenbrand wrote:
quoted
On 23.01.24 11:34, Ryan Roberts wrote:
quoted
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted
We want to make use of pte_next_pfn() outside of set_ptes(). Let's
simpliy define PFN_PTE_SHIFT, required by pte_next_pfn().
Signed-off-by: David Hildenbrand <redacted>
---
arch/arm/include/asm/pgtable.h | 2 ++
arch/arm64/include/asm/pgtable.h | 2 ++
2 files changed, 4 insertions(+)
unsigned int nr_pages)
mte_sync_tags(pte, nr_pages);
}
+#define PFN_PTE_SHIFT PAGE_SHIFT
I think this is buggy. And so is the arm64 implementation of set_ptes(). It
works fine for 48-bit output address, but for 52-bit OAs, the high bits are
not
kept contigously, so if you happen to be setting a mapping for which the
physical memory block straddles bit 48, this won't work.
Right, as soon as the PTE bits are not contiguous, this stops working,
just like set_ptes() would, which I used as orientation.
quoted
Today, only the 64K base page config can support 52 bits, and for this,
OA[51:48] are stored in PTE[15:12]. But 52 bits for 4K and 16K base pages is
coming (hopefully v6.9) and in this case OA[51:50] are stored in PTE[9:8].
Fortunately we already have helpers in arm64 to abstract this.
So I think arm64 will want to define its own pte_next_pfn():
#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));
}
Digging into the details, on arm64 we have:
#define pte_pfn(pte) (__pte_to_phys(pte) >> PAGE_SHIFT)
and
#define __pte_to_phys(pte) (pte_val(pte) & PTE_ADDR_MASK)
But that implies, that upstream the PFN is always contiguous, no?
But __pte_to_phys() and __phys_to_pte_val() depend on a Kconfig. If PA bits is
52, the bits are not all contiguous:
#ifdef CONFIG_ARM64_PA_BITS_52
static inline phys_addr_t __pte_to_phys(pte_t pte)
{
return (pte_val(pte) & PTE_ADDR_LOW) |
((pte_val(pte) & PTE_ADDR_HIGH) << PTE_ADDR_HIGH_SHIFT);
}
static inline pteval_t __phys_to_pte_val(phys_addr_t phys)
{
return (phys | (phys >> PTE_ADDR_HIGH_SHIFT)) & PTE_ADDR_MASK;
}
#else
#define __pte_to_phys(pte) (pte_val(pte) & PTE_ADDR_MASK)
#define __phys_to_pte_val(phys) (phys)
#endif
Ah, how could I've missed that. Agreed, set_ptes() and this patch are broken.
Do you want to send a patch to implement pte_next_pfn() on arm64, and then use
pte_next_pfn() in set_ptes()? Then I can drop this patch here completely from
this series.
Yes good idea. I probably won't get around to it until tomorrow.
If high bits are used for
something else, then we might produce a garbage PTE on overflow, but that
shouldn't really matter I concluded for folio_pte_batch() purposes, we'd not
detect "belongs to this folio batch" either way.
Exactly.
quoted
Maybe it's likely cleaner to also have a custom pte_next_pfn() on ppc, I just
hope that we don't lose any other arbitrary PTE bits by doing the pte_pgprot().
I don't see the need for ppc to implement pte_next_pfn().
Agreed.
So likely we should then do on top for powerpc (whitespace damage):
Looks like commit 47b8def9358c ("powerpc/mm: Avoid calling
arch_enter/leave_lazy_mmu() in set_ptes") changed from doing the simple
increment to this more complex approach, but the log doesn't say why.
From: David Hildenbrand <hidden> Date: 2024-01-23 11:53:13
On 23.01.24 12:48, Christophe Leroy wrote:
Le 23/01/2024 à 12:38, Ryan Roberts a écrit :
quoted
On 23/01/2024 11:31, David Hildenbrand wrote:
quoted
quoted
quoted
quoted
If high bits are used for
something else, then we might produce a garbage PTE on overflow, but that
shouldn't really matter I concluded for folio_pte_batch() purposes, we'd not
detect "belongs to this folio batch" either way.
Exactly.
quoted
Maybe it's likely cleaner to also have a custom pte_next_pfn() on ppc, I just
hope that we don't lose any other arbitrary PTE bits by doing the pte_pgprot().
I don't see the need for ppc to implement pte_next_pfn().
Agreed.
So likely we should then do on top for powerpc (whitespace damage):
Looks like commit 47b8def9358c ("powerpc/mm: Avoid calling
arch_enter/leave_lazy_mmu() in set_ptes") changed from doing the simple
increment to this more complex approach, but the log doesn't say why.
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 12:01:15
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted hunk
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 some other
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.
Signed-off-by: David Hildenbrand <redacted>
---
include/linux/pgtable.h | 17 +++++-
mm/memory.c | 113 +++++++++++++++++++++++++++++++++-------
2 files changed, 109 insertions(+), 21 deletions(-)
I wrote some documentation for this (based on Matthew's docs for set_ptes() in
my version. Perhaps it makes sense to add it here, given this is overridable by
the arch.
/**
* wrprotect_ptes - Write protect a consecutive set of pages.
* @mm: Address space that the pages are mapped into.
* @addr: Address of first page to write protect.
* @ptep: Page table pointer for the first entry.
* @nr: Number of pages to write protect.
*
* May be overridden by the architecture, else implemented as a loop over
* ptep_set_wrprotect().
*
* Context: The caller holds the page table lock. The PTEs are all in the same
* PMD.
*/
quoted hunk
+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
nit: doesn't the addition of __always_inline really belong in the patch where
you factored this out? (#7)
quoted hunk
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,94 @@ 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 stays 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);+ if (folio_test_anon(folio)) {+ folio_ref_add(folio, nr);+ if (unlikely(folio_try_dup_anon_rmap_ptes(folio, page,+ nr, src_vma))) {
What happens if its not the first page of the batch that fails here? Aren't you
signalling that you need a prealloc'ed page for the wrong pte? Shouldn't you
still batch copy all the way up to the failing page first? Perhaps it all comes
out in the wash and these events are so infrequent that we don't care about the
lost batching opportunity?
@@ -1031,10 +1100,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;
@@ -1065,6 +1135,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.
@@ -1101,9 +1173,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.
From: David Hildenbrand <hidden> Date: 2024-01-23 12:19:40
[...]
I wrote some documentation for this (based on Matthew's docs for set_ptes() in
my version. Perhaps it makes sense to add it here, given this is overridable by
the arch.
/**
* wrprotect_ptes - Write protect a consecutive set of pages.
* @mm: Address space that the pages are mapped into.
* @addr: Address of first page to write protect.
* @ptep: Page table pointer for the first entry.
* @nr: Number of pages to write protect.
*
* May be overridden by the architecture, else implemented as a loop over
* ptep_set_wrprotect().
*
* Context: The caller holds the page table lock. The PTEs are all in the same
* PMD.
*/
I could have sworn I had a documentation at some point. Let me add some,
thanks.
[...]
quoted
+
+ /*
+ * If we likely have to copy, just don't bother with batching. Make
+ * sure that the common "small folio" case stays 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);
+ if (folio_test_anon(folio)) {
+ folio_ref_add(folio, nr);
+ if (unlikely(folio_try_dup_anon_rmap_ptes(folio, page,
+ nr, src_vma))) {
What happens if its not the first page of the batch that fails here? Aren't you
signalling that you need a prealloc'ed page for the wrong pte? Shouldn't you
still batch copy all the way up to the failing page first? Perhaps it all comes
out in the wash and these events are so infrequent that we don't care about the
lost batching opportunity?
I assume you mean the weird corner case that some folio pages in the
range have PAE set, others don't -- and the folio maybe pinned.
In that case, we fallback to individual pages, and might have
preallocated a page although we wouldn't have to preallocate one for
processing the next page (that doesn't have PAE set).
It should all work, although not optimized to the extreme, and as it's a
corner case, we don't particularly care. Hopefully, in the future we'll
only have a single PAE flag per folio.
Or am I missing something?
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 12:25:11
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted hunk
Let's ignore these bits: they are irrelevant for fork, and will likely
be irrelevant for upcoming users such as page unmapping.
Signed-off-by: David Hildenbrand <redacted>
---
mm/memory.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
nit: last char should be a comma (,) not a full stop (.)
+ * the accessed bit, dirty bit and soft-dirty bit.
*/
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 expected_pte = __pte_batch_clear_ignored(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);
+ pte = __pte_batch_clear_ignored(ptep_get(ptep));
if (!pte_same(pte, expected_pte))
break;
I think you'll lose dirty information in the child for private mappings? If the
first pte in a batch is clean, but a subsequent page is dirty, you will end up
setting all the pages in the batch as clean in the child. Previous behavior
would preserve dirty bit for private mappings.
In my version (v3) that did arbitrary batching, I had some fun and games
tracking dirty, write and uffd_wp:
https://lore.kernel.org/linux-arm-kernel/20231204105440.61448-2-ryan.roberts@arm.com/
Also, I think you will currently either set soft dirty on all or none of the
pages in the batch, depending on the value of the first. I previously convinced
myself that the state was unimportant so always cleared it in the child to
provide consistency.
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 12:28:30
On 23/01/2024 12:19, David Hildenbrand wrote:
[...]
quoted
I wrote some documentation for this (based on Matthew's docs for set_ptes() in
my version. Perhaps it makes sense to add it here, given this is overridable by
the arch.
/**
* wrprotect_ptes - Write protect a consecutive set of pages.
* @mm: Address space that the pages are mapped into.
* @addr: Address of first page to write protect.
* @ptep: Page table pointer for the first entry.
* @nr: Number of pages to write protect.
*
* May be overridden by the architecture, else implemented as a loop over
* ptep_set_wrprotect().
*
* Context: The caller holds the page table lock. The PTEs are all in the same
* PMD.
*/
I could have sworn I had a documentation at some point. Let me add some, thanks.
[...]
quoted
quoted
+
+ /*
+ * If we likely have to copy, just don't bother with batching. Make
+ * sure that the common "small folio" case stays 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);
+ if (folio_test_anon(folio)) {
+ folio_ref_add(folio, nr);
+ if (unlikely(folio_try_dup_anon_rmap_ptes(folio, page,
+ nr, src_vma))) {
What happens if its not the first page of the batch that fails here? Aren't you
signalling that you need a prealloc'ed page for the wrong pte? Shouldn't you
still batch copy all the way up to the failing page first? Perhaps it all comes
out in the wash and these events are so infrequent that we don't care about the
lost batching opportunity?
I assume you mean the weird corner case that some folio pages in the range have
PAE set, others don't -- and the folio maybe pinned.
In that case, we fallback to individual pages, and might have preallocated a
page although we wouldn't have to preallocate one for processing the next page
(that doesn't have PAE set).
It should all work, although not optimized to the extreme, and as it's a corner
case, we don't particularly care. Hopefully, in the future we'll only have a
single PAE flag per folio.
Or am I missing something?
No, your explanation makes sense. Just wanted to check this all definitely
worked, because the flow is slightly different to my previous version that was
doing try_dup_rmap page-by-page.
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 12:35:13
On 22/01/2024 19:42, David Hildenbrand wrote:
... 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) won't care.
Signed-off-by: David Hildenbrand <redacted>
From: David Hildenbrand <hidden> Date: 2024-01-23 13:06:23
On 23.01.24 13:25, Ryan Roberts wrote:
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted
Let's ignore these bits: they are irrelevant for fork, and will likely
be irrelevant for upcoming users such as page unmapping.
Signed-off-by: David Hildenbrand <redacted>
---
mm/memory.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
nit: last char should be a comma (,) not a full stop (.)
quoted
+ * the accessed bit, dirty bit and soft-dirty bit.
*/
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 expected_pte = __pte_batch_clear_ignored(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);
+ pte = __pte_batch_clear_ignored(ptep_get(ptep));
if (!pte_same(pte, expected_pte))
break;
I think you'll lose dirty information in the child for private mappings? If the
first pte in a batch is clean, but a subsequent page is dirty, you will end up
setting all the pages in the batch as clean in the child. Previous behavior
would preserve dirty bit for private mappings.
In my version (v3) that did arbitrary batching, I had some fun and games
tracking dirty, write and uffd_wp:
https://lore.kernel.org/linux-arm-kernel/20231204105440.61448-2-ryan.roberts@arm.com/
Also, I think you will currently either set soft dirty on all or none of the
pages in the batch, depending on the value of the first. I previously convinced
myself that the state was unimportant so always cleared it in the child to
provide consistency.
Good points regarding dirty and soft-dirty. I wanted to avoid passing
flags to folio_pte_batch(), but maybe that's just what we need to not
change behavior.
--
Cheers,
David / dhildenb
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 13:42:35
On 23/01/2024 13:06, David Hildenbrand wrote:
On 23.01.24 13:25, Ryan Roberts wrote:
quoted
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted
Let's ignore these bits: they are irrelevant for fork, and will likely
be irrelevant for upcoming users such as page unmapping.
Signed-off-by: David Hildenbrand <redacted>
---
mm/memory.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
vm_area_struct *dst_vma,
set_ptes(dst_vma->vm_mm, addr, dst_pte, pte, nr);
}
+static inline pte_t __pte_batch_clear_ignored(pte_t pte)
+{
+ return pte_clear_soft_dirty(pte_mkclean(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.
nit: last char should be a comma (,) not a full stop (.)
quoted
+ * the accessed bit, dirty bit and soft-dirty bit.
*/
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 expected_pte = __pte_batch_clear_ignored(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);
+ pte = __pte_batch_clear_ignored(ptep_get(ptep));
if (!pte_same(pte, expected_pte))
break;
I think you'll lose dirty information in the child for private mappings? If the
first pte in a batch is clean, but a subsequent page is dirty, you will end up
setting all the pages in the batch as clean in the child. Previous behavior
would preserve dirty bit for private mappings.
In my version (v3) that did arbitrary batching, I had some fun and games
tracking dirty, write and uffd_wp:
https://lore.kernel.org/linux-arm-kernel/20231204105440.61448-2-ryan.roberts@arm.com/
Also, I think you will currently either set soft dirty on all or none of the
pages in the batch, depending on the value of the first. I previously convinced
myself that the state was unimportant so always cleared it in the child to
provide consistency.
Good points regarding dirty and soft-dirty. I wanted to avoid passing flags to
folio_pte_batch(), but maybe that's just what we need to not change behavior.
I think you could not bother with the enforce_uffd_wp - just always enforce
uffd-wp. So that's one simplification vs mine. Then you just need an any_dirty
flag following the same pattern as your any_writable. Then just set dirty on the
whole batch in the child if any were dirty in the parent.
Although now I'm wondering if there is a race here... What happens if a page in
the parent becomes dirty after you have checked it but before you write protect
it? Isn't that already a problem with the current non-batched version? Why do we
even to preserve dirty in the child for private mappings?
From: David Hildenbrand <hidden> Date: 2024-01-23 13:55:12
On 23.01.24 14:42, Ryan Roberts wrote:
On 23/01/2024 13:06, David Hildenbrand wrote:
quoted
On 23.01.24 13:25, Ryan Roberts wrote:
quoted
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted
Let's ignore these bits: they are irrelevant for fork, and will likely
be irrelevant for upcoming users such as page unmapping.
Signed-off-by: David Hildenbrand <redacted>
---
mm/memory.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
vm_area_struct *dst_vma,
set_ptes(dst_vma->vm_mm, addr, dst_pte, pte, nr);
}
+static inline pte_t __pte_batch_clear_ignored(pte_t pte)
+{
+ return pte_clear_soft_dirty(pte_mkclean(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.
nit: last char should be a comma (,) not a full stop (.)
quoted
+ * the accessed bit, dirty bit and soft-dirty bit.
*/
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 expected_pte = __pte_batch_clear_ignored(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);
+ pte = __pte_batch_clear_ignored(ptep_get(ptep));
if (!pte_same(pte, expected_pte))
break;
I think you'll lose dirty information in the child for private mappings? If the
first pte in a batch is clean, but a subsequent page is dirty, you will end up
setting all the pages in the batch as clean in the child. Previous behavior
would preserve dirty bit for private mappings.
In my version (v3) that did arbitrary batching, I had some fun and games
tracking dirty, write and uffd_wp:
https://lore.kernel.org/linux-arm-kernel/20231204105440.61448-2-ryan.roberts@arm.com/
Also, I think you will currently either set soft dirty on all or none of the
pages in the batch, depending on the value of the first. I previously convinced
myself that the state was unimportant so always cleared it in the child to
provide consistency.
Good points regarding dirty and soft-dirty. I wanted to avoid passing flags to
folio_pte_batch(), but maybe that's just what we need to not change behavior.
I think you could not bother with the enforce_uffd_wp - just always enforce
uffd-wp. So that's one simplification vs mine. Then you just need an any_dirty
I think I'll just leave uffd-wp alone for now, corner case with
fork/munmap that can be optimized later on top if really needed.
Regarding soft-dirty (which is set automatically much more often), I can
certainly ignore the bit if !vma_soft_dirty_enabled(vma) [which is true
in most of the cases]. So that's easy to handle. But likely, soft-dirty
for the child is completely unexpressive and should always be cleared.
Have to double check what the vmflag will be for the child process.
flag following the same pattern as your any_writable. Then just set dirty on the
whole batch in the child if any were dirty in the parent.
Regarding dirtying, I'm not 100% sure yet if we should just always dirty
all ptes if any is dirty, or if we should preserve the state for private
VMAs for now.
Although now I'm wondering if there is a race here... What happens if a page in
the parent becomes dirty after you have checked it but before you write protect
it? Isn't that already a problem with the current non-batched version? Why do we
even to preserve dirty in the child for private mappings?
I suspect, because the parent could zap the anon folio. If the folio is
clean, but the PTE dirty, I suspect that we could lose data of the child
if we were to evict that clean folio (swapout).
So I assume we simply copy the dirty PTE bit, so the system knows that
that folio is actually dirty, because one PTE is dirty.
Touching only PTEs avoids having to mess with folio flags.
But that's just pure speculation. E.g., fs/proc/task_mmu.c does some
slightly different accounting if a PTE is dirty. But usually, it checks
if either the PTE or the folios is dirty.
I'll have to do some more digging.
--
Cheers,
David / dhildenb
From: David Hildenbrand <hidden> Date: 2024-01-23 14:13:41
quoted
Although now I'm wondering if there is a race here... What happens if a page in
the parent becomes dirty after you have checked it but before you write protect
it? Isn't that already a problem with the current non-batched version? Why do we
even to preserve dirty in the child for private mappings?
I suspect, because the parent could zap the anon folio. If the folio is
clean, but the PTE dirty, I suspect that we could lose data of the child
if we were to evict that clean folio (swapout).
So I assume we simply copy the dirty PTE bit, so the system knows that
that folio is actually dirty, because one PTE is dirty.
Oh, and regarding your race concern: it's undefined which page state
would see if some write is racing with fork, so it also doesn't matter
if we would copy the PTE dirty bit or not, if it gets set in a racy fashion.
I'll not experiment with:
From 14e83ff2a422a96ce5701f9c8454a49f9ed947e3 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <redacted>
Date: Sat, 30 Dec 2023 12:54:35 +0100
Subject: [PATCH] mm/memory: ignore dirty/accessed/soft-dirty bits in
folio_pte_batch()
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.
Ignore the soft-dirty bit only if the bit doesn't have any meaning in
the src vma.
Signed-off-by: David Hildenbrand <redacted>
---
mm/memory.c | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 14:28:00
On 23/01/2024 14:13, David Hildenbrand wrote:
quoted
quoted
Although now I'm wondering if there is a race here... What happens if a page in
the parent becomes dirty after you have checked it but before you write protect
it? Isn't that already a problem with the current non-batched version? Why do we
even to preserve dirty in the child for private mappings?
I suspect, because the parent could zap the anon folio. If the folio is
clean, but the PTE dirty, I suspect that we could lose data of the child
if we were to evict that clean folio (swapout).
So I assume we simply copy the dirty PTE bit, so the system knows that
that folio is actually dirty, because one PTE is dirty.
Oh, and regarding your race concern: it's undefined which page state
would see if some write is racing with fork, so it also doesn't matter
if we would copy the PTE dirty bit or not, if it gets set in a racy fashion.
Ahh that makes sense. Thanks.
I'll not experiment with:
Looks good as long as its still performant.
quoted hunk
From 14e83ff2a422a96ce5701f9c8454a49f9ed947e3 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <redacted>
Date: Sat, 30 Dec 2023 12:54:35 +0100
Subject: [PATCH] mm/memory: ignore dirty/accessed/soft-dirty bits in
folio_pte_batch()
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.
Ignore the soft-dirty bit only if the bit doesn't have any meaning in
the src vma.
Signed-off-by: David Hildenbrand <redacted>
---
mm/memory.c | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
From: Matthew Wilcox <willy@infradead.org> Date: 2024-01-23 15:01:41
On Tue, Jan 23, 2024 at 10:34:21AM +0000, Ryan Roberts wrote:
quoted
+#define PFN_PTE_SHIFT PAGE_SHIFT
I think this is buggy. And so is the arm64 implementation of set_ptes(). It
works fine for 48-bit output address, but for 52-bit OAs, the high bits are not
kept contigously, so if you happen to be setting a mapping for which the
physical memory block straddles bit 48, this won't work.
I'd like to see the folio allocation that can straddle bit 48 ...
agreed, it's not workable _in general_, but specifically for a memory
allocation from a power-of-two allocator, you'd have to do a 49-bit
allocation (half a petabyte) to care.
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 15:22:06
On 23/01/2024 15:01, Matthew Wilcox wrote:
On Tue, Jan 23, 2024 at 10:34:21AM +0000, Ryan Roberts wrote:
quoted
quoted
+#define PFN_PTE_SHIFT PAGE_SHIFT
I think this is buggy. And so is the arm64 implementation of set_ptes(). It
works fine for 48-bit output address, but for 52-bit OAs, the high bits are not
kept contigously, so if you happen to be setting a mapping for which the
physical memory block straddles bit 48, this won't work.
I'd like to see the folio allocation that can straddle bit 48 ...
agreed, it's not workable _in general_, but specifically for a memory
allocation from a power-of-two allocator, you'd have to do a 49-bit
allocation (half a petabyte) to care.
Hmm good point. So its a hypothetical bug, not an actual bug. Personally I'm
still inclined to "fix" it. Although its going to cost a few more instructions.
Shout if you disagree.
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 19:15:44
On 22/01/2024 19:41, 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.
I'm currently rebasing my contpte work onto this series, and have hit a problem.
I need to expose the "size" of a pte (pte_size()) and skip forward to the start
of the next (cont)pte every time through the folio_pte_batch() loop. But
pte_next_pfn() only allows advancing by 1 pfn; I need to advance by nr pfns:
static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
pte_t *start_ptep, pte_t pte, int max_nr, 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));
- pte_t *ptep = start_ptep + 1;
+ pte_t *ptep = start_ptep;
+ int vfn, nr, i;
bool writable;
if (any_writable)
*any_writable = false;
VM_WARN_ON_FOLIO(!pte_present(pte), folio);
+ vfn = addr >> PAGE_SIZE;
+ nr = pte_size(pte);
+ nr = ALIGN_DOWN(vfn + nr, nr) - vfn;
+ ptep += nr;
+
while (ptep != end_ptep) {
+ pte = ptep_get(ptep);
nr = pte_size(pte);
if (any_writable)
writable = !!pte_write(pte);
pte = __pte_batch_clear_ignored(pte);
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)
+ if (pte_pfn(pte) >= folio_end_pfn)
break;
if (any_writable)
*any_writable |= writable;
- expected_pte = pte_next_pfn(expected_pte);
- ptep++;
+ for (i = 0; i < nr; i++)
+ expected_pte = pte_next_pfn(expected_pte);
+ ptep += nr;
}
return ptep - start_ptep;
}
So I'm wondering if instead of enabling pte_next_pfn() for all the arches,
perhaps its actually better to expose pte_pgprot() for all the arches. Then we
can be much more flexible about generating ptes with pfn_pte(pfn, pgprot).
What do you think?
From: David Hildenbrand <hidden> Date: 2024-01-23 19:33:15
On 23.01.24 20:15, Ryan Roberts wrote:
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted
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.
I'm currently rebasing my contpte work onto this series, and have hit a problem.
I need to expose the "size" of a pte (pte_size()) and skip forward to the start
of the next (cont)pte every time through the folio_pte_batch() loop. But
pte_next_pfn() only allows advancing by 1 pfn; I need to advance by nr pfns:
static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
pte_t *start_ptep, pte_t pte, int max_nr, 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));
- pte_t *ptep = start_ptep + 1;
+ pte_t *ptep = start_ptep;
+ int vfn, nr, i;
bool writable;
if (any_writable)
*any_writable = false;
VM_WARN_ON_FOLIO(!pte_present(pte), folio);
+ vfn = addr >> PAGE_SIZE;
+ nr = pte_size(pte);
+ nr = ALIGN_DOWN(vfn + nr, nr) - vfn;
+ ptep += nr;
+
while (ptep != end_ptep) {
+ pte = ptep_get(ptep);
nr = pte_size(pte);
if (any_writable)
writable = !!pte_write(pte);
pte = __pte_batch_clear_ignored(pte);
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)
+ if (pte_pfn(pte) >= folio_end_pfn)
break;
if (any_writable)
*any_writable |= writable;
- expected_pte = pte_next_pfn(expected_pte);
- ptep++;
+ for (i = 0; i < nr; i++)
+ expected_pte = pte_next_pfn(expected_pte);
+ ptep += nr;
}
return ptep - start_ptep;
}
So I'm wondering if instead of enabling pte_next_pfn() for all the arches,
perhaps its actually better to expose pte_pgprot() for all the arches. Then we
can be much more flexible about generating ptes with pfn_pte(pfn, pgprot).
What do you think?
The pte_pgprot() stuff is just nasty IMHO.
Likely it's best to simply convert pte_next_pfn() to something like
pte_advance_pfns(). The we could just have
#define pte_next_pfn(pte) pte_advance_pfns(pte, 1)
That should be fairly easy to do on top (based on PFN_PTE_SHIFT). And
only 3 archs (x86-64, arm64, and powerpc) need slight care to replace a
hardcoded "1" by an integer we pass in.
--
Cheers,
David / dhildenb
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 19:43:10
On 23/01/2024 19:33, David Hildenbrand wrote:
On 23.01.24 20:15, Ryan Roberts wrote:
quoted
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted
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.
I'm currently rebasing my contpte work onto this series, and have hit a problem.
I need to expose the "size" of a pte (pte_size()) and skip forward to the start
of the next (cont)pte every time through the folio_pte_batch() loop. But
pte_next_pfn() only allows advancing by 1 pfn; I need to advance by nr pfns:
static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
pte_t *start_ptep, pte_t pte, int max_nr, 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));
- pte_t *ptep = start_ptep + 1;
+ pte_t *ptep = start_ptep;
+ int vfn, nr, i;
bool writable;
if (any_writable)
*any_writable = false;
VM_WARN_ON_FOLIO(!pte_present(pte), folio);
+ vfn = addr >> PAGE_SIZE;
+ nr = pte_size(pte);
+ nr = ALIGN_DOWN(vfn + nr, nr) - vfn;
+ ptep += nr;
+
while (ptep != end_ptep) {
+ pte = ptep_get(ptep);
nr = pte_size(pte);
if (any_writable)
writable = !!pte_write(pte);
pte = __pte_batch_clear_ignored(pte);
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)
+ if (pte_pfn(pte) >= folio_end_pfn)
break;
if (any_writable)
*any_writable |= writable;
- expected_pte = pte_next_pfn(expected_pte);
- ptep++;
+ for (i = 0; i < nr; i++)
+ expected_pte = pte_next_pfn(expected_pte);
+ ptep += nr;
}
return ptep - start_ptep;
}
So I'm wondering if instead of enabling pte_next_pfn() for all the arches,
perhaps its actually better to expose pte_pgprot() for all the arches. Then we
can be much more flexible about generating ptes with pfn_pte(pfn, pgprot).
What do you think?
The pte_pgprot() stuff is just nasty IMHO.
I dunno; we have pfn_pte() which takes a pfn and a pgprot. It seems reasonable
that we should be able to do the reverse.
Likely it's best to simply convert pte_next_pfn() to something like
pte_advance_pfns(). The we could just have
#define pte_next_pfn(pte) pte_advance_pfns(pte, 1)
That should be fairly easy to do on top (based on PFN_PTE_SHIFT). And only 3
archs (x86-64, arm64, and powerpc) need slight care to replace a hardcoded "1"
by an integer we pass in.
I thought we agreed powerpc was safe to just define PFN_PTE_SHIFT? But, yeah,
the principle works I guess. I guess I can do this change along with my series.
From: David Hildenbrand <hidden> Date: 2024-01-23 20:14:57
On 23.01.24 20:43, Ryan Roberts wrote:
On 23/01/2024 19:33, David Hildenbrand wrote:
quoted
On 23.01.24 20:15, Ryan Roberts wrote:
quoted
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted
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.
I'm currently rebasing my contpte work onto this series, and have hit a problem.
I need to expose the "size" of a pte (pte_size()) and skip forward to the start
of the next (cont)pte every time through the folio_pte_batch() loop. But
pte_next_pfn() only allows advancing by 1 pfn; I need to advance by nr pfns:
static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
pte_t *start_ptep, pte_t pte, int max_nr, 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));
- pte_t *ptep = start_ptep + 1;
+ pte_t *ptep = start_ptep;
+ int vfn, nr, i;
bool writable;
if (any_writable)
*any_writable = false;
VM_WARN_ON_FOLIO(!pte_present(pte), folio);
+ vfn = addr >> PAGE_SIZE;
+ nr = pte_size(pte);
+ nr = ALIGN_DOWN(vfn + nr, nr) - vfn;
+ ptep += nr;
+
while (ptep != end_ptep) {
+ pte = ptep_get(ptep);
nr = pte_size(pte);
if (any_writable)
writable = !!pte_write(pte);
pte = __pte_batch_clear_ignored(pte);
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)
+ if (pte_pfn(pte) >= folio_end_pfn)
break;
if (any_writable)
*any_writable |= writable;
- expected_pte = pte_next_pfn(expected_pte);
- ptep++;
+ for (i = 0; i < nr; i++)
+ expected_pte = pte_next_pfn(expected_pte);
+ ptep += nr;
}
return ptep - start_ptep;
}
So I'm wondering if instead of enabling pte_next_pfn() for all the arches,
perhaps its actually better to expose pte_pgprot() for all the arches. Then we
can be much more flexible about generating ptes with pfn_pte(pfn, pgprot).
What do you think?
The pte_pgprot() stuff is just nasty IMHO.
I dunno; we have pfn_pte() which takes a pfn and a pgprot. It seems reasonable
that we should be able to do the reverse.
But pte_pgprot() is only available on a handful of architectures, no? It
would be nice to have a completely generic pte_next_pfn() /
pte_advance_pfns(), though.
Anyhow, this is all "easy" to rework later. Unless I am missing
something, the low hanging fruit is simply using PFN_PTE_SHIFT for now
that exists on most archs already.
quoted
Likely it's best to simply convert pte_next_pfn() to something like
pte_advance_pfns(). The we could just have
#define pte_next_pfn(pte) pte_advance_pfns(pte, 1)
That should be fairly easy to do on top (based on PFN_PTE_SHIFT). And only 3
archs (x86-64, arm64, and powerpc) need slight care to replace a hardcoded "1"
by an integer we pass in.
I thought we agreed powerpc was safe to just define PFN_PTE_SHIFT? But, yeah,
the principle works I guess. I guess I can do this change along with my series.
It is, if nobody insists on that micro-optimization on powerpc.
If there is good reason to invest more time and effort right now on the
pte_pgprot approach, then please let me know :)
--
Cheers,
David / dhildenb
From: Ryan Roberts <ryan.roberts@arm.com> Date: 2024-01-23 20:43:59
On 23/01/2024 20:14, David Hildenbrand wrote:
On 23.01.24 20:43, Ryan Roberts wrote:
quoted
On 23/01/2024 19:33, David Hildenbrand wrote:
quoted
On 23.01.24 20:15, Ryan Roberts wrote:
quoted
On 22/01/2024 19:41, David Hildenbrand wrote:
quoted
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.
I'm currently rebasing my contpte work onto this series, and have hit a
problem.
I need to expose the "size" of a pte (pte_size()) and skip forward to the start
of the next (cont)pte every time through the folio_pte_batch() loop. But
pte_next_pfn() only allows advancing by 1 pfn; I need to advance by nr pfns:
static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
pte_t *start_ptep, pte_t pte, int max_nr, 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));
- pte_t *ptep = start_ptep + 1;
+ pte_t *ptep = start_ptep;
+ int vfn, nr, i;
bool writable;
if (any_writable)
*any_writable = false;
VM_WARN_ON_FOLIO(!pte_present(pte), folio);
+ vfn = addr >> PAGE_SIZE;
+ nr = pte_size(pte);
+ nr = ALIGN_DOWN(vfn + nr, nr) - vfn;
+ ptep += nr;
+
while (ptep != end_ptep) {
+ pte = ptep_get(ptep);
nr = pte_size(pte);
if (any_writable)
writable = !!pte_write(pte);
pte = __pte_batch_clear_ignored(pte);
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)
+ if (pte_pfn(pte) >= folio_end_pfn)
break;
if (any_writable)
*any_writable |= writable;
- expected_pte = pte_next_pfn(expected_pte);
- ptep++;
+ for (i = 0; i < nr; i++)
+ expected_pte = pte_next_pfn(expected_pte);
+ ptep += nr;
}
return ptep - start_ptep;
}
So I'm wondering if instead of enabling pte_next_pfn() for all the arches,
perhaps its actually better to expose pte_pgprot() for all the arches. Then we
can be much more flexible about generating ptes with pfn_pte(pfn, pgprot).
What do you think?
The pte_pgprot() stuff is just nasty IMHO.
I dunno; we have pfn_pte() which takes a pfn and a pgprot. It seems reasonable
that we should be able to do the reverse.
But pte_pgprot() is only available on a handful of architectures, no? It would
be nice to have a completely generic pte_next_pfn() / pte_advance_pfns(), though.
Anyhow, this is all "easy" to rework later. Unless I am missing something, the
low hanging fruit is simply using PFN_PTE_SHIFT for now that exists on most
archs already.
quoted
quoted
Likely it's best to simply convert pte_next_pfn() to something like
pte_advance_pfns(). The we could just have
#define pte_next_pfn(pte) pte_advance_pfns(pte, 1)
That should be fairly easy to do on top (based on PFN_PTE_SHIFT). And only 3
archs (x86-64, arm64, and powerpc) need slight care to replace a hardcoded "1"
by an integer we pass in.
I thought we agreed powerpc was safe to just define PFN_PTE_SHIFT? But, yeah,
the principle works I guess. I guess I can do this change along with my series.
It is, if nobody insists on that micro-optimization on powerpc.
If there is good reason to invest more time and effort right now on the
pte_pgprot approach, then please let me know :)
No I think you're right. I thought pte_pgprot() was implemented by more arches,
but there are 13 without it, so clearly a lot of effort to plug that gap. I'll
take the approach you suggest with pte_advance_pfns(). It'll just require mods
to x86 and arm64, +/- ppc.
If high bits are used for
something else, then we might produce a garbage PTE on overflow, but that
shouldn't really matter I concluded for folio_pte_batch() purposes, we'd not
detect "belongs to this folio batch" either way.
Exactly.
quoted
Maybe it's likely cleaner to also have a custom pte_next_pfn() on ppc, I just
hope that we don't lose any other arbitrary PTE bits by doing the pte_pgprot().
I don't see the need for ppc to implement pte_next_pfn().
Agreed.
So likely we should then do on top for powerpc (whitespace damage):
Looks like commit 47b8def9358c ("powerpc/mm: Avoid calling
arch_enter/leave_lazy_mmu() in set_ptes") changed from doing the simple
increment to this more complex approach, but the log doesn't say why.
@Aneesh, was that change on purpose?
Because we had a bug with the patch that introduced the change and that
line was confusing. The right thing should have been to add
pte_pfn_next() to make it clear. It was confusing because not all pte
format had pfn at PAGE_SHIFT offset (even though we did use the correct
PTE_RPN_SHIFT in this specific case). To make it simpler I ended up
switching that line to pte_pfn(pte) + 1 .
-aneesh
If high bits are used for
something else, then we might produce a garbage PTE on overflow, but that
shouldn't really matter I concluded for folio_pte_batch() purposes, we'd not
detect "belongs to this folio batch" either way.
Exactly.
quoted
Maybe it's likely cleaner to also have a custom pte_next_pfn() on ppc, I just
hope that we don't lose any other arbitrary PTE bits by doing the pte_pgprot().
I don't see the need for ppc to implement pte_next_pfn().
Agreed.
So likely we should then do on top for powerpc (whitespace damage):