Re: [PATCH v3 01/15] mm: Batch-copy PTE ranges during fork()
From: David Hildenbrand
Date: Mon Dec 04 2023 - 11:00:19 EST
On 04.12.23 16:47, David Hildenbrand wrote:
On 04.12.23 11:54, Ryan Roberts wrote:
Convert copy_pte_range() to copy a set of ptes in a batch. A given batch
maps a physically contiguous block of memory, all belonging to the same
folio. This will likely improve performance by a tiny amount due to
batching the folio reference count management and calling set_ptes()
rather than making individual calls to set_pte_at().
However, the primary motivation for this change is to reduce the number
of tlb maintenance operations that the arm64 backend has to perform
during fork, as it is about to add transparent support for the
"contiguous bit" in its ptes. By write-protecting the parent using the
new ptep_set_wrprotects() (note the 's' at the end) function, the
backend can avoid having to unfold contig ranges of PTEs, which is
expensive, when all ptes in the range are being write-protected.
Similarly, by using set_ptes() rather than set_pte_at() to set up ptes
in the child, the backend does not need to fold a contiguous range once
they are all populated - they can be initially populated as a contiguous
range in the first place.
This change addresses the core-mm refactoring only, and introduces
ptep_set_wrprotects() with a default implementation that calls
ptep_set_wrprotect() for each pte in the range. A separate change will
implement ptep_set_wrprotects() in the arm64 backend to realize the
performance improvement as part of the work to enable contpte mappings.
Signed-off-by: Ryan Roberts <ryan.roberts@xxxxxxx>
---
include/linux/pgtable.h | 13 +++
mm/memory.c | 195 ++++++++++++++++++++++++++++++----------
2 files changed, 162 insertions(+), 46 deletions(-)
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index af7639c3b0a3..1c50f8a0fdde 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -622,6 +622,19 @@ static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addres
}
#endif
+#ifndef ptep_set_wrprotects
+struct mm_struct;
+static inline void ptep_set_wrprotects(struct mm_struct *mm,
+ unsigned long address, pte_t *ptep,
+ unsigned int nr)
+{
+ unsigned int i;
+
+ for (i = 0; i < nr; i++, address += PAGE_SIZE, ptep++)
+ ptep_set_wrprotect(mm, address, ptep);
+}
+#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 1f18ed4a5497..8a87a488950c 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -924,68 +924,162 @@ copy_present_page(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
return 0;
}
+static int folio_nr_pages_cont_mapped(struct folio *folio,
+ struct page *page, pte_t *pte,
+ unsigned long addr, unsigned long end,
+ pte_t ptent, bool enforce_uffd_wp,
+ int *dirty_nr, int *writable_nr)
+{
+ int floops;
+ int i;
+ unsigned long pfn;
+ bool prot_none;
+ bool uffd_wp;
+
+ if (!folio_test_large(folio))
+ return 1;
+
+ /*
+ * Loop either to `end` or to end of folio if its contiguously mapped,
+ * whichever is smaller.
+ */
+ floops = (end - addr) >> PAGE_SHIFT;
+ floops = min_t(int, floops,
+ folio_pfn(folio_next(folio)) - page_to_pfn(page));
+
+ pfn = page_to_pfn(page);
+ prot_none = pte_protnone(ptent);
+ uffd_wp = pte_uffd_wp(ptent);
+
+ *dirty_nr = !!pte_dirty(ptent);
+ *writable_nr = !!pte_write(ptent);
+
+ pfn++;
+ pte++;
+
+ for (i = 1; i < floops; i++) {
+ ptent = ptep_get(pte);
+
+ if (!pte_present(ptent) || pte_pfn(ptent) != pfn ||
+ prot_none != pte_protnone(ptent) ||
+ (enforce_uffd_wp && uffd_wp != pte_uffd_wp(ptent)))
+ break;
+
+ if (pte_dirty(ptent))
+ (*dirty_nr)++;
+ if (pte_write(ptent))
+ (*writable_nr)++;
+
+ pfn++;
+ pte++;
+ }
+
+ return i;
+}
+
/*
- * Copy one pte. Returns 0 if succeeded, or -EAGAIN if one preallocated page
- * is required to copy this pte.
+ * Copy set of contiguous ptes. Returns number of ptes copied if succeeded
+ * (always gte 1), or -EAGAIN if one preallocated page is required to copy the
+ * first pte.
*/
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)
+copy_present_ptes(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma,
+ pte_t *dst_pte, pte_t *src_pte,
+ unsigned long addr, unsigned long end,
+ 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;
+ int nr = 1;
+ bool anon = false;
+ bool enforce_uffd_wp = userfaultfd_wp(dst_vma);
+ int nr_dirty = !!pte_dirty(pte);
+ int nr_writable = !!pte_write(pte);
+ int i, ret;
page = vm_normal_page(src_vma, addr, pte);
- if (page)
+ if (page) {
folio = page_folio(page);
- if (page && 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(page_try_dup_anon_rmap(page, false, 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);
+ anon = folio_test_anon(folio);
+ nr = folio_nr_pages_cont_mapped(folio, page, src_pte, addr, end,
+ pte, enforce_uffd_wp, &nr_dirty,
+ &nr_writable);
+ folio_ref_add(folio, nr);
+
+ for (i = 0; i < nr; i++, page++) {
+ if (anon) {
+ /*
+ * 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.
+ */
+ if (unlikely(page_try_dup_anon_rmap(
+ page, false, src_vma))) {
+ if (i != 0)
+ break;
+ /* Page may be pinned, we have to copy. */
+ folio_ref_sub(folio, nr);
+ ret = copy_present_page(
+ dst_vma, src_vma, dst_pte,
+ src_pte, addr, rss, prealloc,
+ page);
+ return ret == 0 ? 1 : ret;
+ }
+ rss[MM_ANONPAGES]++;
+ VM_BUG_ON(PageAnonExclusive(page));
+ } else {
+ page_dup_file_rmap(page, false);
+ rss[mm_counter_file(page)]++;
+ }
}
- rss[MM_ANONPAGES]++;
- } else if (page) {
- folio_get(folio);
- page_dup_file_rmap(page, false);
- rss[mm_counter_file(page)]++;
- }
This likely looks a lot neater if you keep the existing structure.
For example, you can simply have on the !anon path
} else if (page) {
folio = page_folio(page);
nr = folio_nr_pages_cont_mapped ...
folio_ref_add(folio, nr);
for (i = 0; i < nr; i++, page++)
page_dup_file_rmap(page, false);
rss[mm_counter_file(&folio->page)] += nr;
}
With rmap batching from [1] -- rebased+changed on top of that -- we could turn
that into an effective (untested):
if (page && folio_test_anon(folio)) {
+ nr = folio_nr_pages_cont_mapped(folio, page, src_pte, addr, end,
+ pte, enforce_uffd_wp, &nr_dirty,
+ &nr_writable);
/*
* 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))) {
+ folio_ref_add(folio, nr);
+ if (unlikely(folio_try_dup_anon_rmap_ptes(folio, page, nr, 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);
+ folio_ref_sub(folio, nr);
+ ret = copy_present_page(dst_vma, src_vma, dst_pte,
+ src_pte, addr, rss, prealloc,
+ page);
+ return ret == 0 ? 1 : ret;
}
- rss[MM_ANONPAGES]++;
+ rss[MM_ANONPAGES] += nr;
} else if (page) {
- folio_get(folio);
- folio_dup_file_rmap_pte(folio, page);
- rss[mm_counter_file(page)]++;
+ nr = folio_nr_pages_cont_mapped(folio, page, src_pte, addr, end,
+ pte, enforce_uffd_wp, &nr_dirty,
+ &nr_writable);
+ folio_ref_add(folio, nr);
+ folio_dup_file_rmap_ptes(folio, page, nr);
+ rss[mm_counter_file(page)] += nr;
}
We'll have to test performance, but it could be that we want to specialize
more on !folio_test_large(). That code is very performance-sensitive.
[1] https://lkml.kernel.org/r/20231204142146.91437-1-david@xxxxxxxxxx
--
Cheers,
David / dhildenb