On Wed, Feb 13, 2019 at 04:11:10PM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 13, 2019 at 03:04:51PM -0800, ira.we...@intel.com wrote:
> > From: Ira Weiny
> >
> > To facilitate additional options to get_user_pages_fast() change the
> > singular write parameter to be gup_flags.
>
> So now we have:
>
> long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
> struct page **pages, unsigned int gup_flags);
>
> and
>
> int get_user_pages_fast(unsigned long start, int nr_pages,
> unsigned int gup_flags, struct page **pages)
>
> Does this make any sense? At least the arguments should be in the same
> order, I think.
Yes... and no. see below.
>
> Also this comment:
> /*
> * get_user_pages_unlocked() is suitable to replace the form:
> *
> * down_read(>mmap_sem);
> * get_user_pages(tsk, mm, ..., pages, NULL);
> * up_read(>mmap_sem);
> *
> * with:
> *
> * get_user_pages_unlocked(tsk, mm, ..., pages);
> *
> * It is functionally equivalent to get_user_pages_fast so
> * get_user_pages_fast should be used instead if specific gup_flags
> * (e.g. FOLL_FORCE) are not required.
> */
>
> Needs some attention as the recommendation is now nonsense.
IMO they are not functionally equivalent.
We can't remove *_unlocked() as it is used as both a helper for the arch
specific *_fast() calls, _and_ in drivers. Again I don't know the history here
but it could be that the drivers should never have used the call in the first
place??? Or been converted at some point?
I could change the comment to be something like
/*
* get_user_pages_unlocked() is only to be used by arch specific
* get_user_pages_fast() calls. Drivers should be calling
* get_user_pages_fast()
*/
Instead of the current comment.
And change the drivers to get_user_pages_fast().
However, I'm not sure if these drivers need the FOLL_TOUCH flag which
*_unlocked() adds for them. And adding FOLL_TOUCH to *_fast() is not going to
give the same functionality.
It _looks_ like we can add FOLL_TOUCH functionality to the fast path in the
generic code. I'm not sure about the arch's.
If we did that then we can have those drivers use FOLL_TOUCH or not in *_fast()
if they want/need.
>
> Honestly a proper explanation of why two functions exist would be
> great at this point :)
I've not researched it. I do agree that there seems to be a lot of calls in
this file and the differences are subtle.
Ira
>
> Jason
