On Thu 25-07-13 18:25:36, Johannes Weiner wrote:
> The x86 fault handler bails in the middle of error handling when the
> task has a fatal signal pending. For a subsequent patch this is a
> problem in OOM situations because it relies on
> pagefault_out_of_memory() being called even when the task has been
> killed, to perform proper per-task OOM state unwinding.
>
> Shortcutting the fault like this is a rather minor optimization that
> saves a few instructions in rare cases. Just remove it for
> user-triggered faults.
OK, I thought that this optimization tries to prevent calling OOM
because the current might release some memory but that wasn't the
intention of b80ef10e8 (x86: Move do_page_fault()'s error path under
unlikely()).
> Use the opportunity to split the fault retry handling from actual
> fault errors and add locking documentation that reads suprisingly
> similar to ARM's.
>
> Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
Reviewed-by: Michal Hocko <mhocko@xxxxxxx>
> ---
> arch/x86/mm/fault.c | 35 +++++++++++++++++------------------
> 1 file changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 6d77c38..3aaeffc 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -842,23 +842,15 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
> force_sig_info_fault(SIGBUS, code, address, tsk, fault);
> }
>
> -static noinline int
> +static noinline void
> mm_fault_error(struct pt_regs *regs, unsigned long error_code,
> unsigned long address, unsigned int fault)
> {
> - /*
> - * Pagefault was interrupted by SIGKILL. We have no reason to
> - * continue pagefault.
> - */
> - if (fatal_signal_pending(current)) {
> - if (!(fault & VM_FAULT_RETRY))
> - up_read(¤t->mm->mmap_sem);
> - if (!(error_code & PF_USER))
> - no_context(regs, error_code, address, 0, 0);
> - return 1;
> + if (fatal_signal_pending(current) && !(error_code & PF_USER)) {
> + up_read(¤t->mm->mmap_sem);
> + no_context(regs, error_code, address, 0, 0);
> + return;
> }
> - if (!(fault & VM_FAULT_ERROR))
> - return 0;
>
> if (fault & VM_FAULT_OOM) {
> /* Kernel mode? Handle exceptions or die: */
> @@ -866,7 +858,7 @@ mm_fault_error(struct pt_regs *regs, unsigned long error_code,
> up_read(¤t->mm->mmap_sem);
> no_context(regs, error_code, address,
> SIGSEGV, SEGV_MAPERR);
> - return 1;
> + return;
> }
>
> up_read(¤t->mm->mmap_sem);
> @@ -884,7 +876,6 @@ mm_fault_error(struct pt_regs *regs, unsigned long error_code,
> else
> BUG();
> }
> - return 1;
> }
>
> static int spurious_fault_check(unsigned long error_code, pte_t *pte)
> @@ -1189,9 +1180,17 @@ good_area:
> */
> fault = handle_mm_fault(mm, vma, address, flags);
>
> - if (unlikely(fault & (VM_FAULT_RETRY|VM_FAULT_ERROR))) {
> - if (mm_fault_error(regs, error_code, address, fault))
> - return;
> + /*
> + * If we need to retry but a fatal signal is pending, 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 (unlikely((fault & VM_FAULT_RETRY) && fatal_signal_pending(current)))
> + return;
> +
> + if (unlikely(fault & VM_FAULT_ERROR)) {
> + mm_fault_error(regs, error_code, address, fault);
> + return;
> }
>
> /*
> --
> 1.8.3.2
>
--
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
