Re: [PATCH] mm: reuse the unshared swapcache page in do_wp_page

[David Hildenbrand]

On 20.01.22 18:22, Linus Torvalds wrote:
> On Thu, Jan 20, 2022 at 5:46 PM David Hildenbrand <david@xxxxxxxxxx> wrote:
>>
>> I'm, not concerned about fork(), I'm concerned about other false positives.
> 
> Without a fork(), you won't have the THP marked for COW, so is it
> really an issue?

Oh sorry, I should have been clearer.

I absolutely agree that there is no way around COW if there are multiple
sharers of a page :) If you fork() and write to a shared page, you have
to expect that THP get split and that you get a copy. No change on that
front.

At this point I'm trying to assess the impact of the change and how to
eventually mitigate it if it turns out to be a real problem.

(I've been spending a lot of time trying to understand all the
complexity, and sometimes my brain is ... a bit overloaded by all of
that. Well, at least I learn a lot ...)

One scenario I have in mind how we can end up easily with R/O mapped
exclusive THP is a simple fork() with an immediate exec(), unmap() or
exit() of the child. Might not be the most efficient way of doing
things, but that doesn't mean that existing user space doesn't rely on
it not happening. Then, of course, there are other ways to read-protect
THP temporarily (mprotect() and friends), where you wouldn't expect to
lose your THP, but it's somewhat a secondary concern most probably .

Obviously, we need another (temporary/speculative) reference on the THP
or writeback to actually split it. I'm *hoping* that it will be so rare
that we really don't care. I decided to always lock the THP in
do_wp_page() to at least handle page migration and swapcache more reliably.

To answer you question: people optimized the reuse case heavily
previously (reuse_swap_page()), and I can see how it might happen. I'm
hoping that it won't matter in practice, but I'd like to at least
document the impact and have some magic solution in sleeve that I can
just pull out when reports start coming in.

> 
>> Here is what I currently have, I hope that makes sense:
> 
> From a quick look, that patch looks fine to me, but there might be
> something I'm missing... And who knows what odd usage patterns there
> might be in this area. The whole odd Android thing with forking that
> zygote process.

Exactly my thoughts. I'm trying to be very careful.

-- 
Thanks,

David / dhildenb