On Wed, Feb 13, 2019 at 04:11:10PM -0700, Jason Gunthorpe wrote: > On Wed, Feb 13, 2019 at 03:04:51PM -0800, ira.weiny@xxxxxxxxx wrote: > > From: Ira Weiny <ira.weiny@xxxxxxxxx> > > > > 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(&mm->mmap_sem); > * get_user_pages(tsk, mm, ..., pages, NULL); > * up_read(&mm->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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
