On Sat, May 23, 2020 at 10:55 PM Matthew Wilcox [off-list ref] wrote:

On Sat, May 23, 2020 at 10:11:12PM +0530, Souptick Joarder wrote:

quoted

Renaming the API __get_user_pages_fast() to get_user_pages_
fast_only() to align with pin_user_pages_fast_only().

Please don't split a function name across lines.  That messes
up people who are grepping for the function name in the changelog.

Ok.

quoted

As part of this we will get rid of write parameter.
Instead caller will pass FOLL_WRITE to get_user_pages_fast_only().
This will not change any existing functionality of the API.

All the callers are changed to pass FOLL_WRITE.

Updated the documentation of the API.

Everything you have done here is an improvement, and I'd be happy to
see it go in (after fixing the bug I note below).

But in reading through it, I noticed almost every user ...

quoted

-     if (__get_user_pages_fast(hva, 1, 1, &page) == 1) {
+     if (get_user_pages_fast_only(hva, 1, FOLL_WRITE, &page) == 1) {

passes '1' as the second parameter.  So do we want to add:

static inline bool get_user_page_fast_only(unsigned long addr,
                unsigned int gup_flags, struct page **pagep)
{
        return get_user_pages_fast_only(addr, 1, gup_flags, pagep) == 1;
}

Yes, this can be added. Does get_user_page_fast_only() naming is fine ?

quoted

@@ -2797,10 +2803,7 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
       * FOLL_FAST_ONLY is required in order to match the API description of
       * this routine: no fall back to regular ("slow") GUP.
       */
-     unsigned int gup_flags = FOLL_GET | FOLL_FAST_ONLY;
-
-     if (write)
-             gup_flags |= FOLL_WRITE;
+     gup_flags = FOLL_GET | FOLL_FAST_ONLY;

Er ... gup_flags |=, surely?

Poor mistake.

@@ -1998,7 +1998,7 @@ int gfn_to_page_many_atomic(struct

kvm_memory_slot *slot, gfn_t gfn,
        if (entry < nr_pages)
                return 0;

-       return __get_user_pages_fast(addr, nr_pages, 1, pages);
+       return get_user_pages_fast(addr, nr_pages, FOLL_WRITE, pages);

Also this needs to be corrected.