[PATCH v2 0/4] mm/hugetlb: Early cow on fork, and a few cleanups

[PATCH v2 0/4] mm/hugetlb: Early cow on fork, and a few cleanups

[Peter Xu]

v2:
- pass in 1 to alloc_huge_page() last param [Mike]
- reduce comment, unify the comment in one place [Linus]
- add r-bs for Mike and Miaohe

As reported by Gal [1], we still miss the code clip to handle early cow for
hugetlb case, which is true.  Again, it still feels odd to fork() after using a
few huge pages, especially if they're privately mapped to me..  However I do
agree with Gal and Jason in that we should still have that since that'll
complete the early cow on fork effort at least, and it'll still fix issues
where buffers are not well under control and not easy to apply MADV_DONTFORK.

The first two patches (1-2) are some cleanups I noticed when reading into the
hugetlb reserve map code.  I think it's good to have but they're not necessary
for fixing the fork issue.

The last two patches (3-4) is the real fix.

I tested this with a fork() after some vfio-pci assignment, so I'm pretty sure
the page copy path could trigger well (page will be accounted right after the
fork()), but I didn't do data check since the card I assigned is some random
nic.  Gal, please feel free to try this if you have better way to verify the
series.

  https://github.com/xzpeter/linux/tree/fork-cow-pin-huge

Please review, thanks!

[1] https://lore.kernel.org/lkml/27564187-4a08-f187-5a84-3df50009f6ca@amazon.com/

Peter Xu (4):
  hugetlb: Dedup the code to add a new file_region
  hugetlg: Break earlier in add_reservation_in_range() when we can
  mm: Introduce page_needs_cow_for_dma() for deciding whether cow
  hugetlb: Do early cow when page pinned on src mm

 include/linux/mm.h |  21 +++++++++
 mm/huge_memory.c   |   8 +---
 mm/hugetlb.c       | 114 +++++++++++++++++++++++++++++++++------------
 mm/internal.h      |   5 --
 mm/memory.c        |   7 +--
 5 files changed, 108 insertions(+), 47 deletions(-)

-- 
2.26.2

[PATCH v2 1/4] hugetlb: Dedup the code to add a new file_region

[Peter Xu]

Introduce hugetlb_resv_map_add() helper to add a new file_region rather than
duplication the similar code twice in add_reservation_in_range().

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 mm/hugetlb.c | 51 +++++++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 24 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 18f6ee317900..d2859c2aecc9 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -321,6 +321,24 @@ static void coalesce_file_region(struct resv_map *resv, struct file_region *rg)
 	}
 }
 
+static inline long
+hugetlb_resv_map_add(struct resv_map *map, struct file_region *rg, long from,
+		     long to, struct hstate *h, struct hugetlb_cgroup *cg,
+		     long *regions_needed)
+{
+	struct file_region *nrg;
+
+	if (!regions_needed) {
+		nrg = get_file_region_entry_from_cache(map, from, to);
+		record_hugetlb_cgroup_uncharge_info(cg, h, map, nrg);
+		list_add(&nrg->link, rg->link.prev);
+		coalesce_file_region(map, nrg);
+	} else
+		*regions_needed += 1;
+
+	return to - from;
+}
+
 /*
  * Must be called with resv->lock held.
  *
@@ -336,7 +354,7 @@ static long add_reservation_in_range(struct resv_map *resv, long f, long t,
 	long add = 0;
 	struct list_head *head = &resv->regions;
 	long last_accounted_offset = f;
-	struct file_region *rg = NULL, *trg = NULL, *nrg = NULL;
+	struct file_region *rg = NULL, *trg = NULL;
 
 	if (regions_needed)
 		*regions_needed = 0;
@@ -365,18 +383,11 @@ static long add_reservation_in_range(struct resv_map *resv, long f, long t,
 		/* Add an entry for last_accounted_offset -> rg->from, and
 		 * update last_accounted_offset.
 		 */
-		if (rg->from > last_accounted_offset) {
-			add += rg->from - last_accounted_offset;
-			if (!regions_needed) {
-				nrg = get_file_region_entry_from_cache(
-					resv, last_accounted_offset, rg->from);
-				record_hugetlb_cgroup_uncharge_info(h_cg, h,
-								    resv, nrg);
-				list_add(&nrg->link, rg->link.prev);
-				coalesce_file_region(resv, nrg);
-			} else
-				*regions_needed += 1;
-		}
+		if (rg->from > last_accounted_offset)
+			add += hugetlb_resv_map_add(resv, rg,
+						    last_accounted_offset,
+						    rg->from, h, h_cg,
+						    regions_needed);
 
 		last_accounted_offset = rg->to;
 	}
