linux-kernel - Re: [PATCH] mm: move idle swap cache pages to the tail of LRU after COW

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 18 May 2021 17:25:50 -1000
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Huang Ying <ying.huang@...el.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Linux-MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Matthew Wilcox <willy@...radead.org>,
        Peter Xu <peterx@...hat.com>, Hugh Dickins <hughd@...gle.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Mel Gorman <mgorman@...e.de>, Rik van Riel <riel@...riel.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Michal Hocko <mhocko@...nel.org>,
        Dave Hansen <dave.hansen@...el.com>,
        Tim Chen <tim.c.chen@...el.com>
Subject: Re: [PATCH] mm: move idle swap cache pages to the tail of LRU after COW

On Tue, May 18, 2021 at 5:17 PM Linus Torvalds
<torvalds@...ux-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