Date: Fri, 30 Oct 2009 06:12:11 +0200 From: Giorgos Keramidas <keramida@freebsd.org> To: Kostik Belousov <kostikbel@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r198590 - head/sys/kern Message-ID: <87d445a5b8.fsf@kobe.laptop> In-Reply-To: <20091030032240.GD2147@deviant.kiev.zoral.com.ua> (Kostik Belousov's message of "Fri, 30 Oct 2009 05:22:40 %2B0200") References: <200910291434.n9TEYOVJ099388@svn.freebsd.org> <871vklbxyf.fsf@kobe.laptop> <20091030032240.GD2147@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
--=-=-=
> No, this was caused by the r198507 fragment of postsig(), as well as a
> fragment from the trapsignal(), that added the call to kern_sigprocmask()
> instead of direct manipulation of thread signal mask.
>
>> AFAICT, postsig() is called with proc->p_sigacts->ps_mtx locked, so when
>> we are recursing when reschedule_signals() tries to lock it once more.
>
> Yes, the same statement holds for trapsignal().
>
>> Since we are holding the proc lock in kern_sigprocmask(), is it safe to
>> assert that we own ps_mtx, drop it and re-acquire it immediately after
>> calling reschedule_signals()?
>
> No. Most callers of kern_sigprocmask() do not hold curproc->ps_mtx.
> Only postsig() and trapsig() call kern_sigprocmask() with ps_mtx locked,
> so I have to special-case them.
>
> Could you, please, test the following patch ? What application did
> exposed the issue ?
Logging out of GNOME consistently triggers this. The thread that is
active in kgdb is gnome-session:
(kgdb) info threads
...
138 Thread 100142 (PID=2314: gnome-session) doadump () at pcpu.h:246
137 Thread 100100 (PID=2314: gnome-session/initial thread)
sched_switch (td=0xc6a86230, newtd=0xc6564230, flags=1538)
at /usr/src/sys/kern/sched_ule.c:1864
...
I'll try the patch in a few hours and report back. Thanks! :)
> diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
> index 7f5cfa3..e174df1 100644
> --- a/sys/kern/kern_sig.c
> +++ b/sys/kern/kern_sig.c
> @@ -220,7 +220,7 @@ static int sigproptbl[NSIG] = {
> SA_KILL|SA_PROC, /* SIGUSR2 */
> };
>
> -static void reschedule_signals(struct proc *p, sigset_t block);
> +static void reschedule_signals(struct proc *p, sigset_t block, int flags);
>
> static void
> sigqueue_start(void)
> @@ -1024,7 +1024,7 @@ kern_sigprocmask(struct thread *td, int how, sigset_t *set, sigset_t *oset,
> * possibly waking it up.
> */
> if (p->p_numthreads != 1)
> - reschedule_signals(p, new_block);
> + reschedule_signals(p, new_block, flags);
>
> if (!(flags & SIGPROCMASK_PROC_LOCKED))
> PROC_UNLOCK(p);
> @@ -1859,13 +1859,11 @@ trapsignal(struct thread *td, ksiginfo_t *ksi)
> #endif
> (*p->p_sysent->sv_sendsig)(ps->ps_sigact[_SIG_IDX(sig)],
> ksi, &td->td_sigmask);
> - SIGSETOR(td->td_sigmask, ps->ps_catchmask[_SIG_IDX(sig)]);
> - if (!SIGISMEMBER(ps->ps_signodefer, sig)) {
> - SIGEMPTYSET(mask);
> + mask = ps->ps_catchmask[_SIG_IDX(sig)];
> + if (!SIGISMEMBER(ps->ps_signodefer, sig))
> SIGADDSET(mask, sig);
> - kern_sigprocmask(td, SIG_BLOCK, &mask, NULL,
> - SIGPROCMASK_PROC_LOCKED);
> - }
> + kern_sigprocmask(td, SIG_BLOCK, &mask, NULL,
> + SIGPROCMASK_PROC_LOCKED | SIGPROCMASK_PS_LOCKED);
> if (SIGISMEMBER(ps->ps_sigreset, sig)) {
> /*
> * See kern_sigaction() for origin of this code.
> @@ -2401,7 +2399,7 @@ stopme:
> }
>
> static void
> -reschedule_signals(struct proc *p, sigset_t block)
> +reschedule_signals(struct proc *p, sigset_t block, int flags)
> {
> struct sigacts *ps;
> struct thread *td;
> @@ -2419,12 +2417,14 @@ reschedule_signals(struct proc *p, sigset_t block)
>
> td = sigtd(p, i, 0);
> signotify(td);
> - mtx_lock(&ps->ps_mtx);
> + if (!(flags & SIGPROCMASK_PS_LOCKED))
> + mtx_lock(&ps->ps_mtx);
> if (p->p_flag & P_TRACED || SIGISMEMBER(ps->ps_sigcatch, i))
> tdsigwakeup(td, i, SIG_CATCH,
> (SIGISMEMBER(ps->ps_sigintr, i) ? EINTR :
> ERESTART));
> - mtx_unlock(&ps->ps_mtx);
> + if (!(flags & SIGPROCMASK_PS_LOCKED))
> + mtx_unlock(&ps->ps_mtx);
> }
> }
>
> @@ -2452,7 +2452,7 @@ tdsigcleanup(struct thread *td)
> SIGFILLSET(unblocked);
> SIGSETNAND(unblocked, td->td_sigmask);
> SIGFILLSET(td->td_sigmask);
> - reschedule_signals(p, unblocked);
> + reschedule_signals(p, unblocked, 0);
>
> }
>
> @@ -2734,15 +2734,11 @@ postsig(sig)
> } else
> returnmask = td->td_sigmask;
>
> - kern_sigprocmask(td, SIG_BLOCK,
> - &ps->ps_catchmask[_SIG_IDX(sig)], NULL,
> - SIGPROCMASK_PROC_LOCKED);
> - if (!SIGISMEMBER(ps->ps_signodefer, sig)) {
> - SIGEMPTYSET(mask);
> + mask = ps->ps_catchmask[_SIG_IDX(sig)];
> + if (!SIGISMEMBER(ps->ps_signodefer, sig))
> SIGADDSET(mask, sig);
> - kern_sigprocmask(td, SIG_BLOCK, &mask, NULL,
> - SIGPROCMASK_PROC_LOCKED);
> - }
> + kern_sigprocmask(td, SIG_BLOCK, &mask, NULL,
> + SIGPROCMASK_PROC_LOCKED | SIGPROCMASK_PS_LOCKED);
>
> if (SIGISMEMBER(ps->ps_sigreset, sig)) {
> /*
> diff --git a/sys/sys/signalvar.h b/sys/sys/signalvar.h
> index b9a54f0..c27a128 100644
> --- a/sys/sys/signalvar.h
> +++ b/sys/sys/signalvar.h
> @@ -319,6 +319,7 @@ extern int kern_logsigexit; /* Sysctl variable kern.logsigexit */
> /* flags for kern_sigprocmask */
> #define SIGPROCMASK_OLD 0x0001
> #define SIGPROCMASK_PROC_LOCKED 0x0002
> +#define SIGPROCMASK_PS_LOCKED 0x0004
>
> /*
> * Machine-independent functions:
--=-=-=
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.11 (FreeBSD)
iEYEARECAAYFAkrqZ6IACgkQ1g+UGjGGA7Yt8ACfRNfjWFWhjV4Ntic7dAXOZh5D
7CgAn2r+bLuCSbWm/sOugWfUcZ4DzST2
=gTB4
-----END PGP SIGNATURE-----
--=-=-=--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?87d445a5b8.fsf>