Re: [PATCH v3] mm/gup: Allow real explicit breaking of COW

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

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

On Tue, Aug 11, 2020 at 1:06 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Maybe trying to reuse the page just isn't worth it?

Well, the attached patch boots, and hasn't slowed kernel compiles
down. But it may do horrible things due to leaving swap cache pages
and KSM pages to be reaped by the memory scanner, instead of being
reused.

I wouldn't notice, I have too much memory in this machine anyway.

It might have positive side effects too, of course. Not waiting for
the page lock in the page fault case could be a big win on some loads.
We do_wp_page() was one of the paths to the page lock that caused the
nasty latency spikes (I'm not sure it was a dominant one, but it was
up there).

So maybe it is worth running some test loads on. And while this patch
doesn't do it, applying this should mean that you can just revert all
the COW games entirely, and we can remove the should_force_cow_break()
from the GUP paths.

(Also - if this actually works, we can get rid of reuse_ksm_page(),
this was the only user)

                 Linus

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

 mm/memory.c | 58 ++++++++++++++++------------------------------------------
 1 file changed, 16 insertions(+), 42 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index c39a13b09602..b4a57730539d 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2927,50 +2927,24 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
 	 * not dirty accountable.
 	 */
 	if (PageAnon(vmf->page)) {
-		int total_map_swapcount;
-		if (PageKsm(vmf->page) && (PageSwapCache(vmf->page) ||
-					   page_count(vmf->page) != 1))
+		struct page *page = vmf->page;
+
+		if (page_count(page) != 1)
+			goto copy;
+		if (!trylock_page(page))
+			goto copy;
+		if (page_mapcount(page) != 1 && page_count(page) != 1) {
+			unlock_page(page);
 			goto copy;
-		if (!trylock_page(vmf->page)) {
-			get_page(vmf->page);
-			pte_unmap_unlock(vmf->pte, vmf->ptl);
-			lock_page(vmf->page);
-			vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd,
-					vmf->address, &vmf->ptl);
-			if (!pte_same(*vmf->pte, vmf->orig_pte)) {
-				update_mmu_tlb(vma, vmf->address, vmf->pte);
-				unlock_page(vmf->page);
-				pte_unmap_unlock(vmf->pte, vmf->ptl);
-				put_page(vmf->page);
-				return 0;
-			}
-			put_page(vmf->page);
-		}
-		if (PageKsm(vmf->page)) {
-			bool reused = reuse_ksm_page(vmf->page, vmf->vma,
-						     vmf->address);
-			unlock_page(vmf->page);
-			if (!reused)
-				goto copy;
-			wp_page_reuse(vmf);
-			return VM_FAULT_WRITE;
-		}
-		if (reuse_swap_page(vmf->page, &total_map_swapcount)) {
-			if (total_map_swapcount == 1) {
-				/*
-				 * The page is all ours. Move it to
-				 * our anon_vma so the rmap code will
-				 * not search our parent or siblings.
-				 * Protected against the rmap code by
-				 * the page lock.
-				 */
-				page_move_anon_rmap(vmf->page, vma);
-			}
-			unlock_page(vmf->page);
-			wp_page_reuse(vmf);
-			return VM_FAULT_WRITE;
 		}
-		unlock_page(vmf->page);
+		/*
+		 * Ok, we've got the only map reference, and the only
+		 * page count reference, and the page is locked,
+		 * it's dark out, and we're wearing sunglasses. Hit it.
+		 */
+		wp_page_reuse(vmf);
+		unlock_page(page);
+		return VM_FAULT_WRITE;
 	} else if (unlikely((vma->vm_flags & (VM_WRITE|VM_SHARED)) ==
 					(VM_WRITE|VM_SHARED))) {
 		return wp_page_shared(vmf);