From owner-freebsd-threads@FreeBSD.ORG Thu Nov 27 17:14:15 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 5E431395; Thu, 27 Nov 2014 17:14:15 +0000 (UTC) Received: from 0.mx.codelabs.ru (0.mx.codelabs.ru [144.206.233.71]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id DE2FAF81; Thu, 27 Nov 2014 17:14:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=codelabs.ru; s=three; h=Sender:In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date; bh=HTDW7Kl5K9x9DRq3cgw31ZlzaOdyFxpJGEXF1Gsjj4U=; b=RwOqqrn3r5AW9G/xaNouEavEHZKLRjMktRaABSF8fcgwYWI3I7BVNznys6b5pLOg/l2qA6meTayfkkKmsK01I89cQxxKG1rDwTTk49Uza+KEouRTYJmP+GxEsWjXACMRXsk1r8Ey/yfAYtrxUuSnSi7gz9k54HvwyZEn2mwWZ/QnqHxtg2P3TLPdq7lpKIt5LOtnUBB8y7o9GzfOM4kwvM6SC0CjvIhECvIjiNRkyNm1LgLZWk5DKMY+D5eV8szlyoBub5E+77HSV1ayD6wGLwOMe0eaqZqaDVANB1B8+SRAfuEALnlD4m49Yl6VrH47Bv6bwKHLeAVAgIsVqLa3IKCfSqGP7iNHV3ysBxzvgGVlWxdhHai9o8qrDYajdxh9UX/U3pYxCEgwehm2onvwX2ot5oIh1RBSu4UILd6JfjmZZtiCpzS/VxyOYqu0u1+E4JRE6ldpW4HZCnXUPpbHMVr9ycBFYxKEblE1SrJUCOjlAjhKuM2Fr3aRrztB/88okNWhpE7KM2buH1a7Ry7Ie1XmDKhLbyDVc7vzZ3pwn+r2r4X8X9p4BinimuwOj5pnP32uwuDDXZF02qe7Ek+W8qsstcC4KrFAbnXoQJEGiBofJ92VGVo+iu8T8wn0NyHbZc9dL3pH02hxwqwM7Es0U49CoIuyEVNfKPq7gJSZ9jc=; Received: from void.codelabs.ru (void.codelabs.ru [144.206.233.66]) by 0.mx.codelabs.ru with esmtpsa (TLSv1.2:DHE-RSA-AES256-GCM-SHA384:256) id 1Xu2dv-0007Fn-G1; Thu, 27 Nov 2014 21:14:11 +0400 Date: Thu, 27 Nov 2014 20:14:09 +0300 From: Eygene Ryabinkin To: Konstantin Belousov Subject: Re: [CFR][PATCH] Call proper signal handler from libthr for non-SA_SIGINFO case Message-ID: 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: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="ggC1wwkyYWqEab//" Content-Disposition: inline In-Reply-To: <20141127101405.GP17068@kib.kiev.ua> Sender: rea@codelabs.ru 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 17:14:15 -0000 --ggC1wwkyYWqEab// Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. 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_NOTOW= NED) }}} inside kern_sigprocmask to have a bit stronger assertion that covers unlocked case. > 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; > =20 > p =3D td->td_proc; > - if (!(flags & SIGPROCMASK_PROC_LOCKED)) > + if ((flags & SIGPROCMASK_PROC_LOCKED) !=3D 0) > + PROC_LOCK_ASSERT(p, MA_OWNED); > + else > PROC_LOCK(p); > + mtx_assert(&p->p_sigacts->ps_mtx, (flags & SIGPROCMASK_PS_LOCKED) !=3D 0 > + ? MA_OWNED : MA_NOTOWNED); > if (oset !=3D NULL) > *oset =3D td->td_sigmask; > =20 > @@ -2510,9 +2514,11 @@ reschedule_signals(struct proc *p, sigset_t block,= int flags) > int sig; > =20 > PROC_LOCK_ASSERT(p, MA_OWNED); > + ps =3D p->p_sigacts; > + mtx_assert(&ps->ps_mtx, (flags & SIGPROCMASK_PS_LOCKED) !=3D 0 ? > + MA_OWNED : MA_NOTOWNED); > if (SIGISEMPTY(p->p_siglist)) > return; > - ps =3D p->p_sigacts; > SIGSETAND(block, p->p_siglist); > while ((sig =3D sig_ffs(&block)) !=3D 0) { > SIGDELSET(block, sig); >=20 And, being in the nit-picking mode, I'd apply the following patch {{{ Index: sys/kern/kern_sig.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- 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 !=3D 1) - reschedule_signals(p, new_block, flags); + reschedule_signals(p, new_block, flags & SIGPROCMASK_PS_LOCKED); } =20 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. --=20 Eygene Ryabinkin ,,,^..^,,, [ Life's unfair - but root password helps! | codelabs.ru ] [ 82FE 06BC D497 C0DE 49EC 4FF0 16AF 9EAE 8152 ECFB | freebsd.org ] --ggC1wwkyYWqEab// Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iL4EABEKAGYFAlR3W+FfFIAAAAAALgAoaXNzdWVyLWZwckBub3RhdGlvbnMub3Bl bnBncC5maWZ0aGhvcnNlbWFuLm5ldDgyRkUwNkJDRDQ5N0MwREU0OUVDNEZGMDE2 QUY5RUFFODE1MkVDRkIACgkQFq+eroFS7Pv7gQEAlILqnlWcwtZAtK/9jWGbtZVo Lk009Gc7LxI5RFQR8v8A/RJokGaXIfwmmbbVHyotU16CQOOiUumqKMOaKeVYnRPl =2XAU -----END PGP SIGNATURE----- --ggC1wwkyYWqEab//--