On Wed, Nov 18, 2015 at 01:29:38PM +0100, Jan Kara wrote: > On Mon 16-11-15 19:35:14, Dave Hansen wrote: > > > > From: Dave Hansen <dave.han...@linux.intel.com> > > > > get_user_pages_locked() appears to be for use when a caller needs > > to know that its lock on mmap_sem was invalidated by the gup > > call. > > > > But, get_vaddr_frames() is not one of those users. It > > unconditionally locks the mmap_sem and unconditionally unlocks it > > after the gup call. It takes no special action and does not need > > to know whether its lock was invalidated or not. > > > > Replace get_user_pages_locked() with a vanilla get_user_pages() > > and save a few lines of code. > > > > Note that this was the *ONLY* use of get_user_pages_locked() in > > the entire kernel tree. > > I've used get_user_pages_locked() because of a comment before that function > saying: > > * We can leverage the VM_FAULT_RETRY functionality in the page fault > * paths better by using either get_user_pages_locked() or > * get_user_pages_unlocked(). > * > * get_user_pages_locked() is suitable to replace the form: > * > * down_read(&mm->mmap_sem); > * do_something() > * get_user_pages(tsk, mm, ..., pages, NULL); > * up_read(&mm->mmap_sem); > * > * to: > * > * int locked = 1; > * down_read(&mm->mmap_sem); > * do_something() > * get_user_pages_locked(tsk, mm, ..., pages, &locked); > * if (locked) > * up_read(&mm->mmap_sem); > > So I understood it as a way to reduce mmap_sem hold time by doing a try > first. Did I understand that comment wrong?
That is correct. get_user_pages_locked should not be downgraded to get_user_pages or it can actually break userfaultfd, as userfaultfd needs to be allowed to drop the mmap_sem within handle_mm_fault, to function correctly. Furthermore get_user_pages_locked allows for higher SMP scalability as it can take advantage of the optimized pagecache code that drops the mmap_sem before waiting for I/O (and then it retries the page fault after the I/O is complete). The only case where get_user_pages without FOLL_FAULT_ALLOW_RETRY makes any sense is the get_dump_page case which is actually not using get_user_pages in the first place, so ideally get_user_pages should be obsoleted and dropped. Thanks, Andrea -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
