Re: [PATCH for review] [11/48] x86_64: During VM oom condition, kill all threads in process group

Re: [PATCH for review] [11/48] x86_64: During VM oom condition, kill all threads in process group

[Christoph Hellwig]

On Thu, Jul 19, 2007 at 03:48:40PM +0200, Andi Kleen wrote:
> 
> From: Will Schmidt <will_schmidt@vnet.ibm.com>
> 
> During a VM oom condition, kill all threads in the process group.
> 
> We have had complaints where a threaded application is left in a bad state
> after one of it's threads is killed when we hit a VM: out_of_memory condition.
> 
> Killing just one of the process threads can leave the application in a bad
> state, whereas killing the entire process group would allow for the
> application to restart, or otherwise handled, and makes it very obvious that
> something has gone wrong.
> 
> This change allows the entire process group to be taken down, rather than just
> the one thread.

Shouldn't we have one patc hthat does this for every architecture instead
of going through arch maintainers and probably losing half of them?

> Index: linux/arch/x86_64/mm/fault.c
> ===================================================================
> --- linux.orig/arch/x86_64/mm/fault.c
> +++ linux/arch/x86_64/mm/fault.c
> @@ -569,7 +569,7 @@ out_of_memory:
>  	}
>  	printk("VM: killing process %s\n", tsk->comm);
>  	if (error_code & 4)
> -		do_exit(SIGKILL);
> +		do_group_exit(SIGKILL);
>  	goto no_context;
>  
>  do_sigbus:

Re: [PATCH for review] [11/48] x86_64: During VM oom condition, kill all threads in process group

[Geert Uytterhoeven]

On Thu, 19 Jul 2007, Christoph Hellwig wrote:
> On Thu, Jul 19, 2007 at 03:48:40PM +0200, Andi Kleen wrote:
> > From: Will Schmidt <will_schmidt@vnet.ibm.com>
> > 
> > During a VM oom condition, kill all threads in the process group.
> > 
> > We have had complaints where a threaded application is left in a bad state
> > after one of it's threads is killed when we hit a VM: out_of_memory condition.
> > 
> > Killing just one of the process threads can leave the application in a bad
> > state, whereas killing the entire process group would allow for the
> > application to restart, or otherwise handled, and makes it very obvious that
> > something has gone wrong.
> > 
> > This change allows the entire process group to be taken down, rather than just
> > the one thread.
> 
> Shouldn't we have one patc hthat does this for every architecture instead
> of going through arch maintainers and probably losing half of them?

Yes, please ;-)

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

return -EMAINTAINER_TOO_BUSY

Re: [PATCH for review] [11/48] x86_64: During VM oom condition, kill all threads in process group

[Will Schmidt]

On Thu, 2007-07-19 at 16:14 +0200, Geert Uytterhoeven wrote:
> On Thu, 19 Jul 2007, Christoph Hellwig wrote:
> > On Thu, Jul 19, 2007 at 03:48:40PM +0200, Andi Kleen wrote:
> > > From: Will Schmidt <will_schmidt@vnet.ibm.com>
> > > 
> > > During a VM oom condition, kill all threads in the process group.
> > > 
> > > We have had complaints where a threaded application is left in a bad state
> > > after one of it's threads is killed when we hit a VM: out_of_memory condition.
> > > 
> > > Killing just one of the process threads can leave the application in a bad
> > > state, whereas killing the entire process group would allow for the
> > > application to restart, or otherwise handled, and makes it very obvious that
> > > something has gone wrong.
> > > 
> > > This change allows the entire process group to be taken down, rather than just
> > > the one thread.
> > 
> > Shouldn't we have one patc hthat does this for every architecture instead
> > of going through arch maintainers and probably losing half of them?
> 
> Yes, please ;-)

Ok, i'm convinced. :-)      I'll spin up an all-arch encompassing patch
in the next day or so. 

-Will



> 
> Gr{oetje,eeting}s,
> 
> 						Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> return -EMAINTAINER_TOO_BUSY

