Re: [PATCH 1/5] mm: Introduce mm_struct.has_pinned

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1203 bytes --]

On Mon, Sep 28, 2020 at 12:36 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I'll do the pte wrprotect/restore removal. Anybody willing to do
> and test the sequence count approach?

So the wrprotect removal is trivial, with most of it being about the comments.

However, when I look at this, I am - once again - tempted to just add a

        if (__page_mapcount(page) > 1)
                return 1;

there too. Because we know it's a private mapping (shared mappings we
checked for with the "is_cow_mapping()" earlier), and the only case we
really care about is the one where the page is only mapped in the
current mm (because that's what a write pinning will have done, and as
mentioned, a read pinning doesn't do anything wrt fork() right now
anyway).

So if it's mapped in another mm, the COW clearly hasn't been broken by
a pin, and a read pinned page had already gone through a fork.

But the more I look at this code, the more I go "ok, I want somebody
to actually test this with the rdma case".

So I'll attach my suggested patch, but I won't actually commit it. I'd
really like to have this tested, possibly _together_ with the sequence
count addition..

               Linus

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 2530 bytes --]

 mm/memory.c | 46 ++++++++++------------------------------------
 1 file changed, 10 insertions(+), 36 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index fcfc4ca36eba..4a7e89d35ecf 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -806,8 +806,6 @@ copy_present_page(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 		return 1;
 
 	/*
-	 * The trick starts.
-	 *
 	 * What we want to do is to check whether this page may
 	 * have been pinned by the parent process.  If so,
 	 * instead of wrprotect the pte on both sides, we copy
@@ -815,46 +813,22 @@ copy_present_page(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 	 * the pinned page won't be randomly replaced in the
 	 * future.
 	 *
-	 * To achieve this, we do the following:
-	 *
-	 * 1. Write-protect the pte if it's writable.  This is
-	 *    to protect concurrent write fast-gup with
-	 *    FOLL_PIN, so that we'll fail the fast-gup with
-	 *    the write bit removed.
-	 *
-	 * 2. Check page_maybe_dma_pinned() to see whether this
-	 *    page may have been pinned.
+	 * The page pinning checks are just "has this mm ever
+	 * seen pinning", along with the (inexact) check of
+	 * the page count. That might give false positives for
+	 * for pinning, but it will work correctly.
 	 *
-	 * The order of these steps is important to serialize
-	 * against the fast-gup code (gup_pte_range()) on the
-	 * pte check and try_grab_compound_head(), so that
-	 * we'll make sure either we'll capture that fast-gup
-	 * so we'll copy the pinned page here, or we'll fail
-	 * that fast-gup.
-	 *
-	 * NOTE! Even if we don't end up copying the page,
-	 * we won't undo this wrprotect(), because the normal
-	 * reference copy will need it anyway.
-	 */
-	if (pte_write(pte))
-		ptep_set_wrprotect(src_mm, addr, src_pte);
-
-	/*
-	 * These are the "normally we can just copy by reference"
-	 * checks.
+	 * Another heuristic is to just check the mapcount for
+	 * this page. If it is mapped elsewhere, it already is
+	 * not an exclusively pinned page, and doing another
+	 * "copy by reference" isn't going to matter.
 	 */
 	if (likely(!atomic_read(&src_mm->has_pinned)))
 		return 1;
 	if (likely(!page_maybe_dma_pinned(page)))
 		return 1;
-
-	/*
-	 * Uhhuh. It looks like the page might be a pinned page,
-	 * and we actually need to copy it. Now we can set the
-	 * source pte back to being writable.
-	 */
-	if (pte_write(pte))
-		set_pte_at(src_mm, addr, src_pte, pte);
+	if (__page_mapcount(page) > 1)
+		return 1;
 
 	new_page = *prealloc;
 	if (!new_page)