Subject: signal dolock hacks
To: None <tech-kern@netbsd.org>
From: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
List: tech-kern
Date: 10/28/2006 00:20:00
--NextPart-20061027220303-2900001
Content-Type: Text/Plain; charset=us-ascii

hi,

the attached diff is to remove signal "dolock" hacks.
it's related to PR/32962 and PR/34895.
please review.

YAMAMOTO Takashi

--NextPart-20061027220303-2900001
Content-Type: Text/Plain; charset=us-ascii
Content-Disposition: attachment; filename="a.diff"

Index: kern/kern_sig.c
===================================================================
--- kern/kern_sig.c	(revision 1865)
+++ kern/kern_sig.c	(revision 1869)
@@ -92,7 +92,7 @@ static int	build_corename(struct proc *,
 static void	ksiginfo_exithook(struct proc *, void *);
 static void	ksiginfo_queue(struct proc *, const ksiginfo_t *, ksiginfo_t **);
 static ksiginfo_t *ksiginfo_dequeue(struct proc *, int);
-static void	kpsignal2(struct proc *, const ksiginfo_t *, int);
+static void	kpsignal2(struct proc *, const ksiginfo_t *);
 
 sigset_t	contsigmask, stopsigmask, sigcantmask;
 
@@ -801,7 +801,7 @@ sys_kill(struct lwp *l, void *v, registe
 		if (error)
 			return error;
 		if (signum)
-			kpsignal2(p, &ksi, 1);
+			kpsignal2(p, &ksi);
 		return (0);
 	}
 	switch (SCARG(uap, pid)) {
@@ -843,7 +843,7 @@ killpg1(struct lwp *l, ksiginfo_t *ksi, 
 				continue;
 			nfound++;
 			if (signum)
-				kpsignal2(p, ksi, 1);
+				kpsignal2(p, ksi);
 		}
 		proclist_unlock_read();
 	} else {
@@ -864,7 +864,7 @@ killpg1(struct lwp *l, ksiginfo_t *ksi, 
 				continue;
 			nfound++;
 			if (signum && P_ZOMBIE(p) == 0)
-				kpsignal2(p, ksi, 1);
+				kpsignal2(p, ksi);
 		}
 	}
 	return (nfound ? 0 : ESRCH);
@@ -956,7 +956,7 @@ trapsignal(struct lwp *l, const ksiginfo
 		/* XXX for core dump/debugger */
 		p->p_sigctx.ps_signo = ksi->ksi_signo;
 		p->p_sigctx.ps_code = ksi->ksi_trap;
-		kpsignal2(p, ksi, 1);
+		kpsignal2(p, ksi);
 	}
 }
 
