From: Linus Torvalds <torvalds@linux-foundation.org>
To: Peter Xu <peterx@redhat.com>
Cc: Linux-MM <linux-mm@kvack.org>,
Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
David Hildenbrand <david@redhat.com>,
Hugh Dickins <hughd@google.com>,
Maya Gokhale <gokhale2@llnl.gov>,
Jerome Glisse <jglisse@redhat.com>,
Pavel Emelyanov <xemul@virtuozzo.com>,
Johannes Weiner <hannes@cmpxchg.org>,
Martin Cracauer <cracauer@cons.org>,
Marty McFadden <mcfadden8@llnl.gov>, Shaohua Li <shli@fb.com>,
Andrea Arcangeli <aarcange@redhat.com>,
Mike Kravetz <mike.kravetz@oracle.com>,
Denis Plotnikov <dplotnikov@virtuozzo.com>,
Mike Rapoport <rppt@linux.vnet.ibm.com>,
Mel Gorman <mgorman@suse.de>,
"Kirill A . Shutemov" <kirill@shutemov.name>,
"Dr . David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [PATCH v3 7/7] mm/gup: Allow VM_FAULT_RETRY for multiple times
Date: Wed, 11 Sep 2019 10:47:59 +0100 [thread overview]
Message-ID: <CAHk-=wh03Qx6zNS_yhhsf5gPah=2=mi7+dKMNCVrKhw6+894ag@mail.gmail.com> (raw)
In-Reply-To: <20190911071007.20077-8-peterx@redhat.com>
On Wed, Sep 11, 2019 at 8:11 AM Peter Xu <peterx@redhat.com> wrote:
>
> This is the gup counterpart of the change that allows the
> VM_FAULT_RETRY to happen for more than once. One thing to mention is
> that we must check the fatal signal here before retry because the GUP
> can be interrupted by that, otherwise we can loop forever.
I still get nervous about the signal handling here.
I'm not entirely sure we get it right even before your patch series.
Right now, __get_user_pages() can return -ERESTARTSYS when it's killed:
/*
* If we have a pending SIGKILL, don't keep faulting pages and
* potentially allocating memory.
*/
if (fatal_signal_pending(current)) {
ret = -ERESTARTSYS;
goto out;
}
and I don't think your series changes that. And note how this is true
_regardless_ of any FOLL_xyz flags (and we don't pass the
FAULT_FLAG_xyz flags at all, they get generated deeper down if we
actually end up faulting things in).
So this part of the patch:
+ if (fatal_signal_pending(current))
+ goto out;
+
*locked = 1;
- lock_dropped = true;
down_read(&mm->mmap_sem);
ret = __get_user_pages(tsk, mm, start, 1, flags | FOLL_TRIED,
- pages, NULL, NULL);
+ pages, NULL, locked);
+ if (!*locked) {
+ /* Continue to retry until we succeeded */
+ BUG_ON(ret != 0);
+ goto retry;
just makes me go "that can't be right". The fatal_signal_pending() is
pointless and would probably better be something like
if (down_read_killable(&mm->mmap_sem) < 0)
goto out;
and then _after_ calling __get_user_pages(), the whole "negative error
handling" should be more obvious.
The BUG_ON(ret != 0) makes me nervous, but it might be fine (I guess
the fatal signal handling has always been done before the lock is
released?).
But exactly *because* __get_user_pages() can already return on fatal
signals, I think it should also set FAULT_FLAG_KILLABLE when faulting
things in. I don't think it does right now, so it doesn't actually
necessarily check fatal signals in a timely manner (not _during_ the
fault, only _before_ it).
See what I'm reacting to?
And maybe I'm wrong. Maybe I misread the code, and your changes. And
at least some of these logic error predate your changes, I just was
hoping that since this whole "react to signals" is very much what your
patch series is working on, you'd look at this.
Ok?
Linus
next prev parent reply other threads:[~2019-09-11 9:48 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-11 7:10 [PATCH v3 0/7] mm: Page fault enhancements Peter Xu
2019-09-11 7:10 ` [PATCH v3 1/7] mm/gup: Rename "nonblocking" to "locked" where proper Peter Xu
2019-09-11 7:10 ` [PATCH v3 2/7] mm: Introduce FAULT_FLAG_DEFAULT Peter Xu
2019-09-11 7:10 ` [PATCH v3 3/7] mm: Introduce FAULT_FLAG_INTERRUPTIBLE Peter Xu
2019-09-11 10:15 ` David Hildenbrand
2019-09-11 7:10 ` [PATCH v3 4/7] mm: Return faster for non-fatal signals in user mode faults Peter Xu
2019-09-11 7:10 ` [PATCH v3 5/7] userfaultfd: Don't retake mmap_sem to emulate NOPAGE Peter Xu
2019-09-11 7:10 ` [PATCH v3 6/7] mm: Allow VM_FAULT_RETRY for multiple times Peter Xu
2019-09-11 7:10 ` [PATCH v3 7/7] mm/gup: " Peter Xu
2019-09-11 9:45 ` [PATCH v3.1 " Peter Xu
2019-09-11 9:47 ` Linus Torvalds [this message]
2019-09-12 3:05 ` [PATCH v3 " Peter Xu
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='CAHk-=wh03Qx6zNS_yhhsf5gPah=2=mi7+dKMNCVrKhw6+894ag@mail.gmail.com' \
--to=torvalds@linux-foundation.org \
--cc=aarcange@redhat.com \
--cc=cracauer@cons.org \
--cc=david@redhat.com \
--cc=dgilbert@redhat.com \
--cc=dplotnikov@virtuozzo.com \
--cc=gokhale2@llnl.gov \
--cc=hannes@cmpxchg.org \
--cc=hughd@google.com \
--cc=jglisse@redhat.com \
--cc=kirill@shutemov.name \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mcfadden8@llnl.gov \
--cc=mgorman@suse.de \
--cc=mike.kravetz@oracle.com \
--cc=peterx@redhat.com \
--cc=rppt@linux.vnet.ibm.com \
--cc=shli@fb.com \
--cc=xemul@virtuozzo.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).