Date: Fri, 28 Nov 2014 12:13:12 +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: <20141128101312.GT17068@kib.kiev.ua> In-Reply-To: <Y9IeRbaEfX1V2/ZQ54l6PUaYmWw@1d%2BaJAniZP50FCDdGj54nd51%2Bks> References: <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> <Y9IeRbaEfX1V2/ZQ54l6PUaYmWw@1d%2BaJAniZP50FCDdGj54nd51%2Bks>
next in thread | previous in thread | raw e-mail | index | archive | help
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().
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141128101312.GT17068>