[PATCH respin, was PATCH for review] During VM oom condition, kill all threads in process group

[Will Schmidt]

During VM oom condition, kill all threads in process group.

We have had complaints where a threaded application is left in a bad
state after one of it's threads is killed when we hit a VM: out_of_memory
condition.
Killing just one of the process threads can leave the application in a
bad state, whereas killing the entire process group would allow for
the application to restart, or be otherwise handled, and makes it very
obvious that something has gone wrong.

This change allows the entire process group to be taken down, rather
than just the one thread.

Signed-off-by: Will Schmidt <will_schmidt@vnet.ibm.com>

---
 This patch hits all arches except for x86_64 and powerpc; those arches
have already been updated with this change.

diff --git a/arch/alpha/mm/fault.c b/arch/alpha/mm/fault.c
index a0e18da..25154df 100644
--- a/arch/alpha/mm/fault.c
+++ b/arch/alpha/mm/fault.c
@@ -197,7 +197,7 @@ do_page_fault(unsigned long address, unsigned long mmcsr,
 	       current->comm, current->pid);
 	if (!user_mode(regs))
 		goto no_context;
-	do_exit(SIGKILL);
+	do_group_exit(SIGKILL);
 
  do_sigbus:
 	/* Send a sigbus, regardless of whether we were in kernel
diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 846cce4..59ed1d0 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -266,7 +266,7 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 		 * the page fault gracefully.
 		 */
 		printk("VM: killing process %s\n", tsk->comm);
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 		return 0;
 	}
 	if (fault & VM_FAULT_SIGBUS) {
diff --git a/arch/arm26/mm/fault.c b/arch/arm26/mm/fault.c
index dec638a..df14681 100644
--- a/arch/arm26/mm/fault.c
+++ b/arch/arm26/mm/fault.c
@@ -246,7 +246,7 @@ int do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 		 * us that made us unable to handle the page fault gracefully.
 		 */
 		printk("VM: killing process %s\n", tsk->comm);
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	}
 	else{
 		__do_user_fault(tsk, addr, fsr, fault == -1 ? SEGV_ACCERR : SEGV_MAPERR, regs);
diff --git a/arch/avr32/mm/fault.c b/arch/avr32/mm/fault.c
index ae2d2c5..11472f8 100644
--- a/arch/avr32/mm/fault.c
+++ b/arch/avr32/mm/fault.c
@@ -216,7 +216,7 @@ out_of_memory:
 	}
 	printk("VM: Killing process %s\n", tsk->comm);
 	if (user_mode(regs))
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	goto no_context;
 
 do_sigbus:
diff --git a/arch/cris/mm/fault.c b/arch/cris/mm/fault.c
index 8672ab7..8aab814 100644
--- a/arch/cris/mm/fault.c
+++ b/arch/cris/mm/fault.c
@@ -360,7 +360,7 @@ do_page_fault(unsigned long address, struct pt_regs *regs,
 	up_read(&mm->mmap_sem);
 	printk("VM: killing process %s\n", tsk->comm);
 	if (user_mode(regs))
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	goto no_context;
 
  do_sigbus:
diff --git a/arch/frv/mm/fault.c b/arch/frv/mm/fault.c
index 6798fa0..05093d4 100644
--- a/arch/frv/mm/fault.c
+++ b/arch/frv/mm/fault.c
@@ -259,7 +259,7 @@ asmlinkage void do_page_fault(int datammu, unsigned long esr0, unsigned long ear
 	up_read(&mm->mmap_sem);
 	printk("VM: killing process %s\n", current->comm);
 	if (user_mode(__frame))
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	goto no_context;
 
  do_sigbus:
diff --git a/arch/i386/mm/fault.c b/arch/i386/mm/fault.c
index 01ffdd4..2bc2592 100644
--- a/arch/i386/mm/fault.c
+++ b/arch/i386/mm/fault.c
@@ -597,7 +597,7 @@ out_of_memory:
 	}
 	printk("VM: killing process %s\n", tsk->comm);
 	if (error_code & 4)
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	goto no_context;
 
 do_sigbus:
diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c
index 73ccb60..320edf8 100644
--- a/arch/ia64/mm/fault.c
+++ b/arch/ia64/mm/fault.c
@@ -273,6 +273,6 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re
 	}
 	printk(KERN_CRIT "VM: killing process %s\n", current->comm);
 	if (user_mode(regs))
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	goto no_context;
 }
