From owner-svn-src-head@freebsd.org Sun Oct 4 16:30:06 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 6131C42CADF; Sun, 4 Oct 2020 16:30:06 +0000 (UTC) (envelope-from kib@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4C48My1wCZz3frH; Sun, 4 Oct 2020 16:30:06 +0000 (UTC) (envelope-from kib@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 2501E271BA; Sun, 4 Oct 2020 16:30:06 +0000 (UTC) (envelope-from kib@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 094GU6N5038651; Sun, 4 Oct 2020 16:30:06 GMT (envelope-from kib@FreeBSD.org) Received: (from kib@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 094GU5MQ038649; Sun, 4 Oct 2020 16:30:05 GMT (envelope-from kib@FreeBSD.org) Message-Id: <202010041630.094GU5MQ038649@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: kib set sender to kib@FreeBSD.org using -f From: Konstantin Belousov Date: Sun, 4 Oct 2020 16:30:05 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r366428 - in head/sys: kern sys X-SVN-Group: head X-SVN-Commit-Author: kib X-SVN-Commit-Paths: in head/sys: kern sys X-SVN-Commit-Revision: 366428 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 04 Oct 2020 16:30:06 -0000 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 **);