@@ -384,17 +395,9 @@ static long add_reservation_in_range(struct resv_map *resv, long f, long t,
 	/* Handle the case where our range extends beyond
 	 * last_accounted_offset.
 	 */
-	if (last_accounted_offset < t) {
-		add += t - last_accounted_offset;
-		if (!regions_needed) {
-			nrg = get_file_region_entry_from_cache(
-				resv, last_accounted_offset, t);
-			record_hugetlb_cgroup_uncharge_info(h_cg, h, resv, nrg);
-			list_add(&nrg->link, rg->link.prev);
-			coalesce_file_region(resv, nrg);
-		} else
-			*regions_needed += 1;
-	}
+	if (last_accounted_offset < t)
+		add += hugetlb_resv_map_add(resv, rg, last_accounted_offset,
+					    t, h, h_cg, regions_needed);
 
 	VM_BUG_ON(add < 0);
 	return add;
-- 
2.26.2

[PATCH v2 2/4] hugetlg: Break earlier in add_reservation_in_range() when we can

[Peter Xu]

All the regions maintained in hugetlb reserved map is inclusive on "from" but
exclusive on "to".  We can break earlier even if rg->from==t because it already
means no possible intersection.

This does not need a Fixes in all cases because when it happens (rg->from==t)
we'll not break out of the loop while we should, however the next thing we'd do
is still add the last file_region we'd need and quit the loop in the next
round.  So this change is not a bugfix (since the old code should still run
okay iiuc), but we'd better still touch it up to make it logically sane.

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 mm/hugetlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index d2859c2aecc9..9e6ea96bf33b 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -377,7 +377,7 @@ static long add_reservation_in_range(struct resv_map *resv, long f, long t,
 		/* When we find a region that starts beyond our range, we've
 		 * finished.
 		 */
-		if (rg->from > t)
+		if (rg->from >= t)
 			break;
 
 		/* Add an entry for last_accounted_offset -> rg->from, and
-- 
2.26.2

[PATCH v2 3/4] mm: Introduce page_needs_cow_for_dma() for deciding whether cow

[Peter Xu]

We've got quite a few places (pte, pmd, pud) that explicitly checked against
whether we should break the cow right now during fork().  It's easier to
provide a helper, especially before we work the same thing on hugetlbfs.

Since we'll reference is_cow_mapping() in mm.h, move it there too.  Actually it
suites mm.h more since internal.h is mm/ only, but mm.h is exported to the
whole kernel.  With that we should expect another patch to use is_cow_mapping()
whenever we can across the kernel since we do use it quite a lot but it's
always done with raw code against VM_* flags.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/linux/mm.h | 21 +++++++++++++++++++++
 mm/huge_memory.c   |  8 ++------
 mm/internal.h      |  5 -----
 mm/memory.c        |  7 +------
 4 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index ecdf8a8cd6ae..6ea20721d349 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1291,6 +1291,27 @@ static inline bool page_maybe_dma_pinned(struct page *page)
 		GUP_PIN_COUNTING_BIAS;
 }
 
+static inline bool is_cow_mapping(vm_flags_t flags)
+{
+	return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
+}
+
+/*
+ * This should most likely only be called during fork() to see whether we
+ * should break the cow immediately for a page on the src mm.
+ */
+static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma,
+					  struct page *page)
+{
+	if (!is_cow_mapping(vma->vm_flags))
+		return false;
+
+	if (!atomic_read(&vma->vm_mm->has_pinned))
+		return false;
+
+	return page_maybe_dma_pinned(page);
+}
+
 #if defined(CONFIG_SPARSEMEM) && !defined(CONFIG_SPARSEMEM_VMEMMAP)
 #define SECTION_IN_PAGE_FLAGS
 #endif
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 9237976abe72..dbff6c7eda67 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1095,9 +1095,7 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 	 * best effort that the pinned pages won't be replaced by another
 	 * random page during the coming copy-on-write.
 	 */
-	if (unlikely(is_cow_mapping(vma->vm_flags) &&
-		     atomic_read(&src_mm->has_pinned) &&
-		     page_maybe_dma_pinned(src_page))) {
+	if (unlikely(page_needs_cow_for_dma(vma, src_page))) {
 		pte_free(dst_mm, pgtable);
 		spin_unlock(src_ptl);
 		spin_unlock(dst_ptl);
@@ -1209,9 +1207,7 @@ int copy_huge_pud(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 	}
 
 	/* Please refer to comments in copy_huge_pmd() */
-	if (unlikely(is_cow_mapping(vma->vm_flags) &&
-		     atomic_read(&src_mm->has_pinned) &&
-		     page_maybe_dma_pinned(pud_page(pud)))) {
+	if (unlikely(page_needs_cow_for_dma(vma, pud_page(pud)))) {
 		spin_unlock(src_ptl);
 		spin_unlock(dst_ptl);
 		__split_huge_pud(vma, src_pud, addr);
diff --git a/mm/internal.h b/mm/internal.h
index 25d2b2439f19..24eec93d0dac 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -296,11 +296,6 @@ static inline unsigned int buddy_order(struct page *page)
  */
 #define buddy_order_unsafe(page)	READ_ONCE(page_private(page))
 
-static inline bool is_cow_mapping(vm_flags_t flags)
-{
-	return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
-}
-
 /*
  * These three helpers classifies VMAs for virtual memory accounting.
  */
diff --git a/mm/memory.c b/mm/memory.c
index feff48e1465a..b2849e1d4aab 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -800,9 +800,6 @@ copy_present_page(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
 	struct mm_struct *src_mm = src_vma->vm_mm;
 	struct page *new_page;
 
-	if (!is_cow_mapping(src_vma->vm_flags))
-		return 1;
-
 	/*
 	 * What we want to do is to check whether this page may
 	 * have been pinned by the parent process.  If so,
@@ -816,9 +813,7 @@ copy_present_page(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
 	 * the page count. That might give false positives for
 	 * for pinning, but it will work correctly.
 	 */
-	if (likely(!atomic_read(&src_mm->has_pinned)))
-		return 1;
-	if (likely(!page_maybe_dma_pinned(page)))
+	if (likely(!page_needs_cow_for_dma(src_vma, page)))
 		return 1;
 
 	new_page = *prealloc;
-- 
2.26.2

[PATCH v2 4/4] hugetlb: Do early cow when page pinned on src mm

[Peter Xu]

This is the last missing piece of the COW-during-fork effort when there're
pinned pages found.  One can reference 70e806e4e645 ("mm: Do early cow for
pinned pages during fork() for ptes", 2020-09-27) for more information, since
we do similar things here rather than pte this time, but just for hugetlb.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 mm/hugetlb.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 56 insertions(+), 5 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 9e6ea96bf33b..5793936e00ef 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3734,11 +3734,27 @@ static bool is_hugetlb_entry_hwpoisoned(pte_t pte)
 		return false;
 }
 
+static void
+hugetlb_copy_page(struct vm_area_struct *vma, pte_t *ptep, unsigned long addr,
+		  struct page *old_page, struct page *new_page)
+{
+	struct hstate *h = hstate_vma(vma);
+	unsigned int psize = pages_per_huge_page(h);
+
+	copy_user_huge_page(new_page, old_page, addr, vma, psize);
+	__SetPageUptodate(new_page);
+	ClearPagePrivate(new_page);
+	set_page_huge_active(new_page);
+	set_huge_pte_at(vma->vm_mm, addr, ptep, make_huge_pte(vma, new_page, 1));
+	hugepage_add_new_anon_rmap(new_page, vma, addr);
+	hugetlb_count_add(psize, vma->vm_mm);
+}
+
 int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 			    struct vm_area_struct *vma)
 {
 	pte_t *src_pte, *dst_pte, entry, dst_entry;
-	struct page *ptepage;
+	struct page *ptepage, *prealloc = NULL;
 	unsigned long addr;
 	int cow;
 	struct hstate *h = hstate_vma(vma);
@@ -3787,7 +3803,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 		dst_entry = huge_ptep_get(dst_pte);
 		if ((dst_pte == src_pte) || !huge_pte_none(dst_entry))
 			continue;
-
+again:
 		dst_ptl = huge_pte_lock(h, dst, dst_pte);
 		src_ptl = huge_pte_lockptr(h, src, src_pte);
 		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
@@ -3816,6 +3832,39 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 			}
 			set_huge_swap_pte_at(dst, addr, dst_pte, entry, sz);
 		} else {
+			entry = huge_ptep_get(src_pte);
+			ptepage = pte_page(entry);
+			get_page(ptepage);
+
+			/*
+			 * This is a rare case where we see pinned hugetlb
+			 * pages while they're prone to COW.  We need to do the
+			 * COW earlier during fork.
+			 *
+			 * When pre-allocating the page we need to be without
+			 * all the locks since we could sleep when allocate.
+			 */
+			if (unlikely(page_needs_cow_for_dma(vma, ptepage))) {
+				if (!prealloc) {
+					put_page(ptepage);
+					spin_unlock(src_ptl);
+					spin_unlock(dst_ptl);
+					prealloc = alloc_huge_page(vma, addr, 1);
+					if (!prealloc) {
+						ret = -ENOMEM;
+						break;
+					}
+					goto again;
+				}
+				hugetlb_copy_page(vma, dst_pte, addr, ptepage,
+						  prealloc);
+				put_page(ptepage);
+				spin_unlock(src_ptl);
+				spin_unlock(dst_ptl);
+				prealloc = NULL;
+				continue;
+			}
+
 			if (cow) {
 				/*
 				 * No need to notify as we are downgrading page
@@ -3826,9 +3875,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 				 */
 				huge_ptep_set_wrprotect(src, addr, src_pte);
 			}
-			entry = huge_ptep_get(src_pte);
-			ptepage = pte_page(entry);
-			get_page(ptepage);
+
 			page_dup_rmap(ptepage, true);
 			set_huge_pte_at(dst, addr, dst_pte, entry);
 			hugetlb_count_add(pages_per_huge_page(h), dst);
@@ -3842,6 +3889,10 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 	else
 		i_mmap_unlock_read(mapping);
 
+	/* Free the preallocated page if not used at last */
+	if (prealloc)
+		put_page(prealloc);
+
 	return ret;
 }
 
-- 
2.26.2

Re: [PATCH v2 3/4] mm: Introduce page_needs_cow_for_dma() for deciding whether cow

[Linus Torvalds]

On Thu, Feb 4, 2021 at 6:50 AM Peter Xu <peterx@redhat.com> wrote:
>
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1291,6 +1291,27 @@ static inline bool page_maybe_dma_pinned(struct page *page)
>                 GUP_PIN_COUNTING_BIAS;
>  }
>
> +static inline bool is_cow_mapping(vm_flags_t flags)
> +{
> +       return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
> +}

Oh, and I just realized: moving this to <linux/mm.h> means that this
patch could/should also get rid of

 - manual copy of this in mm/hugetlb.c:

        cow = (vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;

 - private #define for the same thing in fs/proc/task_mmu.c

    #define is_cow_mapping(flags) (((flags) & (VM_SHARED |
VM_MAYWRITE)) == VM_MAYWRITE)

 - manual copy in drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c:

        bool is_cow_mapping =
                (vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;

I guess it could be a later cleanup patch too, but maybe best done in
this series just to not forget about it.

               Linus

Re: [PATCH v2 3/4] mm: Introduce page_needs_cow_for_dma() for deciding whether cow

[Linus Torvalds]

On Thu, Feb 4, 2021 at 6:50 AM Peter Xu <peterx@redhat.com> wrote:
>
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1291,6 +1291,27 @@ static inline bool page_maybe_dma_pinned(struct page *page)
>                 GUP_PIN_COUNTING_BIAS;
>  }
>
> +static inline bool is_cow_mapping(vm_flags_t flags)
> +{
> +       return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
> +}

Oh, and I just realized: moving this to <linux/mm.h> means that this
patch could/should also get rid of

 - manual copy of this in mm/hugetlb.c:

        cow = (vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;

 - private #define for the same thing in fs/proc/task_mmu.c

    #define is_cow_mapping(flags) (((flags) & (VM_SHARED |
VM_MAYWRITE)) == VM_MAYWRITE)

 - manual copy in drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c:

        bool is_cow_mapping =
                (vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;

I guess it could be a later cleanup patch too, but maybe best done in
this series just to not forget about it.

               Linus

Re: [PATCH v2 3/4] mm: Introduce page_needs_cow_for_dma() for deciding whether cow

[Peter Xu]

On Thu, Feb 04, 2021 at 09:54:37AM -0800, Linus Torvalds wrote:
> On Thu, Feb 4, 2021 at 6:50 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -1291,6 +1291,27 @@ static inline bool page_maybe_dma_pinned(struct page *page)
> >                 GUP_PIN_COUNTING_BIAS;
> >  }
> >
> > +static inline bool is_cow_mapping(vm_flags_t flags)
> > +{
> > +       return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
> > +}
> 
> Oh, and I just realized: moving this to <linux/mm.h> means that this
> patch could/should also get rid of
> 
>  - manual copy of this in mm/hugetlb.c:
> 
>         cow = (vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
> 
>  - private #define for the same thing in fs/proc/task_mmu.c
> 
>     #define is_cow_mapping(flags) (((flags) & (VM_SHARED |
> VM_MAYWRITE)) == VM_MAYWRITE)
> 
>  - manual copy in drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c:
> 
>         bool is_cow_mapping =
>                 (vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
> 
> I guess it could be a later cleanup patch too, but maybe best done in
> this series just to not forget about it.

Will do in v3 if there is.  Or I'll post a standalone patch to not disturb Gal
just in case he's kicked a test.  Thanks,

-- 
Peter Xu

[PATCH v2 5/4] mm: Use is_cow_mapping() across tree where proper

[Peter Xu]

After is_cow_mapping() is exported in mm.h, replace some manual checks
elsewhere throughout the tree but start to use the new helper.

Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
Cc: Roland Scheidegger <sroland@vmware.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c | 4 +---
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c   | 2 +-
 fs/proc/task_mmu.c                         | 2 --
 mm/hugetlb.c                               | 4 +---
 4 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
index 0b76b3d17d4c..22c4a0e9d250 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
@@ -500,8 +500,6 @@ vm_fault_t vmw_bo_vm_huge_fault(struct vm_fault *vmf,
 	vm_fault_t ret;
 	pgoff_t fault_page_size;
 	bool write = vmf->flags & FAULT_FLAG_WRITE;
-	bool is_cow_mapping =
-		(vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
 
 	switch (pe_size) {
 	case PE_SIZE_PMD:
@@ -518,7 +516,7 @@ vm_fault_t vmw_bo_vm_huge_fault(struct vm_fault *vmf,
 	}
 
 	/* Always do write dirty-tracking and COW on PTE level. */
-	if (write && (READ_ONCE(vbo->dirty) || is_cow_mapping))
+	if (write && (READ_ONCE(vbo->dirty) || is_cow_mapping(vma->vm_flags)))
 		return VM_FAULT_FALLBACK;
 
 	ret = ttm_bo_vm_reserve(bo, vmf);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
index 3c03b1746661..cb9975889e2f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
@@ -49,7 +49,7 @@ int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
 	vma->vm_ops = &vmw_vm_ops;
 
 	/* Use VM_PFNMAP rather than VM_MIXEDMAP if not a COW mapping */
-	if ((vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) != VM_MAYWRITE)
+	if (!is_cow_mapping(vma->vm_flags))
 		vma->vm_flags = (vma->vm_flags & ~VM_MIXEDMAP) | VM_PFNMAP;
 
 	return 0;
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 602e3a52884d..96c1682025f9 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1036,8 +1036,6 @@ struct clear_refs_private {
 
 #ifdef CONFIG_MEM_SOFT_DIRTY
 
-#define is_cow_mapping(flags) (((flags) & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE)
-
 static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
 {
 	struct page *page;
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 5793936e00ef..684d91b68b60 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3756,15 +3756,13 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
 	pte_t *src_pte, *dst_pte, entry, dst_entry;
 	struct page *ptepage, *prealloc = NULL;
 	unsigned long addr;
-	int cow;
+	int cow = is_cow_mapping(vma->vm_flags);
 	struct hstate *h = hstate_vma(vma);
 	unsigned long sz = huge_page_size(h);
 	struct address_space *mapping = vma->vm_file->f_mapping;
 	struct mmu_notifier_range range;
 	int ret = 0;
 
-	cow = (vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
-
 	if (cow) {
 		mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, src,
 					vma->vm_start,
-- 
2.26.2

Re: [PATCH v2 5/4] mm: Use is_cow_mapping() across tree where proper

[Linus Torvalds]

On Thu, Feb 4, 2021 at 12:21 PM Peter Xu <peterx@redhat.com> wrote:
>
> After is_cow_mapping() is exported in mm.h, replace some manual checks
> elsewhere throughout the tree but start to use the new helper.

Thanks,

                  Linus

Re: [PATCH v2 5/4] mm: Use is_cow_mapping() across tree where proper

[Linus Torvalds]

On Thu, Feb 4, 2021 at 12:21 PM Peter Xu <peterx@redhat.com> wrote:
>
> After is_cow_mapping() is exported in mm.h, replace some manual checks
> elsewhere throughout the tree but start to use the new helper.

Thanks,

                  Linus

Re: [PATCH v2 3/4] mm: Introduce page_needs_cow_for_dma() for deciding whether cow

[Jason Gunthorpe]

On Thu, Feb 04, 2021 at 09:50:32AM -0500, Peter Xu wrote:
> We've got quite a few places (pte, pmd, pud) that explicitly checked against
> whether we should break the cow right now during fork().  It's easier to
> provide a helper, especially before we work the same thing on hugetlbfs.
> 
> Since we'll reference is_cow_mapping() in mm.h, move it there too.  Actually it
> suites mm.h more since internal.h is mm/ only, but mm.h is exported to the
> whole kernel.  With that we should expect another patch to use is_cow_mapping()
> whenever we can across the kernel since we do use it quite a lot but it's
> always done with raw code against VM_* flags.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
>  include/linux/mm.h | 21 +++++++++++++++++++++
>  mm/huge_memory.c   |  8 ++------
>  mm/internal.h      |  5 -----
>  mm/memory.c        |  7 +------
>  4 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index ecdf8a8cd6ae..6ea20721d349 100644
> +++ b/include/linux/mm.h
> @@ -1291,6 +1291,27 @@ static inline bool page_maybe_dma_pinned(struct page *page)
>  		GUP_PIN_COUNTING_BIAS;
>  }
>  
> +static inline bool is_cow_mapping(vm_flags_t flags)
> +{

It feels a bit more logical to pass in a struct vm_area_struct *' here?

Jason

Re: [PATCH v2 4/4] hugetlb: Do early cow when page pinned on src mm

[Mike Kravetz]

On 2/4/21 6:50 AM, Peter Xu wrote:
> This is the last missing piece of the COW-during-fork effort when there're
> pinned pages found.  One can reference 70e806e4e645 ("mm: Do early cow for
> pinned pages during fork() for ptes", 2020-09-27) for more information, since
> we do similar things here rather than pte this time, but just for hugetlb.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  mm/hugetlb.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 56 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 9e6ea96bf33b..5793936e00ef 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3734,11 +3734,27 @@ static bool is_hugetlb_entry_hwpoisoned(pte_t pte)
>  		return false;
>  }
>  
> +static void
> +hugetlb_copy_page(struct vm_area_struct *vma, pte_t *ptep, unsigned long addr,
> +		  struct page *old_page, struct page *new_page)
> +{
> +	struct hstate *h = hstate_vma(vma);
> +	unsigned int psize = pages_per_huge_page(h);
> +
> +	copy_user_huge_page(new_page, old_page, addr, vma, psize);

copy_user_huge_page calls cond_resched() and has might_sleep().  Imagine
the time it takes to copy 1G.  Usually called without holding locks, but
this new code is calling it with ptl locks held.  The copy should be done
outside the ptl, but you will need the ptl to update the pte/rmap.  So,
doing all this within one neat helper like this may not be possible.

> +	__SetPageUptodate(new_page);
> +	ClearPagePrivate(new_page);
> +	set_page_huge_active(new_page);

Code to replace the above ClearPagePrivate and set_page_huge_active is
in Andrew's tree.  With changes in Andrew's tree, this would be:

	ClearHPageRestoreReserve(new_page);
	SetHPageMigratable(new_page);

Ideally, the SetHPageMigratable would be done after the set_pte and add_rmap
so the page does not get migrated before these operations.  However, this
can not happen since we are holding the ptl.  So, no problem here.  If code
is restructured to call copy_user_huge_page outside ptl, keep this in mind.

Also, technically ClearHPageRestoreReserve is not needed as it would not be
set by alloc_huge_page because we did not consume a reserve.  However, better
to leave in place in case someone wants to use helper for something else.

> +	set_huge_pte_at(vma->vm_mm, addr, ptep, make_huge_pte(vma, new_page, 1));
> +	hugepage_add_new_anon_rmap(new_page, vma, addr);
> +	hugetlb_count_add(psize, vma->vm_mm);
> +}
> +
>  int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>  			    struct vm_area_struct *vma)
>  {
>  	pte_t *src_pte, *dst_pte, entry, dst_entry;
> -	struct page *ptepage;
> +	struct page *ptepage, *prealloc = NULL;
>  	unsigned long addr;
>  	int cow;
>  	struct hstate *h = hstate_vma(vma);
> @@ -3787,7 +3803,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>  		dst_entry = huge_ptep_get(dst_pte);
>  		if ((dst_pte == src_pte) || !huge_pte_none(dst_entry))
>  			continue;
> -
> +again:
>  		dst_ptl = huge_pte_lock(h, dst, dst_pte);
>  		src_ptl = huge_pte_lockptr(h, src, src_pte);
>  		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
> @@ -3816,6 +3832,39 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>  			}
>  			set_huge_swap_pte_at(dst, addr, dst_pte, entry, sz);
>  		} else {
> +			entry = huge_ptep_get(src_pte);
> +			ptepage = pte_page(entry);
> +			get_page(ptepage);
> +
> +			/*
> +			 * This is a rare case where we see pinned hugetlb
> +			 * pages while they're prone to COW.  We need to do the
> +			 * COW earlier during fork.
> +			 *
> +			 * When pre-allocating the page we need to be without
> +			 * all the locks since we could sleep when allocate.
> +			 */
> +			if (unlikely(page_needs_cow_for_dma(vma, ptepage))) {
> +				if (!prealloc) {
> +					put_page(ptepage);
> +					spin_unlock(src_ptl);
> +					spin_unlock(dst_ptl);
> +					prealloc = alloc_huge_page(vma, addr, 1);
> +					if (!prealloc) {

alloc_huge_page will return error codes, so you need to check IS_ERR(prealloc)
not just NULL.
-- 
Mike Kravetz

> +						ret = -ENOMEM;
> +						break;
> +					}
> +					goto again;
> +				}
> +				hugetlb_copy_page(vma, dst_pte, addr, ptepage,
> +						  prealloc);
> +				put_page(ptepage);
> +				spin_unlock(src_ptl);
> +				spin_unlock(dst_ptl);
> +				prealloc = NULL;
> +				continue;
> +			}
> +
>  			if (cow) {
>  				/*
>  				 * No need to notify as we are downgrading page
> @@ -3826,9 +3875,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>  				 */
>  				huge_ptep_set_wrprotect(src, addr, src_pte);
>  			}
> -			entry = huge_ptep_get(src_pte);
> -			ptepage = pte_page(entry);
> -			get_page(ptepage);
> +
>  			page_dup_rmap(ptepage, true);
>  			set_huge_pte_at(dst, addr, dst_pte, entry);
>  			hugetlb_count_add(pages_per_huge_page(h), dst);
> @@ -3842,6 +3889,10 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>  	else
>  		i_mmap_unlock_read(mapping);
>  
> +	/* Free the preallocated page if not used at last */
> +	if (prealloc)
> +		put_page(prealloc);
> +
>  	return ret;
>  }
>  
> 

Re: [PATCH v2 3/4] mm: Introduce page_needs_cow_for_dma() for deciding whether cow

[Peter Xu]

On Thu, Feb 04, 2021 at 07:20:00PM -0400, Jason Gunthorpe wrote:
> > +static inline bool is_cow_mapping(vm_flags_t flags)
> > +{
> 
> It feels a bit more logical to pass in a struct vm_area_struct *' here?

Agree, but only if I'm adding this as a new function.  Though it's a code
movement from internal.h to mm.h, so I avoided touching all the users of this
function that existed.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 4/4] hugetlb: Do early cow when page pinned on src mm

[Peter Xu]

On Thu, Feb 04, 2021 at 03:25:37PM -0800, Mike Kravetz wrote:
> On 2/4/21 6:50 AM, Peter Xu wrote:
> > This is the last missing piece of the COW-during-fork effort when there're
> > pinned pages found.  One can reference 70e806e4e645 ("mm: Do early cow for
> > pinned pages during fork() for ptes", 2020-09-27) for more information, since
> > we do similar things here rather than pte this time, but just for hugetlb.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  mm/hugetlb.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 56 insertions(+), 5 deletions(-)
> > 
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 9e6ea96bf33b..5793936e00ef 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -3734,11 +3734,27 @@ static bool is_hugetlb_entry_hwpoisoned(pte_t pte)
> >  		return false;
> >  }
> >  
> > +static void
> > +hugetlb_copy_page(struct vm_area_struct *vma, pte_t *ptep, unsigned long addr,
> > +		  struct page *old_page, struct page *new_page)
> > +{
> > +	struct hstate *h = hstate_vma(vma);
> > +	unsigned int psize = pages_per_huge_page(h);
> > +
> > +	copy_user_huge_page(new_page, old_page, addr, vma, psize);
> 
> copy_user_huge_page calls cond_resched() and has might_sleep().  Imagine
> the time it takes to copy 1G.  Usually called without holding locks, but
> this new code is calling it with ptl locks held.  The copy should be done
> outside the ptl, but you will need the ptl to update the pte/rmap.  So,
> doing all this within one neat helper like this may not be possible.

Right, I'll move the copy outside, thanks for spotting this.

> 
> > +	__SetPageUptodate(new_page);
> > +	ClearPagePrivate(new_page);
> > +	set_page_huge_active(new_page);
> 
> Code to replace the above ClearPagePrivate and set_page_huge_active is
> in Andrew's tree.  With changes in Andrew's tree, this would be:
> 
> 	ClearHPageRestoreReserve(new_page);
> 	SetHPageMigratable(new_page);

Indeed these names are much better than using the default ones.  At the
meantime I'll rebase to linux-next/akpm.  Sorry it's always not easy for me to
find the right branch...

> 
> Ideally, the SetHPageMigratable would be done after the set_pte and add_rmap
> so the page does not get migrated before these operations.  However, this
> can not happen since we are holding the ptl.  So, no problem here.  If code
> is restructured to call copy_user_huge_page outside ptl, keep this in mind.
> 
> Also, technically ClearHPageRestoreReserve is not needed as it would not be
> set by alloc_huge_page because we did not consume a reserve.  However, better
> to leave in place in case someone wants to use helper for something else.

OK, I'll keep it for clearness.

> 
> > +	set_huge_pte_at(vma->vm_mm, addr, ptep, make_huge_pte(vma, new_page, 1));
> > +	hugepage_add_new_anon_rmap(new_page, vma, addr);
> > +	hugetlb_count_add(psize, vma->vm_mm);
> > +}
> > +
> >  int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
> >  			    struct vm_area_struct *vma)
> >  {
> >  	pte_t *src_pte, *dst_pte, entry, dst_entry;
> > -	struct page *ptepage;
> > +	struct page *ptepage, *prealloc = NULL;
> >  	unsigned long addr;
> >  	int cow;
> >  	struct hstate *h = hstate_vma(vma);
> > @@ -3787,7 +3803,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
> >  		dst_entry = huge_ptep_get(dst_pte);
> >  		if ((dst_pte == src_pte) || !huge_pte_none(dst_entry))
> >  			continue;
> > -
> > +again:
> >  		dst_ptl = huge_pte_lock(h, dst, dst_pte);
> >  		src_ptl = huge_pte_lockptr(h, src, src_pte);
> >  		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);

Side question: Mike, do you know why we need this lock_nested()?  Could the src
lock be taken due to any reason already?  It confused me when I read the chunk.

> > @@ -3816,6 +3832,39 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
> >  			}
> >  			set_huge_swap_pte_at(dst, addr, dst_pte, entry, sz);
> >  		} else {
> > +			entry = huge_ptep_get(src_pte);
> > +			ptepage = pte_page(entry);
> > +			get_page(ptepage);
> > +
> > +			/*
> > +			 * This is a rare case where we see pinned hugetlb
> > +			 * pages while they're prone to COW.  We need to do the
> > +			 * COW earlier during fork.
> > +			 *
> > +			 * When pre-allocating the page we need to be without
> > +			 * all the locks since we could sleep when allocate.
> > +			 */
> > +			if (unlikely(page_needs_cow_for_dma(vma, ptepage))) {
> > +				if (!prealloc) {
> > +					put_page(ptepage);
> > +					spin_unlock(src_ptl);
> > +					spin_unlock(dst_ptl);
> > +					prealloc = alloc_huge_page(vma, addr, 1);
> > +					if (!prealloc) {
> 
> alloc_huge_page will return error codes, so you need to check IS_ERR(prealloc)
> not just NULL.

Definitely, I'll fix.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 4/4] hugetlb: Do early cow when page pinned on src mm

[Mike Kravetz]

On 2/4/21 5:43 PM, Peter Xu wrote:
> On Thu, Feb 04, 2021 at 03:25:37PM -0800, Mike Kravetz wrote:
>> On 2/4/21 6:50 AM, Peter Xu wrote:
>>> This is the last missing piece of the COW-during-fork effort when there're
>>> pinned pages found.  One can reference 70e806e4e645 ("mm: Do early cow for
>>> pinned pages during fork() for ptes", 2020-09-27) for more information, since
>>> we do similar things here rather than pte this time, but just for hugetlb.
>>>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>  mm/hugetlb.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++-----
>>>  1 file changed, 56 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>> index 9e6ea96bf33b..5793936e00ef 100644
>>> --- a/mm/hugetlb.c
>>> +++ b/mm/hugetlb.c
>>> +	__SetPageUptodate(new_page);
>>> +	ClearPagePrivate(new_page);
>>> +	set_page_huge_active(new_page);
>>
>> Code to replace the above ClearPagePrivate and set_page_huge_active is
>> in Andrew's tree.  With changes in Andrew's tree, this would be:
>>
>> 	ClearHPageRestoreReserve(new_page);
>> 	SetHPageMigratable(new_page);
> 
> Indeed these names are much better than using the default ones.  At the
> meantime I'll rebase to linux-next/akpm.  Sorry it's always not easy for me to
> find the right branch...

No worries.  I only know because I recently changed these.

...
>>> @@ -3787,7 +3803,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>>>  		dst_entry = huge_ptep_get(dst_pte);
>>>  		if ((dst_pte == src_pte) || !huge_pte_none(dst_entry))
>>>  			continue;
>>> -
>>> +again:
>>>  		dst_ptl = huge_pte_lock(h, dst, dst_pte);
>>>  		src_ptl = huge_pte_lockptr(h, src, src_pte);
>>>  		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
> 
> Side question: Mike, do you know why we need this lock_nested()?  Could the src
> lock be taken due to any reason already?  It confused me when I read the chunk.

I see that it was added with commit 4647875819aa.  That was when huge pages
used the single per-mm ptl.  Lockdep seemed to complain about taking
&mm->page_table_lock twice.   Certainly, source and destination mm can not
be the same.  Right?  I do not have the full history, but it 'looks' like
lockdep might have been confused and this was added to keep it quiet.

BTW - Copy page range for 'normal' pages has the same spin_lock_nested().
-- 
Mike Kravetz

Re: [PATCH v2 4/4] hugetlb: Do early cow when page pinned on src mm

[Zhang, Wei]

Hi Peter,

Gal and I worked together. We tested the patch v2 and can confirm it is working as intended.

Thank you very much for your quick response!

Sincerely,

Wei Zhang


On 2/4/21, 6:51 AM, "Peter Xu" <peterx@redhat.com> wrote:

    CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



    This is the last missing piece of the COW-during-fork effort when there're
    pinned pages found.  One can reference 70e806e4e645 ("mm: Do early cow for
    pinned pages during fork() for ptes", 2020-09-27) for more information, since
    we do similar things here rather than pte this time, but just for hugetlb.

    Signed-off-by: Peter Xu <peterx@redhat.com>
    ---
     mm/hugetlb.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++-----
     1 file changed, 56 insertions(+), 5 deletions(-)

    diff --git a/mm/hugetlb.c b/mm/hugetlb.c
    index 9e6ea96bf33b..5793936e00ef 100644
    --- a/mm/hugetlb.c
    +++ b/mm/hugetlb.c
    @@ -3734,11 +3734,27 @@ static bool is_hugetlb_entry_hwpoisoned(pte_t pte)
                    return false;
     }

    +static void
    +hugetlb_copy_page(struct vm_area_struct *vma, pte_t *ptep, unsigned long addr,
    +                 struct page *old_page, struct page *new_page)
    +{
    +       struct hstate *h = hstate_vma(vma);
    +       unsigned int psize = pages_per_huge_page(h);
    +
    +       copy_user_huge_page(new_page, old_page, addr, vma, psize);
    +       __SetPageUptodate(new_page);
    +       ClearPagePrivate(new_page);
    +       set_page_huge_active(new_page);
    +       set_huge_pte_at(vma->vm_mm, addr, ptep, make_huge_pte(vma, new_page, 1));
    +       hugepage_add_new_anon_rmap(new_page, vma, addr);
    +       hugetlb_count_add(psize, vma->vm_mm);
    +}
    +
     int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
                                struct vm_area_struct *vma)
     {
            pte_t *src_pte, *dst_pte, entry, dst_entry;
    -       struct page *ptepage;
    +       struct page *ptepage, *prealloc = NULL;
            unsigned long addr;
            int cow;
            struct hstate *h = hstate_vma(vma);
    @@ -3787,7 +3803,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
                    dst_entry = huge_ptep_get(dst_pte);
                    if ((dst_pte == src_pte) || !huge_pte_none(dst_entry))
                            continue;
    -
    +again:
                    dst_ptl = huge_pte_lock(h, dst, dst_pte);
                    src_ptl = huge_pte_lockptr(h, src, src_pte);
                    spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
    @@ -3816,6 +3832,39 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
                            }
                            set_huge_swap_pte_at(dst, addr, dst_pte, entry, sz);
                    } else {
    +                       entry = huge_ptep_get(src_pte);
    +                       ptepage = pte_page(entry);
    +                       get_page(ptepage);
    +
    +                       /*
    +                        * This is a rare case where we see pinned hugetlb
    +                        * pages while they're prone to COW.  We need to do the
    +                        * COW earlier during fork.
    +                        *
    +                        * When pre-allocating the page we need to be without
    +                        * all the locks since we could sleep when allocate.
    +                        */
    +                       if (unlikely(page_needs_cow_for_dma(vma, ptepage))) {
    +                               if (!prealloc) {
    +                                       put_page(ptepage);
    +                                       spin_unlock(src_ptl);
    +                                       spin_unlock(dst_ptl);
    +                                       prealloc = alloc_huge_page(vma, addr, 1);
    +                                       if (!prealloc) {
    +                                               ret = -ENOMEM;
    +                                               break;
    +                                       }
    +                                       goto again;
    +                               }
    +                               hugetlb_copy_page(vma, dst_pte, addr, ptepage,
    +                                                 prealloc);
    +                               put_page(ptepage);
    +                               spin_unlock(src_ptl);
    +                               spin_unlock(dst_ptl);
    +                               prealloc = NULL;
    +                               continue;
    +                       }
    +
                            if (cow) {
                                    /*
                                     * No need to notify as we are downgrading page
    @@ -3826,9 +3875,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
                                     */
                                    huge_ptep_set_wrprotect(src, addr, src_pte);
                            }
    -                       entry = huge_ptep_get(src_pte);
    -                       ptepage = pte_page(entry);
    -                       get_page(ptepage);
    +
                            page_dup_rmap(ptepage, true);
                            set_huge_pte_at(dst, addr, dst_pte, entry);
                            hugetlb_count_add(pages_per_huge_page(h), dst);
    @@ -3842,6 +3889,10 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
            else
                    i_mmap_unlock_read(mapping);

    +       /* Free the preallocated page if not used at last */
    +       if (prealloc)
    +               put_page(prealloc);
    +
            return ret;
     }

    --
    2.26.2

Re: [PATCH v2 4/4] hugetlb: Do early cow when page pinned on src mm

[Peter Xu]

On Fri, Feb 05, 2021 at 02:58:33PM +0000, Zhang, Wei wrote:
> Hi Peter,

Hi, Wei,

> 
> Gal and I worked together. We tested the patch v2 and can confirm it is working as intended.
> 
> Thank you very much for your quick response!

My thanks too on the quick testing!  Sorry that I'll need to post v3; please
feel free to hold off the testing until I got some r-bs with the new version.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 4/4] hugetlb: Do early cow when page pinned on src mm

[Peter Xu]

On Thu, Feb 04, 2021 at 09:11:24PM -0800, Mike Kravetz wrote:

[...]

> >>> @@ -3787,7 +3803,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
> >>>  		dst_entry = huge_ptep_get(dst_pte);
> >>>  		if ((dst_pte == src_pte) || !huge_pte_none(dst_entry))
> >>>  			continue;
> >>> -
> >>> +again:
> >>>  		dst_ptl = huge_pte_lock(h, dst, dst_pte);
> >>>  		src_ptl = huge_pte_lockptr(h, src, src_pte);
> >>>  		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
> > 
> > Side question: Mike, do you know why we need this lock_nested()?  Could the src
> > lock be taken due to any reason already?  It confused me when I read the chunk.
> 
> I see that it was added with commit 4647875819aa.  That was when huge pages
> used the single per-mm ptl.  Lockdep seemed to complain about taking
> &mm->page_table_lock twice.   Certainly, source and destination mm can not
> be the same.  Right?

Right, at least that's my understanding..

> I do not have the full history, but it 'looks' like
> lockdep might have been confused and this was added to keep it quiet.
> 
> BTW - Copy page range for 'normal' pages has the same spin_lock_nested().

Yes.  I'll need to take the same lock in v3, so I think I'll just follow.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 4/4] hugetlb: Do early cow when page pinned on src mm

[Gal Pressman]

On 05/02/2021 17:51, Peter Xu wrote:
> On Fri, Feb 05, 2021 at 02:58:33PM +0000, Zhang, Wei wrote:
>> Hi Peter,
> 
> Hi, Wei,
> 
>>
>> Gal and I worked together. We tested the patch v2 and can confirm it is working as intended.
>>
>> Thank you very much for your quick response!
> 
> My thanks too on the quick testing!  Sorry that I'll need to post v3; please
> feel free to hold off the testing until I got some r-bs with the new version.

BTW, it might already be fixed in v3, but I encountered a compilation warning
with this series:

mm/memory.c: In function ‘copy_present_page’:
mm/memory.c:800:20: warning: unused variable ‘src_mm’ [-Wunused-variable]
  struct mm_struct *src_mm = src_vma->vm_mm;
                    ^~~~~~

Re: [PATCH v2 4/4] hugetlb: Do early cow when page pinned on src mm

[Peter Xu]

On Sun, Feb 07, 2021 at 11:09:29AM +0200, Gal Pressman wrote:
> On 05/02/2021 17:51, Peter Xu wrote:
> > On Fri, Feb 05, 2021 at 02:58:33PM +0000, Zhang, Wei wrote:
> >> Hi Peter,
> > 
> > Hi, Wei,
> > 
> >>
> >> Gal and I worked together. We tested the patch v2 and can confirm it is working as intended.
> >>
> >> Thank you very much for your quick response!
> > 
> > My thanks too on the quick testing!  Sorry that I'll need to post v3; please
> > feel free to hold off the testing until I got some r-bs with the new version.
> 
> BTW, it might already be fixed in v3, but I encountered a compilation warning
> with this series:
> 
> mm/memory.c: In function ‘copy_present_page’:
> mm/memory.c:800:20: warning: unused variable ‘src_mm’ [-Wunused-variable]
>   struct mm_struct *src_mm = src_vma->vm_mm;
>                     ^~~~~~

Not yet, thanks for raising this, Gal.  I'll wait for some more review feedback
on v3 and send a new version with it fixed.

-- 
Peter Xu