diff --git a/arch/m32r/mm/fault.c b/arch/m32r/mm/fault.c
index 676a1c4..70a766a 100644
--- a/arch/m32r/mm/fault.c
+++ b/arch/m32r/mm/fault.c
@@ -278,7 +278,7 @@ out_of_memory:
 	}
 	printk("VM: killing process %s\n", tsk->comm);
 	if (error_code & ACE_USERMODE)
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	goto no_context;
 
 do_sigbus:
diff --git a/arch/m68k/mm/fault.c b/arch/m68k/mm/fault.c
index 578b48f..eaa6186 100644
--- a/arch/m68k/mm/fault.c
+++ b/arch/m68k/mm/fault.c
@@ -188,7 +188,7 @@ out_of_memory:
 
 	printk("VM: killing process %s\n", current->comm);
 	if (user_mode(regs))
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 
 no_context:
 	current->thread.signo = SIGBUS;
diff --git a/arch/mips/mm/fault.c b/arch/mips/mm/fault.c
index 521771b..5699c77 100644
--- a/arch/mips/mm/fault.c
+++ b/arch/mips/mm/fault.c
@@ -180,7 +180,7 @@ out_of_memory:
 	}
 	printk("VM: killing process %s\n", tsk->comm);
 	if (user_mode(regs))
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	goto no_context;
 
 do_sigbus:
diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c
index 7899ab8..1c091b4 100644
--- a/arch/parisc/mm/fault.c
+++ b/arch/parisc/mm/fault.c
@@ -263,6 +263,6 @@ no_context:
 	up_read(&mm->mmap_sem);
 	printk(KERN_CRIT "VM: killing process %s\n", current->comm);
 	if (user_mode(regs))
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	goto no_context;
 }
diff --git a/arch/ppc/mm/fault.c b/arch/ppc/mm/fault.c
index b98244e..94913dd 100644
--- a/arch/ppc/mm/fault.c
+++ b/arch/ppc/mm/fault.c
@@ -297,7 +297,7 @@ out_of_memory:
 	}
 	printk("VM: killing process %s\n", current->comm);
 	if (user_mode(regs))
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	return SIGKILL;
 
 do_sigbus:
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 5405519..f5bd497 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -218,7 +218,7 @@ static int do_out_of_memory(struct pt_regs *regs, unsigned long error_code,
 	}
 	printk("VM: killing process %s\n", tsk->comm);
 	if (regs->psw.mask & PSW_MASK_PSTATE)
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	do_no_context(regs, error_code, address);
 	return 0;
 }
diff --git a/arch/sh/mm/fault.c b/arch/sh/mm/fault.c
index 964c676..b0d5170 100644
--- a/arch/sh/mm/fault.c
+++ b/arch/sh/mm/fault.c
@@ -215,7 +215,7 @@ out_of_memory:
 	}
 	printk("VM: killing process %s\n", tsk->comm);
 	if (user_mode(regs))
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	goto no_context;
 
 do_sigbus:
diff --git a/arch/sh64/mm/fault.c b/arch/sh64/mm/fault.c
index 0d069d8..dd81c66 100644
--- a/arch/sh64/mm/fault.c
+++ b/arch/sh64/mm/fault.c
@@ -334,7 +334,7 @@ out_of_memory:
 	}
 	printk("VM: killing process %s\n", tsk->comm);
 	if (user_mode(regs))
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	goto no_context;
 
 do_sigbus:
