Messages in this thread | | | Date | Tue, 14 Feb 2012 15:03:23 -0800 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH] sysrq: Use SEND_SIG_FORCED instead of force_sig() |
| |
On Wed, Feb 15, 2012 at 02:50:17AM +0400, Anton Vorontsov wrote: > Change send_sig_all() to use do_send_sig_info(SEND_SIG_FORCED) > instead of force_sig(SIGKILL). With the recent changes we do not > need force_ to kill the CLONE_NEWPID tasks. > > And this is more correct. force_sig() can race with the exiting > thread, while do_send_sig_info(group => true) kill the whole > process. > > Suggested-by: Oleg Nesterov <oleg@redhat.com> > Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org> > --- > > On Fri, Feb 10, 2012 at 09:10:08PM +0100, Oleg Nesterov wrote: > > > --- a/drivers/tty/sysrq.c > > > +++ b/drivers/tty/sysrq.c > > > @@ -324,9 +324,12 @@ static void send_sig_all(int sig) > > > > > > read_lock(&tasklist_lock); > > > for_each_process(p) { > > > - if (p->mm && !is_global_init(p)) > > > - /* Not swapper, init nor kernel thread */ > > > - force_sig(sig, p); > > > + if (p->flags & PF_KTHREAD) > > > + continue; > > > + if (is_global_init(p)) > > > + continue; > > > + > > > + force_sig(sig, p); > > > } > > > read_unlock(&tasklist_lock); > > > > Obviously I agree with this change. > > > > But where does this read_lock(tasklist) come from? > > It came from this patch: http://lkml.org/lkml/2012/2/7/24 > > > We discussed this with Anton. Yes, tasklist ensures that > > force_sig() can't crash the kernel. But it is still wrong > > and should not be used. > > > > I think send_sig_all() should use SEND_SIG_FORCED (this > > depends on the patches I sent to Andrew), in this case > > tasklist is not needed. > > Well, I think the lock is still a good thing: we don't want > any new processes to be created while we kill others. > > I might be wrong, but copy_process() issues recalc_sigpending() > under tasklist lock especially the for this scenario. > > So, in this and in OOM cases we have to be precise (unlike LMK). > Sysrq is a rare thing, so there is actually should be no problem > with holding the lock. > > So, how about this patch? > > Greg, can we take it via -mm tree, as it depends on a few > sched patches?
That's fine with me: Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
| |