Quoting Michael S. Tsirkin (2018-04-05 20:34:08)
On Thu, Apr 05, 2018 at 11:43:27AM -0700, Linus Torvalds wrote:
On Thu, Apr 5, 2018 at 11:28 AM, Michael S. Tsirkin mst@redhat.com wrote:
to repeat what you are saying IIUC __get_user_pages_fast returns 0 if it can't pin any pages and that is by design. Returning 0 on error isn't usual I think so I guess this behaviour should we well documented.
Arguably it happens elsewhere too, and not just in the kernel. "read()" at past the end of a file is not an error, you'll just get 0 for EOF.
So it's not really "returning 0 on error".
It really is simply returning the number of pages it got. End of story. That number of pages can be smaller than the requested number of pages, and _that_ is due to some error, but note how it can return "5" on error too - you asked for 10 pages, but the error happened in the middle!
So the right way to check for error is to bverify that you get the number of pages that you asked for. If you don't, something bad happened.
Of course, many users don't actually care about "I didn't get everything". They only care about "did I get _something_". Then that 0 ends up being the error case, but note how it depends on the caller.
What about get_user_pages_fast though?
We do seem to special-case the first page there. I'm not sure it's a good idea. But like the __get_user_pages_fast(), we seem to have users that know about the particular semantics and depend on it.
It's all ugly, I agree.
End result: we can't just change semantics of either of them.
At least not without going through every single user and checking that they are ok with it.
Which I guess I could be ok with. Maybe changing the semantics of __get_user_pages_fast() is acceptable, if you just change it *everywhere* (which includes not just he users, but also the couple of architecture-specific versions of that same function that we have.
LinusOK I hope I understood what you are saying here.
At least drivers/gpu/drm/i915/i915_gem_userptr.c seems to get it wrong:
pinned = __get_user_pages_fast(obj->userptr.ptr, if (pinned < 0) { pages = ERR_PTR(pinned); pinned = 0; } else if (pinned < num_pages) { pages = __i915_gem_userptr_get_pages_schedule(obj); active = pages == ERR_PTR(-EAGAIN); } else { pages = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages); active = !IS_ERR(pages); }The <0 path is never taken.
Please note that it only recently lost other paths that set an error beforehand. Not exactly wrong since the short return is expected and handled.
Cc maintainers - should that driver be changed to use get_user_pages_fast?
It's not allowed to fault. __gup_fast has that comment, gup_fast does not. -Chris