On Fri, Dec 17, 2021 at 2:29 PM David Hildenbrand david@redhat.com wrote:
While I do care about future use cases, I cannot possibly see fork() not requiring the mmap_lock in the foreseeable future. Just so much depends on it as of now.
It's not that *fork()* depends on it.
Of course fork() takes the mmap_sem.
It's that fast-gup really really doesn't want it, and can't take it.
So any fast-gup user fundamentally cannot look at mapcount(), because that would be fundamentally wrong and racy, and could race with fork.
And yet, as far as I can tell, that's *exactly* what your gup patches do, with gup_pte_range() adding
+ if (!pte_write(pte) && gup_must_unshare(flags, page, false)) { + put_compound_head(head, 1, flags); + goto pte_unmap; + }
which looks at the page mapcount without holding the mmap sem at all.
And see my other email - I think there are other examples of your patches looking at data that isn't stable because you don't hold the right locks.
And you can't even do the optimistic case without taking the lock, because in your world, a COW that optimistically copies in the case of a race condition is fundamentally *wrong* and buggy. Because in your world-view, GUP and COW are very different and have different rules, but you need things to be *exact*, and they aren't.
And none of this is anything at least I can think about, because I don't see what the "design" is.
I really have a hard time following what the rules actually are. You seem to think that "page_mapcount()" is a really simple rule, and I fundamentally disagree. It's a _very_ complicated thing indeed, with locking issues, AND YOU ACTIVELY VIOLATE THE LOCKING RULES!
See why I'm so unhappy?
We *did* do the page_mapcount() thing. It was bad. It forced COW to always take the page lock. There's a very real reason why I'm pushing my "let's have a _design_ here", instead of your "let's look at page_mapcount without even doing the locking".
And yes, I *know* that fork() takes the mmap_sem, and likely always will. That really isn't the problem here. The problem is that your page_mapcount() paths DO NOT take that lock.
Btw, maybe I'm misreading things. I looked at the individual patches, I didn't apply them, maybe I missed something. But I don't think I am.
Linus