LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Hugh Dickins <hughd@google.com>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Hugh Dickins <hughd@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Mike Rapoport <rppt@linux.ibm.com>,
	Andrea Arcangeli <aarcange@redhat.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Borislav Petkov <bp@suse.de>, Pavel Machek <pavel@ucw.cz>,
	Dave Hansen <dave.hansen@linux.intel.com>
Subject: Re: [PATCH] mm/gup: continue VM_FAULT_RETRY processing event for pre-faults
Date: Sun, 26 May 2019 13:17:03 -0700 (PDT)	[thread overview]
Message-ID: <alpine.LSU.2.11.1905261250590.2394@eggly.anvils> (raw)
In-Reply-To: <20190526193651.spvm2vtrwxlhsjrv@linutronix.de>

On Sun, 26 May 2019, Sebastian Andrzej Siewior wrote:
> 
> Okay. The GUP functions are not properly documented for my taste. There
> is no indication whether or not the mm_sem has to be acquired prior
> invoking it. Following the call chain of get_user_pages() I ended up in
> __get_user_pages_locked() `locked = NULL' indicated that mm_sem is no
> acquired and then I saw this:
> |                 if (!locked)
> |                         /* VM_FAULT_RETRY couldn't trigger, bypass */
> |                         return ret;
> 
> kind of suggesting that it is okay to invoke it without holding the
> mm_sem prefault. It passed a few tests and then
> 	https://lkml.kernel.org/r/1556657902.6132.13.camel@lca.pw
> 
> happened. After that, I switched to the locked variant and the problem
> disappeared (also I noticed that MPX code is invoked within ->mmap()).

Ah, I wasn't following what happened here while it was in linux-next.

I certainly agree that all the variants are very confusing, and the
matter of mmap_sem particularly so. Because it used to be a simple
rule that you needed to hold mmap_sem to call get_user_pages(); but
that can be so contended that get_user_pages_fast(), and VM_FAULT_RETRY,
optimizations came in, then interfaces like get_user_pages_locked() and
get_user_pages_unlocked().

I think when you say that you "switched to the locked variant", you're
writing of when you switched to using get_user_pages_unlocked(), which
takes and releases the mmap_sem itself: yes, using get_user_pages()
without holding mmap_sem would have been open to serious errors.

The documentation in comments above get_user_pages_locked() and
get_user_pages_unlocked() looks rather good to me, actually.

But this is all good reason to use the less challenging
fault_in_pages_writable() instead. (And also saves all those GUP
developers, who from time to time have to search through all users
of GUP in one form or another, to make this or that improvement,
from having to visit arch/x86/kernel/fpu/signal.c: they will
quietly thank you.)

Hugh

  reply	other threads:[~2019-05-26 20:17 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-26 17:33 [PATCH] x86/fpu: Use fault_in_pages_writeable() for pre-faulting Sebastian Andrzej Siewior
2019-05-26 17:35 ` Sebastian Andrzej Siewior
2019-05-26 19:25   ` Hugh Dickins
2019-05-28 11:54     ` My emacs problem -- was " Pavel Machek
2019-05-29  4:18 ` Andrew Morton
2019-05-29  7:25   ` [PATCH v2] " Sebastian Andrzej Siewior
2019-05-14 14:29     ` [PATCH] mm/gup: continue VM_FAULT_RETRY processing event for pre-faults Mike Rapoport
2019-05-16 16:25       ` Andrei Vagin
2019-05-21 15:53       ` Mike Rapoport
2019-05-22 19:21       ` Andrew Morton
2019-05-22 19:43         ` Sebastian Andrzej Siewior
2019-05-24 22:22           ` Hugh Dickins
2019-05-25  8:45             ` Sebastian Andrzej Siewior
2019-05-25 18:09               ` Hugh Dickins
2019-05-26 19:36                 ` Sebastian Andrzej Siewior
2019-05-26 20:17                   ` Hugh Dickins [this message]
2019-05-26 17:25             ` Pavel Machek
2019-05-22 20:38         ` Mike Rapoport
2019-05-22 21:18           ` Andrew Morton
2019-06-22 17:51             ` Andrea Arcangeli
2019-06-06 17:25       ` [tip:x86/urgent] x86/fpu: Use fault_in_pages_writeable() for pre-faulting tip-bot for Hugh Dickins
2019-05-29 21:29     ` [PATCH v2] " Chris Wilson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LSU.2.11.1905261250590.2394@eggly.anvils \
    --to=hughd@google.com \
    --cc=aarcange@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=bigeasy@linutronix.de \
    --cc=bp@suse.de \
    --cc=dave.hansen@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=pavel@ucw.cz \
    --cc=rppt@linux.ibm.com \
    --subject='Re: [PATCH] mm/gup: continue VM_FAULT_RETRY processing event for pre-faults' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).