Re: [PATCH 2/4] coredump: speedup SIGKILL sending

Re: [PATCH 2/4] coredump: speedup SIGKILL sending

[Lee Revell]

On Fri, 2006-04-07 at 02:06 +0400, Oleg Nesterov wrote:
> With this patch a thread group is killed atomically under ->siglock.
> This is faster because we can use sigaddset() instead of force_sig_info()
> and this is used in further patches.
> 
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

Won't this cause huge latencies when a process with lots of threads is
killed?

Lee

Re: [PATCH 2/4] coredump: speedup SIGKILL sending

[Lee Revell]

On Fri, 2006-04-07 at 03:55 +0400, Oleg Nesterov wrote:
> On 04/06, Lee Revell wrote:
> > On Fri, 2006-04-07 at 02:06 +0400, Oleg Nesterov wrote:
> > > With this patch a thread group is killed atomically under ->siglock.
> > > This is faster because we can use sigaddset() instead of force_sig_info()
> > > and this is used in further patches.
> > > 
> > > Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> > 
> > Won't this cause huge latencies when a process with lots of threads is
> > killed?
> 
> Yes, irqs are disabled. But this is not worse than 'kill -9 pid', note
> that __group_complete_signal() or zap_other_threads() do the same.

Those have been problematic in the past.  I am just wondering if this
will be a latency regression, or if changes elsewhere in your patch
negate the effect.

I'm just concerned because it was a lot of work over ~2 years to get 2.6
to perform decently in this area, and we have regressed since 2.6.14 (VM
issue).

Lee

[PATCH 2/4] coredump: speedup SIGKILL sending

[Oleg Nesterov]

With this patch a thread group is killed atomically under ->siglock.
This is faster because we can use sigaddset() instead of force_sig_info()
and this is used in further patches.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- rc1/fs/exec.c~2_KILL	2006-04-06 15:10:28.000000000 +0400
+++ rc1/fs/exec.c	2006-04-06 15:19:33.000000000 +0400
@@ -1387,17 +1387,24 @@ static void format_corename(char *corena
 static void zap_process(struct task_struct *start, int *ptraced)
 {
 	struct task_struct *t;
+	unsigned long flags;
+
+	spin_lock_irqsave(&start->sighand->siglock, flags);
 
 	t = start;
 	do {
 		if (t != current && t->mm) {
 			t->mm->core_waiters++;
-			force_sig_specific(SIGKILL, t);
+			sigaddset(&t->pending.signal, SIGKILL);
+			signal_wake_up(t, 1);
+
 			if (unlikely(t->ptrace) &&
 			    unlikely(t->parent->mm == t->mm))
 				*ptraced = 1;
 		}
 	} while ((t = next_thread(t)) != start);
+
+	spin_unlock_irqrestore(&start->sighand->siglock, flags);
 }
 
 static void zap_threads (struct mm_struct *mm)

Re: [PATCH 2/4] coredump: speedup SIGKILL sending

[Oleg Nesterov]

On 04/06, Lee Revell wrote:
> On Fri, 2006-04-07 at 02:06 +0400, Oleg Nesterov wrote:
> > With this patch a thread group is killed atomically under ->siglock.
> > This is faster because we can use sigaddset() instead of force_sig_info()
> > and this is used in further patches.
> > 
> > Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> 
> Won't this cause huge latencies when a process with lots of threads is
> killed?

Yes, irqs are disabled. But this is not worse than 'kill -9 pid', note
that __group_complete_signal() or zap_other_threads() do the same.

Oleg.

Re: [PATCH 2/4] coredump: speedup SIGKILL sending

[Lee Revell]

On Sat, 2006-04-08 at 03:28 +0400, Oleg Nesterov wrote:
> zap_process() disables irqs while traversing ->thread_group list.
> So yes, if a process has a lot of threads it will be a latency regression.
> (but again, __group_complete_signal() does the same while delivering this
> signal, so I don't think this change can make things worse).
> 
> However this allows us to avoid tasklist_lock in zap_threads() so I think
> it is worth it. Please note that tasklist_lock was held while iterating
> over _all_ threads in system, not only current's thread group.
> 

OK sounds good.

Lee

Re: [PATCH 2/4] coredump: speedup SIGKILL sending

[Oleg Nesterov]

On 04/06, Lee Revell wrote:
> On Fri, 2006-04-07 at 03:55 +0400, Oleg Nesterov wrote:
> > On 04/06, Lee Revell wrote:
> > > On Fri, 2006-04-07 at 02:06 +0400, Oleg Nesterov wrote:
> > > > With this patch a thread group is killed atomically under ->siglock.
> > > > This is faster because we can use sigaddset() instead of force_sig_info()
> > > > and this is used in further patches.
> > > >
> > > > Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> > >
> > > Won't this cause huge latencies when a process with lots of threads is
> > > killed?
> >
> > Yes, irqs are disabled. But this is not worse than 'kill -9 pid', note
> > that __group_complete_signal() or zap_other_threads() do the same.
>
> Those have been problematic in the past.  I am just wondering if this
> will be a latency regression, or if changes elsewhere in your patch
> negate the effect.

zap_process() disables irqs while traversing ->thread_group list.
So yes, if a process has a lot of threads it will be a latency regression.
(but again, __group_complete_signal() does the same while delivering this
signal, so I don't think this change can make things worse).

However this allows us to avoid tasklist_lock in zap_threads() so I think
it is worth it. Please note that tasklist_lock was held while iterating
over _all_ threads in system, not only current's thread group.

Oleg.

Re: [PATCH 2/4] coredump: speedup SIGKILL sending

[Roland McGrath]

> With this patch a thread group is killed atomically under ->siglock.
> This is faster because we can use sigaddset() instead of force_sig_info()
> and this is used in further patches.
> 
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

Looks good to me.

Signed-off-by: Roland McGrath <roland@redhat.com>


Thanks,
Roland