On Tue, 26 Jun 2007 03:39:23 +0530 "Satyam Sharma" <[EMAIL PROTECTED]> wrote:
> Hi Jeff, > > [ Trimmed netdev from Cc: list, added Christoph. ] > > On 6/26/07, Jeff Layton <[EMAIL PROTECTED]> wrote: > > On Tue, 26 Jun 2007 01:11:20 +0530 > > "Satyam Sharma" <[EMAIL PROTECTED]> 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 I have no issue with it. I just didn't feel confident enough to know whether that might have harmful side effects. Right offhand I don't see why adding a force_sig() to kthread_stop() would be an issue. It seems like if you're wanting to stop the thread anyway then a signal probably wouldn't hurt anything. -- Jeff Layton <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/