svn commit: r366428 - in head/sys: kern sys

Konstantin Belousov kib at FreeBSD.org
Sun Oct 4 16:30:06 UTC 2020


Author: kib
Date: Sun Oct  4 16:30:05 2020
New Revision: 366428
URL: https://svnweb.freebsd.org/changeset/base/366428

Log:
  Refactor sleepq_catch_signals().
  
  - Extract suspension check into sig_ast_checksusp() helper.
  - Extract signal check and calculation of the interruption errno into
    sig_ast_needsigchk() helper.
  The helpers are moved to kern_sig.c which is the proper place for
  signal-related code.
  
  Improve control flow in sleepq_catch_signals(), to handle ret == 0
  (can sleep) and ret != 0 (interrupted) only once, by separating
  checking code into sleepq_check_ast_sq_locked(), which return value is
  interpreted at single location.
  
  Reviewed by:	markj
  Tested by:	pho
  Sponsored by:	The FreeBSD Foundation
  MFC after:	2 weeks
  Differential revision:	https://reviews.freebsd.org/D26628

Modified:
  head/sys/kern/kern_sig.c
  head/sys/kern/subr_sleepqueue.c
  head/sys/sys/signalvar.h

Modified: head/sys/kern/kern_sig.c
==============================================================================
--- head/sys/kern/kern_sig.c	Sun Oct  4 16:27:49 2020	(r366427)
+++ head/sys/kern/kern_sig.c	Sun Oct  4 16:30:05 2020	(r366428)
@@ -3139,6 +3139,71 @@ postsig(int sig)
 	return (1);
 }
 
+int
+sig_ast_checksusp(struct thread *td)
+{
+	struct proc *p;
+	int ret;
+
+	p = td->td_proc;
+	PROC_LOCK_ASSERT(p, MA_OWNED);
+
+	if ((td->td_flags & TDF_NEEDSUSPCHK) == 0)
+		return (0);
+
+	ret = thread_suspend_check(1);
+	MPASS(ret == 0 || ret == EINTR || ret == ERESTART);
+	return (ret);
+}
+
+int
+sig_ast_needsigchk(struct thread *td)
+{
+	struct proc *p;
+	struct sigacts *ps;
+	int ret, sig;
+
+	p = td->td_proc;
+	PROC_LOCK_ASSERT(p, MA_OWNED);
+
+	if ((td->td_flags & TDF_NEEDSIGCHK) == 0)
+		return (0);
+
+	ps = p->p_sigacts;
+	mtx_lock(&ps->ps_mtx);
+	sig = cursig(td);
+	if (sig == -1) {
+		mtx_unlock(&ps->ps_mtx);
+		KASSERT((td->td_flags & TDF_SBDRY) != 0, ("lost TDF_SBDRY"));
+		KASSERT(TD_SBDRY_INTR(td),
+		    ("lost TDF_SERESTART of TDF_SEINTR"));
+		KASSERT((td->td_flags & (TDF_SEINTR | TDF_SERESTART)) !=
+		    (TDF_SEINTR | TDF_SERESTART),
+		    ("both TDF_SEINTR and TDF_SERESTART"));
+		ret = TD_SBDRY_ERRNO(td);
+	} else if (sig != 0) {
+		ret = SIGISMEMBER(ps->ps_sigintr, sig) ? EINTR : ERESTART;
+		mtx_unlock(&ps->ps_mtx);
+	} else {
+		mtx_unlock(&ps->ps_mtx);
+		ret = 0;
+	}
+
+	/*
+	 * Do not go into sleep if this thread was the ptrace(2)
+	 * attach leader.  cursig() consumed SIGSTOP from PT_ATTACH,
+	 * but we usually act on the signal by interrupting sleep, and
+	 * should do that here as well.
+	 */
+	if ((td->td_dbgflags & TDB_FSTP) != 0) {
+		if (ret == 0)
+			ret = EINTR;
+		td->td_dbgflags &= ~TDB_FSTP;
+	}
+
+	return (ret);
+}
+
 void
 proc_wkilled(struct proc *p)
 {

Modified: head/sys/kern/subr_sleepqueue.c
==============================================================================
--- head/sys/kern/subr_sleepqueue.c	Sun Oct  4 16:27:49 2020	(r366427)
+++ head/sys/kern/subr_sleepqueue.c	Sun Oct  4 16:30:05 2020	(r366428)
@@ -433,33 +433,20 @@ sleepq_sleepcnt(const void *wchan, int queue)
 	return (sq->sq_blockedcnt[queue]);
 }
 
