Messages in this thread | | | Date | Tue, 26 Jun 2007 03:39:23 +0530 | From | "Satyam Sharma" <> | Subject | Re: [PATCH] RFC: have tcp_recvmsg() check kthread_should_stop() and treat it as if it were signalled |
| |
Hi Jeff,
[ Trimmed netdev from Cc: list, added Christoph. ]
On 6/26/07, Jeff Layton <jlayton@redhat.com> wrote: > On Tue, 26 Jun 2007 01:11:20 +0530 > "Satyam Sharma" <satyam.sharma@gmail.com> wrote: > > [...] > > Yes, why not embed a send_sig(SIGKILL) just before the wake_up_process() > > in kthread_stop() itself? > > > > Looking at some happily out-of-date comments in the kthread code, I can > > guess that at some point of time (perhaps very early drafts) Rusty actually > > *did* implement the whole kthread_stop() functionality using signals, but > > I suspect it might've been discarded and the kthread_stop_info approach > > used instead to protect from spurious signals from userspace. (?) > > > > So could we have signals in _addition_ to kthread_stop_info and change > > kthread_should_stop() to check for both: > > > > kthread_stop_info.k == current && signal_pending(current) > > > > If !kthread_should_stop() && signal_pending(current) => spurious signal, > > so just flush and discard (in the kthread). > > [...] > > Why is it wrong for kthreads to let signals through? We can ignore out > > all signals we're not interested in, and flush the spurious ones ... > > otherwise there really isn't much those kthreads can do that get blocked > > in such functions, is there? > > Yes, after I wrote that I began to question that assumption too. I was > pretty much going on a statement by Christoph Hellwig on an earlier > patch that I did:
Ok, I found both the threads / patches you referred to ...
> -----[snip]------ > The right way to fix this is to stop sending signals at all and have > a kernel-internal way to get out of kernel_recvmsg. Uses of signals by > kernel thread generally are bugs. > -----[snip]------ > > Though this makes no sense to me. I don't see any reason why kthreads > can't use signals, and hacking support for breaking out of sleeping > functions seems redundant.
Right, signals _are_ the "signalling" mechanism all through kernel code already, anything else would clearly be redundant.
But I've listened / participated in other discussions about kthreads and signals and the general feeling is that (somebody correct me if I'm wrong) kernel threads are a kernel _implementation detail_ after all, and good design must ensure that userspace be unaware of even their existence. And I agree with that, but the real ugly uses of signals by kernel threads are those cases where we want to export a full signals-based interface to our kthread to userspace (such cases do exist in mainline, I think). But that's clearly not the case with the usage here.
> My latest patch for cifsd has it block all signals from userspace > and uses force_sig() instead of send_sig() when trying to stop the > thread. This seems to work pretty well and still insulates the thread > from userspace signals.
Thanks, I find this solution much cleaner too, so now we avoid any sort of spuriousness getting in from userspace (and pretty much takes care of all the checking-if-spurious-and-flushing business I referred to earlier).
But how about making this part of kthreads proper? Functions such as skb_recv_datagram etc are pretty standard (and others such would also exist or get added in due time) and it is not exactly intuitive for a developer to add a force_sig(SIGKILL) before the kthread_stop() to ensure that the kthread using such functions does exit properly. [ I can foresee cases in the future when such functions are added to kthreads that did not have them previously, and suddenly someone reports a regression that the kthread stops exiting cleanly. ]
Satyam - 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/
| |