[CFR][PATCH] Call proper signal handler from libthr for non-SA_SIGINFO case

[Eygene Ryabinkin]

Fri, Nov 28, 2014 at 12:13:12PM +0200, Konstantin Belousov wrote:
> On Thu, Nov 27, 2014 at 08:14:09PM +0300, Eygene Ryabinkin wrote:
> > 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).

kern_sigprocmask() will.  But calling something with such set of
flags (that guarantee that both locks are taken), but not checking
for one of the locks that it is really so it a bit inconsistent.

> > 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.

Got it, thanks!

> > 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(). 

Since reschedule_signals() now unconditionally needs PROC_LOCK to be
owned, it doesn't care for at least SIGPROCMASK_PROC_LOCKED bit in
flags.  And, for example, if you'll pass the flags to any routine
called from reschedule_signals() that checks for
SIGPROCMASK_PROC_LOCKED and acquires the lock when this bit is
deasserted, it will loudly (as you explained above) fail trying to
take non-recursive lock for the case when original flags passed to
reschedule_signals() have no SIGPROCMASK_PROC_LOCKED bit enabled.
-- 
Eygene Ryabinkin                                        ,,,^..^,,,
[ Life's unfair - but root password helps!           | codelabs.ru ]
[ 82FE 06BC D497 C0DE 49EC  4FF0 16AF 9EAE 8152 ECFB | freebsd.org ]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 343 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-threads/attachments/20141128/881f8e3a/attachment.sig>