On Thu, Sep 24, 2020 at 10:35:17AM -0400, Peter Xu wrote: > If so, I'd hope you won't disagree that I still move the get_page() out of the > "if (wp)". Not only it's a shared operation no matter whether "if (wp)" or > not, but I'm afraid it would confuse future readers on a special ordering on > the get_page() and the wrprotect(), especially with the comment above. Sure, you could add a comment before the page_maybe_dma_pinned that it could be fused with get_page() > Yes. It's kind of related here on whether we can still use wrprotect() to > guard against fast-gup, though. So my understanding is that we should still at > least need the other patch [1] that I proposed in the other thread to force > break-cow for read-only gups (that patch is not only for fast-gup, of course). Probably, I haven't intensively studied that patch, and it should go along with edits to some of the callers.. > But I agree that should be another bigger topic. I hope we don't need to pick > that patch up someday by another dma report on read-only pinned pages... In RDMA we found long ago that read only pins don't work well, I think most other places are likely the same - the problems are easy enough to hit. Something like your COW break patch on read is really needed to allow read-only GUP. > Regarding the solution here, I think we can also cover read-only fast-gup too > in the future - IIUC what we need to do is to make it pte_protnone() instead of > pte_wrprotect(), then in the fault handler we should identify this special > pte_protnone() against numa balancing (change_prot_numa()). I think it should > work fine too, iiuc, because I don't think we should migrate a page at all if > it's pinned for any reason... With your COW breaking patch the read only fast-gup should break the COW because of the write protect, just like for the write side. Not seeing why we need to do something more? Jason
