Path: csiph.com!aioe.org!bofh.it!news.nic.it!robomod From: Oleg Nesterov Newsgroups: linux.kernel Subject: Re: [RFC PATCH 06/18] signal/kthread: Initial implementation of kthread signal handling Date: Sun, 07 Jun 2015 00:00:01 +0200 Message-ID: References: X-Original-To: Petr Mladek MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.18 (2008-05-17) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 Sender: robomod@news.nic.it List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Approved: robomod@news.nic.it Lines: 101 Organization: linux.* mail to news gateway X-Original-Cc: Andrew Morton , Tejun Heo , Ingo Molnar , Peter Zijlstra , Richard Weinberger , Steven Rostedt , David Woodhouse , linux-mtd@lists.infradead.org, Trond Myklebust , Anna Schumaker , linux-nfs@vger.kernel.org, Chris Mason , "Paul E. McKenney" , Thomas Gleixner , Linus Torvalds , Jiri Kosina , Borislav Petkov , Michal Hocko , live-patching@vger.kernel.org, linux-api@vger.kernel.org, linux-kernel@vger.kernel.org X-Original-Date: Sat, 6 Jun 2015 23:58:16 +0200 X-Original-Message-ID: <20150606215816.GB15591@redhat.com> X-Original-References: <1433516477-5153-1-git-send-email-pmladek@suse.cz> <1433516477-5153-7-git-send-email-pmladek@suse.cz> X-Original-Sender: linux-kernel-owner@vger.kernel.org Xref: aioe.org linux.kernel:1159961 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-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/