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

Linus Torvalds
2020-09-28T19:50:27Z

[-- 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)