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

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Peter Xu <peterx@redhat.com>
Cc: Jan Kara <jack@suse.cz>, Andrea Arcangeli <aarcange@redhat.com>,
	Linux-MM <linux-mm@kvack.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Marty Mcfadden <mcfadden8@llnl.gov>,
	"Maya B . Gokhale" <gokhale2@llnl.gov>,
	Jann Horn <jannh@google.com>, Christoph Hellwig <hch@lst.de>,
	Oleg Nesterov <oleg@redhat.com>,
	Kirill Shutemov <kirill@shutemov.name>
Subject: Re: [PATCH v3] mm/gup: Allow real explicit breaking of COW
Date: Fri, 21 Aug 2020 12:05:19 -0700	[thread overview]
Message-ID: <CAHk-=wj5Oyg0LeAxSw_vizerm=sLd=sHfcVecZMKPZn6kNbbXA@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=wj3fY2dch7Ah-MLbJcfU3buoXgJnwc1zWFSjVujSd39GA@mail.gmail.com>

On Fri, Aug 21, 2020 at 11:23 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But the PageKsm() page_count() issue I didn't even realize. That worries me.

Well, the fix is simple, although I don't love the magic PageKsm
semantics that hide it from the page count.

But since (a) a Ksm page is presumably normally shared (ie things like
all zeroes) and (b) copying should always be safe, just do that.

The case we *used* to have with trying to reuse the KSM page seems
like it's not just adding complexity, it's optimizing for entirely the
wrong case.

Check both before and after getting the page lock, for the same reason
we do it for the page count.

The logic there matches the "reuse swap page", but while that old
logic may have made sense 20 years ago, the swap cache case should be
*so* rare these days that it feels completely pointless to try to
reuse it.

Aggressively doing a new allocation, copy, and freeing the old swap
cache page is quite possibly cheaper than taking the page lock anyway,
but more importantly, it's not a case that should normally trigger in
the first place.

That said, looking at this code again, I get the feeling that the
mapcount check is pointless.

Afaik, page_count() should always be larger than page_mapcount(), so
if mapcount is > 1, then we'd have caught it with the page_count()
check.

Hmm? Am I popssibly missing some other subtle special case?

Are there any THP issues? Again, doing the copy should always be the
safe thing to do, and since we get the page lock for the reuse case I
think we're ok on that front.

What else possible special cases could we hit?

                Linus