[lkml]   [2007]   [Jun]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] RFC: have tcp_recvmsg() check kthread_should_stop() and treat it as if it were signalled
    Hi Oleg,

    Thanks for your comments, I'm still not convinced, however.

    On 6/26/07, Oleg Nesterov <> wrote:
    > On 06/26, Satyam Sharma wrote:
    > >
    > > Yes, why not embed a send_sig(SIGKILL) just before the wake_up_process()
    > > in kthread_stop() itself?
    > 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.

    [ Well, first of all, anybody who sends a possibly-blocking-forever
    function like tcp_recvmsg() to a *workqueue* needs to get his head
    checked. ]

    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.

    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),
    and we'll actually end up having a nice little situation on our hands if
    we make the mistake of calling flush_workqueue on such a wq.

    Note that the exact scenario you're talking about wouldn't mean the
    kthread getting killed before it's supposed to be stopped anyway.
    force_sig is not a synchronous wakeup, and also note that tcp_recvmsg()
    or skb_recv_datagram() etc will exit (and are supposed to exit) cleanly on
    seeing a signal.

    > > 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)
    > No, this can't work in general. Some kthreads do flush_signals/dequeue_signal,
    > so TIF_SIGPENDING can be lost anyway.

    Yup, I had thought of precisely this issue yesterday as well. The mental note
    I made to myself was that the force_sig(SIGKILL) and wake_up_process() in
    kthread_stop() must be atomic so that the following race is not possible:

    #1 -> thread that invokes kthread_stop()
    #2 -> kthread to be stopped, (may be) currently in wait_event_interruptible(),
    such that there is a bigger loop over the wait_event_interruptible()
    itself, which puts task back to sleep if this was a spurious wake up
    (if _not_ due to a signal).

    Thread #1 Thread #2
    ========= =========

    skb_recv_datagram() ->

    <scheduled out>

    <wakes up, sees the pending signal,
    breaks out of wait_for_packet()
    and skb_recv_datagram() back out to
    our kthread code itself, but there
    we see that kthread_should_stop() is
    NOT yet true, we also see this
    spurious signal, flush it, and call
    skb_recv_datagram() all over again>
    skb_recv_datagram() ->

    <scheduled in>
    kthread_stop() -> wake_up_process()

    <this time we don't even break out of
    the skb_recv_datagram() either, as
    no signals are pending any more>

    i.e. thread #2 still does not exit cleanly. The root of the problem is that
    functions such as skb_recv_datagram() -> wait_for_packet() handle spurious
    wakeups *internally* by themselves, so our kthread does not get a chance to
    check for kthread_should_stop().

    Of course, above race is true only for kthreads that do flush signals on
    seeing spurious ones periodically. If it did not, then skb_recv_datagram()
    called second time above would again have broken out because of
    signal_pending() and we wouldn't have gone back to sleep. But we have to
    be on safer side and avoid races *irrespective* of what the kthread might
    or might not do, so let's _not_ depend on _assumed kthread behaviour_.

    I suspect the above race be avoided by making force_sig() and
    wake_up_process() atomic in kthread_stop() itself, please correct me
    if I'm horribly wrong.

    > I personally think Jeff's idea to use force_sig() is right. kthread_create()
    > doesn't use CLONE_SIGHAND, so it is safe to change ->sighand->actionp[].
    > (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 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?

    Please comment.

    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2007-06-27 00:59    [W:0.032 / U:10.332 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site