MIME-Version: 1.0
References: <20220113140318.11117-1-zhangliang5@huawei.com> <YeA5oP/iaxtVPHb3@casper.infradead.org>
In-Reply-To: <YeA5oP/iaxtVPHb3@casper.infradead.org>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu, 13 Jan 2022 08:37:43 -0800
Message-ID: <CAHk-=wjB0i-B=U-DhpAajQx3f6bp1X==neLOrg0jwq29mgz=3g@mail.gmail.com>
Subject: Re: [PATCH] mm: reuse the unshared swapcache page in do_wp_page
To:     Matthew Wilcox <willy@infradead.org>
Cc:     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, David Hildenbrand <david@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jan 13, 2022 at 6:39 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> Let's bring Linus in on this, but I think this reintroduces all of the
> mapcount problems that we've been discussing recently.
>
> How about this as an alternative?

No, at that point reuse_swap_page() is the better thing to do.

Don't play games with page_count() (or even worse games with
swap_count). The page count is only stable if it's 1. Any other value
means that it can fluctuate due to concurrent lookups, some of which
can be done locklessly under RCU.

The biggest problem in the COW path used to be that it was completely
incomprehensible. Plus it had that pointless synchronization if the
page was locked for entirely unrelated reasons.

Doing a "trylock()" - and just copying if it fails fixes that
pointless "let's wait because we know somebody else is doing something
to this page".

And doing the

               if (PageSwapCache(page) && reuse_swap_page(page, NULL)) {

thing after holding the lock is certainly not incomprehensible.

I just wanted to try to avoid that on the assumption that swap really
isn't all that relevant any more - even swap cache.

The most incomprehensible part in that sequence is actually the KSM
tests. I think they are BS and should be removed (the page_count()
check should be sufficient), but they are so incomprehensible that I
left them when I did my crapectomy.

             Linus