On Thu, May 11, 2023 at 08:31:02PM -0400, Peter Xu wrote: > E.g., with current code we could at last have FAULT_FLAG_RETRY_NOWAIT set > even if with a FOLL_UNLOCKABLE gup which doesn't make a lot of > sense. I would say NOWAIT and UNLOCKABLE are different things. UNLOCKABLE says the mmap sem is allowed to be unlocked, which is true, and NOWAIT says it shouldn't "wait" (which is something more nebulous than just sleep). In FOLL_ flag terms it would be fine if the mmap sem was unlocked while doing NOWAIT - even though the fault hanlder will not doe this. The only caller is fine with this too. !UNLOCKABLE literally means not to ever drop the mmap lock which is not something KVM needs at all. So I'd say it is fine as is. A caller should never assume that calling an unlocked function or passing null locked means that the mmap sem won't be unlocked while running indirectly because of other GUP flags. If it wants this behavior it needs to ask for it explicitly with a locked GUP call and a NULL locked. > Since at it, the same commit added unconditional FOLL_UNLOCKABLE in > faultin_vma_page_range(), which is code-wise correct becuase the helper > only has one user right now and it always has "locked" set. Not quite, it is correct because that is the API contract of this function. The caller must provide a non-NULL locked and non-NULL locked at the external interfaces always mean it can be unlocked while running. > However it can be abused if someone reuse faultin_vma_page_range() > in other call sites in the future. Add a sanity check for that, > also add the missing comment for UNLOCKABLE. Then we have bigger problems because the API has become confusing if a non-NULL locked somehow means 'don't ever unlock', but only sometimes. Jason
