On Fri, Nov 17, 2017 at 09:32:15PM +0000, Al Viro wrote:
> And for get_user_pages() itself it's even more ridiculous - vmalist (the last
> argument) is non-NULL in only one caller. Which uses it only to check if all
> of the VMAs happen to be hugetlb ones, apparently.
>
> FWIW, I wanted to trim the users of those two suckers and see what remains.
> And then go through those with maintainers of subsystems in question, to
> see what is really wanted there. That's for the coming cycle, though...
Incidentally, why the hell do we need that notify_drop argument of
__get_user_pages_locked()? Its only use is (and has always been)
if (notify_drop && lock_dropped && *locked) {
/*
* We must let the caller know we temporarily dropped the lock
* and so the critical section protected by it was lost.
*/
up_read(&mm->mmap_sem);
*locked = 0;
}
in the very end of __get_user_pages_locked(). There are 4 callers:
get_user_pages_locked() and get_user_pages_remote() pass true,
get_user_pages() and __get_user_pages_unlocked() - false.
get_user_pages() passes NULL for 'locked', so we _never_ get
to the body of that if() anyway - lock_dropped is only set if
locked != NULL.
And in __get_user_pages_unlocked() we have
ret = __get_user_pages_locked(tsk, mm, start, nr_pages, pages, NULL,
&locked, false, gup_flags | FOLL_TOUCH);
if (locked)
up_read(&mm->mmap_sem);
Suppose we passed true instead of false here. If that if (notify_drop...)
still has not triggered, nothing has changed. If it *has* triggered, we had
*locked (i.e. locked from the __get_user_pages_unlocked() stack frame)
non-zero, so we'd just have dropped ->mmap_sem just before returning to
__get_user_pages_unlocked() instead of doing that just after. And set
*locked to zero, so that __get_user_pages_unlocked() won't end up dropping
it.
Looks like we can bloody well get rid of that argument and do just
if (lock_dropped && *locked) {
/*
* We must let the caller know we temporarily dropped the lock
* and so the critical section protected by it was lost.
*/
up_read(&mm->mmap_sem);
*locked = 0;
}
in __get_user_pages_locked(), or am I missing something subtle there? Andrea?
