Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759798AbXF0MYE (ORCPT ); Wed, 27 Jun 2007 08:24:04 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1756058AbXF0MXw (ORCPT ); Wed, 27 Jun 2007 08:23:52 -0400 Received: from mail.screens.ru ([213.234.233.54]:44206 "EHLO mail.screens.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755916AbXF0MXv (ORCPT ); Wed, 27 Jun 2007 08:23:51 -0400 Date: Wed, 27 Jun 2007 16:24:37 +0400 From: Oleg Nesterov To: Satyam Sharma Cc: Jeff Layton , Herbert Xu , linux-kernel@vger.kernel.org, netdev@vger.kernel.org, "Eric W. Biederman" , linux-cifs-client@lists.samba.org Subject: Re: [PATCH] RFC: have tcp_recvmsg() check kthread_should_stop() and treat it as if it were signalled Message-ID: <20070627122437.GA158@tv-sign.ru> References: <20070608123527.9b4cdafe.jlayton@redhat.com> <20070609070826.7bd3480c.jlayton@redhat.com> <20070626115449.GA92@tv-sign.ru> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.11 Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2988 Lines: 78 On 06/27, Satyam Sharma wrote: > > Thanks for your comments, I'm still not convinced, however. An perhaps you are right. I don't have a very strong opinion on that. Still I can't understand why it is better if kthread_stop() sends a signal as well. Contrary, I believe we should avoid signals when it comes to kernel threads. One can always use force_sig() or allow_signal() + send_sig() when it is really needed, like cifs does. > On 6/26/07, Oleg Nesterov wrote: > > > >Personally, I don't think we should do this. > > > >kthread_stop() doesn't always mean "kill this thread asap". Suppose that > >CPU_DOWN does kthread_stop(workqueue->thread) but doesn't flush the queue > >before that (we did so before 2.6.22 and perhaps we will do again). Now > >work_struct->func() doing tcp_recvmsg() or wait_event_interruptible() > >fails, > >but this is probably not that we want. > > Anyway, I think _all_ usages of kthread_stop() in the kernel *do* want > the thread to stop *right then*. After all, kthread_stop() doesn't even > return (gets blocked on wait_for_completion()) till it knows the target > kthread *has* exited completely. Yes, kthread_stop(k) means that k should exit eventually, but I don't think that kthread_stop() should try to force the exit. > And if a workqueue is blocked on tcp_recvmsg() or skb_recv_datagram() > or some such, I don't see how that flush_workqueue (if that is what you > meant) would succeed anyway (unless you do send the signal too), timeout, but this was just a silly example. I am talking about the case when wait_event_interruptible() should not fail (unless something bad happens) inside the "while (!kthread_should_stop())" loop. Note also that kthread could use TASK_INTERRUPTIBLE sleep because it doesn't want to contribute to loadavg, and because it knows that all signals are ignored. > Note that the exact scenario you're talking about wouldn't mean the > kthread getting killed before it's supposed to be stopped anyway. Yes sure, we can't kill the kernel thread via signal. I meant we can have some unexpected failure. > >(offtopic) > > > > cifs_mount: > > > > send_sig(SIGKILL,srvTcp->tsk,1); > > tsk = srvTcp->tsk; > > if(tsk) > > kthread_stop(tsk); > > > >This "if(tsk)" looks wrong to me. > > I think it's bogus myself. [ Added linux-cifs-client@lists.samba.org to Cc: > ] > > >Can srvTcp->tsk be NULL? If yes, send_sig() > >is not safe. Can srvTcp->tsk become NULL after send_sig() ? If yes, this > >check is racy, and kthread_stop() is not safe. > > That's again something the atomicity I proposed above could avoid? I think this "if(tsk)" is just bogus, and should be killed. Oleg. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/