Date: Thu, 27 Nov 2014 12:14:06 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Eygene Ryabinkin <rea@freebsd.org> 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: <20141127101405.GP17068@kib.kiev.ua> In-Reply-To: <t6N4RkaeaRObVTAlzRpdhmmcDSg@yxQcZffeD9XmruHr2cPBouRqdXo> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141127101405.GP17068>