On Tue, Dec 21, 2021 at 12:58 AM David Hildenbrand david@redhat.com wrote:
On 21.12.21 02:03, Jason Gunthorpe wrote:
I'm having a hard time imagining how gup_fast can maintain any sort of bit - it lacks all forms of locks so how can we do an atomic test and set between two pieces of data?
And exactly that is to be figured out.
So my preference would be to just always maintain the "exclusive to this VM" bit in the 'struct page', because that makes things easier to think about.
[ Of course - the bit could be reversed, and be a 'not exclusive to this VM' bit, semantically the set-or-cleared issue doesn't matter. Also, when I talk about some "exclusive to this VM" bit, I'm purely talking about pages that are marked PageAnon(), so the bit may or may not even exist for other pager types ]
And then all GUP-fast would need to do is to refuse to look up a page that isn't exclusive to that VM. We already have the situation that GUP-fast can fail for non-writable pages etc, so it's just another test.
Note that I am trying to make also any kind of R/O pins on an anonymous page work as expected as well, to fix any kind of GUP after fork() and GUP before fork(). So taking a R/O pin on an !PageAnonExclusive() page similarly has to make sure that the page is exclusive -- even if it's mapped R/O (!).
I do think the existing "maybe_pinned()" logic is fine for that. The "exclusive to this VM" bit can be used to *help* that decision - because only an exclusive page can be pinned - bit I don't think it should _replace_ that logic.
There's a quite fundamental difference between
(a) COW and GUP: these two operations _have_ to know that they get an exclusive page in order to re-use or look up the page respectively
(b) the pre-cow logic in fork() or the "add this to the swap cache" logic in vmscan that decides whether a page can be turned into a COW page by adding a reference coutn to it (whether due to fork or swap cache doesn't matter - the end result is the same).
The difference is that in (a) the thing we *have* to get right is whether a page is exclusively owned by that VM or not. We can COW too much, but we can never share a page unless it's exclusive. That's true whether it's pinned or not.
In (b), the "have to get right" is different. In (b), it's perfectly ok to COW an exclusive page and turn it non-exclusive. But we must never COW a pinned page.
So (a) and (b) are very different situations, and have different logic.
If we always maintain an exclusive bit for AnonPage pages, then both (a) and (b) can use that bit, but they'll use it very differently. In (a) we'll refuse to look it up and will force a 'handle_mm_fault()' to get an exclusive copy. And in (b), we just use it as a "we know only exclusive pages can be pinned", so it's just another check for page_needs_cow_for_dma(), the same way we currently check "MMF_HAS_PINNED" to narrow down the whole "page count indicates this may be a pinned page" question.
And the "page is exclusive" would actually be the *common* case for almost all pages. Any time you've written to a page and you haven't forked after the write (and it hasn't been turned into a swap page), that page would be exclusive to that VM.
Doesn't this seem like really straightforward semantics to maintain (and think about)?
I'd like the exclusive page bit to *not* be directly about "has this page been pinned" exactly because we already have too many special cases for GUP. It would be nicer to have a page bit that has very clear semantics even in the absence of GUP.
Linus