diff --git a/arch/sparc/mm/fault.c b/arch/sparc/mm/fault.c
index 50747fe..e4d9c8e 100644
--- a/arch/sparc/mm/fault.c
+++ b/arch/sparc/mm/fault.c
@@ -369,7 +369,7 @@ out_of_memory:
 	up_read(&mm->mmap_sem);
 	printk("VM: killing process %s\n", tsk->comm);
 	if (from_user)
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	goto no_context;
 
 do_sigbus:
diff --git a/arch/sparc64/mm/fault.c b/arch/sparc64/mm/fault.c
index 17123e9..13fdfa3 100644
--- a/arch/sparc64/mm/fault.c
+++ b/arch/sparc64/mm/fault.c
@@ -466,7 +466,7 @@ out_of_memory:
 	up_read(&mm->mmap_sem);
 	printk("VM: killing process %s\n", current->comm);
 	if (!(regs->tstate & TSTATE_PRIV))
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	goto handle_kernel_fault;
 
 intr_or_no_mm:
diff --git a/arch/xtensa/mm/fault.c b/arch/xtensa/mm/fault.c
index 1600406..399df5c 100644
--- a/arch/xtensa/mm/fault.c
+++ b/arch/xtensa/mm/fault.c
@@ -150,7 +150,7 @@ out_of_memory:
 	}
 	printk("VM: killing process %s\n", current->comm);
 	if (user_mode(regs))
-		do_exit(SIGKILL);
+		do_group_exit(SIGKILL);
 	bad_page_fault(regs, address, SIGKILL);
 	return;
 

Re: [PATCH respin, was PATCH for review] During VM oom condition, kill all threads in process group

[Andrew Morton]

On Mon, 23 Jul 2007 13:09:45 -0500
Will Schmidt <will_schmidt@vnet.ibm.com> wrote:

> During VM oom condition, kill all threads in process group.
> 
> We have had complaints where a threaded application is left in a bad
> state after one of it's threads is killed when we hit a VM: out_of_memory
> condition.
> Killing just one of the process threads can leave the application in a
> bad state, whereas killing the entire process group would allow for
> the application to restart, or be otherwise handled, and makes it very
> obvious that something has gone wrong.
> 
> This change allows the entire process group to be taken down, rather
> than just the one thread.

Just checking...

blackfin
h8300
m68knommu
uml
v850

were not changed.  Intentional?

Re: [PATCH respin, was PATCH for review] During VM oom condition, kill all threads in process group

[Will Schmidt]

On Mon, 2007-07-23 at 14:16 -0700, Andrew Morton wrote:
> On Mon, 23 Jul 2007 13:09:45 -0500
> Will Schmidt <will_schmidt@vnet.ibm.com> wrote:
> 
> > During VM oom condition, kill all threads in process group.
> > 
> > We have had complaints where a threaded application is left in a bad
> > state after one of it's threads is killed when we hit a VM: out_of_memory
> > condition.
> > Killing just one of the process threads can leave the application in a
> > bad state, whereas killing the entire process group would allow for
> > the application to restart, or be otherwise handled, and makes it very
> > obvious that something has gone wrong.
> > 
> > This change allows the entire process group to be taken down, rather
> > than just the one thread.
> 
> Just checking...
> 
> blackfin
> h8300
> m68knommu
> uml
> v850
> 
> were not changed.  Intentional?

Yes.  Those arch's don't have the VM oom code that I could see.   There
is an occasional do_exit() reference elsewhere in the fault handler
code, for reasons other than VM oom, so I deliberately didn't touch
those either.    

Re: [PATCH respin, was PATCH for review] During VM oom condition, kill all threads in process group

[Christoph Hellwig]

On Tue, Jul 24, 2007 at 09:28:44AM -0500, Will Schmidt wrote:
> > Just checking...
> > 
> > blackfin
> > h8300
> > m68knommu
> > uml
> > v850
> > 
> > were not changed.  Intentional?
> 
> Yes.  Those arch's don't have the VM oom code that I could see.   There
> is an occasional do_exit() reference elsewhere in the fault handler
> code, for reasons other than VM oom, so I deliberately didn't touch
> those either.    

