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

[Linus Torvalds]

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