Re: [PATCH 2/3] [PATCH powerpc] during VM oom condition, kill all threads in process group

Re: [PATCH 2/3] [PATCH powerpc] during VM oom condition, kill all threads in process group

[Will Schmidt]

Whoops..  sorry about any reply bounces, I flubbed the cc to
linuxppc-dev@ozlabs.org . 

-Will

On Tue, 2007-05-06 at 12:48 -0500, Will Schmidt wrote:
> When we get into a state where VM has ran out of memory, and it's time to
> thwack a process, we should take out the entire process group, rather than
> just one thread.
> 
> Tested on POWER5.
> 
> Signed-off-by: Will Schmidt <will_schmidt@vnet.ibm.com>
> ---
> 
>  arch/powerpc/mm/fault.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 03aeb3a..9afe871 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -392,8 +392,10 @@ out_of_memory:
>  		goto survive;
>  	}
>  	printk("VM: killing process %s\n", current->comm);
> -	if (user_mode(regs))
> +	if (user_mode(regs)) {
> +		zap_other_threads(current);
>  		do_exit(SIGKILL);
> +	}
>  	return SIGKILL;
>  
>  do_sigbus:
> 
> 

Re: [PATCH 1/3] [PATCH i386] during VM oom condition, kill all threads in process group

[Will Schmidt]

On Thu, 2007-06-07 at 17:10 -0700, Andrew Morton wrote:
> On Thu, 7 Jun 2007 18:16:21 -0500
> Anton Blanchard <anton@samba.org> wrote:
> 
> >  
> > Hi,
> > 
> > > zap_other_threads() requires tasklist_lock.

Yup, I missed that.   Thanks for pointing it out.

> > > 
> > > If we're going to do this then we should probably create some new function
> > > (with a better name) which takes tasklsit_lock and then calls
> > > zap_other_threads().

I expect this will be a write_lock_irq() since zap_other_threads will be
doing a bit more than just reading the task info.

This will be down in a do-page-fault failure path (see
arch/*/mm/fault.c).  I wonder if calling write_lock is going to be safe,
or if its possible to get into a deadlock?  i.e. should I branch back up
to the survive: label if I can't take the lock?  Would that even be
sufficient? or is it not an issue here? 

> > > 
> > > Does this patch fix any observed-in-the-real-world problem?  If so, please
> > > describe it.
> > 
> > Yeah we have had complaints where threaded apps have only one thread
> > shot down instead of the entire process. This leaves the application in
> > a bad state, whereas if it had been killed cleanly the application could
> > have restarted.
> > 
> > My understanding is that fatal signals should kill all threads in the
> > group.
> > 
> 
> OK, well could we please get all that info appropriatelt captured in #2's
> changelog?
Yup, next spin I'll add more to the changelog. 

> 
> Other architectures will probably need to implement this.

-Will

Re: [PATCH 1/3] [PATCH i386] during VM oom condition, kill all threads in process group

[Andrew Morton]

On Fri, 08 Jun 2007 14:19:18 -0500
Will Schmidt <will_schmidt@vnet.ibm.com> wrote:

> > > > zap_other_threads() requires tasklist_lock.
> 
> Yup, I missed that.   Thanks for pointing it out.
> 
> > > > 
> > > > If we're going to do this then we should probably create some new function
> > > > (with a better name) which takes tasklsit_lock and then calls
> > > > zap_other_threads().
> 
> I expect this will be a write_lock_irq() since zap_other_threads will be
> doing a bit more than just reading the task info.

No, I think read_lock() will be sufficient.

In fact, it's probably the case that rcu_read_lock() is now sufficient
locking coverage for zap_other_threads() (cc's people).

It had better be, because do_group_exit() forgot to take tasklist_lock.  It
is perhaps relying upon spin_lock()'s hidden rcu_read_lock() properties
without so much as a code comment, which would be somewhat nasty of it.

You could perhaps just call do_group_exit() from within the fault handler,
btw.

> This will be down in a do-page-fault failure path (see
> arch/*/mm/fault.c).  I wonder if calling write_lock is going to be safe,
> or if its possible to get into a deadlock?  i.e. should I branch back up
> to the survive: label if I can't take the lock?  Would that even be
> sufficient? or is it not an issue here? 

You can take the lock in the fault handler.  Nobody should be getting
pagefaults while holding tasklist_lock.  (Well, a vmalloc fault might, but
that's a special-case which doesn't allocate memory or anything like that).

