Message-ID: <0c44a89d-06a7-d0bb-e71e-7947d651f4d1@redhat.com>
Date:   Thu, 13 Jan 2022 18:55:27 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Content-Language: en-US
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Matthew Wilcox <willy@infradead.org>,
        Liang Zhang <zhangliang5@huawei.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux-MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        wangzhigang17@huawei.com
References: <20220113140318.11117-1-zhangliang5@huawei.com>
 <YeA5oP/iaxtVPHb3@casper.infradead.org>
 <CAHk-=wjB0i-B=U-DhpAajQx3f6bp1X==neLOrg0jwq29mgz=3g@mail.gmail.com>
 <172ccfbb-7e24-db21-7d84-8c8d8c3805fd@redhat.com>
 <CAHk-=wi21DZ4H5uLnn2QgAeAUqg0wNPboijC0OgDDk1e7TdkPw@mail.gmail.com>
 <c3f34084-7315-e0c5-55db-d1cb006979f4@redhat.com>
 <CAHk-=wjv+beg2gRNdERANGfaGcqwDzzVD5RDD07FcrE5c6k-XA@mail.gmail.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH] mm: reuse the unshared swapcache page in do_wp_page
In-Reply-To: <CAHk-=wjv+beg2gRNdERANGfaGcqwDzzVD5RDD07FcrE5c6k-XA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 13.01.22 18:44, Linus Torvalds wrote:
> On Thu, Jan 13, 2022 at 9:25 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> I might be missing something, but it's not only about whether we can remove
>> the page from the swap cache, it's about whether we can reuse the page
>> exclusively in a process with write access, avoiding a COW. And for that we
>> have to check if it's mapped somewhere else already (readable).
> 
> No.
> 
> The "try to remove from swap cache" is one thing. That uses the swap count.

However, reuse_swap_page() currently does multiple things, and that's part of the issue.

> 
> The "see if we can reuse this page for COW" is a completely different
> test, and that's the "page_count() == 1" one.
> 
> The two should not be mixed up with each other. Just don't do it.
> There's no reason - except for legacy confusion that should be
> actively avoided and removed.
> 
> IOW, the COW path would do
> 
>  trylock - copy if fails
>  try to remove from swap cache
>  if page_count() is now 1, we can reuse it

I thought about that exact sequence as well. I remember stumbling over
one of the other users of reuse_swap_page() that would require more thought
-- do_swap_page(). There, we essentially do a COW before having the
page mapped. (nasty)

I'll give it more thought.

> 
> Note how the "try to remove from swap cache" is entirely independent
> of whether we then reuse it or not.
> 
> And yes, we can have optimistic other tests - like not even bothering
> to trylock if we can see that the page-count is so elevated that it
> makes no difference and trying to remove from swap cache would be just
> pointless extra work (both the removal itself, and then potentially
> later re-additions).
> 
> But those should be seen for what they are - not important for
> semantics, only a "don't bother, this page has so many users that we
> already know that removing the swapcache one doesn't make any
> difference at all".

Right.

> 
> Now, it's possible that I'm missing something, but I think this kind
> of clarity is very much what we should aim for. Clear rules, no mixing
> of "can I COW this" with "can I remove this from the swap cache".

I consider reuse_swap_page() at this point just absolutely nasty.

While we're at it, is there a real reason we can't simplify to

diff --git a/mm/memory.c b/mm/memory.c
index e8e2144cbfa6..ab114a5862a0 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3295,7 +3295,7 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
                        goto copy;
                if (!trylock_page(page))
                        goto copy;
-               if (PageKsm(page) || page_mapcount(page) != 1 || page_count(page) != 1) {
+               if (PageKsm(page) || page_count(page) != 1) {
                        unlock_page(page);
                        goto copy;

Our page mapping has to hold a reference, so it seems unnecessary to check both.

> 
> And now I need to start my travel nightmare, so I'll be effectively
> offline for the next 24 hours ;(

Happy traveling then :) No worries, I'll be working on all this more
than 24 hours, especially PageAnonExclusive() that makes my head
hurt when it comes to swap, but this discussion already helps a lot
on how to eventually sort it all out.

-- 
Thanks,

David / dhildenb