Re: [PATCH v4 05/10] mm: Return faster for non-fatal signals in user mode faults

[Peter Xu]

 On Mon, Sep 23, 2019 at 11:03:49AM -0700, Linus Torvalds wrote:
> On Sun, Sep 22, 2019 at 9:26 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
> >
> > This patch is a preparation of removing that special path by allowing
> > the page fault to return even faster if we were interrupted by a
> > non-fatal signal during a user-mode page fault handling routine.
> 
> So I really wish saome other vm person would also review these things,
> but looking over this series once more, this is the patch I probably
> like the least.
> 
> And the reason I like it the least is that I have a hard time
> explaining to myself what the code does and why, and why it's so full
> of this pattern:
> 
> > -       if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
> > +       if ((fault & VM_FAULT_RETRY) &&
> > +           fault_should_check_signal(user_mode(regs)))
> >                 return;
> 
> which isn't all that pretty.
> 
> Why isn't this just
> 
>   static bool fault_signal_pending(unsigned int fault_flags, struct
> pt_regs *regs)
>   {
>         return (fault_flags & VM_FAULT_RETRY) &&
>                 (fatal_signal_pending(current) ||
>                  (user_mode(regs) && signal_pending(current)));
>   }
> 
> and then most of the users would be something like
> 
>         if (fault_signal_pending(fault, regs))
>                 return;
> 
> and the exceptions could do their own thing.
> 
> Now the code is prettier and more understandable, I feel.
> 
> And if something doesn't follow this pattern, maybe it either _should_
> follow that pattern or it should just not use the helper but explain
> why it has an unusual pattern.

I see the point on why this patch is disliked - Yeh it should look
better to have a single function to cover the most common cases.
Besides, I attempted to squash the extra signal_pending() check into
some existing code path but maybe it's not really benefiting much
while instead it makes the review even harder.  So I plan to isolate
those paths out too, from something like:

====================================
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -291,14 +291,15 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)

        fault = __do_page_fault(mm, addr, fsr, flags, tsk);

-       /* If we need to retry but a fatal signal is pending, handle the
+       /* If we need to retry but a signal is pending, try to handle the
         * signal first. We do not need to release the mmap_sem because
         * it would already be released in __lock_page_or_retry in
         * mm/filemap.c. */
-       if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current)) {
-               if (!user_mode(regs))
+       if (unlikely(fault & VM_FAULT_RETRY && signal_pending(current))) {
+               if (fatal_signal_pending(current) && !user_mode(regs))
                        goto no_context;
-               return 0;
+               if (user_mode(regs))
+                       return 0;
        }
====================================

into:

====================================
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -301,6 +301,11 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 		return 0;
 	}
 
+	/* Fast path to handle user mode signals */
+	if ((fault & VM_FAULT_RETRY) && user_mode(regs) &&
+	    signal_pending(current))
+		return 0;
+
 	/*
 	 * Major/minor page fault accounting is only done on the
 	 * initial attempt. If we go through a retry, it is extremely
====================================

I hope it'll be better with that.  A complete patch attached too.

Thanks,

-- 
Peter Xu