MIME-Version: 1.0
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>
In-Reply-To: <172ccfbb-7e24-db21-7d84-8c8d8c3805fd@redhat.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu, 13 Jan 2022 09:14:48 -0800
Message-ID: <CAHk-=wi21DZ4H5uLnn2QgAeAUqg0wNPboijC0OgDDk1e7TdkPw@mail.gmail.com>
Subject: Re: [PATCH] mm: reuse the unshared swapcache page in do_wp_page
To:     David Hildenbrand <david@redhat.com>
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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jan 13, 2022 at 8:48 AM David Hildenbrand <david@redhat.com> wrote:
>
> I'm wondering if we can get rid of the mapcount checks in
> reuse_swap_page() and instead check for page_count() and swapcount only.

Honestly, I think even checking page_count() is pointless.

If the page has users, then that's fine. That's irrelevant for whether
it's a swap page or not, no?

So if you want to remove it from the swap cache, all that matters is that

 (a) you have it locked so that there can be no new users trying to mix it up

 (b) there are no swapcount() users of this page (which don't have a
ref to the page itself, they only have a swap entry), so that you
can't have somebody trying to look it up (whether some racy
"concurrent" lookup _or_ any later one, since we're about to remove
the swap cache association).

Why would "map_count()" matter - it's now many times the *page* is
mapped, it's irrelevant to swap cache status? And for the same reason,
what difference does "page_count()" have?

One big reason I did the COW rewrite was literally that the rules were
pure voodoo and made no sense at all. There was no underlying logic,
it was just a random collection of random tests that didn't have any
logical architecture to them.

Our VM is really really complicated already, so I really want our code
to make sense.

So even if I'm entirely wrong in my swap/map-count arguments above,
I'd like whatever patches in this area to be really well commented and
have some fundamental rules and logic to them so that people can read
the code and go "Ok, makes sense".

Please?

         Linus