* Re: [patch 4/4] mm, oom: remove statically defined arch functions of same name
2012-11-14 9:15 ` [patch 4/4] mm, oom: remove statically defined arch functions of same name David Rientjes
@ 2012-11-14 13:47 ` Michal Hocko
2012-11-15 8:48 ` Kamezawa Hiroyuki
1 sibling, 0 replies; 3+ messages in thread
From: Michal Hocko @ 2012-11-14 13:47 UTC (permalink / raw)
To: David Rientjes
Cc: Paul Mundt, linux-sh, Greg Kroah-Hartman, x86, linux-kernel,
linux-mm, Ingo Molnar, Paul Mackerras, KOSAKI Motohiro,
H. Peter Anvin, Andrew Morton, linuxppc-dev, Thomas Gleixner,
KAMEZAWA Hiroyuki
On Wed 14-11-12 01:15:28, David Rientjes wrote:
> out_of_memory() is a globally defined function to call the oom killer.
> x86, sh, and powerpc all use a function of the same name within file
> scope in their respective fault.c unnecessarily. Inline the functions
> into the pagefault handlers to clean the code up.
Yes I like it. It is really confusing to have a local function with the
same name.
>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Paul Mundt <lethal@linux-sh.org>
> Signed-off-by: David Rientjes <rientjes@google.com>
Reviewed-by: Michal Hocko <mhocko@suse.cz>
> ---
> arch/powerpc/mm/fault.c | 27 ++++++++++++---------------
> arch/sh/mm/fault.c | 19 +++++++------------
> arch/x86/mm/fault.c | 23 ++++++++---------------
> 3 files changed, 27 insertions(+), 42 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -113,19 +113,6 @@ static int store_updates_sp(struct pt_regs *regs)
> #define MM_FAULT_CONTINUE -1
> #define MM_FAULT_ERR(sig) (sig)
>
> -static int out_of_memory(struct pt_regs *regs)
> -{
> - /*
> - * We ran out of memory, or some other thing happened to us that made
> - * us unable to handle the page fault gracefully.
> - */
> - up_read(¤t->mm->mmap_sem);
> - if (!user_mode(regs))
> - return MM_FAULT_ERR(SIGKILL);
> - pagefault_out_of_memory();
> - return MM_FAULT_RETURN;
> -}
> -
> static int do_sigbus(struct pt_regs *regs, unsigned long address)
> {
> siginfo_t info;
> @@ -169,8 +156,18 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr, int fault)
> return MM_FAULT_CONTINUE;
>
> /* Out of memory */
> - if (fault & VM_FAULT_OOM)
> - return out_of_memory(regs);
> + if (fault & VM_FAULT_OOM) {
> + up_read(¤t->mm->mmap_sem);
> +
> + /*
> + * We ran out of memory, or some other thing happened to us that
> + * made us unable to handle the page fault gracefully.
> + */
> + if (!user_mode(regs))
> + return MM_FAULT_ERR(SIGKILL);
> + pagefault_out_of_memory();
> + return MM_FAULT_RETURN;
> + }
>
> /* Bus error. x86 handles HWPOISON here, we'll add this if/when
> * we support the feature in HW
> diff --git a/arch/sh/mm/fault.c b/arch/sh/mm/fault.c
> --- a/arch/sh/mm/fault.c
> +++ b/arch/sh/mm/fault.c
> @@ -301,17 +301,6 @@ bad_area_access_error(struct pt_regs *regs, unsigned long error_code,
> __bad_area(regs, error_code, address, SEGV_ACCERR);
> }
>
> -static void out_of_memory(void)
> -{
> - /*
> - * We ran out of memory, call the OOM killer, and return the userspace
> - * (which will retry the fault, or kill us if we got oom-killed):
> - */
> - up_read(¤t->mm->mmap_sem);
> -
> - pagefault_out_of_memory();
> -}
> -
> static void
> do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address)
> {
> @@ -353,8 +342,14 @@ mm_fault_error(struct pt_regs *regs, unsigned long error_code,
> no_context(regs, error_code, address);
> return 1;
> }
> + up_read(¤t->mm->mmap_sem);
>
> - out_of_memory();
> + /*
> + * We ran out of memory, call the OOM killer, and return the
> + * userspace (which will retry the fault, or kill us if we got
> + * oom-killed):
> + */
> + pagefault_out_of_memory();
> } else {
> if (fault & VM_FAULT_SIGBUS)
> do_sigbus(regs, error_code, address);
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -803,20 +803,6 @@ bad_area_access_error(struct pt_regs *regs, unsigned long error_code,
> __bad_area(regs, error_code, address, SEGV_ACCERR);
> }
>
> -/* TODO: fixup for "mm-invoke-oom-killer-from-page-fault.patch" */
> -static void
> -out_of_memory(struct pt_regs *regs, unsigned long error_code,
> - unsigned long address)
> -{
> - /*
> - * We ran out of memory, call the OOM killer, and return the userspace
> - * (which will retry the fault, or kill us if we got oom-killed):
> - */
> - up_read(¤t->mm->mmap_sem);
> -
> - pagefault_out_of_memory();
> -}
> -
> static void
> do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
> unsigned int fault)
> @@ -879,7 +865,14 @@ mm_fault_error(struct pt_regs *regs, unsigned long error_code,
> return 1;
> }
>
> - out_of_memory(regs, error_code, address);
> + up_read(¤t->mm->mmap_sem);
> +
> + /*
> + * We ran out of memory, call the OOM killer, and return the
> + * userspace (which will retry the fault, or kill us if we got
> + * oom-killed):
> + */
> + pagefault_out_of_memory();
> } else {
> if (fault & (VM_FAULT_SIGBUS|VM_FAULT_HWPOISON|
> VM_FAULT_HWPOISON_LARGE))
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [patch 4/4] mm, oom: remove statically defined arch functions of same name
2012-11-14 9:15 ` [patch 4/4] mm, oom: remove statically defined arch functions of same name David Rientjes
2012-11-14 13:47 ` Michal Hocko
@ 2012-11-15 8:48 ` Kamezawa Hiroyuki
1 sibling, 0 replies; 3+ messages in thread
From: Kamezawa Hiroyuki @ 2012-11-15 8:48 UTC (permalink / raw)
To: David Rientjes
Cc: Paul Mundt, linux-sh, Greg Kroah-Hartman, x86, linux-kernel,
Michal Hocko, linux-mm, Ingo Molnar, Paul Mackerras,
KOSAKI Motohiro, H. Peter Anvin, Andrew Morton, linuxppc-dev,
Thomas Gleixner
(2012/11/14 18:15), David Rientjes wrote:
> out_of_memory() is a globally defined function to call the oom killer.
> x86, sh, and powerpc all use a function of the same name within file
> scope in their respective fault.c unnecessarily. Inline the functions
> into the pagefault handlers to clean the code up.
>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Paul Mundt <lethal@linux-sh.org>
> Signed-off-by: David Rientjes <rientjes@google.com>
I think this is good.
Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
^ permalink raw reply [flat|nested] 3+ messages in thread