On Thu, Jan 13, 2011 at 7:43 AM, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > On Thu, Jan 13, 2011 at 4:53 AM, Gleb Natapov <gleb@xxxxxxxxxx> wrote: >> >> I implemented get_user_pages_nowait() on top of your patch. In my testing >> it works as expected when used inside KVM. Does this looks OK to you? > > It looks reasonable, although I suspect the subtle behavior wrt the > mmap_sem means that you should not expose the magic bare > FAULT_FLAG_ALLOW_RETRY flag to the __get_user_pages() thing. It's just > too easy to introduce bugs, methinks. > > So I'd suggest > > Â- drop FOLL_RETRY > > Â- Âmake FOLL_NOWAIT set both (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_NOWAIT) > > and that way the get_user_pages() thing will never release the > mmap_sem, and you never have any subtle locking issues for that > particular interface. > > But some other VM person should look at it too. I haven't been following and am not really looking yet, but should express a preference: that Gleb keep it as he had it (if that works), rather than making FOLL_NOWAIT do a combination. A couple of months ago I needed to add a FOLL flag myself, and made a patch to use the same space for FOLL flags and FAULT_FLAGs, to end such ugliness as you see in this patch: + ((foll_flags & FOLL_WRITE) ? + FAULT_FLAG_WRITE : 0) | + ((foll_flags & FOLL_RETRY) ? + FAULT_FLAG_ALLOW_RETRY : 0) | + ((foll_flags & FOLL_NOWAIT) ? + FAULT_FLAG_RETRY_NOWAIT : 0)); But I missed my window, I think several have been added since, with more on the way: I intend to put it together again for mmotm once the dust has settled a little. __get_user_pages already has wrappers for friendly usage, I think it's okay for the wrappers to have special knowledge of what's needed. Hugh -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
