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, ¤t->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.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
