lkml.org 
[lkml]   [2015]   [Jun]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH 06/18] signal/kthread: Initial implementation of kthread signal handling
    On 06/05, Petr Mladek wrote:
    >
    > The main question is how much it should follow POSIX and the signal
    > handling of user space processes. On one hand, we want to be as close
    > as possible.

    Why? Let the kthread decide what it should if it gets, say, SIGSTOP.

    > Finally, kthread_do_signal() is called on a safe place in the main
    > iterant kthread cycle. Then we will not need any special code for
    > signals when using this kthread API.

    OK, I will not comment other parts of iterant API in this thread.

    But as for signal handling, to me a single kthread_iterant->do_signal()
    callback looks better. Rather than multiple callbacks passed as
    ->kthread_sa_handler.

    That single callback can deque a signal and decide what it should do.

    > + spin_lock_irqsave(&sighand->siglock, flags);
    > +
    > + if (unlikely(signal->flags & SIGNAL_CLD_MASK)) {
    > + WARN(1, "there are no parents for kernel threads\n");
    > + signal->flags &= ~SIGNAL_CLD_MASK;
    > + }
    > +
    > + for (;;) {
    > + struct k_sigaction *ka;
    > +
    > + signr = dequeue_signal(current, &current->blocked, &ksig.info);
    > +
    > + if (!signr)
    > + break;
    > +
    > + ka = &sighand->action[signr-1];
    > +
    > + /* Do nothing for ignored signals */
    > + if (ka->sa.kthread_sa_handler == KTHREAD_SIG_IGN)
    > + continue;

    Again, I agree something like the simple kthread_dequeue_signal() makes
    sense. Say, to drop the ignore signal like this code does. Although I
    do not think this is really important, SIG_IGN is only possible if this
    kthread does something strange. Say, blocks/unblocs the ignored signal.

    > +
    > + /* Run the custom handler if any */
    > + if (ka->sa.kthread_sa_handler != KTHREAD_SIG_DFL) {
    > + ksig.ka = *ka;
    > +
    > + if (ka->sa.sa_flags & SA_ONESHOT)
    > + ka->sa.kthread_sa_handler = KTHREAD_SIG_DFL;
    > +
    > + spin_unlock_irqrestore(&sighand->siglock, flags);
    > + /* could run directly for kthreads */
    > + ksig.ka.sa.kthread_sa_handler(signr);
    > + freezable_cond_resched();
    > + goto relock;

    Well. But for what? A simple "switch (signr)" after kthread_dequeue_signal()
    can do the same. Or, speaking of kthread_iterant_fn() it can even dequeue the
    signal and pass it to kti->whatever(signr).

    > + if (sig_kernel_ignore(signr))
    > + continue;

    For what? Why a kthread should unignore (say) SIGWINCH if it is not going
    to react?

    > + if (sig_kernel_stop(signr)) {
    > + __set_current_state(TASK_STOPPED);
    > + spin_unlock_irqrestore(&sighand->siglock, flags);
    > + /* Don't run again until woken by SIGCONT or SIGKILL */
    > + freezable_schedule();
    > + goto relock;

    Yes this avoids the race with SIGCONT. But as I said we can add another
    trivial helper which checks JOBCTL_STOP_DEQUEUED. So a kthread can do
    this itself.

    To me, SIG_DFL behaviour just makes makes no sense when it comes to
    kthreads. I do not even think this can simplify the code. Unlike user-
    space task, kthread can happily dequeue SIGSTOP, so why should we mimic
    the userspace SIG_DFL logic.


    > + /* Death signals, but try to terminate cleanly */
    > + kthread_stop_current();
    > + __flush_signals(current);
    > + break;

    The same.

    Oleg.



    \
     
     \ /
      Last update: 2015-06-07 00:21    [W:4.475 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site