| Message ID | 1433516477-5153-7-git-send-email-pmladek@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat 2015-06-06 23:58:16, Oleg Nesterov wrote: > 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. I agree that it does not make sense to follow POSIX too much for kthreads. They are special. Signals can be send from userspace and userspace should not be able to affect the core kernel services, for example, to stop or kill some kthreas. On the other hand, I was surprised how signals were handled in the few kthreads. It looked like a mess to me. For example, lockd dropped locks when it got SIGKILL, r8712_cmd_thread() allowed SIGTERM and then just flushed the signal. To be honest, this patch set does _not_ make any big change. All signals are still ignored by default. There is default action only for SIGSTOP and fatal signals. On the other hand, it should motivate developers to use the signals a reasonable way. And if nothing, it will help to handle the signals correctly without races. > > 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. I think that we should make it independent on the iterant kthread API. I thought about handling signals also in the kthread worker and workqueues. One of the side plans here is to make freezing more reliable. Freezer already pokes normal processes using a fake signal. The same technique might be usable also for kthreads. A signal could wake them and navigate to a safe place (fridge). Another example is kthread_stop(). It sets some flag, wakes the kthread, and waits for completion. IMHO, it would be more elegant to send a signal that would wake the kthread and push it to the safe place where signals might be handled and where the kthread might get terminated. The advantage is that a pending signal will not allow to sleep on a location that is not safe for the given action (freezing, stopping) and might skip even more sleeps on the way. > 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. Yup, I can't find any usecase for this at the moment. I do not resist on SIG_IGN handling. I did this only for completness. I thought that any similarity with the normal userspace handling would be useful: the-least-surprise approach. Well, note that allow_signal() sets some "crazy" value (2) for the signal handler. IMHO, we should check for these values and handle them reasonably even in kthreads. It will make the code more secure. > > + > > + /* 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). I wanted to make it independent on the iterant API. Also if you want to handle more signals, you need even more code, e.g. the cycle, cond_resched(). So, I think that some generic helper is useful. > > + if (sig_kernel_ignore(signr)) > > + continue; > > For what? Why a kthread should unignore (say) SIGWINCH if it is not going > to react? I do not resist on this. > > + 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. 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? I think that we should support only the standard handling of SIGSTOP. It is closely related with SIGCONT. Both are manipulated in prepare_signal(). I think that it is nice to share the code here and dangerous to use it othrewise. > 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. Maybe, we should handle only SIGSTOP here and do not mimic the standard behavior for the other sig_kernel_stop(), sig_kernel_ignore(), and death signals here. I think about printing some warning in case that the handler is not defined. Best Regards, Petr > > + /* 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Let me first repeat that I agree that everything is subjective ;) On 06/08, Petr Mladek wrote: > > To be honest, this patch set does _not_ make any big change. But to me it does because (again, imo) it adds the a) unnecessary and b) wrong interface. But yes, yes, I agree that most (all?) of kthread/signal (ab)users need cleanups. And fixes. > I think that we should make it independent on the iterant kthread API. Yes! please. Then we can discuss this again and perhaps reconsider this API. So I am going to ignore some parts of your email. I am sleeping, please let me know if I missed something important ;) > Well, note that allow_signal() sets some "crazy" value (2) for the > signal handler. IMHO, we should check for these values and handle > them reasonably even in kthreads. It will make the code more secure. Not sure I understand. The crazy "2" value just means that kthread wants to recieve and dequeue this signal. I agree with the good name for this hard-coded number in advance. > > > + > > > + /* 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). > > I wanted to make it independent on the iterant API. Also if you want to > handle more signals, you need even more code, e.g. the cycle, > cond_resched(). So, I think that some generic helper is useful. I do not. Contrary, I think this needs more code in the likely case. Anyway, this API won't have too many users, so I don't even this this is that important. > > > + 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. > > 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 signr = kthread_signal_dequeue(); switch (signr) { case SIGSTOP: something_else(); kthread_do_signal_stop(); ... } > I think that we should support only the standard handling of > SIGSTOP. It is closely related with SIGCONT. Agreed. If kthread wants to actually sleep in TASK_STOPPED state then it should know about SIGCONT. > > 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. > > Maybe, we should handle only SIGSTOP So far I even disagree with SIGSTOP "default" semantics. I simply see no value. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Petr. On Fri, Jun 05, 2015 at 05:01:05PM +0200, Petr Mladek wrote: > Several kthreads already handle signals some way. They do so > in different ways, search for allow_signal(). > > This patch allows to handle the signals in a more uniform way using > kthread_do_signal(). > > 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. On the other hand, we need to be very careful. All signals > were ignored until now, except for the few kthreads that somehow > handled them. > > POSIX behavior might cause some surprises and crazy bug reports. > For example, imagine a home user stopping or killing kswapd because > it makes the system too busy. It is like shooting in its own leg. > > Also the signals are already used a strange way. For example, klockd > drops all locks when it gets SIGKILL. This was added before initial > git commit, so the archaeology was not easy. The most likely explanation > is that it is used during the system shutdown. It would make sense > to drop all locks when user space processes are killed and before > filesystems get unmounted. > > Now, the patch does the following compromise: > > + defines default actions for SIGSTOP, SIGCONT, and fatal signals > + custom actions can be defined by kthread_sigaction() > + all signals are still ignored by default > > Well, fatal signals do not terminate the kthread immediately. They just > set a flag to terminate the kthread, flush all other pending signals, > and return to the main kthread cycle. The kthread is supposed to check > the flag, leave the main cycle, do some cleanup actions when necessary > and return from the kthread. > > Note that kthreads are running in the kernel address space. It makes > the life much easier. The signal handler can be called directly and > we do not need any arch-specific code to process it in the userspace. > Also we do not care about ptrace. > > 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. > > This change does not affect any existing kthread. The plan is to > use them only in safe kthreads that can be converted into > the iterant kthread API. While I agree that it'd be great to consolidate the existing kthread signal users, I feel quite uncomfortable with doing full-fledged signal handling for kthreads which try to mimic the userland behavior. Most of the default behaviors don't make sense for kthreads and it'd be awful to have different kthreads interpreting arbitrary signals for differing custom behaviors, which often happens when there's no default. While we do have several kthread signal users, they aren't too many and majority of them use it to allow userland to tell it to shutdown and there seem to be a couple which use HUP/USR1 to cancel whatever it's processing / waiting on at the moment. Hmmm... jffs uses STOP/CONT too. I don't know how this should be done but let's please try to 1. Encourage uniform behaviors across the signals. 2. Ultimately discourage usage of signals on kthreads as this closely ties implementation detail (use of single kthread) to the userland visible interface in a way where we can't easily get back out of. For example, what if jffs needs its gc workers to be multi-threaded and per-NUMA for high-iops devices later on? Thanks.
On Tue, 9 Jun 2015, Tejun Heo wrote: > While I agree that it'd be great to consolidate the existing kthread > signal users, I feel quite uncomfortable with doing full-fledged > signal handling for kthreads which try to mimic the userland behavior. > Most of the default behaviors don't make sense for kthreads and it'd > be awful to have different kthreads interpreting arbitrary signals for > differing custom behaviors, which often happens when there's no > default. I don't think the ultimate goal is to mimic what userspace does; especially when it comes to default actions. To me, the ultimate goal (*) is to make it possible for kthread to be able to decide whether it wants "some kind of default behavior" (however that'd be defined), or "ignore all", or "just handle this set of signals with these handlers", and make API for this that would avoid every kthread implementing its own complete signal handling. (*) Well, the ultimate goal really is to bring sanity to how kthreads are handling their main loop. Trying to bring some consistency to how kthreads are handling signals is just an (optional) added value on top of that. > While we do have several kthread signal users, they aren't too many and > majority of them use it to allow userland to tell it to shutdown and Yeah. Via SIGKILL. Or SIGTERM. Or SIGINT. Or SIGQUIT. Not really consistent either. > there seem to be a couple which use HUP/USR1 to cancel whatever it's > processing / waiting on at the moment. Hmmm... jffs uses STOP/CONT too. > I don't know how this should be done but let's please try to > > 1. Encourage uniform behaviors across the signals. Fully agreed. > 2. Ultimately discourage usage of signals on kthreads as this closely > ties implementation detail (use of single kthread) to the userland > visible interface in a way where we can't easily get back out of. > For example, what if jffs needs its gc workers to be multi-threaded > and per-NUMA for high-iops devices later on? What kind of multi-threading kthreads are you referring to here? Something more sophisticated than simply spawning several per-CPU (or per-whatever-resource) full-fledged kthreads?
Hello, Jiri. On Tue, Jun 09, 2015 at 02:15:24PM +0200, Jiri Kosina wrote: > To me, the ultimate goal (*) is to make it possible for kthread to be able > to decide whether it wants "some kind of default behavior" (however that'd > be defined), or "ignore all", or "just handle this set of signals with > these handlers", and make API for this that would avoid every kthread > implementing its own complete signal handling. Yes, cleaning up the current usages like above would be great but one concern is that the above might inadvertantly encourage usage of signals in kthreads which I don't think is a good idea. It'd be great if we can consolidate the current users while also making it clear that we shouldn't be adding new ones. > > While we do have several kthread signal users, they aren't too many and > > majority of them use it to allow userland to tell it to shutdown and > > Yeah. Via SIGKILL. Or SIGTERM. Or SIGINT. Or SIGQUIT. Not really > consistent either. Exactly, and it's pretty confusing from userland. Why do some kthreads take SIGTERM but not SIGKILL while others do the other way and yet others ignore them all? This is too low level for a userland visible interface which is tied too closely to the implementation detail (usage of one kthread) and often unclear in terms of the meaning of the action. > > there seem to be a couple which use HUP/USR1 to cancel whatever it's > > processing / waiting on at the moment. Hmmm... jffs uses STOP/CONT too. > > > I don't know how this should be done but let's please try to > > > > 1. Encourage uniform behaviors across the signals. > > Fully agreed. > > > 2. Ultimately discourage usage of signals on kthreads as this closely > > ties implementation detail (use of single kthread) to the userland > > visible interface in a way where we can't easily get back out of. > > For example, what if jffs needs its gc workers to be multi-threaded > > and per-NUMA for high-iops devices later on? > > What kind of multi-threading kthreads are you referring to here? Something > more sophisticated than simply spawning several per-CPU (or > per-whatever-resource) full-fledged kthreads? Becoming simple multi-threaded or per-cpu are good examples but these things not too rarely develop into something which needs more sophiscation. e.g. jobs which process considerable amount data are usually best served by NUMA-node-affine workers roughly matching the number of CPUs in each node. workqueue is half way there but still lacking an automatic way to regulate concurrency in those cases. For certain use cases, you really can't avoid employing a pool of workers and once things get there, having tied userland interface to a single kthread becomes pretty awkward. It sure works for certain use cases and sending signals to kthreads *might* be considered a "softer" interface where userland is meddling with kernel implementation details that it can be changed later on but it can still be painful for users depending on it. Thanks.
Hi Oleg, I am sorry for the late reply. I wanted to think more before answering all the mails. On Mon 2015-06-08 23:13:36, Oleg Nesterov wrote: > I do not. Contrary, I think this needs more code in the likely case. > Anyway, this API won't have too many users, so I don't even this this > is that important. > > > > > + 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. > > > > 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(); } It means that we will have spin_lock() in one function and spin_unlock() in another one. This is what I meant with the strange semantic. This is why I think that it might be cleaner to implement some generic kthread_do_signal() or so and allow to (re)define/add sigactions via callbacks. Note that I am not aware of any kthread that would use SIGSTOP non-standard way. Anyway, I am going to concentrate on the main structure of the kthread API and will put the controversial signal handling a side for now. I will get back to it when converting the few kthreads that use signals. I will think more about your feedback in the meantime. Best Regards, Petr -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(¤t->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-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon 2015-06-15 21:14:29, Oleg Nesterov wrote: > Ah, understand. You think that we need to take ->siglock in advance > to avoid the race with SIGCONT? exactly > 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(¤t->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. Heureka, I have got it. I have previously missed the meaning of the JOBCTL_STOP_DEQUEUED bit. Thanks for explanation. Best Regards, Petr -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/signal.h b/include/linux/signal.h index 06e54762c281..41e30310bc3b 100644 --- a/include/linux/signal.h +++ b/include/linux/signal.h @@ -294,6 +294,7 @@ extern int get_signal(struct ksignal *ksig); extern void signal_setup_done(int failed, struct ksignal *ksig, int stepping); extern void exit_signals(struct task_struct *tsk); extern void kthread_sigaction(int, __kthread_sighandler_t); +extern int kthread_do_signal(void); static inline void allow_signal(int sig) { diff --git a/kernel/kthread.c b/kernel/kthread.c index 688bb4cfd807..185902d0e293 100644 --- a/kernel/kthread.c +++ b/kernel/kthread.c @@ -426,6 +426,9 @@ static int kthread_iterant_fn(void *kti_ptr) if (kti->func) kti->func(data); + if (signal_pending(current)) + kthread_do_signal(); + /* this check is safe also for non-freezable kthreads */ } while (!kthread_freezable_should_stop(NULL)); diff --git a/kernel/signal.c b/kernel/signal.c index ca6bdb411449..cdca53965c3d 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -23,6 +23,7 @@ #include <linux/ptrace.h> #include <linux/signal.h> #include <linux/signalfd.h> +#include <linux/kthread.h> #include <linux/ratelimit.h> #include <linux/tracehook.h> #include <linux/capability.h> @@ -2365,6 +2366,81 @@ relock: } /** + * kthread_do_signal - process signals delivered to kernel threads + * + * Kthreads are running in the kernel address space. It makes the life + * much easier. The signal handler can be called directly and we do not + * need any arch-specific code to process it in the userspace. + * Also we do not care about ptrace. + */ +int kthread_do_signal(void) +{ + struct ksignal ksig; + struct sighand_struct *sighand = current->sighand; + struct signal_struct *signal = current->signal; + unsigned long flags; + int signr; + + try_to_freeze(); +relock: + 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; + + /* 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; + } + + /* Default actions */ + if (sig_kernel_ignore(signr)) + continue; + + 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; + } + + /* Death signals, but try to terminate cleanly */ + kthread_stop_current(); + __flush_signals(current); + break; + } + spin_unlock_irqrestore(&sighand->siglock, flags); + + return 0; +} + +/** * signal_delivered - * @ksig: kernel signal struct * @stepping: nonzero if debugger single-step or block-step in use
Several kthreads already handle signals some way. They do so in different ways, search for allow_signal(). This patch allows to handle the signals in a more uniform way using kthread_do_signal(). 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. On the other hand, we need to be very careful. All signals were ignored until now, except for the few kthreads that somehow handled them. POSIX behavior might cause some surprises and crazy bug reports. For example, imagine a home user stopping or killing kswapd because it makes the system too busy. It is like shooting in its own leg. Also the signals are already used a strange way. For example, klockd drops all locks when it gets SIGKILL. This was added before initial git commit, so the archaeology was not easy. The most likely explanation is that it is used during the system shutdown. It would make sense to drop all locks when user space processes are killed and before filesystems get unmounted. Now, the patch does the following compromise: + defines default actions for SIGSTOP, SIGCONT, and fatal signals + custom actions can be defined by kthread_sigaction() + all signals are still ignored by default Well, fatal signals do not terminate the kthread immediately. They just set a flag to terminate the kthread, flush all other pending signals, and return to the main kthread cycle. The kthread is supposed to check the flag, leave the main cycle, do some cleanup actions when necessary and return from the kthread. Note that kthreads are running in the kernel address space. It makes the life much easier. The signal handler can be called directly and we do not need any arch-specific code to process it in the userspace. Also we do not care about ptrace. 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. This change does not affect any existing kthread. The plan is to use them only in safe kthreads that can be converted into the iterant kthread API. Signed-off-by: Petr Mladek <pmladek@suse.cz> --- include/linux/signal.h | 1 + kernel/kthread.c | 3 ++ kernel/signal.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+)