From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1162504AbdKRVpl (ORCPT <rfc822;w@1wt.eu>);
        Sat, 18 Nov 2017 16:45:41 -0500
Received: from zeniv.linux.org.uk ([195.92.253.2]:57442 "EHLO
        ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1763633AbdKRVpd (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Sat, 18 Nov 2017 16:45:33 -0500
Date: Sat, 18 Nov 2017 21:45:31 +0000
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Andrea Arcangeli <aarcange@redhat.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [git pull] vfs.git get_user_pages_fast() conversion
Message-ID: <20171118214531.GT21978@ZenIV.linux.org.uk>
References: <20171117030205.GL21978@ZenIV.linux.org.uk>
 <CA+55aFy+gsHRm+rBmimU3-PS_NUWyAd5Aq12=ygezQdX7_7LAg@mail.gmail.com>
 <20171117213215.GQ21978@ZenIV.linux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20171117213215.GQ21978@ZenIV.linux.org.uk>
User-Agent: Mutt/1.9.0 (2017-09-02)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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?