Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210519013313.1274454-1-ying.huang@intel.com> <CAHk-=wig4SQjBLtmFODs6oXukwrO0o5CPUVzxT2UvJM9jvwSiA@mail.gmail.com>
In-Reply-To: <CAHk-=wig4SQjBLtmFODs6oXukwrO0o5CPUVzxT2UvJM9jvwSiA@mail.gmail.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 18 May 2021 17:25:50 -1000
Message-ID: <CAHk-=wgUMC03e1Ost6q4gb7+2jWO=vBHW63OYaOHOHvi39esng@mail.gmail.com>
Subject: Re: [PATCH] mm: move idle swap cache pages to the tail of LRU after COW
To:     Huang Ying <ying.huang@intel.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Linux-MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Matthew Wilcox <willy@infradead.org>,
        Peter Xu <peterx@redhat.com>, Hugh Dickins <hughd@google.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Mel Gorman <mgorman@suse.de>, Rik van Riel <riel@surriel.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Michal Hocko <mhocko@kernel.org>,
        Dave Hansen <dave.hansen@intel.com>,
        Tim Chen <tim.c.chen@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, May 18, 2021 at 5:17 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> This looks sensible to me (and numbers talk!), but as Rik says, it
> would probably be a good idea to move the trylock_page()/unlock_page()
> into try_to_free_idle_swapcache(), and that would make the calling
> side a whole lot cleaner and easier to read.

To keep the error handling simple, and keep that "if that didn't work,
just return" logic in you had, doing it as two functions like:

  static inline void locked_try_to_free_idle_swapcache(struct page *page)
  { .. your current try_to_free_idle_swapcache() .. }

  void try_to_free_idle_swapcache(struct page *page)
  {
        if (trylock_page(page)) {
                locked_try_to_free_idle_swapcache(page);
                unlock_page(page);
        }
  }

would keep that readability and simplicity.

And then the wp_page_copy() code ends up being

        if (page_copied && PageSwapCache(old_page) && !page_mapped(old_page))
                      try_to_free_idle_swapcache(old_page);

which looks pretty sensible to me: if we copied the page, and the old
page is a no longer mapped swap cache page, let's try to free it.

That's still a hell of a long conditional, partly because of those
long names. But at least it's conceptually fairly straightforward and
easy to understand what's going on.

No?

               Linus