On Fri, Dec 17, 2021 at 2:43 PM David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> The pages stay PageAnon(). swap-backed pages simply set a bit IIRC.
> mapcount still applies.
Our code-base is too large for me to remember all the details, but if
we still end up having PageAnon for swapbacked pages, then mapcount
can increase from another process faulting in an pte with that swap
entry.
And mmap_sem doesn't protect against that. Again, page_lock() does.
And taking the page lock was a big performance issue.
One of the reasons that new COW handling is so nice is that you can do
things like
if (!trylock_page(page))
goto copy;
exactly because in the a/b world order, the copy case is always safe.
In your model, as far as I can tell, you leave the page read-only and
a subsequent COW fault _can_ happen, which means that now the
subsequent COW needs to b every very careful, because if it ever
copies a page that was GUP'ed, you just broke the rules.
So COWing too much is a bug (because it breaks the page from the GUP),
but COWing too little is an even worse problem (because it measn that
now the GUP user can see data it shouldn't have seen).
Our old code literally COWed too little. It's why all those changes
happened in the first place.
This is why I'm pushing that whole story line of
(1) COW is based purely on refcounting, because that's the only thing
that obviously can never COW too little.
(2) GUP pre-COWs (the thing I called the "(a)" rule earlier) and then
makes sure to not mark pinned pages COW again (that "(b)" rule).
and here "don't use page_mapcount()" really is about that (1).
You do seem to have kept (1) in that your COW rules don't seem to
change (but maybe I missed it), but because your GUP-vs-COW semantics
are very different indeed, I'm not at all convinced about (2).
Linus
