lkml.org 
[lkml]   [2015]   [Nov]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 02/37] mm, frame_vector: do not use get_user_pages_locked()
    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.hansen@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


    \
     
     \ /
      Last update: 2015-11-18 18:21    [W:4.450 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site