Skip to contentSkip to conversation

Remove duplicate code in signal.c

PATCH In review Paul E. McKenney 2 participants 3 messages Oct 23, 2005

lore
Share & tools
lore

Paul E. McKenney

author
#0 lore raw

PATCH 1/1 Remove duplicate code in signal.c

Hello!

The following patch combines a bit of redundant code between force_sig_info() and force_sig_specific(). Tested on x86 and ppc64.

Signed-off-by [email protected]

---

 signal.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)
kernel/signal.c +2 -10
diff -urpNa -X dontdiff linux-2.6.14-rc2-rt7/kernel/signal.c linux-2.6.14-rc2-rt7-force_sig/kernel/signal.c
@@ -920,8 +920,8 @@ force_sig_info(int sig, struct siginfo *
 	if (sigismember(&t->blocked, sig) || t->sighand->action[sig-1].sa.sa_handler == SIG_IGN) {
 		t->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
 		sigdelset(&t->blocked, sig);
-		recalc_sigpending_tsk(t);
 	}
+	recalc_sigpending_tsk(t);
 	ret = specific_send_sig_info(sig, info, t);
 	spin_unlock_irqrestore(&t->sighand->siglock, flags);
 

@@ -931,15 +931,7 @@ force_sig_info(int sig, struct siginfo *
 void
 force_sig_specific(int sig, struct task_struct *t)
 {
-	unsigned long int flags;
-
-	spin_lock_irqsave(&t->sighand->siglock, flags);
-	if (t->sighand->action[sig-1].sa.sa_handler == SIG_IGN)
-		t->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
-	sigdelset(&t->blocked, sig);
-	recalc_sigpending_tsk(t);
-	specific_send_sig_info(sig, (void *)2, t);
-	spin_unlock_irqrestore(&t->sighand->siglock, flags);
+	force_sig_info(sig, (void *)2, t);
 }
 
 /*
#1 lore raw
Quoted 5 lines · Paul E. McKenney

Hello!

The following patch combines a bit of redundant code between force_sig_info() and force_sig_specific(). Tested on x86 and ppc64.

Some minor nitpicks ...

kernel/signal.c force_sig_info(int sig, struct siginfo * · line 922

Quoted 5 lines

+++ linux-2.6.14-rc2-rt7-force_sig/kernel/signal.c 2005-09-29 18:41:07.000000000 -0700

kernel/signal.c +0 -0
@@ -920,8 +920,8 @@ force_sig_info(int sig, struct siginfo *
        if (sigismember(&t->blocked, sig) || t->sighand->action[sig-1].sa.sa_handler == SIG_IGN) {
                t->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
                sigdelset(&t->blocked, sig);

May be it would be more readable to do:

	if (handler == SIG_IGN)
		handler = SIG_DFL;

	if (sigismember(->blocked, sig)) // probably unneeded at all
		sigdelset(->blocked, sig);

kernel/signal.c force_sig_info(int sig, struct siginfo * · line 924

Quoted 3 lines
kernel/signal.c +1 -1
-               recalc_sigpending_tsk(t);
        }
+       recalc_sigpending_tsk(t);

I never understood why can't we just do:

	set_tsk_thread_flag(TIF_SIGPENDING);

If this signal is not pending yet specific_send_siginfo() will set this flag anyway.

kernel/signal.c force_sig_info(int sig, struct siginfo * · line 934

Quoted 3 lines
kernel/signal.c +1 -2
-       specific_send_sig_info(sig, (void *)2, t);
-       spin_unlock_irqrestore(&t->sighand->siglock, flags);
+       force_sig_info(sig, (void *)2, t);

Paul, if you think this patch should go into the -mm tree first, could you rediff this patch against -mm ?

- specific_send_sig_info(sig, SEND_SIG_FORCED, t);
+ force_sig_info(sig, SEND_SIG_FORCED, t);

Oleg.

author
#2 lore raw
Quoted 17 lines · Oleg Nesterov
Quoted 5 lines · Paul E. McKenney

Hello!

The following patch combines a bit of redundant code between force_sig_info() and force_sig_specific(). Tested on x86 and ppc64.

Some minor nitpicks ...

Review force_sig_info(int sig, struct siginfo * · line 922

Quoted 5 lines

+++ linux-2.6.14-rc2-rt7-force_sig/kernel/signal.c 2005-09-29 18:41:07.000000000 -0700

(quoted diff) +0 -0
@@ -920,8 +920,8 @@ force_sig_info(int sig, struct siginfo *
        if (sigismember(&t->blocked, sig) || t->sighand->action[sig-1].sa.sa_handler == SIG_IGN) {
                t->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
                sigdelset(&t->blocked, sig);

May be it would be more readable to do:

Quoted 5 lines
	if (handler == SIG_IGN)
		handler = SIG_DFL;

	if (sigismember(->blocked, sig)) // probably unneeded at all
		sigdelset(->blocked, sig);

Seems reasonable to me.

Quoted 10 lines

Review force_sig_info(int sig, struct siginfo * · line 924

Quoted 3 lines
(quoted diff) +1 -1
-               recalc_sigpending_tsk(t);
        }
+       recalc_sigpending_tsk(t);

I never understood why can't we just do:

	set_tsk_thread_flag(TIF_SIGPENDING);

If this signal is not pending yet specific_send_siginfo() will set this flag anyway.

My guess is that putting the general logic into recalc_sigpending_tsk() prevents some bugs that might otherwise show up if someone forgets one of the conditions that can result in a signal being asserted. But in this case, it seems pretty safe. We really do want to force a signal. But it is a minor optimization, so I left it as is for now.

Review

Quoted 9 lines

Review force_sig_info(int sig, struct siginfo * · line 925

Quoted 3 lines
(quoted diff) +1 -2
-       specific_send_sig_info(sig, (void *)2, t);
-       spin_unlock_irqrestore(&t->sighand->siglock, flags);
+       force_sig_info(sig, (void *)2, t);

Paul, if you think this patch should go into the -mm tree first, could you rediff this patch against -mm ?

(quoted diff) +1 -1
- 	specific_send_sig_info(sig, SEND_SIG_FORCED, t);
+	force_sig_info(sig, SEND_SIG_FORCED, t);

Some time in -mm would certainly not hurt. The patch below is against 2.6.14-rc4-mm1, though Andrew asks that they be against a recent Linus tree (see 5c in http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt).
In any case, "SEND_SIG_FORCED" seems much nicer than "(void *)2". ;-)

Thanx, Paul

kernel/signal.c +5 -11
diff -urpNa -X dontdiff linux-2.6.14-rc4-mm1/kernel/signal.c linux-2.6.14-rc4-mm1-force-sig/kernel/signal.c
@@ -889,11 +889,13 @@ force_sig_info(int sig, struct siginfo *
 	int ret;
 
 	spin_lock_irqsave(&t->sighand->siglock, flags);
-	if (sigismember(&t->blocked, sig) || t->sighand->action[sig-1].sa.sa_handler == SIG_IGN) {
+	if (t->sighand->action[sig-1].sa.sa_handler == SIG_IGN) {
 		t->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
+	}
+	if (sigismember(&t->blocked, sig)) {
 		sigdelset(&t->blocked, sig);
-		recalc_sigpending_tsk(t);
 	}
+	recalc_sigpending_tsk(t);
 	ret = specific_send_sig_info(sig, info, t);
 	spin_unlock_irqrestore(&t->sighand->siglock, flags);
 

@@ -903,15 +905,7 @@ force_sig_info(int sig, struct siginfo *
 void
 force_sig_specific(int sig, struct task_struct *t)
 {
-	unsigned long int flags;
-
-	spin_lock_irqsave(&t->sighand->siglock, flags);
-	if (t->sighand->action[sig-1].sa.sa_handler == SIG_IGN)
-		t->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
-	sigdelset(&t->blocked, sig);
-	recalc_sigpending_tsk(t);
-	specific_send_sig_info(sig, SEND_SIG_FORCED, t);
-	spin_unlock_irqrestore(&t->sighand->siglock, flags);
+	force_sig_info(sig, SEND_SIG_FORCED, t);
 }
 
 /*