On 13.01.22 18:44, Linus Torvalds wrote: > On Thu, Jan 13, 2022 at 9:25 AM David Hildenbrand <david@xxxxxxxxxx> wrote: >> >> I might be missing something, but it's not only about whether we can remove >> the page from the swap cache, it's about whether we can reuse the page >> exclusively in a process with write access, avoiding a COW. And for that we >> have to check if it's mapped somewhere else already (readable). > > No. > > The "try to remove from swap cache" is one thing. That uses the swap count. However, reuse_swap_page() currently does multiple things, and that's part of the issue. > > The "see if we can reuse this page for COW" is a completely different > test, and that's the "page_count() == 1" one. > > The two should not be mixed up with each other. Just don't do it. > There's no reason - except for legacy confusion that should be > actively avoided and removed. > > IOW, the COW path would do > > trylock - copy if fails > try to remove from swap cache > if page_count() is now 1, we can reuse it I thought about that exact sequence as well. I remember stumbling over one of the other users of reuse_swap_page() that would require more thought -- do_swap_page(). There, we essentially do a COW before having the page mapped. (nasty) I'll give it more thought. > > Note how the "try to remove from swap cache" is entirely independent > of whether we then reuse it or not. > > And yes, we can have optimistic other tests - like not even bothering > to trylock if we can see that the page-count is so elevated that it > makes no difference and trying to remove from swap cache would be just > pointless extra work (both the removal itself, and then potentially > later re-additions). > > But those should be seen for what they are - not important for > semantics, only a "don't bother, this page has so many users that we > already know that removing the swapcache one doesn't make any > difference at all". Right. > > Now, it's possible that I'm missing something, but I think this kind > of clarity is very much what we should aim for. Clear rules, no mixing > of "can I COW this" with "can I remove this from the swap cache". I consider reuse_swap_page() at this point just absolutely nasty. While we're at it, is there a real reason we can't simplify to diff --git a/mm/memory.c b/mm/memory.c index e8e2144cbfa6..ab114a5862a0 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -3295,7 +3295,7 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf) goto copy; if (!trylock_page(page)) goto copy; - if (PageKsm(page) || page_mapcount(page) != 1 || page_count(page) != 1) { + if (PageKsm(page) || page_count(page) != 1) { unlock_page(page); goto copy; Our page mapping has to hold a reference, so it seems unnecessary to check both. > > And now I need to start my travel nightmare, so I'll be effectively > offline for the next 24 hours ;( Happy traveling then :) No worries, I'll be working on all this more than 24 hours, especially PageAnonExclusive() that makes my head hurt when it comes to swap, but this discussion already helps a lot on how to eventually sort it all out. -- Thanks, David / dhildenb