Jason, a question for you at the bottom.
On 11/4/19 11:52 AM, Jerome Glisse wrote: ...
CASE 3: ODP
RDMA hardware with page faulting support. Here, a well-written driver doesn't
CASE3: Hardware with page fault support
Here, a well-written ....
Ah, OK. So just drop the first sentence, yes.
...
*/- gup_flags |= FOLL_REMOTE | FOLL_PIN;
Wouldn't it be better to not add pin_longterm_pages_remote() until it can be properly implemented ?
Well, the problem is that I need each call site that requires FOLL_PIN to use a proper wrapper. It's the FOLL_PIN that is the focus here, because there is a hard, bright rule, which is: if and only if a caller sets FOLL_PIN, then the dma-page tracking happens, and put_user_page() must be called.
So this leaves me with only two reasonable choices:
a) Convert the call site as above: pin_longterm_pages_remote(), which sets FOLL_PIN (the key point!), and leaves the FOLL_LONGTERM situation exactly as it has been so far. When the FOLL_LONGTERM situation is fixed, the call site *might* not need any changes to adopt the working gup.c code.
b) Convert the call site to pin_user_pages_remote(), which also sets FOLL_PIN, and also leaves the FOLL_LONGTERM situation exactly as before. There would also be a comment at the call site, to the effect of, "this is the wrong call to make: it really requires FOLL_LONGTERM behavior".
When the FOLL_LONGTERM situation is fixed, the call site will need to be changed to pin_longterm_pages_remote().
So you can probably see why I picked (a).
But right now nobody has FOLL_LONGTERM and FOLL_REMOTE. So you should never have the need for pin_longterm_pages_remote(). My fear is that longterm has implication and it would be better to not drop this implication by adding a wrapper that does not do what the name says.
So do not introduce pin_longterm_pages_remote() until its first user happens. This is option c)
Almost forgot, though: there is already another user: Infiniband:
drivers/infiniband/core/umem_odp.c:646: npages = pin_longterm_pages_remote(owning_process, owning_mm,
odp do not need that, i thought the HMM convertion was already upstream but seems not, in any case odp do not need the longterm case it only so best is to revert that user to gup_fast or something until it get converted to HMM.
Note for Jason: the (a) or (b) items are talking about the vfio case, which is one of the two call sites that now use pin_longterm_pages_remote(), and the other one is infiniband:
drivers/infiniband/core/umem_odp.c:646: npages = pin_longterm_pages_remote(owning_process, owning_mm, drivers/vfio/vfio_iommu_type1.c:353: ret = pin_longterm_pages_remote(NULL, mm, vaddr, 1,
Jerome, Jason: I really don't want to revert the put_page() to put_user_page() conversions that are already throughout the IB driver--pointless churn, right? I'd rather either delete them in Jason's tree, or go with what I have here while waiting for the deletion.
Maybe we should just settle on (a) or (b), so that the IB driver ends up with the wrapper functions? In fact, if it's getting deleted, then I'd prefer leaving it at (a), since that's simple...
Jason should weigh in on how he wants this to go, with respect to branching and merging, since it sounds like that will conflict with the hmm branch (ha, I'm overdue in reviewing his mmu notifier series, that's what I get for being late).
thanks,
John Hubbard NVIDIA