Messages in this thread | | | Date | Wed, 27 Jun 2007 16:24:37 +0400 | From | Oleg Nesterov <> | Subject | Re: [PATCH] RFC: have tcp_recvmsg() check kthread_should_stop() and treat it as if it were signalled |
| |
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 <oleg@tv-sign.ru> 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/
| |