From owner-freebsd-threads@FreeBSD.ORG Thu Nov 27 10:14:11 2014 Return-Path: Delivered-To: freebsd-threads@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id C6B00AC4; Thu, 27 Nov 2014 10:14:11 +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 68EE7B22; Thu, 27 Nov 2014 10:14:11 +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 sARAE6vo025493 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 27 Nov 2014 12:14:06 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.9.2 kib.kiev.ua sARAE6vo025493 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id sARAE6P4025492; Thu, 27 Nov 2014 12:14:06 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 27 Nov 2014 12:14:06 +0200 From: Konstantin Belousov To: Eygene Ryabinkin Subject: Re: [CFR][PATCH] Call proper signal handler from libthr for non-SA_SIGINFO case Message-ID: <20141127101405.GP17068@kib.kiev.ua> References: <20141120194925.GK17068@kib.kiev.ua> <20141121165658.GP17068@kib.kiev.ua> <20141121203227.GS17068@kib.kiev.ua> <20141126221734.GM17068@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: Thu, 27 Nov 2014 10:14:12 -0000 On Thu, Nov 27, 2014 at 07:21:14AM +0300, Eygene Ryabinkin wrote: > "td->td_ru.ru_nsignals++;" perhaps doesn't belong to the new function, > but it is a matter of taste. By the way, shouldn't Well, there is no common theme in the operations done in the postsig_done(), except some fiddling with the signal state after the frame is pushed to usermode stack. I have trouble formulating the purpose of the function in the introductory comment, due to this. ru_nsignals++ is not better or worse than any other operation performed. > "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. I believe that the following patch is due. It is better way to ensure the invariants, instead of adding assert to postsig_done(). 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);