-/*
- * Marks the pending sleep of the current thread as interruptible and
- * makes an initial check for pending signals before putting a thread
- * to sleep. Enters and exits with the thread lock held.  Thread lock
- * may have transitioned from the sleepq lock to a run lock.
- */
 static int
-sleepq_catch_signals(const void *wchan, int pri)
+sleepq_check_ast_sc_locked(struct thread *td, struct sleepqueue_chain *sc)
 {
-	struct sleepqueue_chain *sc;
-	struct sleepqueue *sq;
-	struct thread *td;
 	struct proc *p;
-	struct sigacts *ps;
-	int sig, ret;
+	int ret;
 
-	ret = 0;
-	td = curthread;
-	p = curproc;
-	sc = SC_LOOKUP(wchan);
 	mtx_assert(&sc->sc_lock, MA_OWNED);
-	MPASS(wchan != NULL);
+
+	ret = 0;
 	if ((td->td_pflags & TDP_WAKEUP) != 0) {
 		td->td_pflags &= ~TDP_WAKEUP;
 		ret = EINTR;
 		thread_lock(td);
-		goto out;
+		return (0);
 	}
 
 	/*
@@ -467,91 +454,89 @@ sleepq_catch_signals(const void *wchan, int pri)
 	 * thread.  If not, we can switch immediately.
 	 */
 	thread_lock(td);
-	if ((td->td_flags & (TDF_NEEDSIGCHK | TDF_NEEDSUSPCHK)) != 0) {
-		thread_unlock(td);
-		mtx_unlock_spin(&sc->sc_lock);
-		CTR3(KTR_PROC, "sleepq catching signals: thread %p (pid %ld, %s)",
-			(void *)td, (long)p->p_pid, td->td_name);
-		PROC_LOCK(p);
-		/*
-		 * Check for suspension first. Checking for signals and then
-		 * suspending could result in a missed signal, since a signal
-		 * can be delivered while this thread is suspended.
-		 */
-		if ((td->td_flags & TDF_NEEDSUSPCHK) != 0) {
-			ret = thread_suspend_check(1);
-			MPASS(ret == 0 || ret == EINTR || ret == ERESTART);
-			if (ret != 0) {
-				PROC_UNLOCK(p);
-				mtx_lock_spin(&sc->sc_lock);
-				thread_lock(td);
-				goto out;
-			}
-		}
-		if ((td->td_flags & TDF_NEEDSIGCHK) != 0) {
-			ps = p->p_sigacts;
-			mtx_lock(&ps->ps_mtx);
-			sig = cursig(td);
-			if (sig == -1) {
-				mtx_unlock(&ps->ps_mtx);
-				KASSERT((td->td_flags & TDF_SBDRY) != 0,
-				    ("lost TDF_SBDRY"));
-				KASSERT(TD_SBDRY_INTR(td),
-				    ("lost TDF_SERESTART of TDF_SEINTR"));
-				KASSERT((td->td_flags &
-				    (TDF_SEINTR | TDF_SERESTART)) !=
-				    (TDF_SEINTR | TDF_SERESTART),
-				    ("both TDF_SEINTR and TDF_SERESTART"));
-				ret = TD_SBDRY_ERRNO(td);
-			} else if (sig != 0) {
-				ret = SIGISMEMBER(ps->ps_sigintr, sig) ?
-				    EINTR : ERESTART;
-				mtx_unlock(&ps->ps_mtx);
-			} else {
-				mtx_unlock(&ps->ps_mtx);
-			}
+	if ((td->td_flags & (TDF_NEEDSIGCHK | TDF_NEEDSUSPCHK)) == 0)
+		return (0);
 
-			/*
-			 * Do not go into sleep if this thread was the
-			 * ptrace(2) attach leader.  cursig() consumed
-			 * SIGSTOP from PT_ATTACH, but we usually act
-			 * on the signal by interrupting sleep, and
-			 * should do that here as well.
-			 */
-			if ((td->td_dbgflags & TDB_FSTP) != 0) {
-				if (ret == 0)
-					ret = EINTR;
-				td->td_dbgflags &= ~TDB_FSTP;
-			}
-		}
-		/*
-		 * Lock the per-process spinlock prior to dropping the PROC_LOCK
-		 * to avoid a signal delivery race.  PROC_LOCK, PROC_SLOCK, and
-		 * thread_lock() are currently held in tdsendsignal().
-		 */
-		PROC_SLOCK(p);
-		mtx_lock_spin(&sc->sc_lock);
+	thread_unlock(td);
+	mtx_unlock_spin(&sc->sc_lock);
+
+	p = td->td_proc;
+	CTR3(KTR_PROC, "sleepq catching signals: thread %p (pid %ld, %s)",
+		(void *)td, (long)p->p_pid, td->td_name);
+	PROC_LOCK(p);
+
+	/*
+	 * Check for suspension first. Checking for signals and then
+	 * suspending could result in a missed signal, since a signal
+	 * can be delivered while this thread is suspended.
+	 */
+	ret = sig_ast_checksusp(td);
+	if (ret != 0) {
 		PROC_UNLOCK(p);
+		mtx_lock_spin(&sc->sc_lock);
 		thread_lock(td);
-		PROC_SUNLOCK(p);
+		return (ret);
 	}
-	if (ret == 0) {
-		sleepq_switch(wchan, pri);
-		return (0);
-	}
-out:
+
+	ret = sig_ast_needsigchk(td);
+
 	/*
-	 * There were pending signals and this thread is still
-	 * on the sleep queue, remove it from the sleep queue.
+	 * Lock the per-process spinlock prior to dropping the
+	 * PROC_LOCK to avoid a signal delivery race.
+	 * PROC_LOCK, PROC_SLOCK, and thread_lock() are
+	 * currently held in tdsendsignal().
 	 */
-	if (TD_ON_SLEEPQ(td)) {
-		sq = sleepq_lookup(wchan);
-		sleepq_remove_thread(sq, td);
-	}
-	MPASS(td->td_lock != &sc->sc_lock);
-	mtx_unlock_spin(&sc->sc_lock);
-	thread_unlock(td);
+	PROC_SLOCK(p);
+	mtx_lock_spin(&sc->sc_lock);
+	PROC_UNLOCK(p);
+	thread_lock(td);
+	PROC_SUNLOCK(p);
 
+	return (ret);
+}
+
+/*
+ * Marks the pending sleep of the current thread as interruptible and
+ * makes an initial check for pending signals before putting a thread
+ * to sleep. Enters and exits with the thread lock held.  Thread lock
+ * may have transitioned from the sleepq lock to a run lock.
+ */
+static int
+sleepq_catch_signals(const void *wchan, int pri)
+{
+	struct thread *td;
+	struct sleepqueue_chain *sc;
+	struct sleepqueue *sq;
+	int ret;
+
+	sc = SC_LOOKUP(wchan);
+	mtx_assert(&sc->sc_lock, MA_OWNED);
+	MPASS(wchan != NULL);
+	td = curthread;
+
+	ret = sleepq_check_ast_sc_locked(td, sc);
+	THREAD_LOCK_ASSERT(td, MA_OWNED);
+	mtx_assert(&sc->sc_lock, MA_OWNED);
+
+	if (ret == 0) {
+		/*
+		 * No pending signals and no suspension requests found.
+		 * Switch the thread off the cpu.
+		 */
+		sleepq_switch(wchan, pri);
+	} else {
+		/*
+		 * There were pending signals and this thread is still
+		 * on the sleep queue, remove it from the sleep queue.
+		 */
+		if (TD_ON_SLEEPQ(td)) {
+			sq = sleepq_lookup(wchan);
+			sleepq_remove_thread(sq, td);
+		}
+		MPASS(td->td_lock != &sc->sc_lock);
+		mtx_unlock_spin(&sc->sc_lock);
+		thread_unlock(td);
+	}
 	return (ret);
 }
 

Modified: head/sys/sys/signalvar.h
==============================================================================
--- head/sys/sys/signalvar.h	Sun Oct  4 16:27:49 2020	(r366427)
+++ head/sys/sys/signalvar.h	Sun Oct  4 16:30:05 2020	(r366428)
@@ -399,6 +399,8 @@ void	sigacts_copy(struct sigacts *dest, struct sigacts
 void	sigacts_free(struct sigacts *ps);
 struct sigacts *sigacts_hold(struct sigacts *ps);
 int	sigacts_shared(struct sigacts *ps);
+int	sig_ast_checksusp(struct thread *td);
+int	sig_ast_needsigchk(struct thread *td);
 void	sig_drop_caught(struct proc *p);
 void	sigexit(struct thread *td, int sig) __dead2;
 int	sigev_findtd(struct proc *p, struct sigevent *sigev, struct thread **);


More information about the svn-src-all mailing list