Date: Thu, 27 Nov 2014 20:14:09 +0300 From: Eygene Ryabinkin <rea@freebsd.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: davidxu@FreeBSD.org, freebsd-threads@FreeBSD.org Subject: Re: [CFR][PATCH] Call proper signal handler from libthr for non-SA_SIGINFO case Message-ID: <Y9IeRbaEfX1V2/ZQ54l6PUaYmWw@1d%2BaJAniZP50FCDdGj54nd51%2Bks> In-Reply-To: <20141127101405.GP17068@kib.kiev.ua> References: <KBP9w7ztz0PP7uy58G1ODYb/voI@1d%2BaJAniZP50FCDdGj54nd51%2Bks> <20141120194925.GK17068@kib.kiev.ua> <ngq2VsHPChtgNNL/z%2BlRaAXEeTI@1d%2BaJAniZP50FCDdGj54nd51%2Bks> <20141121165658.GP17068@kib.kiev.ua> <cxMeZKcIhV0OesPiyimV3oGOays@W98LlNNy5PXu%2BS3VxPCL6cVmtdM> <20141121203227.GS17068@kib.kiev.ua> <jFEd1ENuyp8%2BKOBFhFLA4ucYsq0@btiPEv4nTmhOyKpuIO1ud%2BMiSV0> <20141126221734.GM17068@kib.kiev.ua> <t6N4RkaeaRObVTAlzRpdhmmcDSg@yxQcZffeD9XmruHr2cPBouRqdXo> <20141127101405.GP17068@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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_NOTOWNED)
}}}
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;
>
> 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.
--
Eygene Ryabinkin ,,,^..^,,,
[ Life's unfair - but root password helps! | codelabs.ru ]
[ 82FE 06BC D497 C0DE 49EC 4FF0 16AF 9EAE 8152 ECFB | freebsd.org ]
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iL4EABEKAGYFAlR3W+FfFIAAAAAALgAoaXNzdWVyLWZwckBub3RhdGlvbnMub3Bl
bnBncC5maWZ0aGhvcnNlbWFuLm5ldDgyRkUwNkJDRDQ5N0MwREU0OUVDNEZGMDE2
QUY5RUFFODE1MkVDRkIACgkQFq+eroFS7Pv7gQEAlILqnlWcwtZAtK/9jWGbtZVo
Lk009Gc7LxI5RFQR8v8A/RJokGaXIfwmmbbVHyotU16CQOOiUumqKMOaKeVYnRPl
=2XAU
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Y9IeRbaEfX1V2/ZQ54l6PUaYmWw>