Messages in this thread | | | From | Linus Torvalds <> | Date | Tue, 12 Apr 2011 07:32:20 -0700 | Subject | Re: [PATCH 4/6] signal: sigprocmask() should do retarget_shared_pending() |
| |
On Mon, Apr 11, 2011 at 10:21 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > 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.
Hmm. I worry about the overhead of this, and I'm not 100% convinced we need it.
But my _biggest_ objection to the series is a purely technical one:
> --- sigprocmask/include/linux/signal.h~4_sigprocmask_retarget 2011-04-06 21:33:50.000000000 +0200 > +++ sigprocmask/include/linux/signal.h 2011-04-11 18:16:51.000000000 +0200 > @@ -2131,6 +2131,11 @@ int sigprocmask(int how, sigset_t *set, > } > > spin_lock_irq(&tsk->sighand->siglock); > + if (signal_pending(tsk) && !thread_group_empty(tsk)) { > + sigset_t not_newblocked; > + signorsets(¬_newblocked, ¤t->blocked, &newset); > + retarget_shared_pending(tsk, ¬_newblocked); > + } > tsk->blocked = newset; > recalc_sigpending(); > spin_unlock_irq(&tsk->sighand->siglock);
I absolutely detest how you made "sigprocmask()" the main interface to do this all, and then add new callers.
It's a horrid interface with that crazy "how" argument, and comes out of the user-space system call interface. If we make kernel users do this, especially critical ones like the signal handling code, please just extract out just the actual "set new signal mask" part.
So please just introduce a "sig_set_blocked()" or something, without the crazy "switch (how)" crud, and make sigprocmask() and everybody else use _that_ instead.
That would make me much happier about the patch series, I suspect.
Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |