On Thu, May 27, 2021 at 6:53 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
>
> OTOH, freeing is a two-liner reusing the swap unmap code:
>
> if (page_copied)
> free_swap_cache(old_page);
>
> Linus, what do you think?
I'm ok with that version, the important thing was
(a) avoiding the unconditional page lock we used to have (well, it
first did "trylock", but if tht failed it would then get a page ref,
and do the unconditional lock_page())
(b) avoid the re-use based on "mapcount" that had problems with
non-mapped page references (ie GUP)
and
(c) that I wanted to see some numbers rather than just blindly
re-introduce free_swap_cache()
But doing the above two-liner in wp_page_copy() doesn't have the
(a)/(b) issues, and if we have numbers that it helps, then that takes
care of (c) too.
Of course, I don't think it's just that two-liner, because you'd
actually have to export (or move )that "free_swap_cache()" function
that is now private to swapfile.c.
But no, I'm not adverse to the above at all, I just had the above reservations.
I was worried about non-swap behavior (which the old code had with
that whole unconditional page locking whether needed or not), but
free_swap_cache() should be basically free for the non-swap behavior
since it doesn't even do the trylock until after it has checked that
it is now an unmapped swap cache page.
Linus
