Re: [mm/gup] a308c71bf1: stress-ng.vm-splice.ops_per_sec -95.6% regression

From: Linus Torvalds <torvalds@linux-foundation.org>
To: kernel test robot <rong.a.chen@intel.com>, Jann Horn <jannh@google.com>
Cc: Peter Xu <peterx@redhat.com>, LKML <linux-kernel@vger.kernel.org>,
	lkp@lists.01.org, kernel test robot <lkp@intel.com>,
	"Huang, Ying" <ying.huang@intel.com>,
	Feng Tang <feng.tang@intel.com>,
	zhengjun.xing@intel.com
Subject: Re: [mm/gup] a308c71bf1: stress-ng.vm-splice.ops_per_sec -95.6% regression
Date: Wed, 4 Nov 2020 10:29:59 -0800	[thread overview]
Message-ID: <CAHk-=wiRnRsS4CqLypK533G2Ho=NVTt_s-e9KXZ=b0ptOSB15A@mail.gmail.com> (raw)
In-Reply-To: <20201102091428.GK31092@shao2-debian>

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

On Mon, Nov 2, 2020 at 1:15 AM kernel test robot <rong.a.chen@intel.com> wrote:
>
> Greeting,
>
> FYI, we noticed a -95.6% regression of stress-ng.vm-splice.ops_per_sec due to commit:
>
> commit: a308c71bf1e6e19cc2e4ced31853ee0fc7cb439a ("mm/gup: Remove enfornced COW mechanism")
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master

Note that this is just the reverse of the previous 2000% improvement
reported by the test robot here:

    https://lore.kernel.org/lkml/20200611040453.GK12456@shao2-debian/

and the explanation seems to remain the same:

    https://lore.kernel.org/lkml/CAG48ez1v1b4X5LgFya6nvi33-TWwqna_dc5jGFVosqQhdn_Nkg@mail.gmail.com/

IOW, this is testing a special case (zero page lookup) that the "force
COW" patches happened to turn into a regular case (COW creating a
regular page from the zero page).

The question is whether we should care about the zero page for gup_fast lookup.

If we do care, then the proper fix is likely simply to allow the zero
page in fast-gup, the same way we already do in slow-gup.

ENTIRELY UNTESTED PATCH ATTACHED.

Rong - mind testing this? I don't think the zero-page _should_ be
something that real loads care about, but hey, maybe people do want to
do things like splice zeroes very efficiently..

And note the "untested" part of the patch. It _looks_ fairly obvious,
but maybe I'm missing something.

            Linus

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

 mm/gup.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 102877ed77a4..8966d3c89e5d 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2182,8 +2182,11 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,
 				undo_dev_pagemap(nr, nr_start, flags, pages);
 				goto pte_unmap;
 			}
-		} else if (pte_special(pte))
-			goto pte_unmap;
+		} else if (pte_special(pte)) {
+			if (!is_zero_pfn(pte_pfn(pte)))
+				goto pte_unmap;
+			/* We'll allow the zero page - like follow_page_pte() does */
+		}
 
 		VM_BUG_ON(!pfn_valid(pte_pfn(pte)));
 		page = pte_page(pte);