Re: [PATCH 1/3] [PATCH i386] during VM oom condition, kill all threads in process group

[Will Schmidt]

On Fri, 2007-06-08 at 12:32 -0700, Andrew Morton wrote:
> On Fri, 08 Jun 2007 14:19:18 -0500
> Will Schmidt <will_schmidt@vnet.ibm.com> wrote:
> 
> > > > > zap_other_threads() requires tasklist_lock.
> > 

> In fact, it's probably the case that rcu_read_lock() is now sufficient
> locking coverage for zap_other_threads() (cc's people).
> 
> It had better be, because do_group_exit() forgot to take tasklist_lock.  It
> is perhaps relying upon spin_lock()'s hidden rcu_read_lock() properties
> without so much as a code comment, which would be somewhat nasty of it.

> You could perhaps just call do_group_exit() from within the fault
> handler,
> btw.

Yup, so looks like I can actually replace the existing do_exit() call
with do_group_exit().   I'll sit on this for a bit to give other folks a
chance to comment on which lock call is sufficient, read_lock() or
rcu_read_lock(), etc;  and do_group_exit()'s issue with taking
tasklist_lock. 

Thanks,

-Will

Re: [PATCH 1/3] [PATCH i386] during VM oom condition, kill all threads in process group

[Eric W. Biederman]

Will Schmidt <will_schmidt@vnet.ibm.com> writes:

> On Fri, 2007-06-08 at 12:32 -0700, Andrew Morton wrote:
>> On Fri, 08 Jun 2007 14:19:18 -0500
>> Will Schmidt <will_schmidt@vnet.ibm.com> wrote:
>> 
>> > > > > zap_other_threads() requires tasklist_lock.
>> > 
>
>> In fact, it's probably the case that rcu_read_lock() is now sufficient
>> locking coverage for zap_other_threads() (cc's people).
>> 
>> It had better be, because do_group_exit() forgot to take tasklist_lock.  It
>> is perhaps relying upon spin_lock()'s hidden rcu_read_lock() properties
>> without so much as a code comment, which would be somewhat nasty of it.
>
>> You could perhaps just call do_group_exit() from within the fault
>> handler,
>> btw.
>
> Yup, so looks like I can actually replace the existing do_exit() call
> with do_group_exit().   I'll sit on this for a bit to give other folks a
> chance to comment on which lock call is sufficient, read_lock() or
> rcu_read_lock(), etc;  and do_group_exit()'s issue with taking
> tasklist_lock. 

No.  The rcu_read_lock is not sufficient.
Yes.  sighand->siglock is enough, and we explicitly take it in
do_group_exit before calling zap_other_threads.

Unless I have completely miss-understood this thread.

Eric

Re: [PATCH 1/3] [PATCH i386] during VM oom condition, kill all threads in process group

[Oleg Nesterov]

On 06/08, Eric W. Biederman wrote:
>
> Will Schmidt <will_schmidt@vnet.ibm.com> writes:
> 
> > On Fri, 2007-06-08 at 12:32 -0700, Andrew Morton wrote:
> >> On Fri, 08 Jun 2007 14:19:18 -0500
> >> Will Schmidt <will_schmidt@vnet.ibm.com> wrote:
> >> 
> >> > > > > zap_other_threads() requires tasklist_lock.
> >> > 
> >
> >> In fact, it's probably the case that rcu_read_lock() is now sufficient
> >> locking coverage for zap_other_threads() (cc's people).
> >> 
> >> It had better be, because do_group_exit() forgot to take tasklist_lock.  It
> >> is perhaps relying upon spin_lock()'s hidden rcu_read_lock() properties
> >> without so much as a code comment, which would be somewhat nasty of it.
> >
> >> You could perhaps just call do_group_exit() from within the fault
> >> handler,
> >> btw.
> >
> > Yup, so looks like I can actually replace the existing do_exit() call
> > with do_group_exit().   I'll sit on this for a bit to give other folks a
> > chance to comment on which lock call is sufficient, read_lock() or
> > rcu_read_lock(), etc;  and do_group_exit()'s issue with taking
> > tasklist_lock. 
> 
> No.  The rcu_read_lock is not sufficient.
> Yes.  sighand->siglock is enough, and we explicitly take it in
> do_group_exit before calling zap_other_threads.

Yes, we don't need tasklist_lock (or rcu_read_lock).

de_thread() calls zap_other_threads() under tasklist_lock, but this
is because we can change child_reaper.

Oleg.