Re: [PATCH v2 0/7] mm: Page fault enhancements

[Peter Xu]

On Thu, Sep 05, 2019 at 02:06:04PM -0700, Linus Torvalds wrote:
> On Thu, Sep 5, 2019 at 3:15 AM Peter Xu <peterx@xxxxxxxxxx> wrote:
> >
> > This series is split out of userfaultfd-wp series to only cover the
> > general page fault changes, since it seems to make sense itself.
> 
> The series continues to look sane to me, but I'd like VM people to
> take a look. I see a few reviewed-by's, it would be nice to see more
> comments from people. I'd like to see Andrea in particular say "yeah,
> this looks all good to me".

Yes I agree.  I would appreciate if either Andrea or any of the other
mm experts can comment on this patchset.

> 
> Also a question on how this will get to me - it smells like Andrew's
> -mm tree to me, both from a VM and a userfaultfd angle (and looking
> around, at least a couple of previous patches by Peter have gone that
> way).
> 
> And it would be lovely to have actual _numbers_ for the alleged
> latency improvements. I 100% believe them, but still, numbers rule.

If the question was about the userspace signal handling - IMHO it's
not really a latency number that I can measure, but it's some
functional difference just like what dfa37dc3fc1f6f wanted to solve
previously (though that solution seemed to be causing some other issue
like what have been mentioned in the cover letter on invalid VMA
access), while this series should be a cleaner approach.

To be clear about the functional differnce: if without the userspace
non-fatal handling patch in this series ("mm: Return faster for
non-fatal signals in user mode faults"), we can't use Ctrl-C to stop a
program hanging in handle_userfault(), nor can we use gdb to attach to
that process (we can do it if with dfa37dc3fc1f6f, but again it's not
the clean approach).  And, if with this whole series (hence with "mm:
Return faster for non-fatal signals in user mode faults"), we can do
both (Ctrl-C to stop the process, or gdb attaching to that hanging
process without hanging gdb).

> 
> Talking about latency, what about that retry loop in gup()? That's the
> one I'm not at all convinced about. It doesn't check for signals, so
> if there is some retry logic, it loops forever. Hmm?

Hmm seems to be a valid point... IMHO it'll be fine for non-fatal
signals, because GUPs will still be without FAULT_FLAG_INTERRUPTIBLE
when calling handle_mm_fault(), hence the page fault logic should at
least ignore non-fatal signals.  However I agree that we probably need
a check for fatal signals in __get_user_pages_locked() now.

Thanks,

[1] https://lkml.org/lkml/2017/11/2/833
[2] https://github.com/xzpeter/clibs/blob/master/gpl/userspace/uffd-test/uffd-test.c

-- 
Peter Xu