Except for uml these are the !CONFIG_MMU only architectures, so that makes
a lot of sense.

Re: [PATCH respin, was PATCH for review] During VM oom condition, kill all threads in process group

[Pavel Machek]

Hi!

> 
> During VM oom condition, kill all threads in process group.
> 
> We have had complaints where a threaded application is left in a bad
> state after one of it's threads is killed when we hit a VM: out_of_memory
> condition.
> Killing just one of the process threads can leave the application in a
> bad state, whereas killing the entire process group would allow for
> the application to restart, or be otherwise handled, and makes it very
> obvious that something has gone wrong.
> 
> This change allows the entire process group to be taken down, rather
> than just the one thread.
> 
> Signed-off-by: Will Schmidt <will_schmidt@vnet.ibm.com>

> diff --git a/arch/sparc64/mm/fault.c b/arch/sparc64/mm/fault.c
> index 17123e9..13fdfa3 100644
> --- a/arch/sparc64/mm/fault.c
> +++ b/arch/sparc64/mm/fault.c
> @@ -466,7 +466,7 @@ out_of_memory:
>  	up_read(&mm->mmap_sem);
>  	printk("VM: killing process %s\n", current->comm);
>  	if (!(regs->tstate & TSTATE_PRIV))
> -		do_exit(SIGKILL);
> +		do_group_exit(SIGKILL);
>  	goto handle_kernel_fault;
>  
>  intr_or_no_mm:

is the printk still accurate (does it kill more than one process now)?
Why does it print when it will not really kill the process?

I see similar code across all the archs... would it make sense to
create common helper... or is the helper too trivial?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH respin, was PATCH for review] During VM oom condition, kill all threads in process group

[Will Schmidt]

On Tue, 2007-07-31 at 11:31 +0200, Pavel Machek wrote:
> Hi!
> 
> > 
> > During VM oom condition, kill all threads in process group.
> > 
> > We have had complaints where a threaded application is left in a bad
> > state after one of it's threads is killed when we hit a VM: out_of_memory
> > condition.
> > Killing just one of the process threads can leave the application in a
> > bad state, whereas killing the entire process group would allow for
> > the application to restart, or be otherwise handled, and makes it very
> > obvious that something has gone wrong.
> > 
> > This change allows the entire process group to be taken down, rather
> > than just the one thread.
> > 
> > Signed-off-by: Will Schmidt <will_schmidt@vnet.ibm.com>
> 
> > diff --git a/arch/sparc64/mm/fault.c b/arch/sparc64/mm/fault.c
> > index 17123e9..13fdfa3 100644
> > --- a/arch/sparc64/mm/fault.c
> > +++ b/arch/sparc64/mm/fault.c
> > @@ -466,7 +466,7 @@ out_of_memory:
> >  	up_read(&mm->mmap_sem);
> >  	printk("VM: killing process %s\n", current->comm);
> >  	if (!(regs->tstate & TSTATE_PRIV))
> > -		do_exit(SIGKILL);
> > +		do_group_exit(SIGKILL);
> >  	goto handle_kernel_fault;
> >  
> >  intr_or_no_mm:
> 
> is the printk still accurate (does it kill more than one process now)?

I was going to double-check this morning.. but don't see where
current->comm is copied into a new task_struct.  I thought that all
processes within the group had the same current->comm value, so figure
this is OK.  

> Why does it print when it will not really kill the process?
no idea..  

> I see similar code across all the archs... would it make sense to
> create common helper... or is the helper too trivial?

The checks blocking flow into do_group_exit, like (regs->tstate &
TSTATE_PRIV) for sparc64, or (user_mode(regs)) for powerpc, do vary
across the arch's.    The code could be rearranged to have a helper
containing just the printk and the do_group_exit() call; but I'm not
sure that would be an improvement. 

maybe a    do_group_sigkill_if(condition); helper    :-)


-Will 

> 									Pavel