do_sigaction() returns -ERESTARTNOINTR if signal_pending(). The comment says:
* If there might be a fatal signal pending on multiple
* threads, make sure we take it before changing the action.
I think this is not needed. We should only worry about SIGNAL_GROUP_EXIT case,
bit it implies a pending SIGKILL which can't be cleared by do_sigaction.
Kill this special case.
Signed-off-by: Oleg Nesterov <[email protected]>
--- t/kernel/signal.c~SA_NOPEND 2007-08-20 19:40:31.000000000 +0400
+++ t/kernel/signal.c 2007-08-20 19:43:41.000000000 +0400
@@ -2300,15 +2300,6 @@ int do_sigaction(int sig, struct k_sigac
k = ¤t->sighand->action[sig-1];
spin_lock_irq(¤t->sighand->siglock);
- if (signal_pending(current)) {
- /*
- * If there might be a fatal signal pending on multiple
- * threads, make sure we take it before changing the action.
- */
- spin_unlock_irq(¤t->sighand->siglock);
- return -ERESTARTNOINTR;
- }
-
if (oact)
*oact = *k;
I agree, I don't think this check is buying us anything now.
Thanks,
Roland
On 20-08-2007 18:01, Oleg Nesterov wrote:
> do_sigaction() returns -ERESTARTNOINTR if signal_pending(). The comment says:
>
> * If there might be a fatal signal pending on multiple
> * threads, make sure we take it before changing the action.
>
> I think this is not needed. We should only worry about SIGNAL_GROUP_EXIT case,
> bit it implies a pending SIGKILL which can't be cleared by do_sigaction.
Isn't it for optimization e.g., to skip this 'do while' loop below for
such multiple threads, which would get SIGKILL or SIGSTOP anyway?
Regards,
Jarek P.
On 08/22, Jarek Poplawski wrote:
>
> On 20-08-2007 18:01, Oleg Nesterov wrote:
> > do_sigaction() returns -ERESTARTNOINTR if signal_pending(). The comment says:
> >
> > * If there might be a fatal signal pending on multiple
> > * threads, make sure we take it before changing the action.
> >
> > I think this is not needed. We should only worry about SIGNAL_GROUP_EXIT case,
> > bit it implies a pending SIGKILL which can't be cleared by do_sigaction.
>
> Isn't it for optimization e.g., to skip this 'do while' loop below for
> such multiple threads, which would get SIGKILL or SIGSTOP anyway?
Yes, in that case this 'do while' doesn't make sense. But this is very
unlikely, sigaction() shouldn't be called too much often, better to save
a couple of bytes from icache.
Also, please note that sigaction() is not special, almost any system call
could be started with
if (current->signal->flags & SIGNAL_GROUP_EXIT)
return ANYVALUE;
to "optimize" for the case when the task is dying.
Oleg.
On Wed, Aug 22, 2007 at 02:02:10PM +0400, Oleg Nesterov wrote:
> On 08/22, Jarek Poplawski wrote:
> >
> > On 20-08-2007 18:01, Oleg Nesterov wrote:
> > > do_sigaction() returns -ERESTARTNOINTR if signal_pending(). The comment says:
> > >
> > > * If there might be a fatal signal pending on multiple
> > > * threads, make sure we take it before changing the action.
> > >
> > > I think this is not needed. We should only worry about SIGNAL_GROUP_EXIT case,
> > > bit it implies a pending SIGKILL which can't be cleared by do_sigaction.
> >
> > Isn't it for optimization e.g., to skip this 'do while' loop below for
> > such multiple threads, which would get SIGKILL or SIGSTOP anyway?
>
> Yes, in that case this 'do while' doesn't make sense. But this is very
> unlikely, sigaction() shouldn't be called too much often, better to save
> a couple of bytes from icache.
>
> Also, please note that sigaction() is not special, almost any system call
> could be started with
>
> if (current->signal->flags & SIGNAL_GROUP_EXIT)
> return ANYVALUE;
>
> to "optimize" for the case when the task is dying.
OK, I only wasn't sure this was considered before getting this
"not needed" verdict.
BTW, sometimes, even if something is very unlikely, but very
time-consuming, and skipping this isn't so costly, there could be
a gain at the end. So, maybe it's about individual cases and some
testing too? (At least, it seems, somebody found this so usable,
she/he bothered with such a long comment, and we know it's a last
thing any decent kernel hacker would care to do...)
Jarek P.