Date:   Thu, 13 Jan 2022 21:07:20 +0000
From:   Matthew Wilcox <willy@infradead.org>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     David Hildenbrand <david@redhat.com>,
        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
Subject: Re: [PATCH] mm: reuse the unshared swapcache page in do_wp_page
Message-ID: <YeCUiO7RBqo/VTMh@casper.infradead.org>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wjv+beg2gRNdERANGfaGcqwDzzVD5RDD07FcrE5c6k-XA@mail.gmail.com>
Precedence: bulk

On Thu, Jan 13, 2022 at 09:44:04AM -0800, Linus Torvalds wrote:
> 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
> 
> 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".

I think what you mean is something like ...

+++ b/mm/memory.c
@@ -3290,15 +3290,13 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
        if (PageAnon(vmf->page)) {
                struct page *page = vmf->page;
 
-               /* PageKsm() doesn't necessarily raise the page refcount */
-               if (PageKsm(page) || page_count(page) != 1)
+               /* Lots of people are using this page, just copy */
+               if (page_count(page) > 5)
                        goto copy;
                if (!trylock_page(page))
                        goto copy;
-               if (PageKsm(page) || page_mapcount(page) != 1 || page_count(page) != 1) {
-                       unlock_page(page);
-                       goto copy;
-               }
+               if (!reuse_swap_page(page, NULL))
+                       goto unlock;
                /*
                 * Ok, we've got the only map reference, and the only
                 * page count reference, and the page is locked,
@@ -3311,6 +3309,8 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
                                        (VM_WRITE|VM_SHARED))) {
                return wp_page_shared(vmf);
        }
+unlock:
+       unlock_page(page);
 copy:
        /*
         * Ok, we need to copy. Oh, well..

... with a suitably sensible reuse_swap_page().