From: Jan Kara <jack@suse.cz>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Jan Kara <jack@suse.cz>, Peter Xu <peterx@redhat.com>,
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 17:47:56 +0200 [thread overview]
Message-ID: <20200821154756.GC3432@quack2.suse.cz> (raw)
In-Reply-To: <CAHk-=whXfL5DtinS42dgtNu6i+xchK_VP+XLprQbu_og2J5Dkw@mail.gmail.com>
On Fri 21-08-20 05:27:40, Linus Torvalds wrote:
> On Fri, Aug 21, 2020 at 3:13 AM Jan Kara <jack@suse.cz> wrote:
> >
> > > + if (page_mapcount(page) != 1 && page_count(page) != 1) {
> >
> > So this condition looks strange to me... Did you mean:
> >
> > if (page_mapcount(page) != 1 || page_count(page) != 1)
>
> Duh. Yes.
>
> > > - if (PageKsm(vmf->page)) {
> >
> > Also I know nothing about KSM but looking at reuse_ksm_page() I can see it
> > plays some tricks with page index & mapping even for pages with page_count
> > == 1 so you cannot just drop those bits AFAICT.
>
> Yeah, I wasn't really sure what we want to do.
>
> In an optimal world, I was thinking that we'd actually do exactly what
> we do at munmap time.
>
> Which is not to get the page lock at all. Just look at what
> zap_pte_range() does for an a page when it unmaps it:
>
> page_remove_rmap(page, false);
>
> and that's it. No games.
>
> And guess what? That "'page_remove_rmap()" is what wp_page_copy() already
> does.
I was more concerned about the case where you decide to writeably map (i.e.
wp_page_reuse() path) a PageKsm() page. That path does not touch
page->mapping in your code AFAICS. And AFAIU the code in mm/ksm.c you are
not supposed to writeably map PageKsm() pages without changing
page->mapping (which also effectively makes PageKsm() return false) but I
don't see anything in your code that would achieve that because KSM code
references a page without being accounted in page_count() for $reasons (see
comment before get_ksm_page()) and instead plays tricks with validating
cookies in page->mapping...
> So I really think *all* of these games we play are complete garbage
> and completely wrong.
>
> Because the zap_page_range() path is a *lot* more common than the WP
> path, and triggers for every single page when we do munmap or exit or
> whatever.
>
> So why would WP need to do anything else for correctness? Absolutely
> no reason I can see.
>
> > Also I'm not sure if dropping this is safe for THP - reuse_swap_page()
> > seems to be a misnomer and seems to do also some THP handling.
>
> Again, I think that's a bogus argument.
>
> Because this all is actually not the common path at all, and the thing
> is, the common path does none of these odd games.
>
> I really think this COW handling magic is just legacy garbage because
> people have carried it along forever and everybody is worried about
> it. The fact is, the "copy" case is always safe, because all it does
> is basically the same as zap_page_range() does, with just adding a new
> page instead.
And also here I was more concerned that page_mapcount != 1 || page_count !=
1 check could be actually a weaker check than what reuse_swap_page() does.
So the old code could decide to copy while your new code would decide to go
the wp_page_reuse() path. And for this case I don't see how your "but unmap
path is simple" argument would apply...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2020-08-21 15:48 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-11 18:39 [PATCH v3] mm/gup: Allow real explicit breaking of COW Peter Xu
2020-08-11 19:07 ` Jann Horn
2020-08-11 20:02 ` Peter Xu
2020-08-11 20:22 ` Jann Horn
2020-08-11 21:23 ` Peter Xu
2020-08-11 19:24 ` Linus Torvalds
2020-08-11 20:06 ` Linus Torvalds
2020-08-11 20:46 ` Linus Torvalds
2020-08-11 21:42 ` Peter Xu
2020-08-11 23:10 ` Linus Torvalds
2020-08-20 21:54 ` Peter Xu
2020-08-20 22:01 ` Linus Torvalds
2020-08-21 10:13 ` Jan Kara
2020-08-21 12:27 ` Linus Torvalds
2020-08-21 15:47 ` Jan Kara [this message]
2020-08-21 17:00 ` Linus Torvalds
2020-08-21 18:08 ` Peter Xu
2020-08-21 18:23 ` Linus Torvalds
2020-08-21 19:05 ` Linus Torvalds
2020-08-21 19:06 ` Linus Torvalds
2020-08-21 19:31 ` Peter Xu
2020-08-21 19:42 ` Linus Torvalds
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200821154756.GC3432@quack2.suse.cz \
--to=jack@suse.cz \
--cc=aarcange@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=gokhale2@llnl.gov \
--cc=hch@lst.de \
--cc=jannh@google.com \
--cc=kirill@shutemov.name \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mcfadden8@llnl.gov \
--cc=oleg@redhat.com \
--cc=peterx@redhat.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).