Re: [PATCH] RFC: have tcp_recvmsg() check kthread_should_stop() and treat it as if it were signalled

[Satyam Sharma]

Hi Jeff,

[ Trimmed netdev from Cc: list, added Christoph. ]

On 6/26/07, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Tue, 26 Jun 2007 01:11:20 +0530
> "Satyam Sharma" <satyam.sharma@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/