* [RFC PATCH v2 0/7] signal: sigprocmask fixes @ 2011-04-18 13:44 Oleg Nesterov 2011-04-18 13:44 ` [PATCH v2 1/7] signal: introduce retarget_shared_pending() Oleg Nesterov ` (8 more replies) 0 siblings, 9 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-18 13:44 UTC (permalink / raw) To: Tejun Heo, Linus Torvalds, Andrew Morton Cc: Nikita V. Youshchenko, Matt Fleming, linux-kernel Andrew, please drop V1: signal-introduce-retarget_shared_pending.patch signal-retarget_shared_pending-consider-shared-unblocked-signals-only.patch signal-sigprocmask-narrow-the-scope-of-sigloc.patch signal-sigprocmask-should-do-retarget_shared_pending.patch x86-signal-handle_signal-should-use-sigprocmask.patch x86-signal-sys_rt_sigreturn-should-use-sigprocmask.patch Changes in V2: - 2/7 change retarget_shared_pending() to accept mask, not ~mask - 3/7 is new, it adds the optimization promised in 2/7 - 4/7 add the small comment about current->blocked as Matt suggested - 5/7 add the new helper, set_current_blocked(), suggested by Linus - 8/7 is the new and a bit off-topic cleanup, but sys_rt_sigprocmask() looks so annoying Matt, I didn't dare to keep your reviewed-by tags because of the changes above, hopefully you can re-ack. Once again: if we need this, then we need a lot more (trivial) changes like 6/7 and 7/7. Basically every change of ->blocked should be converted to use set_current_blocked(). OTOH, perhaps this makes sense by itself. Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 1/7] signal: introduce retarget_shared_pending() 2011-04-18 13:44 [RFC PATCH v2 0/7] signal: sigprocmask fixes Oleg Nesterov @ 2011-04-18 13:44 ` Oleg Nesterov 2011-04-22 12:04 ` Matt Fleming 2011-04-25 10:49 ` Tejun Heo 2011-04-18 13:45 ` [PATCH v2 2/7] signal: retarget_shared_pending: consider shared/unblocked signals only Oleg Nesterov ` (7 subsequent siblings) 8 siblings, 2 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-18 13:44 UTC (permalink / raw) To: Tejun Heo, Linus Torvalds, Andrew Morton Cc: Nikita V. Youshchenko, Matt Fleming, linux-kernel No functional changes. Move the notify-other-threads code from exit_signals() to the new helper, retarget_shared_pending(). Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- kernel/signal.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) --- sigprocmask/kernel/signal.c~1_add_retarget_helper 2011-04-17 20:00:13.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-17 21:02:59.000000000 +0200 @@ -2017,10 +2017,25 @@ relock: return signr; } +/* + * It could be that complete_signal() picked us to notify about the + * group-wide signal. Another thread should be notified now to take + * the signal since we will not. + */ +static void retarget_shared_pending(struct task_struct *tsk) +{ + struct task_struct *t; + + t = tsk; + while_each_thread(tsk, t) { + if (!signal_pending(t) && !(t->flags & PF_EXITING)) + recalc_sigpending_and_wake(t); + } +} + void exit_signals(struct task_struct *tsk) { int group_stop = 0; - struct task_struct *t; if (thread_group_empty(tsk) || signal_group_exit(tsk->signal)) { tsk->flags |= PF_EXITING; @@ -2036,14 +2051,7 @@ void exit_signals(struct task_struct *ts if (!signal_pending(tsk)) goto out; - /* - * It could be that __group_complete_signal() choose us to - * notify about group-wide signal. Another thread should be - * woken now to take the signal since we will not. - */ - for (t = tsk; (t = next_thread(t)) != tsk; ) - if (!signal_pending(t) && !(t->flags & PF_EXITING)) - recalc_sigpending_and_wake(t); + retarget_shared_pending(tsk); if (unlikely(tsk->signal->group_stop_count) && !--tsk->signal->group_stop_count) { ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 1/7] signal: introduce retarget_shared_pending() 2011-04-18 13:44 ` [PATCH v2 1/7] signal: introduce retarget_shared_pending() Oleg Nesterov @ 2011-04-22 12:04 ` Matt Fleming 2011-04-25 10:49 ` Tejun Heo 1 sibling, 0 replies; 88+ messages in thread From: Matt Fleming @ 2011-04-22 12:04 UTC (permalink / raw) To: Oleg Nesterov Cc: Tejun Heo, Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, linux-kernel On Mon, 18 Apr 2011 15:44:43 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > No functional changes. Move the notify-other-threads code from exit_signals() > to the new helper, retarget_shared_pending(). > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 1/7] signal: introduce retarget_shared_pending() 2011-04-18 13:44 ` [PATCH v2 1/7] signal: introduce retarget_shared_pending() Oleg Nesterov 2011-04-22 12:04 ` Matt Fleming @ 2011-04-25 10:49 ` Tejun Heo 1 sibling, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-25 10:49 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Mon, Apr 18, 2011 at 03:44:43PM +0200, Oleg Nesterov wrote: > No functional changes. Move the notify-other-threads code from exit_signals() > to the new helper, retarget_shared_pending(). > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 2/7] signal: retarget_shared_pending: consider shared/unblocked signals only 2011-04-18 13:44 [RFC PATCH v2 0/7] signal: sigprocmask fixes Oleg Nesterov 2011-04-18 13:44 ` [PATCH v2 1/7] signal: introduce retarget_shared_pending() Oleg Nesterov @ 2011-04-18 13:45 ` Oleg Nesterov 2011-04-22 12:22 ` Matt Fleming 2011-04-25 10:52 ` Tejun Heo 2011-04-18 13:45 ` [PATCH v2 3/7] signal: retarget_shared_pending: optimize while_each_thread() loop Oleg Nesterov ` (6 subsequent siblings) 8 siblings, 2 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-18 13:45 UTC (permalink / raw) To: Tejun Heo, Linus Torvalds, Andrew Morton Cc: Nikita V. Youshchenko, Matt Fleming, linux-kernel exit_signals() checks signal_pending() before retarget_shared_pending() but this is suboptimal. We can avoid the while_each_thread() loop in case when there are no shared signals visible to us. Add the "shared_pending.signal & ~blocked" check. We don't use tsk->blocked directly but pass ~blocked as an argument, this is needed for the next patch. Note: we can optimize this more. while_each_thread(t) can check t->blocked into account and stop after every pending signal has the new target, see the next patch. Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- kernel/signal.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) --- sigprocmask/kernel/signal.c~2_optimize_retarget 2011-04-17 21:02:59.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-17 21:08:05.000000000 +0200 @@ -2022,10 +2022,15 @@ relock: * group-wide signal. Another thread should be notified now to take * the signal since we will not. */ -static void retarget_shared_pending(struct task_struct *tsk) +static void retarget_shared_pending(struct task_struct *tsk, sigset_t *set) { + sigset_t shared_pending; struct task_struct *t; + sigandsets(&shared_pending, &tsk->signal->shared_pending.signal, set); + if (sigisemptyset(&shared_pending)) + return; + t = tsk; while_each_thread(tsk, t) { if (!signal_pending(t) && !(t->flags & PF_EXITING)) @@ -2036,6 +2041,7 @@ static void retarget_shared_pending(stru void exit_signals(struct task_struct *tsk) { int group_stop = 0; + sigset_t set; if (thread_group_empty(tsk) || signal_group_exit(tsk->signal)) { tsk->flags |= PF_EXITING; @@ -2051,7 +2057,9 @@ void exit_signals(struct task_struct *ts if (!signal_pending(tsk)) goto out; - retarget_shared_pending(tsk); + set = tsk->blocked; + signotset(&set); + retarget_shared_pending(tsk, &set); if (unlikely(tsk->signal->group_stop_count) && !--tsk->signal->group_stop_count) { ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 2/7] signal: retarget_shared_pending: consider shared/unblocked signals only 2011-04-18 13:45 ` [PATCH v2 2/7] signal: retarget_shared_pending: consider shared/unblocked signals only Oleg Nesterov @ 2011-04-22 12:22 ` Matt Fleming 2011-04-25 10:52 ` Tejun Heo 1 sibling, 0 replies; 88+ messages in thread From: Matt Fleming @ 2011-04-22 12:22 UTC (permalink / raw) To: Oleg Nesterov Cc: Tejun Heo, Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, linux-kernel On Mon, 18 Apr 2011 15:45:01 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > exit_signals() checks signal_pending() before retarget_shared_pending() but > this is suboptimal. We can avoid the while_each_thread() loop in case when > there are no shared signals visible to us. > > Add the "shared_pending.signal & ~blocked" check. We don't use tsk->blocked > directly but pass ~blocked as an argument, this is needed for the next patch. > > Note: we can optimize this more. while_each_thread(t) can check t->blocked > into account and stop after every pending signal has the new target, see the > next patch. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 2/7] signal: retarget_shared_pending: consider shared/unblocked signals only 2011-04-18 13:45 ` [PATCH v2 2/7] signal: retarget_shared_pending: consider shared/unblocked signals only Oleg Nesterov 2011-04-22 12:22 ` Matt Fleming @ 2011-04-25 10:52 ` Tejun Heo 2011-04-25 15:20 ` Oleg Nesterov 1 sibling, 1 reply; 88+ messages in thread From: Tejun Heo @ 2011-04-25 10:52 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Mon, Apr 18, 2011 at 03:45:01PM +0200, Oleg Nesterov wrote: > -static void retarget_shared_pending(struct task_struct *tsk) > +static void retarget_shared_pending(struct task_struct *tsk, sigset_t *set) Hmm... @set? Maybe a name which refelcts its purpose, say @target or @consider would be better? Also, it would be really great to have docbook function comment describing the parameter. Other than that, Acked-by: Tejun Heo <tj@kernel.org> Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 2/7] signal: retarget_shared_pending: consider shared/unblocked signals only 2011-04-25 10:52 ` Tejun Heo @ 2011-04-25 15:20 ` Oleg Nesterov 2011-04-25 16:19 ` Tejun Heo 0 siblings, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-04-25 15:20 UTC (permalink / raw) To: Tejun Heo Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On 04/25, Tejun Heo wrote: > > On Mon, Apr 18, 2011 at 03:45:01PM +0200, Oleg Nesterov wrote: > > -static void retarget_shared_pending(struct task_struct *tsk) > > +static void retarget_shared_pending(struct task_struct *tsk, sigset_t *set) > > Hmm... @set? Maybe a name which refelcts its purpose, say @target or > @consider would be better? Also, it would be really great to have > docbook function comment describing the parameter. OK... May be @which ? but I agree with any naming. This series is already in -mm, I'd like to avoid another resend. So I'll send another patch which addresses your comments on top of this series. OK? > Acked-by: Tejun Heo <tj@kernel.org> Thanks! Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 2/7] signal: retarget_shared_pending: consider shared/unblocked signals only 2011-04-25 15:20 ` Oleg Nesterov @ 2011-04-25 16:19 ` Tejun Heo 2011-04-25 17:02 ` Oleg Nesterov 0 siblings, 1 reply; 88+ messages in thread From: Tejun Heo @ 2011-04-25 16:19 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel Hello, On Mon, Apr 25, 2011 at 05:20:40PM +0200, Oleg Nesterov wrote: > > Hmm... @set? Maybe a name which refelcts its purpose, say @target or > > @consider would be better? Also, it would be really great to have > > docbook function comment describing the parameter. > > OK... May be @which ? but I agree with any naming. Yeah, @which sounds fine to me. > This series is already in -mm, I'd like to avoid another resend. So I'll > send another patch which addresses your comments on top of this series. > > OK? Why not route through the signal/ptrace tree? Also, is the tree included in linux-next now? Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 2/7] signal: retarget_shared_pending: consider shared/unblocked signals only 2011-04-25 16:19 ` Tejun Heo @ 2011-04-25 17:02 ` Oleg Nesterov 2011-04-25 17:11 ` Tejun Heo 0 siblings, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-04-25 17:02 UTC (permalink / raw) To: Tejun Heo Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On 04/25, Tejun Heo wrote: > > On Mon, Apr 25, 2011 at 05:20:40PM +0200, Oleg Nesterov wrote: > > This series is already in -mm, I'd like to avoid another resend. So I'll > > send another patch which addresses your comments on top of this series. > > > > OK? > > Why not route through the signal/ptrace tree? I did these changes against the Linus's tree to simplify the review, and because there are completely orthogonal to ptrace changes. Also, I like very much the fact -mm has users/testers. In fact, there are trivial conflicts with the ptrace branch. I think ptrace should be flushed first, so I'll rebase this "sigprocmask" branch when I address all comments. Or do you think I should merge these changes into ptrace branch? I'd like to keep them separate, but I am not sure if I should... Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 2/7] signal: retarget_shared_pending: consider shared/unblocked signals only 2011-04-25 17:02 ` Oleg Nesterov @ 2011-04-25 17:11 ` Tejun Heo 2011-04-26 19:45 ` Oleg Nesterov 0 siblings, 1 reply; 88+ messages in thread From: Tejun Heo @ 2011-04-25 17:11 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel Hey, On Mon, Apr 25, 2011 at 7:02 PM, Oleg Nesterov <oleg@redhat.com> wrote: > I did these changes against the Linus's tree to simplify the review, and > because there are completely orthogonal to ptrace changes. Also, I like > very much the fact -mm has users/testers. > > In fact, there are trivial conflicts with the ptrace branch. I think > ptrace should be flushed first, so I'll rebase this "sigprocmask" branch > when I address all comments. > > Or do you think I should merge these changes into ptrace branch? I'd like > to keep them separate, but I am not sure if I should... I don't know. Signal/ptrace is closely coupled and you would be reviewing/acking anyway, and linux-next has some test coverage (I don't know how much but...), so I think it would be least painful to route these together. You can create separate topic branches for signal and ptrace but I don't think that's required. Anyways, yeah, if there's no objection, I think it would be best to route these together with the ptrace changes. The conflicts wouldn't be trivial and for a reason. Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 2/7] signal: retarget_shared_pending: consider shared/unblocked signals only 2011-04-25 17:11 ` Tejun Heo @ 2011-04-26 19:45 ` Oleg Nesterov 2011-04-28 15:26 ` [PATCHSET] signals-review branch Oleg Nesterov 0 siblings, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-04-26 19:45 UTC (permalink / raw) To: Tejun Heo Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel Hi, On 04/25, Tejun Heo wrote: > Hey, > > On Mon, Apr 25, 2011 at 7:02 PM, Oleg Nesterov <oleg@redhat.com> wrote: > > I did these changes against the Linus's tree to simplify the review, and > > because there are completely orthogonal to ptrace changes. Also, I like > > very much the fact -mm has users/testers. > > > > In fact, there are trivial conflicts with the ptrace branch. I think > > ptrace should be flushed first, so I'll rebase this "sigprocmask" branch > > when I address all comments. > > > > Or do you think I should merge these changes into ptrace branch? I'd like > > to keep them separate, but I am not sure if I should... > > I don't know. Signal/ptrace is closely coupled and you would be > reviewing/acking anyway, and linux-next has some test coverage (I > don't know how much but...), so I think it would be least painful to > route these together. You can create separate topic branches for > signal and ptrace but I don't think that's required. Anyways, yeah, > if there's no objection, I think it would be best to route these > together with the ptrace changes. The conflicts wouldn't be trivial > and for a reason. OK. I tried to update my branch to address the comments from you and Matt, but I got lost inside the git-learning-curve. Will do tomorrow. Until then, could you review the updated version of the new changes we discussed yesterday? (will send in a minute). Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCHSET] signals-review branch 2011-04-26 19:45 ` Oleg Nesterov @ 2011-04-28 15:26 ` Oleg Nesterov 2011-04-30 12:51 ` Tejun Heo 0 siblings, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-04-28 15:26 UTC (permalink / raw) To: Tejun Heo Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel Hello. I collected the patches which were acked by Tejun and Matt and (I hope) Linus agrees with, git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc.git signals-review [PATCH 01/13] signal: introduce retarget_shared_pending() [PATCH 02/13] signal: retarget_shared_pending: consider shared/unblocked signals only [PATCH 03/13] signal: retarget_shared_pending: optimize while_each_thread() loop [PATCH 04/13] signal: sigprocmask: narrow the scope of ->siglock [PATCH 05/13] signal: sigprocmask() should do retarget_shared_pending() [PATCH 06/13] x86: signal: handle_signal() should use set_current_blocked() [PATCH 07/13] x86: signal: sys_rt_sigreturn() should use set_current_blocked() [PATCH 08/13] signal: cleanup sys_rt_sigprocmask() [PATCH 09/13] signal: sys_rt_sigtimedwait: simplify the timeout logic [PATCH 10/13] signal: introduce do_sigtimedwait() to factor out compat/native code [PATCH 11/13] signal: do_sigtimedwait() needs retarget_shared_pending() [PATCH 12/13] signal: rename signandsets() to sigandnsets() [PATCH 13/13] signal: cleanup sys_sigprocmask() I do not want to spam lkml again, all changes are purely cosmetic and hopefully address the comments from you and Matt: all: rediff against ptrace branch [PATCH 02/13] signal: retarget_shared_pending: consider shared/unblocked signals only commit f646e227b88a164a841d6b6dd969d8a45272dd83 retarget_shared_pending: s/set/which/, s/shared_pending/retarget exit_signals: s/set/unblocked [PATCH 03/13] signal: retarget_shared_pending: optimize while_each_thread() loop commit fec9993db093acfc3999a364e31f8adae41fcb28 retarget_shared_pending: remove the unnecessary () in "if ((t->flags & PF_EXITING))" add the small comment [PATCH 05/13] signal: sigprocmask() should do retarget_shared_pending() commit e6fa16ab9c1e9b344428e6fea4d29e3cc4b28fb0 set_current_blocked: add some comments [PATCH 07/13] x86: signal: sys_rt_sigreturn() should use set_current_blocked() commit e9bd3f0faa90084f188830d77723bafe422e486b no changes. I was asked to add the comment, but I have no idea what should be documented. The patch and the code are trivial. [PATCH 08/13] signal: cleanup sys_rt_sigprocmask() commit bb7efee2ca63b08795ffb3cda96fc89d2e641b79 do not touch the "XXX" comment which I misunderstood [PATCH 10/13] signal: introduce do_sigtimedwait() to factor out compat/native code commit 943df1485a8ff0e600729e082e568ece04d4de9e Well. As it was suggested, I added the docbook coment to the new helper. Trivial copy-and-paste from sys_rt_sigtimedwait(), not sure we need these extra 6 lines. Unless you have other comments, I'll merge this into ptrace branch. Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCHSET] signals-review branch 2011-04-28 15:26 ` [PATCHSET] signals-review branch Oleg Nesterov @ 2011-04-30 12:51 ` Tejun Heo 0 siblings, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-30 12:51 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Thu, Apr 28, 2011 at 05:26:13PM +0200, Oleg Nesterov wrote: > I collected the patches which were acked by Tejun and Matt and (I hope) > Linus agrees with, > > git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc.git signals-review > > [PATCH 01/13] signal: introduce retarget_shared_pending() > [PATCH 02/13] signal: retarget_shared_pending: consider shared/unblocked signals only > [PATCH 03/13] signal: retarget_shared_pending: optimize while_each_thread() loop > [PATCH 04/13] signal: sigprocmask: narrow the scope of ->siglock > [PATCH 05/13] signal: sigprocmask() should do retarget_shared_pending() > [PATCH 06/13] x86: signal: handle_signal() should use set_current_blocked() > [PATCH 07/13] x86: signal: sys_rt_sigreturn() should use set_current_blocked() > [PATCH 08/13] signal: cleanup sys_rt_sigprocmask() > [PATCH 09/13] signal: sys_rt_sigtimedwait: simplify the timeout logic > [PATCH 10/13] signal: introduce do_sigtimedwait() to factor out compat/native code > [PATCH 11/13] signal: do_sigtimedwait() needs retarget_shared_pending() > [PATCH 12/13] signal: rename signandsets() to sigandnsets() > [PATCH 13/13] signal: cleanup sys_sigprocmask() Looks all fine to me. I'll base further patches on top of these. Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 3/7] signal: retarget_shared_pending: optimize while_each_thread() loop 2011-04-18 13:44 [RFC PATCH v2 0/7] signal: sigprocmask fixes Oleg Nesterov 2011-04-18 13:44 ` [PATCH v2 1/7] signal: introduce retarget_shared_pending() Oleg Nesterov 2011-04-18 13:45 ` [PATCH v2 2/7] signal: retarget_shared_pending: consider shared/unblocked signals only Oleg Nesterov @ 2011-04-18 13:45 ` Oleg Nesterov 2011-04-22 12:26 ` Matt Fleming 2011-04-25 11:03 ` Tejun Heo 2011-04-18 13:45 ` [PATCH v2 4/7] signal: sigprocmask: narrow the scope of ->siglock Oleg Nesterov ` (5 subsequent siblings) 8 siblings, 2 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-18 13:45 UTC (permalink / raw) To: Tejun Heo, Linus Torvalds, Andrew Morton Cc: Nikita V. Youshchenko, Matt Fleming, linux-kernel retarget_shared_pending() blindly does recalc_sigpending_and_wake() for every sub-thread, this is suboptimal. We can check t->blocked and stop looping once every bit in shared_pending has the new target. Note: we do not take task_is_stopped_or_traced(t) into account, we are not trying to speed up the signal delivery or to avoid the unnecessary (but harmless) signal_wake_up(0) in this unlikely case. Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- kernel/signal.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) --- sigprocmask/kernel/signal.c~3_optimize_retarget_loop 2011-04-17 21:08:05.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-17 21:28:21.000000000 +0200 @@ -2033,8 +2033,18 @@ static void retarget_shared_pending(stru t = tsk; while_each_thread(tsk, t) { - if (!signal_pending(t) && !(t->flags & PF_EXITING)) - recalc_sigpending_and_wake(t); + if ((t->flags & PF_EXITING)) + continue; + if (!has_pending_signals(&shared_pending, &t->blocked)) + continue; + + sigandsets(&shared_pending, &shared_pending, &t->blocked); + + if (!signal_pending(t)) + signal_wake_up(t, 0); + + if (sigisemptyset(&shared_pending)) + break; } } ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 3/7] signal: retarget_shared_pending: optimize while_each_thread() loop 2011-04-18 13:45 ` [PATCH v2 3/7] signal: retarget_shared_pending: optimize while_each_thread() loop Oleg Nesterov @ 2011-04-22 12:26 ` Matt Fleming 2011-04-25 11:03 ` Tejun Heo 1 sibling, 0 replies; 88+ messages in thread From: Matt Fleming @ 2011-04-22 12:26 UTC (permalink / raw) To: Oleg Nesterov Cc: Tejun Heo, Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, linux-kernel On Mon, 18 Apr 2011 15:45:18 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > retarget_shared_pending() blindly does recalc_sigpending_and_wake() for > every sub-thread, this is suboptimal. We can check t->blocked and stop > looping once every bit in shared_pending has the new target. > > Note: we do not take task_is_stopped_or_traced(t) into account, we are > not trying to speed up the signal delivery or to avoid the unnecessary > (but harmless) signal_wake_up(0) in this unlikely case. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Nice. Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 3/7] signal: retarget_shared_pending: optimize while_each_thread() loop 2011-04-18 13:45 ` [PATCH v2 3/7] signal: retarget_shared_pending: optimize while_each_thread() loop Oleg Nesterov 2011-04-22 12:26 ` Matt Fleming @ 2011-04-25 11:03 ` Tejun Heo 1 sibling, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-25 11:03 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel Hello, More nitpicks. On Mon, Apr 18, 2011 at 03:45:18PM +0200, Oleg Nesterov wrote: > --- sigprocmask/kernel/signal.c~3_optimize_retarget_loop 2011-04-17 21:08:05.000000000 +0200 > +++ sigprocmask/kernel/signal.c 2011-04-17 21:28:21.000000000 +0200 > @@ -2033,8 +2033,18 @@ static void retarget_shared_pending(stru > > t = tsk; > while_each_thread(tsk, t) { > - if (!signal_pending(t) && !(t->flags & PF_EXITING)) > - recalc_sigpending_and_wake(t); > + if ((t->flags & PF_EXITING)) Inner () unnecessary. > + continue; > + if (!has_pending_signals(&shared_pending, &t->blocked)) > + continue; > + > + sigandsets(&shared_pending, &shared_pending, &t->blocked); > + > + if (!signal_pending(t)) > + signal_wake_up(t, 0); > + > + if (sigisemptyset(&shared_pending)) > + break; More comments, please. Other than that, Acked-by: Tejun Heo <tj@kernel.org> -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 4/7] signal: sigprocmask: narrow the scope of ->siglock 2011-04-18 13:44 [RFC PATCH v2 0/7] signal: sigprocmask fixes Oleg Nesterov ` (2 preceding siblings ...) 2011-04-18 13:45 ` [PATCH v2 3/7] signal: retarget_shared_pending: optimize while_each_thread() loop Oleg Nesterov @ 2011-04-18 13:45 ` Oleg Nesterov 2011-04-22 12:31 ` Matt Fleming 2011-04-25 11:05 ` Tejun Heo 2011-04-18 13:45 ` [PATCH v2 5/7] signal: sigprocmask() should do retarget_shared_pending() Oleg Nesterov ` (4 subsequent siblings) 8 siblings, 2 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-18 13:45 UTC (permalink / raw) To: Tejun Heo, Linus Torvalds, Andrew Morton Cc: Nikita V. Youshchenko, Matt Fleming, linux-kernel No functional changes, preparation to simplify the review of the next change. 1. We can read current->block lockless, nobody else can ever change this mask. 2. Calculate the resulting sigset_t outside of ->siglock into the temporary variable, then take ->siglock and change ->blocked. Also, kill the stale comment about BKL. Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- kernel/signal.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) --- sigprocmask/kernel/signal.c~4_cleanup_sigprocmask 2011-04-17 21:28:21.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-17 22:16:44.000000000 +0200 @@ -2116,12 +2116,6 @@ long do_no_restart_syscall(struct restar } /* - * We don't need to get the kernel lock - this is all local to this - * particular thread.. (and that's good, because this is _heavily_ - * used by various programs) - */ - -/* * This is also useful for kernel threads that want to temporarily * (or permanently) block certain signals. * @@ -2131,30 +2125,33 @@ long do_no_restart_syscall(struct restar */ int sigprocmask(int how, sigset_t *set, sigset_t *oldset) { - int error; + struct task_struct *tsk = current; + sigset_t newset; - spin_lock_irq(¤t->sighand->siglock); + /* Lockless, only current can change ->blocked, never from irq */ if (oldset) - *oldset = current->blocked; + *oldset = tsk->blocked; - error = 0; switch (how) { case SIG_BLOCK: - sigorsets(¤t->blocked, ¤t->blocked, set); + sigorsets(&newset, &tsk->blocked, set); break; case SIG_UNBLOCK: - signandsets(¤t->blocked, ¤t->blocked, set); + signandsets(&newset, &tsk->blocked, set); break; case SIG_SETMASK: - current->blocked = *set; + newset = *set; break; default: - error = -EINVAL; + return -EINVAL; } + + spin_lock_irq(&tsk->sighand->siglock); + tsk->blocked = newset; recalc_sigpending(); - spin_unlock_irq(¤t->sighand->siglock); + spin_unlock_irq(&tsk->sighand->siglock); - return error; + return 0; } /** ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 4/7] signal: sigprocmask: narrow the scope of ->siglock 2011-04-18 13:45 ` [PATCH v2 4/7] signal: sigprocmask: narrow the scope of ->siglock Oleg Nesterov @ 2011-04-22 12:31 ` Matt Fleming 2011-04-25 11:05 ` Tejun Heo 1 sibling, 0 replies; 88+ messages in thread From: Matt Fleming @ 2011-04-22 12:31 UTC (permalink / raw) To: Oleg Nesterov Cc: Tejun Heo, Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, linux-kernel On Mon, 18 Apr 2011 15:45:38 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > No functional changes, preparation to simplify the review of the next change. > > 1. We can read current->block lockless, nobody else can ever change this mask. > > 2. Calculate the resulting sigset_t outside of ->siglock into the temporary > variable, then take ->siglock and change ->blocked. > > Also, kill the stale comment about BKL. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Thanks for that updated comment about not being able to change ->blocked from irq context! Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 4/7] signal: sigprocmask: narrow the scope of ->siglock 2011-04-18 13:45 ` [PATCH v2 4/7] signal: sigprocmask: narrow the scope of ->siglock Oleg Nesterov 2011-04-22 12:31 ` Matt Fleming @ 2011-04-25 11:05 ` Tejun Heo 1 sibling, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-25 11:05 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Mon, Apr 18, 2011 at 03:45:38PM +0200, Oleg Nesterov wrote: > No functional changes, preparation to simplify the review of the next change. > > 1. We can read current->block lockless, nobody else can ever change this mask. current->blocked > > 2. Calculate the resulting sigset_t outside of ->siglock into the temporary > variable, then take ->siglock and change ->blocked. > > Also, kill the stale comment about BKL. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 5/7] signal: sigprocmask() should do retarget_shared_pending() 2011-04-18 13:44 [RFC PATCH v2 0/7] signal: sigprocmask fixes Oleg Nesterov ` (3 preceding siblings ...) 2011-04-18 13:45 ` [PATCH v2 4/7] signal: sigprocmask: narrow the scope of ->siglock Oleg Nesterov @ 2011-04-18 13:45 ` Oleg Nesterov 2011-04-22 12:46 ` Matt Fleming 2011-04-25 11:14 ` Tejun Heo 2011-04-18 13:46 ` [PATCH v2 6/7] x86: signal: handle_signal() should use set_current_blocked() Oleg Nesterov ` (3 subsequent siblings) 8 siblings, 2 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-18 13:45 UTC (permalink / raw) To: Tejun Heo, Linus Torvalds, Andrew Morton Cc: Nikita V. Youshchenko, Matt Fleming, linux-kernel In short, almost every changing of current->blocked is wrong, or at least can lead to the unexpected results. For example. Two threads T1 and T2, T1 sleeps in sigtimedwait/pause/etc. kill(tgid, SIG) can pick T2 for TIF_SIGPENDING. If T2 calls sigprocmask() and blocks SIG before it notices the pending signal, nobody else can handle this pending shared signal. I am not sure this is bug, but at least this looks strange imho. T1 should not sleep forever, there is a signal which should wake it up. This patch moves the code which actually changes ->blocked into the new helper, set_current_blocked() and changes this code to call retarget_shared_pending() as exit_signals() does. We should only care about the signals we just blocked, we use "newset & ~current->blocked" as a mask. We do not check !sigisemptyset(newblocked), retarget_shared_pending() is cheap unless mask & shared_pending. Note: for this particular case we could simply change sigprocmask() to return -EINTR if signal_pending(), but then we should change other callers and, more importantly, if we need this fix then set_current_blocked() will have more callers and some of them can't restart. See the next patch as a random example. Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- include/linux/signal.h | 1 + kernel/signal.c | 21 ++++++++++++++++----- 2 files changed, 17 insertions(+), 5 deletions(-) --- sigprocmask/include/linux/signal.h~5_sigprocmask_retarget 2011-04-17 22:23:29.000000000 +0200 +++ sigprocmask/include/linux/signal.h 2011-04-17 22:36:41.000000000 +0200 @@ -243,6 +243,7 @@ extern long do_rt_tgsigqueueinfo(pid_t t siginfo_t *info); extern long do_sigpending(void __user *, unsigned long); extern int sigprocmask(int, sigset_t *, sigset_t *); +extern void set_current_blocked(const sigset_t *); extern int show_unhandled_signals; struct pt_regs; --- sigprocmask/kernel/signal.c~5_sigprocmask_retarget 2011-04-17 22:16:44.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-17 22:32:58.000000000 +0200 @@ -2115,6 +2115,21 @@ long do_no_restart_syscall(struct restar return -EINTR; } +void set_current_blocked(const sigset_t *newset) +{ + struct task_struct *tsk = current; + + spin_lock_irq(&tsk->sighand->siglock); + if (signal_pending(tsk) && !thread_group_empty(tsk)) { + sigset_t newblocked; + signandsets(&newblocked, newset, ¤t->blocked); + retarget_shared_pending(tsk, &newblocked); + } + tsk->blocked = *newset; + recalc_sigpending(); + spin_unlock_irq(&tsk->sighand->siglock); +} + /* * This is also useful for kernel threads that want to temporarily * (or permanently) block certain signals. @@ -2146,11 +2161,7 @@ int sigprocmask(int how, sigset_t *set, return -EINVAL; } - spin_lock_irq(&tsk->sighand->siglock); - tsk->blocked = newset; - recalc_sigpending(); - spin_unlock_irq(&tsk->sighand->siglock); - + set_current_blocked(&newset); return 0; } ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 5/7] signal: sigprocmask() should do retarget_shared_pending() 2011-04-18 13:45 ` [PATCH v2 5/7] signal: sigprocmask() should do retarget_shared_pending() Oleg Nesterov @ 2011-04-22 12:46 ` Matt Fleming 2011-04-25 11:14 ` Tejun Heo 1 sibling, 0 replies; 88+ messages in thread From: Matt Fleming @ 2011-04-22 12:46 UTC (permalink / raw) To: Oleg Nesterov Cc: Tejun Heo, Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, linux-kernel On Mon, 18 Apr 2011 15:45:57 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > In short, almost every changing of current->blocked is wrong, or at least > can lead to the unexpected results. > > For example. Two threads T1 and T2, T1 sleeps in sigtimedwait/pause/etc. > kill(tgid, SIG) can pick T2 for TIF_SIGPENDING. If T2 calls sigprocmask() > and blocks SIG before it notices the pending signal, nobody else can handle > this pending shared signal. > > I am not sure this is bug, but at least this looks strange imho. T1 should > not sleep forever, there is a signal which should wake it up. > > This patch moves the code which actually changes ->blocked into the new > helper, set_current_blocked() and changes this code to call > retarget_shared_pending() as exit_signals() does. We should only care about > the signals we just blocked, we use "newset & ~current->blocked" as a mask. > > We do not check !sigisemptyset(newblocked), retarget_shared_pending() is > cheap unless mask & shared_pending. > > Note: for this particular case we could simply change sigprocmask() to > return -EINTR if signal_pending(), but then we should change other callers > and, more importantly, if we need this fix then set_current_blocked() will > have more callers and some of them can't restart. See the next patch as a > random example. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> This looks much simpler to me. Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 5/7] signal: sigprocmask() should do retarget_shared_pending() 2011-04-18 13:45 ` [PATCH v2 5/7] signal: sigprocmask() should do retarget_shared_pending() Oleg Nesterov 2011-04-22 12:46 ` Matt Fleming @ 2011-04-25 11:14 ` Tejun Heo 1 sibling, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-25 11:14 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel Hey, again. > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> But, I really think we can use some comments around here. Things might look obvious now but it isn't very intuitive piece of code. > +void set_current_blocked(const sigset_t *newset) > +{ > + struct task_struct *tsk = current; > + > + spin_lock_irq(&tsk->sighand->siglock); > + if (signal_pending(tsk) && !thread_group_empty(tsk)) { > + sigset_t newblocked; > + signandsets(&newblocked, newset, ¤t->blocked); While you're touching code around here, can you please rename signandsets() to sigandnsets()? It ain't nand!!! Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 6/7] x86: signal: handle_signal() should use set_current_blocked() 2011-04-18 13:44 [RFC PATCH v2 0/7] signal: sigprocmask fixes Oleg Nesterov ` (4 preceding siblings ...) 2011-04-18 13:45 ` [PATCH v2 5/7] signal: sigprocmask() should do retarget_shared_pending() Oleg Nesterov @ 2011-04-18 13:46 ` Oleg Nesterov 2011-04-22 13:45 ` Matt Fleming 2011-04-25 11:19 ` Tejun Heo 2011-04-18 13:46 ` [PATCH v2 7/7] x86: signal: sys_rt_sigreturn() " Oleg Nesterov ` (2 subsequent siblings) 8 siblings, 2 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-18 13:46 UTC (permalink / raw) To: Tejun Heo, Linus Torvalds, Andrew Morton Cc: Nikita V. Youshchenko, Matt Fleming, linux-kernel This is ugly, but if sigprocmask() needs retarget_shared_pending() then handle signal should follow this logic. In theory it is newer correct to add the new signals to current->blocked, the signal handler can sleep/etc so we should notify other threads in case we block the pending signal and nobody else has TIF_SIGPENDING. Of course, this change doesn't make signals faster :/ Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- arch/x86/kernel/signal.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) --- sigprocmask/arch/x86/kernel/signal.c~6_handle_signal 2011-04-15 19:21:30.000000000 +0200 +++ sigprocmask/arch/x86/kernel/signal.c 2011-04-17 23:07:14.000000000 +0200 @@ -682,6 +682,7 @@ static int handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka, sigset_t *oldset, struct pt_regs *regs) { + sigset_t blocked; int ret; /* Are we from a system call? */ @@ -741,12 +742,10 @@ handle_signal(unsigned long sig, siginfo */ regs->flags &= ~X86_EFLAGS_TF; - spin_lock_irq(¤t->sighand->siglock); - sigorsets(¤t->blocked, ¤t->blocked, &ka->sa.sa_mask); + sigorsets(&blocked, ¤t->blocked, &ka->sa.sa_mask); if (!(ka->sa.sa_flags & SA_NODEFER)) - sigaddset(¤t->blocked, sig); - recalc_sigpending(); - spin_unlock_irq(¤t->sighand->siglock); + sigaddset(&blocked, sig); + set_current_blocked(&blocked); tracehook_signal_handler(sig, info, ka, regs, test_thread_flag(TIF_SINGLESTEP)); ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 6/7] x86: signal: handle_signal() should use set_current_blocked() 2011-04-18 13:46 ` [PATCH v2 6/7] x86: signal: handle_signal() should use set_current_blocked() Oleg Nesterov @ 2011-04-22 13:45 ` Matt Fleming 2011-04-25 11:19 ` Tejun Heo 1 sibling, 0 replies; 88+ messages in thread From: Matt Fleming @ 2011-04-22 13:45 UTC (permalink / raw) To: Oleg Nesterov Cc: Tejun Heo, Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, linux-kernel On Mon, 18 Apr 2011 15:46:15 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > This is ugly, but if sigprocmask() needs retarget_shared_pending() then > handle signal should follow this logic. In theory it is newer correct to ^^ never ;-) > add the new signals to current->blocked, the signal handler can sleep/etc > so we should notify other threads in case we block the pending signal and > nobody else has TIF_SIGPENDING. > > Of course, this change doesn't make signals faster :/ > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 6/7] x86: signal: handle_signal() should use set_current_blocked() 2011-04-18 13:46 ` [PATCH v2 6/7] x86: signal: handle_signal() should use set_current_blocked() Oleg Nesterov 2011-04-22 13:45 ` Matt Fleming @ 2011-04-25 11:19 ` Tejun Heo 1 sibling, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-25 11:19 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Mon, Apr 18, 2011 at 03:46:15PM +0200, Oleg Nesterov wrote: > This is ugly, but if sigprocmask() needs retarget_shared_pending() then > handle signal should follow this logic. In theory it is newer correct to never? > add the new signals to current->blocked, the signal handler can sleep/etc > so we should notify other threads in case we block the pending signal and > nobody else has TIF_SIGPENDING. > > Of course, this change doesn't make signals faster :/ I don't think it's gonna make things go much slower either except for pathological cases. > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 7/7] x86: signal: sys_rt_sigreturn() should use set_current_blocked() 2011-04-18 13:44 [RFC PATCH v2 0/7] signal: sigprocmask fixes Oleg Nesterov ` (5 preceding siblings ...) 2011-04-18 13:46 ` [PATCH v2 6/7] x86: signal: handle_signal() should use set_current_blocked() Oleg Nesterov @ 2011-04-18 13:46 ` Oleg Nesterov 2011-04-22 14:14 ` Matt Fleming 2011-04-25 11:21 ` Tejun Heo 2011-04-18 13:47 ` [PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask() Oleg Nesterov 2011-04-18 17:16 ` [RFC PATCH v2 0/7] signal: sigprocmask fixes Linus Torvalds 8 siblings, 2 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-18 13:46 UTC (permalink / raw) To: Tejun Heo, Linus Torvalds, Andrew Morton Cc: Nikita V. Youshchenko, Matt Fleming, linux-kernel Normally sys_rt_sigreturn() restores the old current->blocked which was changed by handle_signal(), and unblocking is always fine. But the debugger or application itself can change frame->uc_sigmask and thus we need set_current_blocked()->retarget_shared_pending(). Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- arch/x86/kernel/signal.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) --- sigprocmask/arch/x86/kernel/signal.c~7_sigreturn 2011-04-17 23:07:14.000000000 +0200 +++ sigprocmask/arch/x86/kernel/signal.c 2011-04-17 23:19:13.000000000 +0200 @@ -601,10 +601,7 @@ long sys_rt_sigreturn(struct pt_regs *re goto badframe; sigdelsetmask(&set, ~_BLOCKABLE); - spin_lock_irq(¤t->sighand->siglock); - current->blocked = set; - recalc_sigpending(); - spin_unlock_irq(¤t->sighand->siglock); + set_current_blocked(&set); if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &ax)) goto badframe; ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 7/7] x86: signal: sys_rt_sigreturn() should use set_current_blocked() 2011-04-18 13:46 ` [PATCH v2 7/7] x86: signal: sys_rt_sigreturn() " Oleg Nesterov @ 2011-04-22 14:14 ` Matt Fleming 2011-04-23 18:12 ` Oleg Nesterov 2011-04-25 11:21 ` Tejun Heo 1 sibling, 1 reply; 88+ messages in thread From: Matt Fleming @ 2011-04-22 14:14 UTC (permalink / raw) To: Oleg Nesterov Cc: Tejun Heo, Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, linux-kernel On Mon, 18 Apr 2011 15:46:41 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > Normally sys_rt_sigreturn() restores the old current->blocked which was > changed by handle_signal(), and unblocking is always fine. > > But the debugger or application itself can change frame->uc_sigmask and > thus we need set_current_blocked()->retarget_shared_pending(). > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> But does sys_sigreturn() also need this change? ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 7/7] x86: signal: sys_rt_sigreturn() should use set_current_blocked() 2011-04-22 14:14 ` Matt Fleming @ 2011-04-23 18:12 ` Oleg Nesterov 0 siblings, 0 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-23 18:12 UTC (permalink / raw) To: Matt Fleming Cc: Tejun Heo, Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, linux-kernel On 04/22, Matt Fleming wrote: > > On Mon, 18 Apr 2011 15:46:41 +0200 > Oleg Nesterov <oleg@redhat.com> wrote: > > > Normally sys_rt_sigreturn() restores the old current->blocked which was > > changed by handle_signal(), and unblocking is always fine. > > > > But the debugger or application itself can change frame->uc_sigmask and > > thus we need set_current_blocked()->retarget_shared_pending(). > > > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> > > Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> Thanks Matt. > But does sys_sigreturn() also need this change? Of course, it needs. From 0/7: Once again: if we need this, then we need a lot more (trivial) changes like 6/7 and 7/7. Basically every change of ->blocked should be converted to use set_current_blocked(). 6 and 7 are simple examples, most of the other changes will look similary. Except sys_rt_sigtimedwait(), it changes both ->real_blocked and blocked, see the patches I sent. sys_sigprocmask() is a bit annoying, but the necessary changes are simple. Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 7/7] x86: signal: sys_rt_sigreturn() should use set_current_blocked() 2011-04-18 13:46 ` [PATCH v2 7/7] x86: signal: sys_rt_sigreturn() " Oleg Nesterov 2011-04-22 14:14 ` Matt Fleming @ 2011-04-25 11:21 ` Tejun Heo 1 sibling, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-25 11:21 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Mon, Apr 18, 2011 at 03:46:41PM +0200, Oleg Nesterov wrote: > Normally sys_rt_sigreturn() restores the old current->blocked which was > changed by handle_signal(), and unblocking is always fine. > > But the debugger or application itself can change frame->uc_sigmask and > thus we need set_current_blocked()->retarget_shared_pending(). > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> > arch/x86/kernel/signal.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > --- sigprocmask/arch/x86/kernel/signal.c~7_sigreturn 2011-04-17 23:07:14.000000000 +0200 > +++ sigprocmask/arch/x86/kernel/signal.c 2011-04-17 23:19:13.000000000 +0200 > @@ -601,10 +601,7 @@ long sys_rt_sigreturn(struct pt_regs *re > goto badframe; > > sigdelsetmask(&set, ~_BLOCKABLE); > - spin_lock_irq(¤t->sighand->siglock); > - current->blocked = set; > - recalc_sigpending(); > - spin_unlock_irq(¤t->sighand->siglock); > + set_current_blocked(&set); Comment! Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask() 2011-04-18 13:44 [RFC PATCH v2 0/7] signal: sigprocmask fixes Oleg Nesterov ` (6 preceding siblings ...) 2011-04-18 13:46 ` [PATCH v2 7/7] x86: signal: sys_rt_sigreturn() " Oleg Nesterov @ 2011-04-18 13:47 ` Oleg Nesterov 2011-04-22 14:30 ` Matt Fleming 2011-04-25 11:26 ` Tejun Heo 2011-04-18 17:16 ` [RFC PATCH v2 0/7] signal: sigprocmask fixes Linus Torvalds 8 siblings, 2 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-18 13:47 UTC (permalink / raw) To: Tejun Heo, Linus Torvalds, Andrew Morton Cc: Nikita V. Youshchenko, Matt Fleming, linux-kernel sys_rt_sigprocmask() looks unnecessarily complicated, simplify it. We can just read current->blocked lockless unconditionally before anything else and then copy-to-user it if needed. At worst we copy 4 words on mips. We could copy-to-user the old mask first and simplify the code even more, but the patch tries to keep the current behaviour: we change current->block even if copy_to_user(oset) fails. Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- kernel/signal.c | 40 +++++++++++++++++----------------------- 1 file changed, 17 insertions(+), 23 deletions(-) --- sigprocmask/kernel/signal.c~8_sys_sigprocmask 2011-04-17 22:32:58.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-18 14:45:57.000000000 +0200 @@ -2172,40 +2172,34 @@ int sigprocmask(int how, sigset_t *set, * @oset: previous value of signal mask if non-null * @sigsetsize: size of sigset_t type */ -SYSCALL_DEFINE4(rt_sigprocmask, int, how, sigset_t __user *, set, +SYSCALL_DEFINE4(rt_sigprocmask, int, how, sigset_t __user *, nset, sigset_t __user *, oset, size_t, sigsetsize) { - int error = -EINVAL; sigset_t old_set, new_set; + int error; - /* XXX: Don't preclude handling different sized sigset_t's. */ + /* Don't preclude handling different sized sigset_t's. */ if (sigsetsize != sizeof(sigset_t)) - goto out; + return -EINVAL; - if (set) { - error = -EFAULT; - if (copy_from_user(&new_set, set, sizeof(*set))) - goto out; + old_set = current->blocked; + + if (nset) { + if (copy_from_user(&new_set, nset, sizeof(sigset_t))) + return -EFAULT; sigdelsetmask(&new_set, sigmask(SIGKILL)|sigmask(SIGSTOP)); - error = sigprocmask(how, &new_set, &old_set); + error = sigprocmask(how, &new_set, NULL); if (error) - goto out; - if (oset) - goto set_old; - } else if (oset) { - spin_lock_irq(¤t->sighand->siglock); - old_set = current->blocked; - spin_unlock_irq(¤t->sighand->siglock); + return error; + } - set_old: - error = -EFAULT; - if (copy_to_user(oset, &old_set, sizeof(*oset))) - goto out; + if (oset) { + if (copy_to_user(oset, &old_set, sizeof(sigset_t))) + return -EFAULT; } - error = 0; -out: - return error; + + return 0; } long do_sigpending(void __user *set, unsigned long sigsetsize) ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask() 2011-04-18 13:47 ` [PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask() Oleg Nesterov @ 2011-04-22 14:30 ` Matt Fleming 2011-04-23 18:20 ` Oleg Nesterov 2011-04-25 11:26 ` Tejun Heo 1 sibling, 1 reply; 88+ messages in thread From: Matt Fleming @ 2011-04-22 14:30 UTC (permalink / raw) To: Oleg Nesterov Cc: Tejun Heo, Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, linux-kernel On Mon, 18 Apr 2011 15:47:00 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > sys_rt_sigprocmask() looks unnecessarily complicated, simplify it. > We can just read current->blocked lockless unconditionally before > anything else and then copy-to-user it if needed. At worst we > copy 4 words on mips. > > We could copy-to-user the old mask first and simplify the code even > more, but the patch tries to keep the current behaviour: we change > current->block even if copy_to_user(oset) fails. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> > --- > > kernel/signal.c | 40 +++++++++++++++++----------------------- > 1 file changed, 17 insertions(+), 23 deletions(-) > > --- sigprocmask/kernel/signal.c~8_sys_sigprocmask 2011-04-17 22:32:58.000000000 +0200 > +++ sigprocmask/kernel/signal.c 2011-04-18 14:45:57.000000000 +0200 > @@ -2172,40 +2172,34 @@ int sigprocmask(int how, sigset_t *set, > * @oset: previous value of signal mask if non-null > * @sigsetsize: size of sigset_t type > */ > -SYSCALL_DEFINE4(rt_sigprocmask, int, how, sigset_t __user *, set, > +SYSCALL_DEFINE4(rt_sigprocmask, int, how, sigset_t __user *, nset, > sigset_t __user *, oset, size_t, sigsetsize) > { > - int error = -EINVAL; > sigset_t old_set, new_set; > + int error; > > - /* XXX: Don't preclude handling different sized sigset_t's. */ > + /* Don't preclude handling different sized sigset_t's. */ > if (sigsetsize != sizeof(sigset_t)) > - goto out; > + return -EINVAL; I don't think it's correct to remove the 'XXX'. The comment is currently saying "We don't handle different sized sigset_t's, but we should", whereas removing the 'XXX' says to me that we _DO_ handle different sized sigset_t's. If you don't like the 'XXX' you could always swap it for a 'TODO'? Otherwise looks like a very nice cleanup. Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask() 2011-04-22 14:30 ` Matt Fleming @ 2011-04-23 18:20 ` Oleg Nesterov 2011-04-23 18:47 ` Matt Fleming 0 siblings, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-04-23 18:20 UTC (permalink / raw) To: Matt Fleming Cc: Tejun Heo, Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, linux-kernel On 04/22, Matt Fleming wrote: > > On Mon, 18 Apr 2011 15:47:00 +0200 > Oleg Nesterov <oleg@redhat.com> wrote: > > > - /* XXX: Don't preclude handling different sized sigset_t's. */ > > + /* Don't preclude handling different sized sigset_t's. */ > > if (sigsetsize != sizeof(sigset_t)) > > - goto out; > > + return -EINVAL; > > I don't think it's correct to remove the 'XXX'. The comment is > currently saying "We don't handle different sized sigset_t's, but we > should", whereas removing the 'XXX' says to me that we _DO_ handle > different sized sigset_t's. Hmm. I think you are right. I simply didn't know what "preclude" means and thus misunderstood the comment. > If you don't like the 'XXX' you could > always swap it for a 'TODO'? I think we should just remove this comment. It is confusing. This check is trivial and does not need any explanation. The comment (and the code) is very old, it predates the git history. I do not think this API will be changed, unlikely we need to handle the different sized sigset_t's. What do you think? > Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> Thanks! Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask() 2011-04-23 18:20 ` Oleg Nesterov @ 2011-04-23 18:47 ` Matt Fleming 0 siblings, 0 replies; 88+ messages in thread From: Matt Fleming @ 2011-04-23 18:47 UTC (permalink / raw) To: Oleg Nesterov Cc: Tejun Heo, Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, linux-kernel On Sat, 23 Apr 2011 20:20:31 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > I think we should just remove this comment. It is confusing. This check > is trivial and does not need any explanation. The comment (and the code) > is very old, it predates the git history. I do not think this API will be > changed, unlikely we need to handle the different sized sigset_t's. > > What do you think? Sure, that is OK with me! -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask() 2011-04-18 13:47 ` [PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask() Oleg Nesterov 2011-04-22 14:30 ` Matt Fleming @ 2011-04-25 11:26 ` Tejun Heo 1 sibling, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-25 11:26 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Mon, Apr 18, 2011 at 03:47:00PM +0200, Oleg Nesterov wrote: > sys_rt_sigprocmask() looks unnecessarily complicated, simplify it. > We can just read current->blocked lockless unconditionally before > anything else and then copy-to-user it if needed. At worst we > copy 4 words on mips. > > We could copy-to-user the old mask first and simplify the code even > more, but the patch tries to keep the current behaviour: we change > current->block even if copy_to_user(oset) fails. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC PATCH v2 0/7] signal: sigprocmask fixes 2011-04-18 13:44 [RFC PATCH v2 0/7] signal: sigprocmask fixes Oleg Nesterov ` (7 preceding siblings ...) 2011-04-18 13:47 ` [PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask() Oleg Nesterov @ 2011-04-18 17:16 ` Linus Torvalds 2011-04-18 17:32 ` Oleg Nesterov 8 siblings, 1 reply; 88+ messages in thread From: Linus Torvalds @ 2011-04-18 17:16 UTC (permalink / raw) To: Oleg Nesterov Cc: Tejun Heo, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Mon, Apr 18, 2011 at 6:44 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > Once again: if we need this, then we need a lot more (trivial) changes > like 6/7 and 7/7. Basically every change of ->blocked should be converted > to use set_current_blocked(). OTOH, perhaps this makes sense by itself. Hmm. The more I think about this, the less I like it. What if the pending thread signal was thread-specific to begin with? For example, if we have a SIGFPE and a SIGKILL that happen at the same time, a dying task may have a SIGFPE pending when it dies, and that SIGFPE should _not_ be just distributed out to the other threads in the thread group. Am I missing something that protects against this? Linus ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC PATCH v2 0/7] signal: sigprocmask fixes 2011-04-18 17:16 ` [RFC PATCH v2 0/7] signal: sigprocmask fixes Linus Torvalds @ 2011-04-18 17:32 ` Oleg Nesterov 2011-04-18 17:40 ` Linus Torvalds 0 siblings, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-04-18 17:32 UTC (permalink / raw) To: Linus Torvalds Cc: Tejun Heo, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On 04/18, Linus Torvalds wrote: > > On Mon, Apr 18, 2011 at 6:44 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > > > Once again: if we need this, then we need a lot more (trivial) changes > > like 6/7 and 7/7. Basically every change of ->blocked should be converted > > to use set_current_blocked(). OTOH, perhaps this makes sense by itself. > > Hmm. The more I think about this, the less I like it. > > What if the pending thread signal was thread-specific to begin with? These patches should not change the current behaviour in this case. We never try to re-target the thread-specific signals. Note that retarget_shared_pending() checks ->signal->shared_pending only. > For example, if we have a SIGFPE and a SIGKILL that happen at the same > time, a dying task may have a SIGFPE pending when it dies, and that > SIGFPE should _not_ be just distributed out to the other threads in > the thread group. Yes, and it won't be. Btw, we do not need to distribute SIGKILL too, we can change retarget_shared_pending() to remove SIGKILL from shared_pending. But this only matters when the caller is exit_signals(), and in this case it should likely notice signal_group_exit() unless SIGKILL (in unlikely case) it comes in between. Or I misunderstood? Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [RFC PATCH v2 0/7] signal: sigprocmask fixes 2011-04-18 17:32 ` Oleg Nesterov @ 2011-04-18 17:40 ` Linus Torvalds 2011-04-23 17:59 ` [PATCH 0/3] do_sigtimedwait() needs retarget_shared_pending() Oleg Nesterov 0 siblings, 1 reply; 88+ messages in thread From: Linus Torvalds @ 2011-04-18 17:40 UTC (permalink / raw) To: Oleg Nesterov Cc: Tejun Heo, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Mon, Apr 18, 2011 at 10:32 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > These patches should not change the current behaviour in this case. > We never try to re-target the thread-specific signals. Note that > retarget_shared_pending() checks ->signal->shared_pending only. Ok, I clearly didn't follow the details closely enough. In that case, this series looks fairly ok to me. I'm still unsure whether it's _required_, but it looks sane and reasonable. Linus ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH 0/3] do_sigtimedwait() needs retarget_shared_pending() 2011-04-18 17:40 ` Linus Torvalds @ 2011-04-23 17:59 ` Oleg Nesterov 2011-04-23 17:59 ` [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic Oleg Nesterov ` (3 more replies) 0 siblings, 4 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-23 17:59 UTC (permalink / raw) To: Linus Torvalds, Andrew Morton Cc: Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel On 04/18, Linus Torvalds wrote: > > In that case, this series looks fairly ok to me. I'm still unsure > whether it's _required_, Indeed! It is a bit annoying, I am not sure too. This "problem" is very, very, old. And nobody complained so far. Except, this _might_ explain the hang reported by Nikita. OTOH. Probably set_current_blocked() makes sense anyway, it would be nice to require that nobody can play with ->blocked directly. We can reconsider retarget_shared_pendind() or simply kill it if it adds the noticeable overhead in practice. So, I'll assume we need this changes. Correctness is always good. All further changes should be simple and straightforward, I am going to trim CC. Except sys_rt_sigtimedwait() is nasty, we need another helper for simplicity. If nobody objects, I'll send at lot of other "needs set_current_blocked" simple patches to Andrew. Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic 2011-04-23 17:59 ` [PATCH 0/3] do_sigtimedwait() needs retarget_shared_pending() Oleg Nesterov @ 2011-04-23 17:59 ` Oleg Nesterov 2011-04-25 11:37 ` Tejun Heo 2011-04-26 10:18 ` Matt Fleming 2011-04-23 17:59 ` [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code Oleg Nesterov ` (2 subsequent siblings) 3 siblings, 2 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-23 17:59 UTC (permalink / raw) To: Linus Torvalds, Andrew Morton Cc: Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel No functional changes, cleanup compat_sys_rt_sigtimedwait() and sys_rt_sigtimedwait(). Calculate the timeout before we take ->siglock, this simplifies and lessens the code. Use timespec_valid() to check the timespec. I don't understand why we are adding (ts.tv_sec || ts.tv_nsec) to timespec_to_jiffies(&ts). Perhaps to ensure we will sleep at least one jiffy if ts != 0? But in this case we should only check tv_nsec, I don't think timespec_to_jiffies() can return zero if tv_sec != 0. In fact I suspect timespec_to_jiffies() can only return zero if tv_sec == tv_nsec == 0 because we add "TICK_NSEC - 1", but I am not sure I understand correctly this math. Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- kernel/signal.c | 46 ++++++++++++++++++++-------------------------- kernel/compat.c | 38 ++++++++++++++++---------------------- 2 files changed, 36 insertions(+), 48 deletions(-) --- sigprocmask/kernel/signal.c~1_sigtimedwait_to 2011-04-22 15:48:33.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-22 19:05:51.000000000 +0200 @@ -2327,7 +2327,7 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s sigset_t these; struct timespec ts; siginfo_t info; - long timeout = 0; + long timeout; /* XXX: Don't preclude handling different sized sigset_t's. */ if (sigsetsize != sizeof(sigset_t)) @@ -2343,41 +2343,35 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s sigdelsetmask(&these, sigmask(SIGKILL)|sigmask(SIGSTOP)); signotset(&these); + timeout = MAX_SCHEDULE_TIMEOUT; if (uts) { if (copy_from_user(&ts, uts, sizeof(ts))) return -EFAULT; - if (ts.tv_nsec >= 1000000000L || ts.tv_nsec < 0 - || ts.tv_sec < 0) + if (!timespec_valid(&ts)) return -EINVAL; + timeout = timespec_to_jiffies(&ts) + (ts.tv_sec || ts.tv_nsec); } spin_lock_irq(¤t->sighand->siglock); sig = dequeue_signal(current, &these, &info); - if (!sig) { - timeout = MAX_SCHEDULE_TIMEOUT; - if (uts) - timeout = (timespec_to_jiffies(&ts) - + (ts.tv_sec || ts.tv_nsec)); - - if (timeout) { - /* - * None ready -- temporarily unblock those we're - * interested while we are sleeping in so that we'll - * be awakened when they arrive. - */ - current->real_blocked = current->blocked; - sigandsets(¤t->blocked, ¤t->blocked, &these); - recalc_sigpending(); - spin_unlock_irq(¤t->sighand->siglock); + if (!sig && timeout) { + /* + * None ready -- temporarily unblock those we're + * interested while we are sleeping in so that we'll + * be awakened when they arrive. + */ + current->real_blocked = current->blocked; + sigandsets(¤t->blocked, ¤t->blocked, &these); + recalc_sigpending(); + spin_unlock_irq(¤t->sighand->siglock); - timeout = schedule_timeout_interruptible(timeout); + timeout = schedule_timeout_interruptible(timeout); - spin_lock_irq(¤t->sighand->siglock); - sig = dequeue_signal(current, &these, &info); - current->blocked = current->real_blocked; - siginitset(¤t->real_blocked, 0); - recalc_sigpending(); - } + spin_lock_irq(¤t->sighand->siglock); + sig = dequeue_signal(current, &these, &info); + current->blocked = current->real_blocked; + siginitset(¤t->real_blocked, 0); + recalc_sigpending(); } spin_unlock_irq(¤t->sighand->siglock); --- sigprocmask/kernel/compat.c~1_sigtimedwait_to 2011-04-14 21:23:22.000000000 +0200 +++ sigprocmask/kernel/compat.c 2011-04-22 19:19:39.000000000 +0200 @@ -893,7 +893,7 @@ compat_sys_rt_sigtimedwait (compat_sigse int sig; struct timespec t; siginfo_t info; - long ret, timeout = 0; + long ret, timeout; if (sigsetsize != sizeof(sigset_t)) return -EINVAL; @@ -904,36 +904,30 @@ compat_sys_rt_sigtimedwait (compat_sigse sigdelsetmask(&s,sigmask(SIGKILL)|sigmask(SIGSTOP)); signotset(&s); + timeout = MAX_SCHEDULE_TIMEOUT; if (uts) { if (get_compat_timespec (&t, uts)) return -EFAULT; - if (t.tv_nsec >= 1000000000L || t.tv_nsec < 0 - || t.tv_sec < 0) + if (!timespec_valid(&t)) return -EINVAL; + timeout = timespec_to_jiffies(&t) + (t.tv_sec || t.tv_nsec); } spin_lock_irq(¤t->sighand->siglock); sig = dequeue_signal(current, &s, &info); - if (!sig) { - timeout = MAX_SCHEDULE_TIMEOUT; - if (uts) - timeout = timespec_to_jiffies(&t) - +(t.tv_sec || t.tv_nsec); - if (timeout) { - current->real_blocked = current->blocked; - sigandsets(¤t->blocked, ¤t->blocked, &s); - - recalc_sigpending(); - spin_unlock_irq(¤t->sighand->siglock); + if (!sig && timeout) { + current->real_blocked = current->blocked; + sigandsets(¤t->blocked, ¤t->blocked, &s); + recalc_sigpending(); + spin_unlock_irq(¤t->sighand->siglock); - timeout = schedule_timeout_interruptible(timeout); + timeout = schedule_timeout_interruptible(timeout); - spin_lock_irq(¤t->sighand->siglock); - sig = dequeue_signal(current, &s, &info); - current->blocked = current->real_blocked; - siginitset(¤t->real_blocked, 0); - recalc_sigpending(); - } + spin_lock_irq(¤t->sighand->siglock); + sig = dequeue_signal(current, &s, &info); + current->blocked = current->real_blocked; + siginitset(¤t->real_blocked, 0); + recalc_sigpending(); } spin_unlock_irq(¤t->sighand->siglock); @@ -943,7 +937,7 @@ compat_sys_rt_sigtimedwait (compat_sigse if (copy_siginfo_to_user32(uinfo, &info)) ret = -EFAULT; } - }else { + } else { ret = timeout?-EINTR:-EAGAIN; } return ret; ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic 2011-04-23 17:59 ` [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic Oleg Nesterov @ 2011-04-25 11:37 ` Tejun Heo 2011-04-25 17:26 ` Oleg Nesterov 2011-04-26 10:18 ` Matt Fleming 1 sibling, 1 reply; 88+ messages in thread From: Tejun Heo @ 2011-04-25 11:37 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Sat, Apr 23, 2011 at 07:59:22PM +0200, Oleg Nesterov wrote: > No functional changes, cleanup compat_sys_rt_sigtimedwait() and > sys_rt_sigtimedwait(). > > Calculate the timeout before we take ->siglock, this simplifies and > lessens the code. Use timespec_valid() to check the timespec. > > I don't understand why we are adding (ts.tv_sec || ts.tv_nsec) to > timespec_to_jiffies(&ts). Perhaps to ensure we will sleep at least > one jiffy if ts != 0? But in this case we should only check tv_nsec, > I don't think timespec_to_jiffies() can return zero if tv_sec != 0. > In fact I suspect timespec_to_jiffies() can only return zero if > tv_sec == tv_nsec == 0 because we add "TICK_NSEC - 1", but I am not > sure I understand correctly this math. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> It might be a good idea to note the weird jiffies calculation with a comment? Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic 2011-04-25 11:37 ` Tejun Heo @ 2011-04-25 17:26 ` Oleg Nesterov 2011-04-25 17:34 ` Linus Torvalds 0 siblings, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-04-25 17:26 UTC (permalink / raw) To: Tejun Heo Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On 04/25, Tejun Heo wrote: > > On Sat, Apr 23, 2011 at 07:59:22PM +0200, Oleg Nesterov wrote: > > > > I don't understand why we are adding (ts.tv_sec || ts.tv_nsec) to > > timespec_to_jiffies(&ts). Perhaps to ensure we will sleep at least > > one jiffy if ts != 0? But in this case we should only check tv_nsec, > > I don't think timespec_to_jiffies() can return zero if tv_sec != 0. > > In fact I suspect timespec_to_jiffies() can only return zero if > > tv_sec == tv_nsec == 0 because we add "TICK_NSEC - 1", but I am not > > sure I understand correctly this math. > > > It might be a good idea to note the weird jiffies calculation with a > comment? If only I knew what this comment could say except /* Why do we add (tv_sec || tv_nsec) ? */ I'd better send 4/3 which simply removes this (I hope) unneeded code. Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic 2011-04-25 17:26 ` Oleg Nesterov @ 2011-04-25 17:34 ` Linus Torvalds 2011-04-25 17:56 ` Oleg Nesterov 0 siblings, 1 reply; 88+ messages in thread From: Linus Torvalds @ 2011-04-25 17:34 UTC (permalink / raw) To: Oleg Nesterov Cc: Tejun Heo, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Mon, Apr 25, 2011 at 10:26 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > If only I knew what this comment could say except > > /* Why do we add (tv_sec || tv_nsec) ? */ > > I'd better send 4/3 which simply removes this (I hope) unneeded code. It's to guarantee that timeout is at least one tick more than asked for, because the rule is that you really have to wait for AT LEAST the time asked for. With the "zero timeout" being special, since that is "immediate". Imagine that you ask for one timer tick - but that you're in the _middle_ of the current one. Waiting for the next timer is going to be too short - you'll only get half a timer tick. So we need to ask for "ceiling(nanoseconds / nanosecondspertick) + 1" to make sure that we really wait _longer_ than asked for. So "+ (tv_sec || tv_nsec)" is just the "+1" for the "not zero timeout" case. Linus ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic 2011-04-25 17:34 ` Linus Torvalds @ 2011-04-25 17:56 ` Oleg Nesterov 2011-04-25 19:38 ` Linus Torvalds 0 siblings, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-04-25 17:56 UTC (permalink / raw) To: Linus Torvalds Cc: Tejun Heo, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On 04/25, Linus Torvalds wrote: > > On Mon, Apr 25, 2011 at 10:26 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > > > If only I knew what this comment could say except > > > > /* Why do we add (tv_sec || tv_nsec) ? */ > > > > I'd better send 4/3 which simply removes this (I hope) unneeded code. > > It's to guarantee that timeout is at least one tick more than asked > for, because the rule is that you really have to wait for AT LEAST the > time asked for. Aah, thanks. This makes sense. I'll add the comment. > So "+ (tv_sec || tv_nsec)" is just the "+1" for the "not zero timeout" case. So, we have timeout = timespec_to_jiffies(ts) + (tv_sec || tv_nsec); ... if (timeout) timeout = schedule_timeout_interruptible(timeout); Perhaps it makes sense to turn this code into timeout = timespec_to_jiffies(ts); if (timeout) // make sure we sleep at least the time we asked for timeout = schedule_timeout_interruptible(timeout + 1); Assuming that timespec_to_jiffies() always returns nonzero if ts is "not zero timeout". I think it does. Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic 2011-04-25 17:56 ` Oleg Nesterov @ 2011-04-25 19:38 ` Linus Torvalds 0 siblings, 0 replies; 88+ messages in thread From: Linus Torvalds @ 2011-04-25 19:38 UTC (permalink / raw) To: Oleg Nesterov Cc: Tejun Heo, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Mon, Apr 25, 2011 at 10:56 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > Assuming that timespec_to_jiffies() always returns nonzero if ts is > "not zero timeout". I think it does. You really need to double-check that it does. We've not always done that right, and while I think "round up" is the right thing to do there, it had better be verified and then a comment added to make sure it doesn't change. But yes, with verification and comment, your "if (timeout) .. timeout+1" would work fine. Linus ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic 2011-04-23 17:59 ` [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic Oleg Nesterov 2011-04-25 11:37 ` Tejun Heo @ 2011-04-26 10:18 ` Matt Fleming 1 sibling, 0 replies; 88+ messages in thread From: Matt Fleming @ 2011-04-26 10:18 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Tejun Heo, Nikita V. Youshchenko, linux-kernel On Sat, 23 Apr 2011 19:59:22 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > No functional changes, cleanup compat_sys_rt_sigtimedwait() and > sys_rt_sigtimedwait(). > > Calculate the timeout before we take ->siglock, this simplifies and > lessens the code. Use timespec_valid() to check the timespec. > > I don't understand why we are adding (ts.tv_sec || ts.tv_nsec) to > timespec_to_jiffies(&ts). Perhaps to ensure we will sleep at least > one jiffy if ts != 0? But in this case we should only check tv_nsec, > I don't think timespec_to_jiffies() can return zero if tv_sec != 0. > In fact I suspect timespec_to_jiffies() can only return zero if > tv_sec == tv_nsec == 0 because we add "TICK_NSEC - 1", but I am not > sure I understand correctly this math. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code 2011-04-23 17:59 ` [PATCH 0/3] do_sigtimedwait() needs retarget_shared_pending() Oleg Nesterov 2011-04-23 17:59 ` [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic Oleg Nesterov @ 2011-04-23 17:59 ` Oleg Nesterov 2011-04-25 11:39 ` Tejun Heo ` (2 more replies) 2011-04-23 18:00 ` [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending() Oleg Nesterov 2011-04-26 19:48 ` [PATCH v2 0/6] sigtimedwait/sigprocmask need retarget_shared_pending() Oleg Nesterov 3 siblings, 3 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-23 17:59 UTC (permalink / raw) To: Linus Torvalds, Andrew Morton Cc: Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel Factor out the common code in sys_rt_sigtimedwait/compat_sys_rt_sigtimedwait to the new helper, do_sigtimedwait(). Perhaps it would be better to move compat_sys_rt_sigtimedwait() into signal.c under CONFIG_COMPAT, then we can make do_sigtimedwait() static. Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- include/linux/signal.h | 1 kernel/signal.c | 71 ++++++++++++++++++++++++++----------------------- kernel/compat.c | 29 ++------------------ 3 files changed, 43 insertions(+), 58 deletions(-) --- sigprocmask/include/linux/signal.h~2_do_sigtimedwait 2011-04-22 15:48:33.000000000 +0200 +++ sigprocmask/include/linux/signal.h 2011-04-22 21:05:48.000000000 +0200 @@ -242,6 +242,7 @@ extern int __group_send_sig_info(int, st extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, siginfo_t *info); extern long do_sigpending(void __user *, unsigned long); +extern int do_sigtimedwait(sigset_t *, siginfo_t *, long); extern int sigprocmask(int, sigset_t *, sigset_t *); extern void set_current_blocked(const sigset_t *); extern int show_unhandled_signals; --- sigprocmask/kernel/signal.c~2_do_sigtimedwait 2011-04-22 19:05:51.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-22 20:57:06.000000000 +0200 @@ -2311,6 +2311,39 @@ int copy_siginfo_to_user(siginfo_t __use #endif +int do_sigtimedwait(sigset_t *these, siginfo_t *info, long timeout) +{ + struct task_struct *tsk = current; + int sig; + + spin_lock_irq(&tsk->sighand->siglock); + sig = dequeue_signal(tsk, these, info); + if (!sig && timeout) { + /* + * None ready -- temporarily unblock those we're + * interested while we are sleeping in so that we'll + * be awakened when they arrive. + */ + tsk->real_blocked = tsk->blocked; + sigandsets(&tsk->blocked, &tsk->blocked, these); + recalc_sigpending(); + spin_unlock_irq(&tsk->sighand->siglock); + + timeout = schedule_timeout_interruptible(timeout); + + spin_lock_irq(&tsk->sighand->siglock); + sig = dequeue_signal(tsk, these, info); + tsk->blocked = tsk->real_blocked; + siginitset(&tsk->real_blocked, 0); + recalc_sigpending(); + } + spin_unlock_irq(&tsk->sighand->siglock); + + if (sig) + return sig; + return timeout ? -EINTR : -EAGAIN; +} + /** * sys_rt_sigtimedwait - synchronously wait for queued signals specified * in @uthese @@ -2323,11 +2356,11 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s siginfo_t __user *, uinfo, const struct timespec __user *, uts, size_t, sigsetsize) { - int ret, sig; sigset_t these; struct timespec ts; siginfo_t info; long timeout; + int ret; /* XXX: Don't preclude handling different sized sigset_t's. */ if (sigsetsize != sizeof(sigset_t)) @@ -2352,39 +2385,11 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s timeout = timespec_to_jiffies(&ts) + (ts.tv_sec || ts.tv_nsec); } - spin_lock_irq(¤t->sighand->siglock); - sig = dequeue_signal(current, &these, &info); - if (!sig && timeout) { - /* - * None ready -- temporarily unblock those we're - * interested while we are sleeping in so that we'll - * be awakened when they arrive. - */ - current->real_blocked = current->blocked; - sigandsets(¤t->blocked, ¤t->blocked, &these); - recalc_sigpending(); - spin_unlock_irq(¤t->sighand->siglock); - - timeout = schedule_timeout_interruptible(timeout); - - spin_lock_irq(¤t->sighand->siglock); - sig = dequeue_signal(current, &these, &info); - current->blocked = current->real_blocked; - siginitset(¤t->real_blocked, 0); - recalc_sigpending(); - } - spin_unlock_irq(¤t->sighand->siglock); + ret = do_sigtimedwait(&these, &info, timeout); - if (sig) { - ret = sig; - if (uinfo) { - if (copy_siginfo_to_user(uinfo, &info)) - ret = -EFAULT; - } - } else { - ret = -EAGAIN; - if (timeout) - ret = -EINTR; + if (ret > 0 && uinfo) { + if (copy_siginfo_to_user(uinfo, &info)) + ret = -EFAULT; } return ret; --- sigprocmask/kernel/compat.c~2_do_sigtimedwait 2011-04-22 19:19:39.000000000 +0200 +++ sigprocmask/kernel/compat.c 2011-04-22 21:20:33.000000000 +0200 @@ -890,7 +890,6 @@ compat_sys_rt_sigtimedwait (compat_sigse { compat_sigset_t s32; sigset_t s; - int sig; struct timespec t; siginfo_t info; long ret, timeout; @@ -913,33 +912,13 @@ compat_sys_rt_sigtimedwait (compat_sigse timeout = timespec_to_jiffies(&t) + (t.tv_sec || t.tv_nsec); } - spin_lock_irq(¤t->sighand->siglock); - sig = dequeue_signal(current, &s, &info); - if (!sig && timeout) { - current->real_blocked = current->blocked; - sigandsets(¤t->blocked, ¤t->blocked, &s); - recalc_sigpending(); - spin_unlock_irq(¤t->sighand->siglock); - - timeout = schedule_timeout_interruptible(timeout); + ret = do_sigtimedwait(&s, &info, timeout); - spin_lock_irq(¤t->sighand->siglock); - sig = dequeue_signal(current, &s, &info); - current->blocked = current->real_blocked; - siginitset(¤t->real_blocked, 0); - recalc_sigpending(); + if (ret > 0 && uinfo) { + if (copy_siginfo_to_user32(uinfo, &info)) + ret = -EFAULT; } - spin_unlock_irq(¤t->sighand->siglock); - if (sig) { - ret = sig; - if (uinfo) { - if (copy_siginfo_to_user32(uinfo, &info)) - ret = -EFAULT; - } - } else { - ret = timeout?-EINTR:-EAGAIN; - } return ret; } ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code 2011-04-23 17:59 ` [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code Oleg Nesterov @ 2011-04-25 11:39 ` Tejun Heo 2011-04-25 11:49 ` Tejun Heo 2011-04-26 10:28 ` Matt Fleming 2 siblings, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-25 11:39 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Sat, Apr 23, 2011 at 07:59:40PM +0200, Oleg Nesterov wrote: > Factor out the common code in sys_rt_sigtimedwait/compat_sys_rt_sigtimedwait > to the new helper, do_sigtimedwait(). > > Perhaps it would be better to move compat_sys_rt_sigtimedwait() into > signal.c under CONFIG_COMPAT, then we can make do_sigtimedwait() static. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code 2011-04-23 17:59 ` [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code Oleg Nesterov 2011-04-25 11:39 ` Tejun Heo @ 2011-04-25 11:49 ` Tejun Heo 2011-04-25 15:33 ` Oleg Nesterov 2011-04-26 10:28 ` Matt Fleming 2 siblings, 1 reply; 88+ messages in thread From: Tejun Heo @ 2011-04-25 11:49 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel Just one more thing. On Sat, Apr 23, 2011 at 07:59:40PM +0200, Oleg Nesterov wrote: > +int do_sigtimedwait(sigset_t *these, siginfo_t *info, long timeout) Maybe @these isn't the base name here? It implies that these are the signals the function is interested in but in reality it is the negation of that. The original function should be blamed for using the same name while negating its meaning but separating out the function makes the inconsitency stand out. Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code 2011-04-25 11:49 ` Tejun Heo @ 2011-04-25 15:33 ` Oleg Nesterov 2011-04-25 16:25 ` Tejun Heo 0 siblings, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-04-25 15:33 UTC (permalink / raw) To: Tejun Heo Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On 04/25, Tejun Heo wrote: > > Just one more thing. > > On Sat, Apr 23, 2011 at 07:59:40PM +0200, Oleg Nesterov wrote: > > +int do_sigtimedwait(sigset_t *these, siginfo_t *info, long timeout) > > Maybe @these isn't the base name here? It implies that these are the > signals the function is interested in but in reality it is the > negation of that. Yees, I simply copied the old name. And yes, I tried to invent the better name but failed. Hmm. I think this patch can do a bit more. We can factor out sigdelsetmask(SIGKILL | SIGSTOP) + signotset() as well, I'll resend. Still, what should be the name? @set? @mask? @which? Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code 2011-04-25 15:33 ` Oleg Nesterov @ 2011-04-25 16:25 ` Tejun Heo 0 siblings, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-25 16:25 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel Hey, On Mon, Apr 25, 2011 at 05:33:16PM +0200, Oleg Nesterov wrote: > On 04/25, Tejun Heo wrote: > > > > Just one more thing. > > > > On Sat, Apr 23, 2011 at 07:59:40PM +0200, Oleg Nesterov wrote: > > > +int do_sigtimedwait(sigset_t *these, siginfo_t *info, long timeout) > > > > Maybe @these isn't the base name here? It implies that these are the ^^^^ oops, best > > signals the function is interested in but in reality it is the > > negation of that. > > Yees, I simply copied the old name. And yes, I tried to invent the > better name but failed. > > Hmm. I think this patch can do a bit more. We can factor out > sigdelsetmask(SIGKILL | SIGSTOP) + signotset() as well, I'll resend. > > Still, what should be the name? @set? @mask? @which? Given that next_signal() and friends already use @mask, probably @mask? As long as positive selection and negative masking are clearly distinguishible... Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code 2011-04-23 17:59 ` [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code Oleg Nesterov 2011-04-25 11:39 ` Tejun Heo 2011-04-25 11:49 ` Tejun Heo @ 2011-04-26 10:28 ` Matt Fleming 2 siblings, 0 replies; 88+ messages in thread From: Matt Fleming @ 2011-04-26 10:28 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Tejun Heo, Nikita V. Youshchenko, linux-kernel On Sat, 23 Apr 2011 19:59:40 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > Factor out the common code in sys_rt_sigtimedwait/compat_sys_rt_sigtimedwait > to the new helper, do_sigtimedwait(). > > Perhaps it would be better to move compat_sys_rt_sigtimedwait() into > signal.c under CONFIG_COMPAT, then we can make do_sigtimedwait() static. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> The changes Tejun suggested seem like a good idea. I also think that moving compat_sys_rt_sigtimedwait() into signal.c makes sense. -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending() 2011-04-23 17:59 ` [PATCH 0/3] do_sigtimedwait() needs retarget_shared_pending() Oleg Nesterov 2011-04-23 17:59 ` [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic Oleg Nesterov 2011-04-23 17:59 ` [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code Oleg Nesterov @ 2011-04-23 18:00 ` Oleg Nesterov 2011-04-25 11:52 ` Tejun Heo 2011-04-26 10:42 ` Matt Fleming 2011-04-26 19:48 ` [PATCH v2 0/6] sigtimedwait/sigprocmask need retarget_shared_pending() Oleg Nesterov 3 siblings, 2 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-23 18:00 UTC (permalink / raw) To: Linus Torvalds, Andrew Morton Cc: Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel do_sigtimedwait() changes current->blocked and thus it needs set_current_bloked()->retarget_shared_pending(). We could use set_current_bloked() directly. It is fine to change ->real_blocked from all-zeroes to ->blocked and vice versa lockless, but this is not immediately clear, looks racy, and needs a huge comment to explain why this is correct. To keep the things simple this patch adds the new static helper, __set_task_blocked() which should be called with ->siglock held. This way we can change both ->real_blocked and ->blocked atomically under ->siglock as the current code does. This is more understandable. Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- kernel/signal.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) --- sigprocmask/kernel/signal.c~3_sigtimedwait_retarget 2011-04-22 20:57:06.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-22 21:49:11.000000000 +0200 @@ -2115,11 +2115,8 @@ long do_no_restart_syscall(struct restar return -EINTR; } -void set_current_blocked(const sigset_t *newset) +static void __set_task_blocked(struct task_struct *tsk, const sigset_t *newset) { - struct task_struct *tsk = current; - - spin_lock_irq(&tsk->sighand->siglock); if (signal_pending(tsk) && !thread_group_empty(tsk)) { sigset_t newblocked; signandsets(&newblocked, newset, ¤t->blocked); @@ -2127,6 +2124,14 @@ void set_current_blocked(const sigset_t } tsk->blocked = *newset; recalc_sigpending(); +} + +void set_current_blocked(const sigset_t *newset) +{ + struct task_struct *tsk = current; + + spin_lock_irq(&tsk->sighand->siglock); + __set_task_blocked(tsk, newset); spin_unlock_irq(&tsk->sighand->siglock); } @@ -2322,7 +2327,8 @@ int do_sigtimedwait(sigset_t *these, sig /* * None ready -- temporarily unblock those we're * interested while we are sleeping in so that we'll - * be awakened when they arrive. + * be awakened when they arrive. Unblocking is always + * fine, we can avoid set_current_blocked(). */ tsk->real_blocked = tsk->blocked; sigandsets(&tsk->blocked, &tsk->blocked, these); @@ -2332,10 +2338,9 @@ int do_sigtimedwait(sigset_t *these, sig timeout = schedule_timeout_interruptible(timeout); spin_lock_irq(&tsk->sighand->siglock); - sig = dequeue_signal(tsk, these, info); - tsk->blocked = tsk->real_blocked; + __set_task_blocked(tsk, &tsk->real_blocked); siginitset(&tsk->real_blocked, 0); - recalc_sigpending(); + sig = dequeue_signal(tsk, these, info); } spin_unlock_irq(&tsk->sighand->siglock); ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending() 2011-04-23 18:00 ` [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending() Oleg Nesterov @ 2011-04-25 11:52 ` Tejun Heo 2011-04-25 16:01 ` Oleg Nesterov 2011-04-26 10:42 ` Matt Fleming 1 sibling, 1 reply; 88+ messages in thread From: Tejun Heo @ 2011-04-25 11:52 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel Hello, On Sat, Apr 23, 2011 at 08:00:00PM +0200, Oleg Nesterov wrote: > do_sigtimedwait() changes current->blocked and thus it needs > set_current_bloked()->retarget_shared_pending(). > > We could use set_current_bloked() directly. It is fine to change > ->real_blocked from all-zeroes to ->blocked and vice versa lockless, > but this is not immediately clear, looks racy, and needs a huge > comment to explain why this is correct. > > To keep the things simple this patch adds the new static helper, > __set_task_blocked() which should be called with ->siglock held. This > way we can change both ->real_blocked and ->blocked atomically under > ->siglock as the current code does. This is more understandable. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> > @@ -2322,7 +2327,8 @@ int do_sigtimedwait(sigset_t *these, sig > /* > * None ready -- temporarily unblock those we're > * interested while we are sleeping in so that we'll > - * be awakened when they arrive. > + * be awakened when they arrive. Unblocking is always > + * fine, we can avoid set_current_blocked(). > */ > tsk->real_blocked = tsk->blocked; > sigandsets(&tsk->blocked, &tsk->blocked, these); Maybe it would be a good idea to introduce a new helper which checks / enforces that the operation indeed is only unblocking? Also, it can be a pure preference but I think _locked suffix is better / more common for APIs which expect the caller to be responsible for locking. Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending() 2011-04-25 11:52 ` Tejun Heo @ 2011-04-25 16:01 ` Oleg Nesterov 2011-04-25 16:27 ` Tejun Heo 0 siblings, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-04-25 16:01 UTC (permalink / raw) To: Tejun Heo Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On 04/25, Tejun Heo wrote: > > > @@ -2322,7 +2327,8 @@ int do_sigtimedwait(sigset_t *these, sig > > /* > > * None ready -- temporarily unblock those we're > > * interested while we are sleeping in so that we'll > > - * be awakened when they arrive. > > + * be awakened when they arrive. Unblocking is always > > + * fine, we can avoid set_current_blocked(). > > */ > > tsk->real_blocked = tsk->blocked; > > sigandsets(&tsk->blocked, &tsk->blocked, these); > > Maybe it would be a good idea to introduce a new helper which checks / > enforces that the operation indeed is only unblocking? I hope nobody will change ->blocked directly, except this function and force_sig_info(). And daemonize/allow_signal/disallow_signal, but there are special and probably we can already kill this deprecated block/unblock code and forbid kernel_thread(CLONE_SIGHAND) + daemonize(). In fact I think daemonize() should go away. So, I don't really think we need another helper to unblock something. > Also, it can > be a pure preference but I think _locked suffix is better / more > common for APIs which expect the caller to be responsible for locking. Again, I can rename... Cough, but in this case please simply suggest another name. set_tsk_blocked_locked? Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending() 2011-04-25 16:01 ` Oleg Nesterov @ 2011-04-25 16:27 ` Tejun Heo 2011-04-25 17:07 ` Oleg Nesterov 0 siblings, 1 reply; 88+ messages in thread From: Tejun Heo @ 2011-04-25 16:27 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel Hello, On Mon, Apr 25, 2011 at 06:01:15PM +0200, Oleg Nesterov wrote: > > Maybe it would be a good idea to introduce a new helper which checks / > > enforces that the operation indeed is only unblocking? > > I hope nobody will change ->blocked directly, except this function > and force_sig_info(). And daemonize/allow_signal/disallow_signal, but > there are special and probably we can already kill this deprecated > block/unblock code and forbid kernel_thread(CLONE_SIGHAND) + daemonize(). > In fact I think daemonize() should go away. > > So, I don't really think we need another helper to unblock something. Oh I see. I thought there would be quite a number of places unblocking directly. If that's not the case, it's fine with me. > > Also, it can > > be a pure preference but I think _locked suffix is better / more > > common for APIs which expect the caller to be responsible for locking. > > Again, I can rename... Cough, but in this case please simply suggest > another name. set_tsk_blocked_locked? Oooh, blocked_locked, didn't see that one coming. Maybe set_tsk_sigmask() and set_tsk_sigmask_locked()? I prefer sigmask to blocked anyway, so... Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending() 2011-04-25 16:27 ` Tejun Heo @ 2011-04-25 17:07 ` Oleg Nesterov 2011-04-25 17:12 ` Tejun Heo 2011-04-26 10:40 ` Matt Fleming 0 siblings, 2 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-25 17:07 UTC (permalink / raw) To: Tejun Heo Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On 04/25, Tejun Heo wrote: > > On Mon, Apr 25, 2011 at 06:01:15PM +0200, Oleg Nesterov wrote: > > > > Again, I can rename... Cough, but in this case please simply suggest > > another name. set_tsk_blocked_locked? > > Oooh, blocked_locked, didn't see that one coming. Maybe > set_tsk_sigmask() but it is not _tsk, it is specially named set_current_blocked() to show that it only applies to current. And _blocked clearly shows what it should change, like set_current_state(). OK, this is purely cosmetic, and __set_tsk_blocked() is static and has a single caller. Can we keep this naming for now? it would be trivial to rename later. Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending() 2011-04-25 17:07 ` Oleg Nesterov @ 2011-04-25 17:12 ` Tejun Heo 2011-04-26 10:40 ` Matt Fleming 1 sibling, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-25 17:12 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Mon, Apr 25, 2011 at 7:07 PM, Oleg Nesterov <oleg@redhat.com> wrote: >> set_tsk_sigmask() > > but it is not _tsk, it is specially named set_current_blocked() to > show that it only applies to current. And _blocked clearly shows what > it should change, like set_current_state(). Ooh, I meant set_current_sigmask(). > OK, this is purely cosmetic, and __set_tsk_blocked() is static and has > a single caller. Can we keep this naming for now? it would be trivial > to rename later. Sure, no biggie. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending() 2011-04-25 17:07 ` Oleg Nesterov 2011-04-25 17:12 ` Tejun Heo @ 2011-04-26 10:40 ` Matt Fleming 1 sibling, 0 replies; 88+ messages in thread From: Matt Fleming @ 2011-04-26 10:40 UTC (permalink / raw) To: Oleg Nesterov Cc: Tejun Heo, Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, linux-kernel On Mon, 25 Apr 2011 19:07:38 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > On 04/25, Tejun Heo wrote: > > > > On Mon, Apr 25, 2011 at 06:01:15PM +0200, Oleg Nesterov wrote: > > > > > > Again, I can rename... Cough, but in this case please simply suggest > > > another name. set_tsk_blocked_locked? > > > > Oooh, blocked_locked, didn't see that one coming. Maybe > > set_tsk_sigmask() > > but it is not _tsk, it is specially named set_current_blocked() to > show that it only applies to current. And _blocked clearly shows what > it should change, like set_current_state(). > > OK, this is purely cosmetic, and __set_tsk_blocked() is static and has > a single caller. Can we keep this naming for now? it would be trivial > to rename later. It might be worth adding a comment to __set_tsk_blocked() saying that it expects to be called with ->siglock held. -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending() 2011-04-23 18:00 ` [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending() Oleg Nesterov 2011-04-25 11:52 ` Tejun Heo @ 2011-04-26 10:42 ` Matt Fleming 1 sibling, 0 replies; 88+ messages in thread From: Matt Fleming @ 2011-04-26 10:42 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Tejun Heo, Nikita V. Youshchenko, linux-kernel On Sat, 23 Apr 2011 20:00:00 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > do_sigtimedwait() changes current->blocked and thus it needs > set_current_bloked()->retarget_shared_pending(). If you do another version of this patch could you fix up the function names in the commit log, s/set_current_bloked/set_current_blocked/ ? Or maybe Andrew can fix it up if he pulls them into -mm. > We could use set_current_bloked() directly. It is fine to change > ->real_blocked from all-zeroes to ->blocked and vice versa lockless, > but this is not immediately clear, looks racy, and needs a huge > comment to explain why this is correct. > > To keep the things simple this patch adds the new static helper, > __set_task_blocked() which should be called with ->siglock held. This > way we can change both ->real_blocked and ->blocked atomically under > ->siglock as the current code does. This is more understandable. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 0/6] sigtimedwait/sigprocmask need retarget_shared_pending() 2011-04-23 17:59 ` [PATCH 0/3] do_sigtimedwait() needs retarget_shared_pending() Oleg Nesterov ` (2 preceding siblings ...) 2011-04-23 18:00 ` [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending() Oleg Nesterov @ 2011-04-26 19:48 ` Oleg Nesterov 2011-04-26 19:48 ` [PATCH v2 1/6] signal: sys_rt_sigtimedwait: simplify the timeout logic Oleg Nesterov ` (5 more replies) 3 siblings, 6 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-26 19:48 UTC (permalink / raw) To: Linus Torvalds, Andrew Morton Cc: Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel Changes: 1/6: remove the evidence of my ignorance from the changelog, += (tv_sec || tv_nsec) is correct 2/6: more factorization, rename the argument, add the comment from Linus. please re-add the acks. 3/6: fix the typos in the changelog, thanks Matt 4-6: new. I really hope that this is the last "nontrivial" changes in this area which need any discussion, everything else looks simple. Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 1/6] signal: sys_rt_sigtimedwait: simplify the timeout logic 2011-04-26 19:48 ` [PATCH v2 0/6] sigtimedwait/sigprocmask need retarget_shared_pending() Oleg Nesterov @ 2011-04-26 19:48 ` Oleg Nesterov 2011-04-26 19:49 ` [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code Oleg Nesterov ` (4 subsequent siblings) 5 siblings, 0 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-26 19:48 UTC (permalink / raw) To: Linus Torvalds, Andrew Morton Cc: Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel No functional changes, cleanup compat_sys_rt_sigtimedwait() and sys_rt_sigtimedwait(). Calculate the timeout before we take ->siglock, this simplifies and lessens the code. Use timespec_valid() to check the timespec. Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> --- kernel/signal.c | 46 ++++++++++++++++++++-------------------------- kernel/compat.c | 38 ++++++++++++++++---------------------- 2 files changed, 36 insertions(+), 48 deletions(-) --- sigprocmask/kernel/signal.c~1_sigtimedwait_to 2011-04-26 19:52:30.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-26 19:53:19.000000000 +0200 @@ -2327,7 +2327,7 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s sigset_t these; struct timespec ts; siginfo_t info; - long timeout = 0; + long timeout; /* XXX: Don't preclude handling different sized sigset_t's. */ if (sigsetsize != sizeof(sigset_t)) @@ -2343,41 +2343,35 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s sigdelsetmask(&these, sigmask(SIGKILL)|sigmask(SIGSTOP)); signotset(&these); + timeout = MAX_SCHEDULE_TIMEOUT; if (uts) { if (copy_from_user(&ts, uts, sizeof(ts))) return -EFAULT; - if (ts.tv_nsec >= 1000000000L || ts.tv_nsec < 0 - || ts.tv_sec < 0) + if (!timespec_valid(&ts)) return -EINVAL; + timeout = timespec_to_jiffies(&ts) + (ts.tv_sec || ts.tv_nsec); } spin_lock_irq(¤t->sighand->siglock); sig = dequeue_signal(current, &these, &info); - if (!sig) { - timeout = MAX_SCHEDULE_TIMEOUT; - if (uts) - timeout = (timespec_to_jiffies(&ts) - + (ts.tv_sec || ts.tv_nsec)); - - if (timeout) { - /* - * None ready -- temporarily unblock those we're - * interested while we are sleeping in so that we'll - * be awakened when they arrive. - */ - current->real_blocked = current->blocked; - sigandsets(¤t->blocked, ¤t->blocked, &these); - recalc_sigpending(); - spin_unlock_irq(¤t->sighand->siglock); + if (!sig && timeout) { + /* + * None ready -- temporarily unblock those we're + * interested while we are sleeping in so that we'll + * be awakened when they arrive. + */ + current->real_blocked = current->blocked; + sigandsets(¤t->blocked, ¤t->blocked, &these); + recalc_sigpending(); + spin_unlock_irq(¤t->sighand->siglock); - timeout = schedule_timeout_interruptible(timeout); + timeout = schedule_timeout_interruptible(timeout); - spin_lock_irq(¤t->sighand->siglock); - sig = dequeue_signal(current, &these, &info); - current->blocked = current->real_blocked; - siginitset(¤t->real_blocked, 0); - recalc_sigpending(); - } + spin_lock_irq(¤t->sighand->siglock); + sig = dequeue_signal(current, &these, &info); + current->blocked = current->real_blocked; + siginitset(¤t->real_blocked, 0); + recalc_sigpending(); } spin_unlock_irq(¤t->sighand->siglock); --- sigprocmask/kernel/compat.c~1_sigtimedwait_to 2011-04-26 19:52:30.000000000 +0200 +++ sigprocmask/kernel/compat.c 2011-04-26 19:53:19.000000000 +0200 @@ -893,7 +893,7 @@ compat_sys_rt_sigtimedwait (compat_sigse int sig; struct timespec t; siginfo_t info; - long ret, timeout = 0; + long ret, timeout; if (sigsetsize != sizeof(sigset_t)) return -EINVAL; @@ -904,36 +904,30 @@ compat_sys_rt_sigtimedwait (compat_sigse sigdelsetmask(&s,sigmask(SIGKILL)|sigmask(SIGSTOP)); signotset(&s); + timeout = MAX_SCHEDULE_TIMEOUT; if (uts) { if (get_compat_timespec (&t, uts)) return -EFAULT; - if (t.tv_nsec >= 1000000000L || t.tv_nsec < 0 - || t.tv_sec < 0) + if (!timespec_valid(&t)) return -EINVAL; + timeout = timespec_to_jiffies(&t) + (t.tv_sec || t.tv_nsec); } spin_lock_irq(¤t->sighand->siglock); sig = dequeue_signal(current, &s, &info); - if (!sig) { - timeout = MAX_SCHEDULE_TIMEOUT; - if (uts) - timeout = timespec_to_jiffies(&t) - +(t.tv_sec || t.tv_nsec); - if (timeout) { - current->real_blocked = current->blocked; - sigandsets(¤t->blocked, ¤t->blocked, &s); - - recalc_sigpending(); - spin_unlock_irq(¤t->sighand->siglock); + if (!sig && timeout) { + current->real_blocked = current->blocked; + sigandsets(¤t->blocked, ¤t->blocked, &s); + recalc_sigpending(); + spin_unlock_irq(¤t->sighand->siglock); - timeout = schedule_timeout_interruptible(timeout); + timeout = schedule_timeout_interruptible(timeout); - spin_lock_irq(¤t->sighand->siglock); - sig = dequeue_signal(current, &s, &info); - current->blocked = current->real_blocked; - siginitset(¤t->real_blocked, 0); - recalc_sigpending(); - } + spin_lock_irq(¤t->sighand->siglock); + sig = dequeue_signal(current, &s, &info); + current->blocked = current->real_blocked; + siginitset(¤t->real_blocked, 0); + recalc_sigpending(); } spin_unlock_irq(¤t->sighand->siglock); @@ -943,7 +937,7 @@ compat_sys_rt_sigtimedwait (compat_sigse if (copy_siginfo_to_user32(uinfo, &info)) ret = -EFAULT; } - }else { + } else { ret = timeout?-EINTR:-EAGAIN; } return ret; ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code 2011-04-26 19:48 ` [PATCH v2 0/6] sigtimedwait/sigprocmask need retarget_shared_pending() Oleg Nesterov 2011-04-26 19:48 ` [PATCH v2 1/6] signal: sys_rt_sigtimedwait: simplify the timeout logic Oleg Nesterov @ 2011-04-26 19:49 ` Oleg Nesterov 2011-04-27 10:09 ` Tejun Heo ` (2 more replies) 2011-04-26 19:49 ` [PATCH v2 3/6] signal: do_sigtimedwait() needs retarget_shared_pending() Oleg Nesterov ` (3 subsequent siblings) 5 siblings, 3 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-26 19:49 UTC (permalink / raw) To: Linus Torvalds, Andrew Morton Cc: Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel Factor out the common code in sys_rt_sigtimedwait/compat_sys_rt_sigtimedwait to the new helper, do_sigtimedwait(). Add the comment to document the extra tick we add to timespec_to_jiffies(ts), thanks to Linus who explained this to me. Perhaps it would be better to move compat_sys_rt_sigtimedwait() into signal.c under CONFIG_COMPAT, then we can make do_sigtimedwait() static. Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- include/linux/signal.h | 2 kernel/signal.c | 104 +++++++++++++++++++++++++++---------------------- kernel/compat.c | 39 ++---------------- 3 files changed, 67 insertions(+), 78 deletions(-) --- sigprocmask/include/linux/signal.h~2_do_sigtimedwait 2011-04-26 19:52:30.000000000 +0200 +++ sigprocmask/include/linux/signal.h 2011-04-26 19:53:42.000000000 +0200 @@ -242,6 +242,8 @@ extern int __group_send_sig_info(int, st extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, siginfo_t *info); extern long do_sigpending(void __user *, unsigned long); +extern int do_sigtimedwait(const sigset_t *, siginfo_t *, + const struct timespec *); extern int sigprocmask(int, sigset_t *, sigset_t *); extern void set_current_blocked(const sigset_t *); extern int show_unhandled_signals; --- sigprocmask/kernel/signal.c~2_do_sigtimedwait 2011-04-26 19:53:19.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-26 19:53:42.000000000 +0200 @@ -2311,6 +2311,60 @@ int copy_siginfo_to_user(siginfo_t __use #endif +int do_sigtimedwait(const sigset_t *which, siginfo_t *info, + const struct timespec *ts) +{ + struct task_struct *tsk = current; + long timeout = MAX_SCHEDULE_TIMEOUT; + sigset_t mask = *which; + int sig; + + if (ts) { + if (!timespec_valid(ts)) + return -EINVAL; + timeout = timespec_to_jiffies(ts); + /* + * We can be close to the next tick, add another one + * to ensure we will wait at least the time asked for. + */ + if (ts->tv_sec || ts->tv_nsec) + timeout++; + } + + /* + * Invert the set of allowed signals to get those we want to block. + */ + sigdelsetmask(&mask, sigmask(SIGKILL) | sigmask(SIGSTOP)); + signotset(&mask); + + spin_lock_irq(&tsk->sighand->siglock); + sig = dequeue_signal(tsk, &mask, info); + if (!sig && timeout) { + /* + * None ready, temporarily unblock those we're interested + * while we are sleeping in so that we'll be awakened when + * they arrive. + */ + tsk->real_blocked = tsk->blocked; + sigandsets(&tsk->blocked, &tsk->blocked, &mask); + recalc_sigpending(); + spin_unlock_irq(&tsk->sighand->siglock); + + timeout = schedule_timeout_interruptible(timeout); + + spin_lock_irq(&tsk->sighand->siglock); + sig = dequeue_signal(tsk, &mask, info); + tsk->blocked = tsk->real_blocked; + siginitset(&tsk->real_blocked, 0); + recalc_sigpending(); + } + spin_unlock_irq(&tsk->sighand->siglock); + + if (sig) + return sig; + return timeout ? -EINTR : -EAGAIN; +} + /** * sys_rt_sigtimedwait - synchronously wait for queued signals specified * in @uthese @@ -2323,11 +2377,10 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s siginfo_t __user *, uinfo, const struct timespec __user *, uts, size_t, sigsetsize) { - int ret, sig; sigset_t these; struct timespec ts; siginfo_t info; - long timeout; + int ret; /* XXX: Don't preclude handling different sized sigset_t's. */ if (sigsetsize != sizeof(sigset_t)) @@ -2336,55 +2389,16 @@ SYSCALL_DEFINE4(rt_sigtimedwait, const s if (copy_from_user(&these, uthese, sizeof(these))) return -EFAULT; - /* - * Invert the set of allowed signals to get those we - * want to block. - */ - sigdelsetmask(&these, sigmask(SIGKILL)|sigmask(SIGSTOP)); - signotset(&these); - - timeout = MAX_SCHEDULE_TIMEOUT; if (uts) { if (copy_from_user(&ts, uts, sizeof(ts))) return -EFAULT; - if (!timespec_valid(&ts)) - return -EINVAL; - timeout = timespec_to_jiffies(&ts) + (ts.tv_sec || ts.tv_nsec); } - spin_lock_irq(¤t->sighand->siglock); - sig = dequeue_signal(current, &these, &info); - if (!sig && timeout) { - /* - * None ready -- temporarily unblock those we're - * interested while we are sleeping in so that we'll - * be awakened when they arrive. - */ - current->real_blocked = current->blocked; - sigandsets(¤t->blocked, ¤t->blocked, &these); - recalc_sigpending(); - spin_unlock_irq(¤t->sighand->siglock); - - timeout = schedule_timeout_interruptible(timeout); - - spin_lock_irq(¤t->sighand->siglock); - sig = dequeue_signal(current, &these, &info); - current->blocked = current->real_blocked; - siginitset(¤t->real_blocked, 0); - recalc_sigpending(); - } - spin_unlock_irq(¤t->sighand->siglock); + ret = do_sigtimedwait(&these, &info, uts ? &ts : NULL); - if (sig) { - ret = sig; - if (uinfo) { - if (copy_siginfo_to_user(uinfo, &info)) - ret = -EFAULT; - } - } else { - ret = -EAGAIN; - if (timeout) - ret = -EINTR; + if (ret > 0 && uinfo) { + if (copy_siginfo_to_user(uinfo, &info)) + ret = -EFAULT; } return ret; --- sigprocmask/kernel/compat.c~2_do_sigtimedwait 2011-04-26 19:53:19.000000000 +0200 +++ sigprocmask/kernel/compat.c 2011-04-26 19:53:42.000000000 +0200 @@ -890,10 +890,9 @@ compat_sys_rt_sigtimedwait (compat_sigse { compat_sigset_t s32; sigset_t s; - int sig; struct timespec t; siginfo_t info; - long ret, timeout; + long ret; if (sigsetsize != sizeof(sigset_t)) return -EINVAL; @@ -901,45 +900,19 @@ compat_sys_rt_sigtimedwait (compat_sigse if (copy_from_user(&s32, uthese, sizeof(compat_sigset_t))) return -EFAULT; sigset_from_compat(&s, &s32); - sigdelsetmask(&s,sigmask(SIGKILL)|sigmask(SIGSTOP)); - signotset(&s); - timeout = MAX_SCHEDULE_TIMEOUT; if (uts) { - if (get_compat_timespec (&t, uts)) + if (get_compat_timespec(&t, uts)) return -EFAULT; - if (!timespec_valid(&t)) - return -EINVAL; - timeout = timespec_to_jiffies(&t) + (t.tv_sec || t.tv_nsec); } - spin_lock_irq(¤t->sighand->siglock); - sig = dequeue_signal(current, &s, &info); - if (!sig && timeout) { - current->real_blocked = current->blocked; - sigandsets(¤t->blocked, ¤t->blocked, &s); - recalc_sigpending(); - spin_unlock_irq(¤t->sighand->siglock); - - timeout = schedule_timeout_interruptible(timeout); + ret = do_sigtimedwait(&s, &info, uts ? &t : NULL); - spin_lock_irq(¤t->sighand->siglock); - sig = dequeue_signal(current, &s, &info); - current->blocked = current->real_blocked; - siginitset(¤t->real_blocked, 0); - recalc_sigpending(); + if (ret > 0 && uinfo) { + if (copy_siginfo_to_user32(uinfo, &info)) + ret = -EFAULT; } - spin_unlock_irq(¤t->sighand->siglock); - if (sig) { - ret = sig; - if (uinfo) { - if (copy_siginfo_to_user32(uinfo, &info)) - ret = -EFAULT; - } - } else { - ret = timeout?-EINTR:-EAGAIN; - } return ret; } ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code 2011-04-26 19:49 ` [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code Oleg Nesterov @ 2011-04-27 10:09 ` Tejun Heo 2011-04-27 21:24 ` Matt Fleming 2011-05-11 16:21 ` Mike Frysinger 2 siblings, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-27 10:09 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Tue, Apr 26, 2011 at 09:49:04PM +0200, Oleg Nesterov wrote: > Factor out the common code in sys_rt_sigtimedwait/compat_sys_rt_sigtimedwait > to the new helper, do_sigtimedwait(). > > Add the comment to document the extra tick we add to timespec_to_jiffies(ts), > thanks to Linus who explained this to me. > > Perhaps it would be better to move compat_sys_rt_sigtimedwait() into > signal.c under CONFIG_COMPAT, then we can make do_sigtimedwait() static. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> But please do consider adding docbook function comment to the new helper function. Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code 2011-04-26 19:49 ` [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code Oleg Nesterov 2011-04-27 10:09 ` Tejun Heo @ 2011-04-27 21:24 ` Matt Fleming 2011-05-11 16:21 ` Mike Frysinger 2 siblings, 0 replies; 88+ messages in thread From: Matt Fleming @ 2011-04-27 21:24 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Tejun Heo, Nikita V. Youshchenko, linux-kernel On Tue, 26 Apr 2011 21:49:04 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > Factor out the common code in sys_rt_sigtimedwait/compat_sys_rt_sigtimedwait > to the new helper, do_sigtimedwait(). > > Add the comment to document the extra tick we add to timespec_to_jiffies(ts), > thanks to Linus who explained this to me. > > Perhaps it would be better to move compat_sys_rt_sigtimedwait() into > signal.c under CONFIG_COMPAT, then we can make do_sigtimedwait() static. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code 2011-04-26 19:49 ` [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code Oleg Nesterov 2011-04-27 10:09 ` Tejun Heo 2011-04-27 21:24 ` Matt Fleming @ 2011-05-11 16:21 ` Mike Frysinger 2011-05-12 18:54 ` Oleg Nesterov 2011-05-13 16:44 ` [PATCH] signal: trivial, fix the "timespec declared inside parameter list" warning Oleg Nesterov 2 siblings, 2 replies; 88+ messages in thread From: Mike Frysinger @ 2011-05-11 16:21 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Tue, Apr 26, 2011 at 15:49, Oleg Nesterov wrote: > --- sigprocmask/include/linux/signal.h~2_do_sigtimedwait 2011-04-26 19:52:30.000000000 +0200 > +++ sigprocmask/include/linux/signal.h 2011-04-26 19:53:42.000000000 +0200 > @@ -242,6 +242,8 @@ extern int __group_send_sig_info(int, st > extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, > siginfo_t *info); > extern long do_sigpending(void __user *, unsigned long); > +extern int do_sigtimedwait(const sigset_t *, siginfo_t *, > + const struct timespec *); > extern int sigprocmask(int, sigset_t *, sigset_t *); > extern void set_current_blocked(const sigset_t *); > extern int show_unhandled_signals; this causes a build warning: In file included from arch/blackfin/kernel/signal.c:8: include/linux/signal.h:246: warning: 'struct timespec' declared inside parameter list include/linux/signal.h:246: warning: its scope is only this definition or declaration, which is probably not what you want -mike ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code 2011-05-11 16:21 ` Mike Frysinger @ 2011-05-12 18:54 ` Oleg Nesterov 2011-05-13 16:44 ` [PATCH] signal: trivial, fix the "timespec declared inside parameter list" warning Oleg Nesterov 1 sibling, 0 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-05-12 18:54 UTC (permalink / raw) To: Mike Frysinger Cc: Linus Torvalds, Andrew Morton, Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel On 05/11, Mike Frysinger wrote: > > On Tue, Apr 26, 2011 at 15:49, Oleg Nesterov wrote: > > --- sigprocmask/include/linux/signal.h~2_do_sigtimedwait 2011-04-26 19:52:30.000000000 +0200 > > +++ sigprocmask/include/linux/signal.h 2011-04-26 19:53:42.000000000 +0200 > > @@ -242,6 +242,8 @@ extern int __group_send_sig_info(int, st > > extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, > > siginfo_t *info); > > extern long do_sigpending(void __user *, unsigned long); > > +extern int do_sigtimedwait(const sigset_t *, siginfo_t *, > > + const struct timespec *); > > extern int sigprocmask(int, sigset_t *, sigset_t *); > > extern void set_current_blocked(const sigset_t *); > > extern int show_unhandled_signals; > > this causes a build warning: > In file included from arch/blackfin/kernel/signal.c:8: > include/linux/signal.h:246: warning: 'struct timespec' declared inside > parameter list Oh, thanks Mike. I'll send the simple fix tomorrow. Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH] signal: trivial, fix the "timespec declared inside parameter list" warning 2011-05-11 16:21 ` Mike Frysinger 2011-05-12 18:54 ` Oleg Nesterov @ 2011-05-13 16:44 ` Oleg Nesterov 2011-05-13 18:09 ` Mike Frysinger 1 sibling, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-05-13 16:44 UTC (permalink / raw) To: Mike Frysinger Cc: Linus Torvalds, Andrew Morton, Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel Fix the compile warning, do_sigtimedwait(struct timespec *) in signal.h needs the forward declaration of timespec. Reported-by: Mike Frysinger <vapier.adi@gmail.com> Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- include/linux/signal.h | 1 + 1 file changed, 1 insertion(+) --- sigprocmask/include/linux/signal.h~15_stw_warning 2011-05-12 20:44:43.000000000 +0200 +++ sigprocmask/include/linux/signal.h 2011-05-13 18:10:40.000000000 +0200 @@ -242,6 +242,7 @@ extern int __group_send_sig_info(int, st extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, siginfo_t *info); extern long do_sigpending(void __user *, unsigned long); +struct timespec; extern int do_sigtimedwait(const sigset_t *, siginfo_t *, const struct timespec *); extern int sigprocmask(int, sigset_t *, sigset_t *); ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH] signal: trivial, fix the "timespec declared inside parameter list" warning 2011-05-13 16:44 ` [PATCH] signal: trivial, fix the "timespec declared inside parameter list" warning Oleg Nesterov @ 2011-05-13 18:09 ` Mike Frysinger 2011-05-16 12:57 ` Oleg Nesterov 0 siblings, 1 reply; 88+ messages in thread From: Mike Frysinger @ 2011-05-13 18:09 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Fri, May 13, 2011 at 12:44, Oleg Nesterov wrote: > --- sigprocmask/include/linux/signal.h~15_stw_warning 2011-05-12 20:44:43.000000000 +0200 > +++ sigprocmask/include/linux/signal.h 2011-05-13 18:10:40.000000000 +0200 > @@ -242,6 +242,7 @@ extern int __group_send_sig_info(int, st > extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, > siginfo_t *info); > extern long do_sigpending(void __user *, unsigned long); > +struct timespec; > extern int do_sigtimedwait(const sigset_t *, siginfo_t *, > const struct timespec *); > extern int sigprocmask(int, sigset_t *, sigset_t *); seems like you'd want to stick this higher up in the file (like after the includes or at the top of the prototype block) so that in the future, you dont have to move it if someone adds a new func before it. -mike ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH] signal: trivial, fix the "timespec declared inside parameter list" warning 2011-05-13 18:09 ` Mike Frysinger @ 2011-05-16 12:57 ` Oleg Nesterov 2011-05-16 12:57 ` [PATCH v2] " Oleg Nesterov 0 siblings, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-05-16 12:57 UTC (permalink / raw) To: Mike Frysinger Cc: Linus Torvalds, Andrew Morton, Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel On 05/13, Mike Frysinger wrote: > > On Fri, May 13, 2011 at 12:44, Oleg Nesterov wrote: > > --- sigprocmask/include/linux/signal.h~15_stw_warning 2011-05-12 20:44:43.000000000 +0200 > > +++ sigprocmask/include/linux/signal.h 2011-05-13 18:10:40.000000000 +0200 > > @@ -242,6 +242,7 @@ extern int __group_send_sig_info(int, st > > extern long do_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig, > > siginfo_t *info); > > extern long do_sigpending(void __user *, unsigned long); > > +struct timespec; > > extern int do_sigtimedwait(const sigset_t *, siginfo_t *, > > const struct timespec *); > > extern int sigprocmask(int, sigset_t *, sigset_t *); > > seems like you'd want to stick this higher up in the file (like after > the includes or at the top of the prototype block) OK, lets move it at the top of the prototype block. I'd like to keep it close to the user. If we do this, then we should probably move pt_regs as well. Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2] signal: trivial, fix the "timespec declared inside parameter list" warning 2011-05-16 12:57 ` Oleg Nesterov @ 2011-05-16 12:57 ` Oleg Nesterov 2011-05-16 17:39 ` Mike Frysinger 2011-05-18 23:37 ` Andrew Morton 0 siblings, 2 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-05-16 12:57 UTC (permalink / raw) To: Mike Frysinger Cc: Linus Torvalds, Andrew Morton, Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel Fix the compile warning, do_sigtimedwait(struct timespec *) in signal.h needs the forward declaration of timespec. Reported-by: Mike Frysinger <vapier.adi@gmail.com> Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- include/linux/signal.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) --- sigprocmask/include/linux/signal.h~15_stw_warning 2011-05-12 20:44:43.000000000 +0200 +++ sigprocmask/include/linux/signal.h 2011-05-16 14:53:08.000000000 +0200 @@ -234,6 +234,9 @@ static inline int valid_signal(unsigned return sig <= _NSIG ? 1 : 0; } +struct timespec; +struct pt_regs; + extern int next_signal(struct sigpending *pending, sigset_t *mask); extern int do_send_sig_info(int sig, struct siginfo *info, struct task_struct *p, bool group); @@ -248,7 +251,6 @@ extern int sigprocmask(int, sigset_t *, extern void set_current_blocked(const sigset_t *); extern int show_unhandled_signals; -struct pt_regs; extern int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka, struct pt_regs *regs, void *cookie); extern void exit_signals(struct task_struct *tsk); ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2] signal: trivial, fix the "timespec declared inside parameter list" warning 2011-05-16 12:57 ` [PATCH v2] " Oleg Nesterov @ 2011-05-16 17:39 ` Mike Frysinger 2011-05-18 23:37 ` Andrew Morton 1 sibling, 0 replies; 88+ messages in thread From: Mike Frysinger @ 2011-05-16 17:39 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Mon, May 16, 2011 at 08:57, Oleg Nesterov wrote: > Fix the compile warning, do_sigtimedwait(struct timespec *) in signal.h > needs the forward declaration of timespec. looks good to me, thanks ! Acked-by: Mike Frysinger <vapier@gentoo.org> -mike ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2] signal: trivial, fix the "timespec declared inside parameter list" warning 2011-05-16 12:57 ` [PATCH v2] " Oleg Nesterov 2011-05-16 17:39 ` Mike Frysinger @ 2011-05-18 23:37 ` Andrew Morton 2011-05-19 18:19 ` Oleg Nesterov 1 sibling, 1 reply; 88+ messages in thread From: Andrew Morton @ 2011-05-18 23:37 UTC (permalink / raw) To: Oleg Nesterov Cc: Mike Frysinger, Linus Torvalds, Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Mon, 16 May 2011 14:57:29 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > Fix the compile warning, do_sigtimedwait(struct timespec *) in signal.h > needs the forward declaration of timespec. > The offending patch is in your tree, so you may as well put this patch in there too. > --- sigprocmask/include/linux/signal.h~15_stw_warning 2011-05-12 20:44:43.000000000 +0200 > +++ sigprocmask/include/linux/signal.h 2011-05-16 14:53:08.000000000 +0200 > @@ -234,6 +234,9 @@ static inline int valid_signal(unsigned > return sig <= _NSIG ? 1 : 0; > } > > +struct timespec; > +struct pt_regs; > + > extern int next_signal(struct sigpending *pending, sigset_t *mask); > extern int do_send_sig_info(int sig, struct siginfo *info, > struct task_struct *p, bool group); Please put the forward declarations at top-of-file. In this case, inside #ifdef __KERNEL__. This reduces the risk of accumulating duplicated forward declarations, as has happened in the past. ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2] signal: trivial, fix the "timespec declared inside parameter list" warning 2011-05-18 23:37 ` Andrew Morton @ 2011-05-19 18:19 ` Oleg Nesterov 2011-05-19 19:21 ` Mike Frysinger 0 siblings, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-05-19 18:19 UTC (permalink / raw) To: Andrew Morton Cc: Mike Frysinger, Linus Torvalds, Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel On 05/18, Andrew Morton wrote: > > On Mon, 16 May 2011 14:57:29 +0200 > Oleg Nesterov <oleg@redhat.com> wrote: > > > Fix the compile warning, do_sigtimedwait(struct timespec *) in signal.h > > needs the forward declaration of timespec. > > > > The offending patch is in your tree, so you may as well put this patch > in there too. Yes, it is already in my tree, sorry for confusion. > > --- sigprocmask/include/linux/signal.h~15_stw_warning 2011-05-12 20:44:43.000000000 +0200 > > +++ sigprocmask/include/linux/signal.h 2011-05-16 14:53:08.000000000 +0200 > > @@ -234,6 +234,9 @@ static inline int valid_signal(unsigned > > return sig <= _NSIG ? 1 : 0; > > } > > > > +struct timespec; > > +struct pt_regs; > > + > > extern int next_signal(struct sigpending *pending, sigset_t *mask); > > extern int do_send_sig_info(int sig, struct siginfo *info, > > struct task_struct *p, bool group); > > Please put the forward declarations at top-of-file. In this case, > inside #ifdef __KERNEL__. This reduces the risk of accumulating > duplicated forward declarations, as has happened in the past. This is what Mike suggested from the very beginnig. Perhaps this would be better but since I already applied this patch... I'd prefer to not test my git skills, unless you or Mike object. Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2] signal: trivial, fix the "timespec declared inside parameter list" warning 2011-05-19 18:19 ` Oleg Nesterov @ 2011-05-19 19:21 ` Mike Frysinger 0 siblings, 0 replies; 88+ messages in thread From: Mike Frysinger @ 2011-05-19 19:21 UTC (permalink / raw) To: Oleg Nesterov Cc: Andrew Morton, Linus Torvalds, Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Thu, May 19, 2011 at 14:19, Oleg Nesterov wrote: > On 05/18, Andrew Morton wrote: >> On Mon, 16 May 2011 14:57:29 +0200 Oleg Nesterov wrote: >> > --- sigprocmask/include/linux/signal.h~15_stw_warning 2011-05-12 20:44:43.000000000 +0200 >> > +++ sigprocmask/include/linux/signal.h 2011-05-16 14:53:08.000000000 +0200 >> > @@ -234,6 +234,9 @@ static inline int valid_signal(unsigned >> > return sig <= _NSIG ? 1 : 0; >> > } >> > >> > +struct timespec; >> > +struct pt_regs; >> > + >> > extern int next_signal(struct sigpending *pending, sigset_t *mask); >> > extern int do_send_sig_info(int sig, struct siginfo *info, >> > struct task_struct *p, bool group); >> >> Please put the forward declarations at top-of-file. In this case, >> inside #ifdef __KERNEL__. This reduces the risk of accumulating >> duplicated forward declarations, as has happened in the past. > > This is what Mike suggested from the very beginnig. Perhaps this > would be better but since I already applied this patch... I'd prefer > to not test my git skills, unless you or Mike object. i'm ok with this version as it's at the top of the existing prototype block. Andrew's version also would work of course. -mike ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 3/6] signal: do_sigtimedwait() needs retarget_shared_pending() 2011-04-26 19:48 ` [PATCH v2 0/6] sigtimedwait/sigprocmask need retarget_shared_pending() Oleg Nesterov 2011-04-26 19:48 ` [PATCH v2 1/6] signal: sys_rt_sigtimedwait: simplify the timeout logic Oleg Nesterov 2011-04-26 19:49 ` [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code Oleg Nesterov @ 2011-04-26 19:49 ` Oleg Nesterov 2011-04-26 19:49 ` [PATCH v2 4/6] signal: cleanup sys_sigprocmask() Oleg Nesterov ` (2 subsequent siblings) 5 siblings, 0 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-26 19:49 UTC (permalink / raw) To: Linus Torvalds, Andrew Morton Cc: Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel do_sigtimedwait() changes current->blocked and thus it needs set_current_blocked()->retarget_shared_pending(). We could use set_current_blocked() directly. It is fine to change ->real_blocked from all-zeroes to ->blocked and vice versa lockless, but this is not immediately clear, looks racy, and needs a huge comment to explain why this is correct. To keep the things simple this patch adds the new static helper, __set_task_blocked() which should be called with ->siglock held. This way we can change both ->real_blocked and ->blocked atomically under ->siglock as the current code does. This is more understandable. Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> --- kernel/signal.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) --- sigprocmask/kernel/signal.c~3_sigtimedwait_retarget 2011-04-26 19:53:42.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-26 19:53:55.000000000 +0200 @@ -2115,11 +2115,8 @@ long do_no_restart_syscall(struct restar return -EINTR; } -void set_current_blocked(const sigset_t *newset) +static void __set_task_blocked(struct task_struct *tsk, const sigset_t *newset) { - struct task_struct *tsk = current; - - spin_lock_irq(&tsk->sighand->siglock); if (signal_pending(tsk) && !thread_group_empty(tsk)) { sigset_t newblocked; signandsets(&newblocked, newset, ¤t->blocked); @@ -2127,6 +2124,14 @@ void set_current_blocked(const sigset_t } tsk->blocked = *newset; recalc_sigpending(); +} + +void set_current_blocked(const sigset_t *newset) +{ + struct task_struct *tsk = current; + + spin_lock_irq(&tsk->sighand->siglock); + __set_task_blocked(tsk, newset); spin_unlock_irq(&tsk->sighand->siglock); } @@ -2343,7 +2348,8 @@ int do_sigtimedwait(const sigset_t *whic /* * None ready, temporarily unblock those we're interested * while we are sleeping in so that we'll be awakened when - * they arrive. + * they arrive. Unblocking is always fine, we can avoid + * set_current_blocked(). */ tsk->real_blocked = tsk->blocked; sigandsets(&tsk->blocked, &tsk->blocked, &mask); @@ -2353,10 +2359,9 @@ int do_sigtimedwait(const sigset_t *whic timeout = schedule_timeout_interruptible(timeout); spin_lock_irq(&tsk->sighand->siglock); - sig = dequeue_signal(tsk, &mask, info); - tsk->blocked = tsk->real_blocked; + __set_task_blocked(tsk, &tsk->real_blocked); siginitset(&tsk->real_blocked, 0); - recalc_sigpending(); + sig = dequeue_signal(tsk, &mask, info); } spin_unlock_irq(&tsk->sighand->siglock); ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 4/6] signal: cleanup sys_sigprocmask() 2011-04-26 19:48 ` [PATCH v2 0/6] sigtimedwait/sigprocmask need retarget_shared_pending() Oleg Nesterov ` (2 preceding siblings ...) 2011-04-26 19:49 ` [PATCH v2 3/6] signal: do_sigtimedwait() needs retarget_shared_pending() Oleg Nesterov @ 2011-04-26 19:49 ` Oleg Nesterov 2011-04-27 10:12 ` Tejun Heo 2011-04-27 21:31 ` Matt Fleming 2011-04-26 19:50 ` [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending() Oleg Nesterov 2011-04-26 19:50 ` [PATCH v2 6/6] signal: rename signandsets() to sigandnsets() Oleg Nesterov 5 siblings, 2 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-26 19:49 UTC (permalink / raw) To: Linus Torvalds, Andrew Morton Cc: Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel Cleanup. Remove the unneeded goto's, we can simply read blocked.sig[0] unconditionally and then copy-to-user it if oset != NULL. Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- kernel/signal.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) --- sigprocmask/kernel/signal.c~4_cleanup_old_sigprocmask 2011-04-26 19:53:55.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-26 19:54:09.000000000 +0200 @@ -2691,29 +2691,28 @@ SYSCALL_DEFINE1(sigpending, old_sigset_t /** * sys_sigprocmask - examine and change blocked signals * @how: whether to add, remove, or set signals - * @set: signals to add or remove (if non-null) + * @nset: signals to add or remove (if non-null) * @oset: previous value of signal mask if non-null * * Some platforms have their own version with special arguments; * others support only sys_rt_sigprocmask. */ -SYSCALL_DEFINE3(sigprocmask, int, how, old_sigset_t __user *, set, +SYSCALL_DEFINE3(sigprocmask, int, how, old_sigset_t __user *, nset, old_sigset_t __user *, oset) { - int error; old_sigset_t old_set, new_set; + int error; - if (set) { - error = -EFAULT; - if (copy_from_user(&new_set, set, sizeof(*set))) - goto out; - new_set &= ~(sigmask(SIGKILL) | sigmask(SIGSTOP)); + old_set = current->blocked.sig[0]; - spin_lock_irq(¤t->sighand->siglock); - old_set = current->blocked.sig[0]; + if (nset) { + if (copy_from_user(&new_set, nset, sizeof(*nset))) + return -EFAULT; + new_set &= ~(sigmask(SIGKILL) | sigmask(SIGSTOP)); error = 0; + spin_lock_irq(¤t->sighand->siglock); switch (how) { default: error = -EINVAL; @@ -2732,19 +2731,15 @@ SYSCALL_DEFINE3(sigprocmask, int, how, o recalc_sigpending(); spin_unlock_irq(¤t->sighand->siglock); if (error) - goto out; - if (oset) - goto set_old; - } else if (oset) { - old_set = current->blocked.sig[0]; - set_old: - error = -EFAULT; + return error; + } + + if (oset) { if (copy_to_user(oset, &old_set, sizeof(*oset))) - goto out; + return -EFAULT; } - error = 0; -out: - return error; + + return 0; } #endif /* __ARCH_WANT_SYS_SIGPROCMASK */ ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 4/6] signal: cleanup sys_sigprocmask() 2011-04-26 19:49 ` [PATCH v2 4/6] signal: cleanup sys_sigprocmask() Oleg Nesterov @ 2011-04-27 10:12 ` Tejun Heo 2011-04-27 21:31 ` Matt Fleming 1 sibling, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-27 10:12 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Tue, Apr 26, 2011 at 09:49:43PM +0200, Oleg Nesterov wrote: > Cleanup. Remove the unneeded goto's, we can simply read blocked.sig[0] > unconditionally and then copy-to-user it if oset != NULL. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 4/6] signal: cleanup sys_sigprocmask() 2011-04-26 19:49 ` [PATCH v2 4/6] signal: cleanup sys_sigprocmask() Oleg Nesterov 2011-04-27 10:12 ` Tejun Heo @ 2011-04-27 21:31 ` Matt Fleming 1 sibling, 0 replies; 88+ messages in thread From: Matt Fleming @ 2011-04-27 21:31 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Tejun Heo, Nikita V. Youshchenko, linux-kernel On Tue, 26 Apr 2011 21:49:43 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > Cleanup. Remove the unneeded goto's, we can simply read blocked.sig[0] > unconditionally and then copy-to-user it if oset != NULL. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending() 2011-04-26 19:48 ` [PATCH v2 0/6] sigtimedwait/sigprocmask need retarget_shared_pending() Oleg Nesterov ` (3 preceding siblings ...) 2011-04-26 19:49 ` [PATCH v2 4/6] signal: cleanup sys_sigprocmask() Oleg Nesterov @ 2011-04-26 19:50 ` Oleg Nesterov 2011-04-26 21:43 ` Linus Torvalds 2011-04-26 19:50 ` [PATCH v2 6/6] signal: rename signandsets() to sigandnsets() Oleg Nesterov 5 siblings, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-04-26 19:50 UTC (permalink / raw) To: Linus Torvalds, Andrew Morton Cc: Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel sys_sigprocmask() changes current->blocked by hand. Convert this code to use sigprocmask() which implies the necessary retarget_shared_pending(). Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- kernel/signal.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) --- sigprocmask/kernel/signal.c~5_old_sigprocmask_retarget 2011-04-26 19:54:09.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-26 19:54:20.000000000 +0200 @@ -2702,6 +2702,7 @@ SYSCALL_DEFINE3(sigprocmask, int, how, o old_sigset_t __user *, oset) { old_sigset_t old_set, new_set; + sigset_t new_full_set; int error; old_set = current->blocked.sig[0]; @@ -2711,25 +2712,12 @@ SYSCALL_DEFINE3(sigprocmask, int, how, o return -EFAULT; new_set &= ~(sigmask(SIGKILL) | sigmask(SIGSTOP)); - error = 0; - spin_lock_irq(¤t->sighand->siglock); - switch (how) { - default: - error = -EINVAL; - break; - case SIG_BLOCK: - sigaddsetmask(¤t->blocked, new_set); - break; - case SIG_UNBLOCK: - sigdelsetmask(¤t->blocked, new_set); - break; - case SIG_SETMASK: - current->blocked.sig[0] = new_set; - break; - } + sigemptyset(&new_full_set); + if (how == SIG_SETMASK) + new_full_set = current->blocked; + new_full_set.sig[0] = new_set; - recalc_sigpending(); - spin_unlock_irq(¤t->sighand->siglock); + error = sigprocmask(how, &new_full_set, NULL); if (error) return error; } ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending() 2011-04-26 19:50 ` [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending() Oleg Nesterov @ 2011-04-26 21:43 ` Linus Torvalds 2011-04-27 12:57 ` Oleg Nesterov 2011-05-01 20:07 ` [PATCH v2 0/1] " Oleg Nesterov 0 siblings, 2 replies; 88+ messages in thread From: Linus Torvalds @ 2011-04-26 21:43 UTC (permalink / raw) To: Oleg Nesterov Cc: Andrew Morton, Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel > + sigemptyset(&new_full_set); > + if (how == SIG_SETMASK) > + new_full_set = current->blocked; > + new_full_set.sig[0] = new_set; Ugh. This is just ugly. Could we not instead turn the whole thing into a "clear these bits" and "set these bits", and get rid of the "how" entirely for the helper function? IOW, we'd have switch (how) { case SIG_BLOCK: clear_bits = 0; set_bits = new_set; break; case SIG_UNBLOCK: clear_bits = new_set; set_bits = 0; break case SIG_SET: clear_bits = low_bits; set_bits = new_set; break; default: return -EINVAL; } and notice how you now can do that helper function *WITHOUT* any conditionals, and just make it do sigprocmask(&clear, &set, NULL); which handles all cases correctly (just "andn clear" + "or set") with no if's or switch'es. This is why I _detest_ that idiotic "sigprocmask()" interface. That "how" parameter is the invention of somebody who didn't understand sets. It's stupid. There is no reason to have multiple different set operations, when in practice all anybody ever wants is the "clear these bits and set those other bits" - an operation that is not only more generic than the idiotic "how", but is _faster_ too, because it involves no conditionals. So I realize that we cannot get away from the broken user interface, but I do not believe that that means that our _internal_ helper functions should use that idiotic and broken interface! I had basically this same comment earlier when you did something similarly mindless for another case. So basic rule should be: if you ever pass "how" to any helper functions, it's broken. Linus ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending() 2011-04-26 21:43 ` Linus Torvalds @ 2011-04-27 12:57 ` Oleg Nesterov 2011-04-27 13:04 ` Tejun Heo 2011-05-01 20:07 ` [PATCH v2 0/1] " Oleg Nesterov 1 sibling, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-04-27 12:57 UTC (permalink / raw) To: Linus Torvalds Cc: Andrew Morton, Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel On 04/26, Linus Torvalds wrote: > > > + sigemptyset(&new_full_set); > > + if (how == SIG_SETMASK) > > + new_full_set = current->blocked; > > + new_full_set.sig[0] = new_set; > > Ugh. This is just ugly. Agreed. > Could we not instead turn the whole thing into a "clear these bits" > and "set these bits", and get rid of the "how" entirely for the helper > function? > > IOW, we'd have > > switch (how) { > case SIG_BLOCK: > clear_bits = 0; > set_bits = new_set; > break; > case SIG_UNBLOCK: > clear_bits = new_set; > set_bits = 0; > break > case SIG_SET: > clear_bits = low_bits; > set_bits = new_set; > break; > default: > return -EINVAL; > } > > and notice how you now can do that helper function *WITHOUT* any > conditionals, and just make it do > > sigprocmask(&clear, &set, NULL); > > which handles all cases correctly (just "andn clear" + "or set") with > no if's or switch'es. > > This is why I _detest_ that idiotic "sigprocmask()" interface. Agreed, but... Yes, sigprocmask(how) is ugly, but there are sys_rt_sigprocmask() and sys_sigprocmask() which have to handle these SIG_* operations anyway. So, I think we should do: 1. Almost all callers of sigprocmask() use SIG_SETMASK, we can simply change them to use set_current_blocked(). 2. Add the new helper (probably like you suggested) and convert other 9 callers. 3. Unexport sigprocmask() and remove the last "*oldset" argument, it should be only used by sys_*sigprocmask() syscalls. But firstly I'd like to finish this "don't change ->blocked directly" conversion. And this patch changes sys_sigprocmask() so that it looks similar to sys_rt_sigprocmask(). What do you think? If you can't accept sys_sigprocmask()->sigprocmask(), will you agree with SYSCALL_DEFINE3(sigprocmask, int, how, old_sigset_t __user *, nset, old_sigset_t __user *, oset) { old_sigset_t old_set, new_set; sigset_t new_blocked; old_set = current->blocked.sig[0]; if (nset) { if (copy_from_user(&new_set, nset, sizeof(*nset))) return -EFAULT; new_set &= ~(sigmask(SIGKILL) | sigmask(SIGSTOP)); new_blocked = current->blocked; switch (how) { case SIG_BLOCK: sigaddsetmask(&new_blocked, new_set); break; case SIG_UNBLOCK: sigdelsetmask(&new_blocked, new_set); break; case SIG_SETMASK: new_blocked.sig[0] = new_set; break; default: return -EINVAL; } set_current_blocked(&new_blocked); } if (oset) { if (copy_to_user(oset, &old_set, sizeof(*oset))) return -EFAULT; } return 0; } ? Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending() 2011-04-27 12:57 ` Oleg Nesterov @ 2011-04-27 13:04 ` Tejun Heo 0 siblings, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-27 13:04 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel Hello, Just my 5 cents. On Wed, Apr 27, 2011 at 02:57:10PM +0200, Oleg Nesterov wrote: > Yes, sigprocmask(how) is ugly, but there are sys_rt_sigprocmask() and > sys_sigprocmask() which have to handle these SIG_* operations anyway. > So, I think we should do: > > 1. Almost all callers of sigprocmask() use SIG_SETMASK, we can > simply change them to use set_current_blocked(). I agree. We don't need to worry about atomicity here, so there's no reason to encode bitops (be it and/or or andn/xor) when the determination of the new value can be simply done in the caller. Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 0/1] signal: sys_sigprocmask() needs retarget_shared_pending() 2011-04-26 21:43 ` Linus Torvalds 2011-04-27 12:57 ` Oleg Nesterov @ 2011-05-01 20:07 ` Oleg Nesterov 2011-05-01 20:08 ` [PATCH v2 1/1] " Oleg Nesterov 1 sibling, 1 reply; 88+ messages in thread From: Oleg Nesterov @ 2011-05-01 20:07 UTC (permalink / raw) To: Linus Torvalds Cc: Andrew Morton, Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel On 04/26, Linus Torvalds wrote: > > > + sigemptyset(&new_full_set); > > + if (how == SIG_SETMASK) > > + new_full_set = current->blocked; > > + new_full_set.sig[0] = new_set; > > Ugh. This is just ugly. OK, please find v2. It doesn't use sigprocmask() helper(). Hopefully this is the last "nontrivial" conversion, everything else looks simple. Oleg. ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 1/1] signal: sys_sigprocmask() needs retarget_shared_pending() 2011-05-01 20:07 ` [PATCH v2 0/1] " Oleg Nesterov @ 2011-05-01 20:08 ` Oleg Nesterov 0 siblings, 0 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-05-01 20:08 UTC (permalink / raw) To: Linus Torvalds Cc: Andrew Morton, Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel sys_sigprocmask() changes current->blocked by hand. Convert this code to use set_current_blocked(). Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- kernel/signal.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) --- sigprocmask/kernel/signal.c~14_old_sigprocmask_retarget 2011-04-30 19:07:11.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-05-01 21:34:51.000000000 +0200 @@ -2900,7 +2900,7 @@ SYSCALL_DEFINE3(sigprocmask, int, how, o old_sigset_t __user *, oset) { old_sigset_t old_set, new_set; - int error; + sigset_t new_blocked; old_set = current->blocked.sig[0]; @@ -2909,27 +2909,23 @@ SYSCALL_DEFINE3(sigprocmask, int, how, o return -EFAULT; new_set &= ~(sigmask(SIGKILL) | sigmask(SIGSTOP)); - error = 0; - spin_lock_irq(¤t->sighand->siglock); + new_blocked = current->blocked; + switch (how) { - default: - error = -EINVAL; - break; case SIG_BLOCK: - sigaddsetmask(¤t->blocked, new_set); + sigaddsetmask(&new_blocked, new_set); break; case SIG_UNBLOCK: - sigdelsetmask(¤t->blocked, new_set); + sigdelsetmask(&new_blocked, new_set); break; case SIG_SETMASK: - current->blocked.sig[0] = new_set; + new_blocked.sig[0] = new_set; break; + default: + return -EINVAL; } - recalc_sigpending(); - spin_unlock_irq(¤t->sighand->siglock); - if (error) - return error; + set_current_blocked(&new_blocked); } if (oset) { ^ permalink raw reply [flat|nested] 88+ messages in thread
* [PATCH v2 6/6] signal: rename signandsets() to sigandnsets() 2011-04-26 19:48 ` [PATCH v2 0/6] sigtimedwait/sigprocmask need retarget_shared_pending() Oleg Nesterov ` (4 preceding siblings ...) 2011-04-26 19:50 ` [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending() Oleg Nesterov @ 2011-04-26 19:50 ` Oleg Nesterov 2011-04-27 10:11 ` Tejun Heo 2011-04-27 21:43 ` Matt Fleming 5 siblings, 2 replies; 88+ messages in thread From: Oleg Nesterov @ 2011-04-26 19:50 UTC (permalink / raw) To: Linus Torvalds, Andrew Morton Cc: Tejun Heo, Nikita V. Youshchenko, Matt Fleming, linux-kernel As Tejun and Linus pointed out, "nand" is the wrong name for "x & ~y", it should be "andn". Rename signandsets() as suggested. Suggested-by: Tejun Heo <tj@kernel.org> Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- include/linux/signal.h | 6 +++--- kernel/signal.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) --- sigprocmask/include/linux/signal.h~6_rename_nand 2011-04-26 19:53:42.000000000 +0200 +++ sigprocmask/include/linux/signal.h 2011-04-26 19:58:01.000000000 +0200 @@ -123,13 +123,13 @@ _SIG_SET_BINOP(sigorsets, _sig_or) #define _sig_and(x,y) ((x) & (y)) _SIG_SET_BINOP(sigandsets, _sig_and) -#define _sig_nand(x,y) ((x) & ~(y)) -_SIG_SET_BINOP(signandsets, _sig_nand) +#define _sig_andn(x,y) ((x) & ~(y)) +_SIG_SET_BINOP(sigandnsets, _sig_andn) #undef _SIG_SET_BINOP #undef _sig_or #undef _sig_and -#undef _sig_nand +#undef _sig_andn #define _SIG_SET_OP(name, op) \ static inline void name(sigset_t *set) \ --- sigprocmask/kernel/signal.c~6_rename_nand 2011-04-26 19:54:20.000000000 +0200 +++ sigprocmask/kernel/signal.c 2011-04-26 19:59:43.000000000 +0200 @@ -592,7 +592,7 @@ static int rm_from_queue_full(sigset_t * if (sigisemptyset(&m)) return 0; - signandsets(&s->signal, &s->signal, mask); + sigandnsets(&s->signal, &s->signal, mask); list_for_each_entry_safe(q, n, &s->list, list) { if (sigismember(mask, q->info.si_signo)) { list_del_init(&q->list); @@ -2119,7 +2119,7 @@ static void __set_task_blocked(struct ta { if (signal_pending(tsk) && !thread_group_empty(tsk)) { sigset_t newblocked; - signandsets(&newblocked, newset, ¤t->blocked); + sigandnsets(&newblocked, newset, ¤t->blocked); retarget_shared_pending(tsk, &newblocked); } tsk->blocked = *newset; @@ -2157,7 +2157,7 @@ int sigprocmask(int how, sigset_t *set, sigorsets(&newset, &tsk->blocked, set); break; case SIG_UNBLOCK: - signandsets(&newset, &tsk->blocked, set); + sigandnsets(&newset, &tsk->blocked, set); break; case SIG_SETMASK: newset = *set; ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 6/6] signal: rename signandsets() to sigandnsets() 2011-04-26 19:50 ` [PATCH v2 6/6] signal: rename signandsets() to sigandnsets() Oleg Nesterov @ 2011-04-27 10:11 ` Tejun Heo 2011-04-27 21:43 ` Matt Fleming 1 sibling, 0 replies; 88+ messages in thread From: Tejun Heo @ 2011-04-27 10:11 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Nikita V. Youshchenko, Matt Fleming, linux-kernel On Tue, Apr 26, 2011 at 09:50:18PM +0200, Oleg Nesterov wrote: > As Tejun and Linus pointed out, "nand" is the wrong name for "x & ~y", > it should be "andn". Rename signandsets() as suggested. > > Suggested-by: Tejun Heo <tj@kernel.org> > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Acked-by: Tejun Heo <tj@kernel.org> Thanks. -- tejun ^ permalink raw reply [flat|nested] 88+ messages in thread
* Re: [PATCH v2 6/6] signal: rename signandsets() to sigandnsets() 2011-04-26 19:50 ` [PATCH v2 6/6] signal: rename signandsets() to sigandnsets() Oleg Nesterov 2011-04-27 10:11 ` Tejun Heo @ 2011-04-27 21:43 ` Matt Fleming 1 sibling, 0 replies; 88+ messages in thread From: Matt Fleming @ 2011-04-27 21:43 UTC (permalink / raw) To: Oleg Nesterov Cc: Linus Torvalds, Andrew Morton, Tejun Heo, Nikita V. Youshchenko, linux-kernel On Tue, 26 Apr 2011 21:50:18 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > As Tejun and Linus pointed out, "nand" is the wrong name for "x & ~y", > it should be "andn". Rename signandsets() as suggested. > > Suggested-by: Tejun Heo <tj@kernel.org> > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Reviewed-by: Matt Fleming <matt.fleming@linux.intel.com> -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 88+ messages in thread
end of thread, other threads:[~2011-05-19 19:21 UTC | newest] Thread overview: 88+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-04-18 13:44 [RFC PATCH v2 0/7] signal: sigprocmask fixes Oleg Nesterov 2011-04-18 13:44 ` [PATCH v2 1/7] signal: introduce retarget_shared_pending() Oleg Nesterov 2011-04-22 12:04 ` Matt Fleming 2011-04-25 10:49 ` Tejun Heo 2011-04-18 13:45 ` [PATCH v2 2/7] signal: retarget_shared_pending: consider shared/unblocked signals only Oleg Nesterov 2011-04-22 12:22 ` Matt Fleming 2011-04-25 10:52 ` Tejun Heo 2011-04-25 15:20 ` Oleg Nesterov 2011-04-25 16:19 ` Tejun Heo 2011-04-25 17:02 ` Oleg Nesterov 2011-04-25 17:11 ` Tejun Heo 2011-04-26 19:45 ` Oleg Nesterov 2011-04-28 15:26 ` [PATCHSET] signals-review branch Oleg Nesterov 2011-04-30 12:51 ` Tejun Heo 2011-04-18 13:45 ` [PATCH v2 3/7] signal: retarget_shared_pending: optimize while_each_thread() loop Oleg Nesterov 2011-04-22 12:26 ` Matt Fleming 2011-04-25 11:03 ` Tejun Heo 2011-04-18 13:45 ` [PATCH v2 4/7] signal: sigprocmask: narrow the scope of ->siglock Oleg Nesterov 2011-04-22 12:31 ` Matt Fleming 2011-04-25 11:05 ` Tejun Heo 2011-04-18 13:45 ` [PATCH v2 5/7] signal: sigprocmask() should do retarget_shared_pending() Oleg Nesterov 2011-04-22 12:46 ` Matt Fleming 2011-04-25 11:14 ` Tejun Heo 2011-04-18 13:46 ` [PATCH v2 6/7] x86: signal: handle_signal() should use set_current_blocked() Oleg Nesterov 2011-04-22 13:45 ` Matt Fleming 2011-04-25 11:19 ` Tejun Heo 2011-04-18 13:46 ` [PATCH v2 7/7] x86: signal: sys_rt_sigreturn() " Oleg Nesterov 2011-04-22 14:14 ` Matt Fleming 2011-04-23 18:12 ` Oleg Nesterov 2011-04-25 11:21 ` Tejun Heo 2011-04-18 13:47 ` [PATCH v2 8/7] signal: cleanup sys_rt_sigprocmask() Oleg Nesterov 2011-04-22 14:30 ` Matt Fleming 2011-04-23 18:20 ` Oleg Nesterov 2011-04-23 18:47 ` Matt Fleming 2011-04-25 11:26 ` Tejun Heo 2011-04-18 17:16 ` [RFC PATCH v2 0/7] signal: sigprocmask fixes Linus Torvalds 2011-04-18 17:32 ` Oleg Nesterov 2011-04-18 17:40 ` Linus Torvalds 2011-04-23 17:59 ` [PATCH 0/3] do_sigtimedwait() needs retarget_shared_pending() Oleg Nesterov 2011-04-23 17:59 ` [PATCH 1/3] signal: sys_rt_sigtimedwait: simplify the timeout logic Oleg Nesterov 2011-04-25 11:37 ` Tejun Heo 2011-04-25 17:26 ` Oleg Nesterov 2011-04-25 17:34 ` Linus Torvalds 2011-04-25 17:56 ` Oleg Nesterov 2011-04-25 19:38 ` Linus Torvalds 2011-04-26 10:18 ` Matt Fleming 2011-04-23 17:59 ` [PATCH 2/3] signal: introduce do_sigtimedwait() to factor out compat/native code Oleg Nesterov 2011-04-25 11:39 ` Tejun Heo 2011-04-25 11:49 ` Tejun Heo 2011-04-25 15:33 ` Oleg Nesterov 2011-04-25 16:25 ` Tejun Heo 2011-04-26 10:28 ` Matt Fleming 2011-04-23 18:00 ` [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending() Oleg Nesterov 2011-04-25 11:52 ` Tejun Heo 2011-04-25 16:01 ` Oleg Nesterov 2011-04-25 16:27 ` Tejun Heo 2011-04-25 17:07 ` Oleg Nesterov 2011-04-25 17:12 ` Tejun Heo 2011-04-26 10:40 ` Matt Fleming 2011-04-26 10:42 ` Matt Fleming 2011-04-26 19:48 ` [PATCH v2 0/6] sigtimedwait/sigprocmask need retarget_shared_pending() Oleg Nesterov 2011-04-26 19:48 ` [PATCH v2 1/6] signal: sys_rt_sigtimedwait: simplify the timeout logic Oleg Nesterov 2011-04-26 19:49 ` [PATCH v2 2/6] signal: introduce do_sigtimedwait() to factor out compat/native code Oleg Nesterov 2011-04-27 10:09 ` Tejun Heo 2011-04-27 21:24 ` Matt Fleming 2011-05-11 16:21 ` Mike Frysinger 2011-05-12 18:54 ` Oleg Nesterov 2011-05-13 16:44 ` [PATCH] signal: trivial, fix the "timespec declared inside parameter list" warning Oleg Nesterov 2011-05-13 18:09 ` Mike Frysinger 2011-05-16 12:57 ` Oleg Nesterov 2011-05-16 12:57 ` [PATCH v2] " Oleg Nesterov 2011-05-16 17:39 ` Mike Frysinger 2011-05-18 23:37 ` Andrew Morton 2011-05-19 18:19 ` Oleg Nesterov 2011-05-19 19:21 ` Mike Frysinger 2011-04-26 19:49 ` [PATCH v2 3/6] signal: do_sigtimedwait() needs retarget_shared_pending() Oleg Nesterov 2011-04-26 19:49 ` [PATCH v2 4/6] signal: cleanup sys_sigprocmask() Oleg Nesterov 2011-04-27 10:12 ` Tejun Heo 2011-04-27 21:31 ` Matt Fleming 2011-04-26 19:50 ` [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending() Oleg Nesterov 2011-04-26 21:43 ` Linus Torvalds 2011-04-27 12:57 ` Oleg Nesterov 2011-04-27 13:04 ` Tejun Heo 2011-05-01 20:07 ` [PATCH v2 0/1] " Oleg Nesterov 2011-05-01 20:08 ` [PATCH v2 1/1] " Oleg Nesterov 2011-04-26 19:50 ` [PATCH v2 6/6] signal: rename signandsets() to sigandnsets() Oleg Nesterov 2011-04-27 10:11 ` Tejun Heo 2011-04-27 21:43 ` Matt Fleming
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.