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.
No functional changes. Move the notify-other-threads code from exit_signals()
to the new helper, retarget_shared_pending().
Signed-off-by: Oleg Nesterov <[email protected]>
---
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) {
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 <[email protected]>
---
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) {
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 <[email protected]>
---
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;
}
}
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 <[email protected]>
---
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;
}
/**
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 <[email protected]>
---
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;
}
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 <[email protected]>
---
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));
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 <[email protected]>
---
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;
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 <[email protected]>
---
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)
On Mon, Apr 18, 2011 at 6:44 AM, Oleg Nesterov <[email protected]> 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
On 04/18, Linus Torvalds wrote:
>
> On Mon, Apr 18, 2011 at 6:44 AM, Oleg Nesterov <[email protected]> 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.
On Mon, Apr 18, 2011 at 10:32 AM, Oleg Nesterov <[email protected]> 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
On Mon, 18 Apr 2011 15:44:43 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
On Mon, 18 Apr 2011 15:45:01 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
On Mon, 18 Apr 2011 15:45:18 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
Nice.
Reviewed-by: Matt Fleming <[email protected]>
On Mon, 18 Apr 2011 15:45:38 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
Thanks for that updated comment about not being able to change
->blocked from irq context!
Reviewed-by: Matt Fleming <[email protected]>
On Mon, 18 Apr 2011 15:45:57 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
This looks much simpler to me.
Reviewed-by: Matt Fleming <[email protected]>
On Mon, 18 Apr 2011 15:46:15 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
On Mon, 18 Apr 2011 15:46:41 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
But does sys_sigreturn() also need this change?
On Mon, 18 Apr 2011 15:47:00 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
> ---
>
> 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 <[email protected]>
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.
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 <[email protected]>
---
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;
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 <[email protected]>
---
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;
}
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 <[email protected]>
---
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);
On 04/22, Matt Fleming wrote:
>
> On Mon, 18 Apr 2011 15:46:41 +0200
> Oleg Nesterov <[email protected]> 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 <[email protected]>
>
> Reviewed-by: Matt Fleming <[email protected]>
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.
On 04/22, Matt Fleming wrote:
>
> On Mon, 18 Apr 2011 15:47:00 +0200
> Oleg Nesterov <[email protected]> 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 <[email protected]>
Thanks!
Oleg.
On Sat, 23 Apr 2011 20:20:31 +0200
Oleg Nesterov <[email protected]> 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
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
--
tejun
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 <[email protected]>
Thanks.
--
tejun
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 <[email protected]>
--
tejun
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
Hey, again.
> Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Tejun Heo <[email protected]>
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
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
> 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
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
It might be a good idea to note the weird jiffies calculation with a
comment?
Thanks.
--
tejun
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
--
tejun
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
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
> @@ -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
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 <[email protected]>
Thanks!
Oleg.
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.
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.
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
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
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
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.
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.
Hey,
On Mon, Apr 25, 2011 at 7:02 PM, Oleg Nesterov <[email protected]> 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
On Mon, Apr 25, 2011 at 7:07 PM, Oleg Nesterov <[email protected]> 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
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.
On Mon, Apr 25, 2011 at 10:26 AM, Oleg Nesterov <[email protected]> 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
On 04/25, Linus Torvalds wrote:
>
> On Mon, Apr 25, 2011 at 10:26 AM, Oleg Nesterov <[email protected]> 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.
On Mon, Apr 25, 2011 at 10:56 AM, Oleg Nesterov <[email protected]> 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
On Sat, 23 Apr 2011 19:59:22 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
--
Matt Fleming, Intel Open Source Technology Center
On Sat, 23 Apr 2011 19:59:40 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
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
On Mon, 25 Apr 2011 19:07:38 +0200
Oleg Nesterov <[email protected]> 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
On Sat, 23 Apr 2011 20:00:00 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
--
Matt Fleming, Intel Open Source Technology Center
Hi,
On 04/25, Tejun Heo wrote:
> Hey,
>
> On Mon, Apr 25, 2011 at 7:02 PM, Oleg Nesterov <[email protected]> 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.
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.
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
---
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;
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 <[email protected]>
---
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;
}
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 <[email protected]>
---
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 */
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
---
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);
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 <[email protected]>
---
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;
}
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 <[email protected]>
Signed-off-by: Oleg Nesterov <[email protected]>
---
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;
> + 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
> 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.
Gripe: Arrgh, apostrophe disease! Linus, youre picking up nasty habits.
Suggestion: Actually, the usual, more flexible implementation, is
"andn mask" + "xor set". That gives all 4 bit operations (nop, set,
clear, and toggle) unique bit combinations.
Not that it's of any use in many cases, but it has better hack value.
That would convert to:
switch (how) {
case SIG_BLOCK:
mask_bits = new_set;
set_bits = new_set;
break;
case SIG_UNBLOCK:
mask_bits = new_set;
set_bits = 0;
break
case SIG_SET:
mask_bits = low_bits | new_set;
set_bits = new_set;
break;
default:
return -EINVAL;
}
If you prefer separate set & clear fields, with both set meaning "toggle",
which admittedly is a more elegant representation, then it's
"andn (set|clear)" + "xor set".
On Tue, Apr 26, 2011 at 6:35 PM, George Spelvin <[email protected]> wrote:
>>
>> which handles all cases correctly (just "andn clear" + "or set") with
>> no if's or switch'es.
>
> Gripe: Arrgh, apostrophe disease! ?Linus, youre picking up nasty habits.
I didn't want to make C keywords into English words. So if it's about
English words it's "no ifs or buts", but when talking about C keywords
it's "if's or switch'es".
That's my stand, and I'll stick to it. At least until next time, when
I might decide on yet another arbitrary rule ;)
> Suggestion: Actually, the usual, more flexible implementation, is
> "andn mask" + "xor set". ?That gives all 4 bit operations (nop, set,
> clear, and toggle) unique bit combinations.
Yeah, we can do that too. I agree it is more flexible, although I
don't think we'd ever need it.
And I'm not sure it's more "usual" - most drivers I've seen (where
this kind of "clear/set mask" issue comes up pretty often) tend to use
the simpler clear/set model, because it tends to make the usage more
sensible. Most users simply don't tend to need the toggle operation.
But my grep-fu to actually show examples is weak. One example I found would be
- drivers/media/video/tw9910.c: tw9910_mask_set()
which uses that "andnot + or" model I suggested.
But I'm sure there are "andnot + xor" uses out there too.
And I don't actually mind either way. I'd certainly be perfectly ok
with the andn/xor model. It's just the "how" model I detest.
Linus
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
But please do consider adding docbook function comment to the new
helper function.
Thanks.
--
tejun
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 <[email protected]>
> Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
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.
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
On Tue, 26 Apr 2011 21:49:04 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
--
Matt Fleming, Intel Open Source Technology Center
On Tue, 26 Apr 2011 21:49:43 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
--
Matt Fleming, Intel Open Source Technology Center
On Tue, 26 Apr 2011 21:50:18 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
> Signed-off-by: Oleg Nesterov <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
--
Matt Fleming, Intel Open Source Technology Center
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.
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
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.
sys_sigprocmask() changes current->blocked by hand. Convert this code
to use set_current_blocked().
Signed-off-by: Oleg Nesterov <[email protected]>
---
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) {
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
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.
Fix the compile warning, do_sigtimedwait(struct timespec *) in signal.h
needs the forward declaration of timespec.
Reported-by: Mike Frysinger <[email protected]>
Signed-off-by: Oleg Nesterov <[email protected]>
---
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 *);
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
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.
Fix the compile warning, do_sigtimedwait(struct timespec *) in signal.h
needs the forward declaration of timespec.
Reported-by: Mike Frysinger <[email protected]>
Signed-off-by: Oleg Nesterov <[email protected]>
---
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);
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 <[email protected]>
-mike
On Mon, 16 May 2011 14:57:29 +0200
Oleg Nesterov <[email protected]> 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.
On 05/18, Andrew Morton wrote:
>
> On Mon, 16 May 2011 14:57:29 +0200
> Oleg Nesterov <[email protected]> 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.
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