Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1159961 > unrolled thread

Re: [RFC PATCH 06/18] signal/kthread: Initial implementation of kthread signal handling

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH 06/18] signal/kthread: Initial implementation of  kthread signal handling Oleg Nesterov <oleg@redhat.com> - 2015-06-07 00:00 +0200
    Re: [RFC PATCH 06/18] signal/kthread: Initial implementation of  kthread signal handling Oleg Nesterov <oleg@redhat.com> - 2015-06-15 21:20 +0200

#1159961 — Re: [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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1165497

Hi Petr,

On 06/15, Petr Mladek wrote:
>
> I am sorry for the late reply. I wanted to think more before answering
> all the mails.

Don't worry I am always late ;)

> On Mon 2015-06-08 23:13:36, Oleg Nesterov wrote:
> > >
> > > Hmm, the helper would have a strange semantic. You need to take
> > > sighand->siglock, dequeue the signal (SIGSTOP), and call
> > > __set_current_state(TASK_STOPPED) before you release the lock.
> > > But what would happen if the dequeued signal is _not_ SIGSTOP?
> >
> > Perhaps I missed your point, but no. If you want to handle SIGSTOP
> > you can do
> >
>
> I think that we need to add:
>
> 	spin_lock_irq(&sighand->siglock);
>
> > 	signr = kthread_signal_dequeue();
> > 	switch (signr) {
> > 	case SIGSTOP:
> > 		something_else();
> > 		kthread_do_signal_stop();
> > 	...
> > 	}
>
> And if we want to avoid any race, kthread_do_signal_stop() should look like:
>
> void kthread_do_signal_stop(unsigned long flags)
> {
> 	struct sighand_struct *sighand = current->sighand;
>
> 	__set_current_state(TASK_STOPPED);
> 	spin_unlock_irqrestore(&sighand->siglock, flags);
> 	/* Don't run again until woken by SIGCONT or SIGKILL */
> 	freezable_schedule();
> }

Ah, understand. You think that we need to take ->siglock in advance
to avoid the race with SIGCONT?

No, we don't. Let me show you the code I suggested again:

	void kthread_do_signal_stop(void)
	{
		spin_lock_irq(&curtent->sighand->siglock);
		if (current->jobctl & JOBCTL_STOP_DEQUEUED)
			__set_current_state(TASK_STOPPED);
		spin_unlock_irq(&current->sighand->siglock);

		schedule();
	}
	
so you can dequeue_signal() and call kthread_do_signal_stop() without
holding ->siglock. We can rely on JOBCTL_STOP_DEQUEUED bit. SIGCONT
clears it, so kthread_do_signal_stop() can't race.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web