From owner-freebsd-threads@FreeBSD.ORG Fri Nov 28 10:13:19 2014 Return-Path: Delivered-To: freebsd-threads@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 77D1DAEE; Fri, 28 Nov 2014 10:13:19 +0000 (UTC) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 00AD7F19; Fri, 28 Nov 2014 10:13:18 +0000 (UTC) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.14.9/8.14.9) with ESMTP id sASADDrY083319 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 28 Nov 2014 12:13:13 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.9.2 kib.kiev.ua sASADDrY083319 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id sASADC8a083317; Fri, 28 Nov 2014 12:13:12 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Fri, 28 Nov 2014 12:13:12 +0200 From: Konstantin Belousov To: Eygene Ryabinkin Subject: Re: [CFR][PATCH] Call proper signal handler from libthr for non-SA_SIGINFO case Message-ID: <20141128101312.GT17068@kib.kiev.ua> References: <20141120194925.GK17068@kib.kiev.ua> <20141121165658.GP17068@kib.kiev.ua> <20141121203227.GS17068@kib.kiev.ua> <20141126221734.GM17068@kib.kiev.ua> <20141127101405.GP17068@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.0 X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on tom.home Cc: davidxu@FreeBSD.org, freebsd-threads@FreeBSD.org X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Nov 2014 10:13:19 -0000 On Thu, Nov 27, 2014 at 08:14:09PM +0300, Eygene Ryabinkin wrote: > Thu, Nov 27, 2014 at 12:14:06PM +0200, Konstantin Belousov wrote: > > On Thu, Nov 27, 2014 at 07:21:14AM +0300, Eygene Ryabinkin wrote: > > > "PROC_LOCK_ASSERT(td->td_proc, MA_OWNED);" be also present single > > > kern_sigprocmask() is called with SIGPROCMASK_PROC_LOCKED? > > > > This is what I suggested in my earlier mail. When I started looking into > > details, I decided not to do it. The reasoning is that the function > > itself touches no process state protected by the proc lock. It is > > kern_sigprocmask which works with process lock-protected data. On the > > other hand, the function works with p_sigacts, which is covered by > > ps_mtx, and I asserted it. > > But sendsig_done() calls kern_sigprocmask with > (SIGPROCMASK_PROC_LOCKED | SIGPROCMASK_PS_LOCKED) thus guaranteeing > some set of locks, so its better to have some ways to support this > assertion. I understand that the current implementation of > kern_sigprocmask() will test such an assertion, but things may change > with time. kern_sigprocmask (will) assert this on its own. (will == after the patch is committed, which I am going to do right now). > > Moreover, having all asserts at the beginning of a function makes > at least myself to immediately recognise which lock(s) should be taken > before this function is to be called, so it is a kind of a documentation. > > > I believe that the following patch is due. It is better way to ensure > > the invariants, instead of adding assert to postsig_done(). > > These are good, but, probably, you can do something like > {{{ > PROC_LOCK_ASSERT(p, (flags & SIGPROCMASK_PROC_LOCKED) ? MA_OWNED : MA_NOTOWNED) > }}} > inside kern_sigprocmask to have a bit stronger assertion that covers > unlocked case. This is not needed. The process lock is not recursive, so the mere call to acquire the lock triggers the assertion on recursive acquire. > > > diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c > > index 15638e0..502f334 100644 > > --- a/sys/kern/kern_sig.c > > +++ b/sys/kern/kern_sig.c > > @@ -998,8 +998,12 @@ kern_sigprocmask(struct thread *td, int how, sigset_t *set, sigset_t *oset, > > int error; > > > > p = td->td_proc; > > - if (!(flags & SIGPROCMASK_PROC_LOCKED)) > > + if ((flags & SIGPROCMASK_PROC_LOCKED) != 0) > > + PROC_LOCK_ASSERT(p, MA_OWNED); > > + else > > PROC_LOCK(p); > > + mtx_assert(&p->p_sigacts->ps_mtx, (flags & SIGPROCMASK_PS_LOCKED) != 0 > > + ? MA_OWNED : MA_NOTOWNED); > > if (oset != NULL) > > *oset = td->td_sigmask; > > > > @@ -2510,9 +2514,11 @@ reschedule_signals(struct proc *p, sigset_t block, int flags) > > int sig; > > > > PROC_LOCK_ASSERT(p, MA_OWNED); > > + ps = p->p_sigacts; > > + mtx_assert(&ps->ps_mtx, (flags & SIGPROCMASK_PS_LOCKED) != 0 ? > > + MA_OWNED : MA_NOTOWNED); > > if (SIGISEMPTY(p->p_siglist)) > > return; > > - ps = p->p_sigacts; > > SIGSETAND(block, p->p_siglist); > > while ((sig = sig_ffs(&block)) != 0) { > > SIGDELSET(block, sig); > > > > And, being in the nit-picking mode, I'd apply the following patch > {{{ > Index: sys/kern/kern_sig.c > =================================================================== > --- sys/kern/kern_sig.c (revision 275189) > +++ sys/kern/kern_sig.c (working copy) > @@ -1043,7 +1043,7 @@ > * signal, possibly waking it up. > */ > if (p->p_numthreads != 1) > - reschedule_signals(p, new_block, flags); > + reschedule_signals(p, new_block, flags & SIGPROCMASK_PS_LOCKED); > } > > out: > }}} > because reschedule_signals() is interested only in SIGPROCMASK_PS_LOCKED, > so there is no reason to pass other bits, since it may lead to some > confusion. I disagree, I wrote reschedule_signals() to take the same set of flags as kern_sigprocmask().