[PATCH] do_sigaction: don't worry about signal_pending()

[PATCH] do_sigaction: don't worry about signal_pending()

[Oleg Nesterov]

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 <oleg@tv-sign.ru>

--- 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 = &current->sighand->action[sig-1];
 
 	spin_lock_irq(&current->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(&current->sighand->siglock);
-		return -ERESTARTNOINTR;
-	}
-
 	if (oact)
 		*oact = *k;
 

Re: [PATCH] do_sigaction: don't worry about signal_pending()

[Roland McGrath]

I agree, I don't think this check is buying us anything now.


Thanks,
Roland

Re: [PATCH] do_sigaction: don't worry about signal_pending()

[Jarek Poplawski]

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.

Re: [PATCH] do_sigaction: don't worry about signal_pending()

[Oleg Nesterov]

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.

Re: [PATCH] do_sigaction: don't worry about signal_pending()

[Jarek Poplawski]

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.