@@ -964,7 +964,7 @@ trapsignal(struct lwp *l, const ksiginfo
  * Fill in signal information and signal the parent for a child status change.
  */
 void
-child_psignal(struct proc *p, int dolock)
+child_psignal(struct proc *p)
 {
 	ksiginfo_t ksi;
 
@@ -976,7 +976,7 @@ child_psignal(struct proc *p, int dolock
 	ksi.ksi_status = p->p_xstat;
 	ksi.ksi_utime = p->p_stats->p_ru.ru_utime.tv_sec;
 	ksi.ksi_stime = p->p_stats->p_ru.ru_stime.tv_sec;
-	kpsignal2(p->p_pptr, &ksi, dolock);
+	kpsignal2(p->p_pptr, &ksi);
 }
 
 /*
@@ -991,21 +991,19 @@ child_psignal(struct proc *p, int dolock
  *     regardless of the signal action (eg, blocked or ignored).
  *
  * Other ignored signals are discarded immediately.
- *
- * XXXSMP: Invoked as psignal() or sched_psignal().
  */
 void
-psignal1(struct proc *p, int signum, int dolock)
+psignal(struct proc *p, int signum)
 {
 	ksiginfo_t ksi;
 
 	KSI_INIT_EMPTY(&ksi);
 	ksi.ksi_signo = signum;
-	kpsignal2(p, &ksi, dolock);
+	kpsignal2(p, &ksi);
 }
 
 void
-kpsignal1(struct proc *p, ksiginfo_t *ksi, void *data, int dolock)
+kpsignal(struct proc *p, ksiginfo_t *ksi, void *data)
 {
 
 	if ((p->p_flag & P_WEXIT) == 0 && data) {
@@ -1022,11 +1020,11 @@ kpsignal1(struct proc *p, ksiginfo_t *ks
 			}
 		}
 	}
-	kpsignal2(p, ksi, dolock);
+	kpsignal2(p, ksi);
 }
 
 static void
-kpsignal2(struct proc *p, const ksiginfo_t *ksi, int dolock)
+kpsignal2(struct proc *p, const ksiginfo_t *ksi)
 {
 	struct lwp *l, *suspended = NULL;
 	struct sadata_vp *vp;
@@ -1039,12 +1037,7 @@ kpsignal2(struct proc *p, const ksiginfo
 	if (signum <= 0 || signum >= NSIG)
 		panic("psignal signal number %d", signum);
 
-	/* XXXSMP: works, but icky */
-	if (dolock) {
-		SCHED_ASSERT_UNLOCKED();
-	} else {
-		SCHED_ASSERT_LOCKED();
-	}
+	SCHED_ASSERT_UNLOCKED();
 #endif
 
 	/*
@@ -1130,10 +1123,7 @@ kpsignal2(struct proc *p, const ksiginfo
 	    && p->p_stat != SSTOP) {
 		p->p_sigctx.ps_sigwaited->ksi_info = ksi->ksi_info;
 		p->p_sigctx.ps_sigwaited = NULL;
-		if (dolock)
-			wakeup_one(&p->p_sigctx.ps_sigwait);
-		else
-			sched_wakeup(&p->p_sigctx.ps_sigwait);
+		wakeup_one(&p->p_sigctx.ps_sigwait);
 		return;
 	}
 
@@ -1164,9 +1154,7 @@ kpsignal2(struct proc *p, const ksiginfo
 		return;
 	}
 
-	/* XXXSMP: works, but icky */
-	if (dolock)
-		SCHED_LOCK(s);
+	SCHED_LOCK(s);
 
 	if (p->p_flag & P_SA) {
 		allsusp = 0;
@@ -1269,15 +1257,12 @@ kpsignal2(struct proc *p, const ksiginfo
 			}
 			sigdelset(&p->p_sigctx.ps_siglist, signum);
 			p->p_xstat = signum;
+			proc_stop(p, 1);	/* XXXSMP: recurse? */
+			SCHED_UNLOCK(s);
 			if ((p->p_pptr->p_flag & P_NOCLDSTOP) == 0) {
-				/*
-				 * XXXSMP: recursive call; don't lock
-				 * the second time around.
-				 */
-				child_psignal(p, 0);
+				child_psignal(p);
 			}
-			proc_stop(p, 1);	/* XXXSMP: recurse? */
-			goto done;
+			goto done_unlocked;
 		}
 
 		if (l == NULL) {
@@ -1394,10 +1379,9 @@ kpsignal2(struct proc *p, const ksiginfo
 	if (action == SIG_CATCH)
 		ksiginfo_queue(p, ksi, &newkp);
  done:
-	/* XXXSMP: works, but icky */
-	if (dolock)
-		SCHED_UNLOCK(s);
+	SCHED_UNLOCK(s);
 
+ done_unlocked:
 	if (newkp)
 		pool_put(&ksiginfo_pool, newkp);
 }
@@ -1496,8 +1480,7 @@ int
 issignal(struct lwp *l)
 {
 	struct proc	*p = l->l_proc;
-	int		s = 0, signum, prop;
-	int		dolock = (l->l_flag & L_SINTR) == 0, locked = !dolock;
+	int		s, signum, prop;
 	sigset_t	ss;
 
 	/* Bail out if we do not own the virtual processor */
@@ -1509,8 +1492,7 @@ issignal(struct lwp *l)
 		 * The process is stopped/stopping. Stop ourselves now that
 		 * we're on the kernel/userspace boundary.
 		 */
-		if (dolock)
-			SCHED_LOCK(s);
+		SCHED_LOCK(s);
 		l->l_stat = LSSTOP;
 		p->p_nrlwps--;
 		if (p->p_flag & P_TRACED)
@@ -1525,8 +1507,6 @@ issignal(struct lwp *l)
 		signum = firstsig(&ss);
 		if (signum == 0) {		 	/* no signal to send */
 			p->p_sigctx.ps_sigcheck = 0;
-			if (locked && dolock)
-				SCHED_LOCK(s);
 			return (0);
 		}
 							/* take the signal! */
@@ -1553,17 +1533,13 @@ issignal(struct lwp *l)
 				goto childresumed;
 
 			if ((p->p_flag & P_FSTRACE) == 0)
-				child_psignal(p, dolock);
-			if (dolock)
-				SCHED_LOCK(s);
+				child_psignal(p);
+			SCHED_LOCK(s);
 			proc_stop(p, 1);
 		sigtraceswitch:
 			mi_switch(l, NULL);
 			SCHED_ASSERT_UNLOCKED();
-			if (dolock)
-				splx(s);
-			else
-				dolock = 1;
+			splx(s);
 
 		childresumed:
 			/*
@@ -1627,17 +1603,13 @@ issignal(struct lwp *l)
 					break;	/* == ignore */
 				p->p_xstat = signum;
 				if ((p->p_pptr->p_flag & P_NOCLDSTOP) == 0)
-					child_psignal(p, dolock);
-				if (dolock)
-					SCHED_LOCK(s);
+					child_psignal(p);
+				SCHED_LOCK(s);
 				proc_stop(p, 1);
 			sigswitch:
 				mi_switch(l, NULL);
 				SCHED_ASSERT_UNLOCKED();
-				if (dolock)
-					splx(s);
-				else
-					dolock = 1;
+				splx(s);
 				break;
 			} else if (prop & SA_IGNORE) {
 				/*
@@ -1676,8 +1648,6 @@ issignal(struct lwp *l)
 						/* leave the signal for later */
 	sigaddset(&p->p_sigctx.ps_siglist, signum);
 	CHECKSIGS(p);
-	if (locked && dolock)
-		SCHED_LOCK(s);
 	return (signum);
 }
 
@@ -2427,7 +2397,7 @@ __sigtimedwait1(struct lwp *l, void *v, 
 	}
 
 	/*
-	 * Silently ignore SA_CANTMASK signals. psignal1() would
+	 * Silently ignore SA_CANTMASK signals. psignal() would
 	 * ignore SA_CANTMASK signals in waitset, we do this
 	 * only for the below siglist check.
 	 */
@@ -2508,7 +2478,7 @@ __sigtimedwait1(struct lwp *l, void *v, 
 	}
 
 	/*
-	 * On error, clear sigwait indication. psignal1() clears it
+	 * On error, clear sigwait indication. psignal() clears it
 	 * in !error case.
 	 */
 	if (error) {
Index: kern/kern_synch.c
===================================================================
--- kern/kern_synch.c	(revision 1865)
+++ kern/kern_synch.c	(revision 1869)
@@ -535,9 +535,11 @@ ltsleep(volatile const void *ident, int 
 	 * stopped, p->p_wchan will be 0 upon return from CURSIG.
 	 */
 	if (catch) {
+		SCHED_UNLOCK(s);
 		l->l_flag |= L_SINTR;
 		if (((sig = CURSIG(l)) != 0) ||
 		    ((p->p_flag & P_WEXIT) && p->p_nlwps > 1)) {
+			SCHED_LOCK(s);
 			if (l->l_wchan != NULL)
 				unsleep(l);
 			l->l_stat = LSONPROC;
@@ -546,9 +548,9 @@ ltsleep(volatile const void *ident, int 
 		}
 		if (l->l_wchan == NULL) {
 			catch = 0;
-			SCHED_UNLOCK(s);
 			goto resume;
 		}
+		SCHED_LOCK(s);
 	} else
 		sig = 0;
 	l->l_stat = LSSLEEP;
@@ -959,31 +961,6 @@ mi_switch(struct lwp *l, struct lwp *new
 	p->p_rtime.tv_sec = s;
 
 	/*
-	 * Check if the process exceeds its CPU resource allocation.
-	 * If over max, kill it.  In any case, if it has run for more
-	 * than 10 minutes, reduce priority to give others a chance.
-	 */
-	rlim = &p->p_rlimit[RLIMIT_CPU];
-	if (s >= rlim->rlim_cur) {
-		/*
-		 * XXXSMP: we're inside the scheduler lock perimeter;
-		 * use sched_psignal.
-		 */
-		if (s >= rlim->rlim_max)
-			sched_psignal(p, SIGKILL);
-		else {
-			sched_psignal(p, SIGXCPU);
-			if (rlim->rlim_cur < rlim->rlim_max)
-				rlim->rlim_cur += 5;
-		}
-	}
-	if (autonicetime && s > autonicetime &&
-	    kauth_cred_geteuid(p->p_cred) && p->p_nice == NZERO) {
-		p->p_nice = autoniceval + NZERO;
-		resetpriority(l);
-	}
-
-	/*
 	 * Process is about to yield the CPU; clear the appropriate
 	 * scheduling flags.
 	 */
@@ -1040,6 +1017,29 @@ mi_switch(struct lwp *l, struct lwp *new
 	microtime(&l->l_cpu->ci_schedstate.spc_runtime);
 
 	/*
+	 * Check if the process exceeds its CPU resource allocation.
+	 * If over max, kill it.  In any case, if it has run for more
+	 * than 10 minutes, reduce priority to give others a chance.
+	 */
+	rlim = &p->p_rlimit[RLIMIT_CPU];
+	if (s >= rlim->rlim_cur) {
+		if (s >= rlim->rlim_max) {
+			psignal(p, SIGKILL);
+		} else {
+			psignal(p, SIGXCPU);
+			if (rlim->rlim_cur < rlim->rlim_max)
+				rlim->rlim_cur += 5;
+		}
+	}
+	if (autonicetime && s > autonicetime &&
+	    kauth_cred_geteuid(p->p_cred) && p->p_nice == NZERO) {
+		SCHED_LOCK(s);
+		p->p_nice = autoniceval + NZERO;
+		resetpriority(l);
+		SCHED_UNLOCK(s);
+	}
+
+	/*
 	 * Reacquire the kernel_lock now.  We do this after we've
 	 * released the scheduler lock to avoid deadlock, and before
 	 * we reacquire the interlock.
Index: kern/sys_process.c
===================================================================
--- kern/sys_process.c	(revision 1865)
+++ kern/sys_process.c	(revision 1869)
@@ -847,7 +847,7 @@ process_checkioperm(struct lwp *l, struc
 void
 process_stoptrace(struct lwp *l)
 {
-	int s = 0, dolock = (l->l_flag & L_SINTR) == 0;
+	int s;
 	struct proc *p = l->l_proc, *pp = p->p_pptr;
 
 	if (pp->p_pid == 1) {
@@ -856,17 +856,15 @@ process_stoptrace(struct lwp *l)
 	}
 
 	p->p_xstat = SIGTRAP;
-	child_psignal(pp, dolock);
+	child_psignal(pp);
 
-	if (dolock)
-		SCHED_LOCK(s);
+	SCHED_LOCK(s);
 
 	proc_stop(p, 1);
 
 	mi_switch(l, NULL);
 	SCHED_ASSERT_UNLOCKED();
 
-	if (dolock)
-		splx(s);
+	splx(s);
 }
 #endif /* KTRACE || PTRACE */
Index: sys/signalvar.h
===================================================================
--- sys/signalvar.h	(revision 1865)
+++ sys/signalvar.h	(revision 1869)
@@ -154,12 +154,9 @@ int	issignal(struct lwp *);
 void	pgsignal(struct pgrp *, int, int);
 void	kpgsignal(struct pgrp *, struct ksiginfo *, void *, int);
 void	postsig(int);
-void	psignal1(struct proc *, int, int);
-void	kpsignal1(struct proc *, struct ksiginfo *, void *, int);
-#define	kpsignal(p, ksi, data)		kpsignal1((p), (ksi), (data), 1)
-#define	psignal(p, sig)			psignal1((p), (sig), 1)
-#define	sched_psignal(p, sig)		psignal1((p), (sig), 0)
-void	child_psignal(struct proc *, int);
+void	psignal(struct proc *, int);
+void	kpsignal(struct proc *, struct ksiginfo *, void *);
+void	child_psignal(struct proc *);
 void	siginit(struct proc *);
 void	trapsignal(struct lwp *, const struct ksiginfo *);
 void	sigexit(struct lwp *, int);

--NextPart-20061027220303-2900001--