Re: signal to process or posix thread
On Wed, Jul 11, 2018 at 03:40:23PM +0300, Paul Irofti wrote:
> > This is my original diff with some twaeks from visa@.
>
> While I think this is a step in the right direction I don't think is the
> proper solution to the problem.
It is not intended as final solution. My problem is that posixtestsuite
does not finish during regress. Signals are not processes resulting
in hangs. Then it is aborted by an overall timeout.
> I don't think this handles pthread_kill() signals and I agree with mpi@
> that the path should be split between thread signals and process signals.
The NetBSD approach seem correct. We need a pending signal list
per thread and per process. This is just a bunch of work.
> I will try to come up with something better in the following days.
> In the meantime if you guys want to commit this bit, I will not
> object.
I am happy that pirofti@ jumps in for the correct fix. I would
prefer if I could get oks for my version now. Then we would finish
posixtestsuite during regress in time and see daily results here
again.
http://bluhm.genua.de/testsuite/posixtestsuite/posixtestsuite.html
This would also help to visualize the effects of pirofti@'s fix.
bluhm
> > Index: kern/kern_sig.c
> > ===
> > RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_sig.c,v
> > retrieving revision 1.220
> > diff -u -p -r1.220 kern_sig.c
> > --- kern/kern_sig.c 28 Apr 2018 03:13:04 - 1.220
> > +++ kern/kern_sig.c 9 Jul 2018 20:36:07 -
> > @@ -1155,14 +1155,17 @@ issignal(struct proc *p)
> > int s;
> >
> > for (;;) {
> > - mask = p->p_siglist & ~p->p_sigmask;
> > + mask = SIGPENDING(p);
> > if (pr->ps_flags & PS_PPWAIT)
> > mask &= ~stopsigmask;
> > if (mask == 0) /* no signal to send */
> > return (0);
> > signum = ffs((long)mask);
> > mask = sigmask(signum);
> > - atomic_clearbits_int(&p->p_siglist, mask);
> > + if (p->p_siglist & mask)
> > + atomic_clearbits_int(&p->p_siglist, mask);
> > + else
> > + atomic_clearbits_int(&pr->ps_mainproc->p_siglist, mask);
> >
> > /*
> > * We should see pending but ignored signals
> > @@ -1840,7 +1843,7 @@ userret(struct proc *p)
> > KERNEL_UNLOCK();
> > }
> >
> > - if (SIGPENDING(p)) {
> > + if (SIGPENDING(p) != 0) {
> > KERNEL_LOCK();
> > while ((signum = CURSIG(p)) != 0)
> > postsig(p, signum);
> > Index: sys/signalvar.h
> > ===
> > RCS file: /data/mirror/openbsd/cvs/src/sys/sys/signalvar.h,v
> > retrieving revision 1.30
> > diff -u -p -r1.30 signalvar.h
> > --- sys/signalvar.h 24 Mar 2018 04:13:59 - 1.30
> > +++ sys/signalvar.h 9 Jul 2018 20:52:50 -
> > @@ -68,7 +68,9 @@ structsigacts {
> > /*
> > * Check if process p has an unmasked signal pending.
> > */
> > -#defineSIGPENDING(p) (((p)->p_siglist & ~(p)->p_sigmask) != 0)
> > +#defineSIGPENDING(p)
> > \
> > + (((p)->p_siglist | (p)->p_p->ps_mainproc->p_siglist) & \
> > + ~(p)->p_sigmask)
> >
> > /*
> > * Determine signal that should be delivered to process p, the current
> > @@ -76,10 +78,9 @@ struct sigacts {
> > * action, the process stops in issignal().
> > */
> > #defineCURSIG(p)
> > \
> > - (((p)->p_siglist == 0 ||\
> > - (((p)->p_p->ps_flags & PS_TRACED) == 0 && \
> > - ((p)->p_siglist & ~(p)->p_sigmask) == 0)) ? \
> > - 0 : issignal(p))
> > + (p)->p_siglist | (p)->p_p->ps_mainproc->p_siglist) == 0) || \
> > + (((p)->p_p->ps_flags & PS_TRACED) == 0 && SIGPENDING(p) == 0)) \
> > + ? 0 : issignal(p))
> >
> > /*
> > * Clear a pending signal from a process.
Re: signal to process or posix thread
On Mon, Jul 09, 2018 at 10:59:54PM +0200, Alexander Bluhm wrote:
> On Mon, Jul 09, 2018 at 11:27:55AM -0900, Philip Guenther wrote:
> > Those signals are handled by the first thread that
> > > doesn't have them masked. In that case, it should be put on the pending
> > > list of the process and any unmasking operation should check the pending
> > > list on whether a signal should be delivered delayed.
> > >
> >
> > Yep.
>
> This is my original diff with some twaeks from visa@.
While I think this is a step in the right direction I don't think is the
proper solution to the problem.
I don't think this handles pthread_kill() signals and I agree with mpi@
that the path should be split between thread signals and process signals.
I am tracking this issue myself for almost two weeks now and while
indeed this diff seems to workaround the posixsuite issue, it does still
manifest in complex scenarios like lang/mono.
I will try to come up with something better in the following days.
In the meantime if you guys want to commit this bit, I will not
object.
> Index: kern/kern_sig.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_sig.c,v
> retrieving revision 1.220
> diff -u -p -r1.220 kern_sig.c
> --- kern/kern_sig.c 28 Apr 2018 03:13:04 - 1.220
> +++ kern/kern_sig.c 9 Jul 2018 20:36:07 -
> @@ -1155,14 +1155,17 @@ issignal(struct proc *p)
> int s;
>
> for (;;) {
> - mask = p->p_siglist & ~p->p_sigmask;
> + mask = SIGPENDING(p);
> if (pr->ps_flags & PS_PPWAIT)
> mask &= ~stopsigmask;
> if (mask == 0) /* no signal to send */
> return (0);
> signum = ffs((long)mask);
> mask = sigmask(signum);
> - atomic_clearbits_int(&p->p_siglist, mask);
> + if (p->p_siglist & mask)
> + atomic_clearbits_int(&p->p_siglist, mask);
> + else
> + atomic_clearbits_int(&pr->ps_mainproc->p_siglist, mask);
>
> /*
>* We should see pending but ignored signals
> @@ -1840,7 +1843,7 @@ userret(struct proc *p)
> KERNEL_UNLOCK();
> }
>
> - if (SIGPENDING(p)) {
> + if (SIGPENDING(p) != 0) {
> KERNEL_LOCK();
> while ((signum = CURSIG(p)) != 0)
> postsig(p, signum);
> Index: sys/signalvar.h
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/sys/signalvar.h,v
> retrieving revision 1.30
> diff -u -p -r1.30 signalvar.h
> --- sys/signalvar.h 24 Mar 2018 04:13:59 - 1.30
> +++ sys/signalvar.h 9 Jul 2018 20:52:50 -
> @@ -68,7 +68,9 @@ struct sigacts {
> /*
> * Check if process p has an unmasked signal pending.
> */
> -#define SIGPENDING(p) (((p)->p_siglist & ~(p)->p_sigmask) != 0)
> +#define SIGPENDING(p)
> \
> + (((p)->p_siglist | (p)->p_p->ps_mainproc->p_siglist) & \
> + ~(p)->p_sigmask)
>
> /*
> * Determine signal that should be delivered to process p, the current
> @@ -76,10 +78,9 @@ struct sigacts {
> * action, the process stops in issignal().
> */
> #define CURSIG(p)
> \
> - (((p)->p_siglist == 0 ||\
> - (((p)->p_p->ps_flags & PS_TRACED) == 0 && \
> - ((p)->p_siglist & ~(p)->p_sigmask) == 0)) ? \
> - 0 : issignal(p))
> + (p)->p_siglist | (p)->p_p->ps_mainproc->p_siglist) == 0) || \
> + (((p)->p_p->ps_flags & PS_TRACED) == 0 && SIGPENDING(p) == 0)) \
> + ? 0 : issignal(p))
>
> /*
> * Clear a pending signal from a process.
Re: signal to process or posix thread
On Mon, Jul 09, 2018 at 11:27:55AM -0900, Philip Guenther wrote:
> Those signals are handled by the first thread that
> > doesn't have them masked. In that case, it should be put on the pending
> > list of the process and any unmasking operation should check the pending
> > list on whether a signal should be delivered delayed.
> >
>
> Yep.
This is my original diff with some twaeks from visa@.
bluhm
Index: kern/kern_sig.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.220
diff -u -p -r1.220 kern_sig.c
--- kern/kern_sig.c 28 Apr 2018 03:13:04 - 1.220
+++ kern/kern_sig.c 9 Jul 2018 20:36:07 -
@@ -1155,14 +1155,17 @@ issignal(struct proc *p)
int s;
for (;;) {
- mask = p->p_siglist & ~p->p_sigmask;
+ mask = SIGPENDING(p);
if (pr->ps_flags & PS_PPWAIT)
mask &= ~stopsigmask;
if (mask == 0) /* no signal to send */
return (0);
signum = ffs((long)mask);
mask = sigmask(signum);
- atomic_clearbits_int(&p->p_siglist, mask);
+ if (p->p_siglist & mask)
+ atomic_clearbits_int(&p->p_siglist, mask);
+ else
+ atomic_clearbits_int(&pr->ps_mainproc->p_siglist, mask);
/*
* We should see pending but ignored signals
@@ -1840,7 +1843,7 @@ userret(struct proc *p)
KERNEL_UNLOCK();
}
- if (SIGPENDING(p)) {
+ if (SIGPENDING(p) != 0) {
KERNEL_LOCK();
while ((signum = CURSIG(p)) != 0)
postsig(p, signum);
Index: sys/signalvar.h
===
RCS file: /data/mirror/openbsd/cvs/src/sys/sys/signalvar.h,v
retrieving revision 1.30
diff -u -p -r1.30 signalvar.h
--- sys/signalvar.h 24 Mar 2018 04:13:59 - 1.30
+++ sys/signalvar.h 9 Jul 2018 20:52:50 -
@@ -68,7 +68,9 @@ structsigacts {
/*
* Check if process p has an unmasked signal pending.
*/
-#defineSIGPENDING(p) (((p)->p_siglist & ~(p)->p_sigmask) != 0)
+#defineSIGPENDING(p)
\
+ (((p)->p_siglist | (p)->p_p->ps_mainproc->p_siglist) & \
+ ~(p)->p_sigmask)
/*
* Determine signal that should be delivered to process p, the current
@@ -76,10 +78,9 @@ struct sigacts {
* action, the process stops in issignal().
*/
#defineCURSIG(p)
\
- (((p)->p_siglist == 0 ||\
- (((p)->p_p->ps_flags & PS_TRACED) == 0 && \
- ((p)->p_siglist & ~(p)->p_sigmask) == 0)) ? \
- 0 : issignal(p))
+ (p)->p_siglist | (p)->p_p->ps_mainproc->p_siglist) == 0) || \
+ (((p)->p_p->ps_flags & PS_TRACED) == 0 && SIGPENDING(p) == 0)) \
+ ? 0 : issignal(p))
/*
* Clear a pending signal from a process.
Re: signal to process or posix thread
On Sun, Jul 1, 2018 at 9:47 AM Joerg Sonnenberger wrote: > On Fri, Jun 29, 2018 at 04:21:17PM +0200, Alexander Bluhm wrote: > > The problem is that POSIX has signals that are sent to processes > > and signals sent to individual threads. Our kernel does not support > > this properly. > > Well, not exactly. POSIX has synchronous and asynchronous signals. I.e. > SIGFPE after a division by zero or SIGBUS/SIGSEGV are typically > synchronous traps thrown by the processing of the instructions of the > current thread. This signals are delivered to the current thread. All > other signals, i.e. those created as side effect of kill(2) are sent to > the process at large. I think Bluhm's wording is more true to POSIX, as there are other methods for a signal to be delivered to a specific thread. To quote POSIX XSH 2.4.1p2: At the time of generation, a determination shall be made whether the signal has been generated for the process or for a specific thread within the process. Signals which are generated by some action attributable to a particular thread, such as a hardware fault, shall be generated for the thread that caused the signal to be generated. Signals that are generated in association with a process ID or process group ID or an asynchronous event, such as terminal activity, shall be generated for the process. Later, it specifies that pthread_kill() generates the signal for the specific thread: The pthread_kill() function shall request that a signal be delivered to the specified thread. Those signals are handled by the first thread that > doesn't have them masked. In that case, it should be put on the pending > list of the process and any unmasking operation should check the pending > list on whether a signal should be delivered delayed. > Yep. Philip Guenther
Re: signal to process or posix thread
On Fri, Jun 29, 2018 at 04:21:17PM +0200, Alexander Bluhm wrote: > The problem is that POSIX has signals that are sent to processes > and signals sent to individual threads. Our kernel does not support > this properly. Well, not exactly. POSIX has synchronous and asynchronous signals. I.e. SIGFPE after a division by zero or SIGBUS/SIGSEGV are typically synchronous traps thrown by the processing of the instructions of the current thread. This signals are delivered to the current thread. All other signals, i.e. those created as side effect of kill(2) are sent to the process at large. Those signals are handled by the first thread that doesn't have them masked. In that case, it should be put on the pending list of the process and any unmasking operation should check the pending list on whether a signal should be delivered delayed. Joerg
Re: signal to process or posix thread
On 29/06/18(Fri) 16:21, Alexander Bluhm wrote: > On Thu, Jun 28, 2018 at 01:54:29PM +0200, Martin Pieuchot wrote: > > > It may happen that the worker thread is in the signal handler and > > > also blocks the signals. > > > > Are you saying that the worker thread modified its mask itself, via > > a syscall, or that the kernel changed `p_sigmask'? > > Unless SA_NODEFER is set, the kernel masks signals while the handler > is running. This is done here: > > if ((sa->sa_flags & SA_NODEFER) == 0) > sa->sa_mask |= sigmask(signum); > ps->ps_catchmask[signum] = sa->sa_mask &~ sigcantmask; > ... > atomic_setbits_int(&p->p_sigmask, ps->ps_catchmask[signum]); > > > > Then all threads block them and ptsignal() > > > sends it to the main thread. In the test program the main thread > > > blocks them forever and the process gets stuck. > > > > Can you point us to the piece of code containing this logic? Is it: > > > > * [...]. Otherwise, mark it pending on the > > * main thread. > > Yes. All threads mask the signal so it ends pending at the main > thread. prsignal() calls ptsignal(ps_mainproc), so this thread is > used if the TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) loop finds > no match. > > > > According to POSIX any thread should process the signal when it > > > unblocks. > > > > Does that mean we should rather mark the signal pending on the thread > > will unblock it? Do we have a way to know that it will unblock it (see > > my first question)? > > The problem is that POSIX has signals that are sent to processes > and signals sent to individual threads. Our kernel does not support > this properly. > > We can improve the heuristic. But still we don't know that the > thread in the signal handler will unblock the signal first. It is > the case for the programs in posixtestsuite, so your suggestion > could work. You mentioned NetBSD in your previous email, did you look at how they solve this problem? > But I think it is less general than my solution. Also > I don't know how to figure out whether a thread is currently > processing a signal handler. And still this does not mean it will > finish and unblock. Here's an idea: We need another field to save which signals are masked because the thread is currently executing a handler. Then when a signal is masked, before looking for a sibling to deliver it, we check if it is masked because of a signal handler. If that's the case we know it can be delivered soon. > My solution has the drawback that signals sent to the main thread > may be handled like signals sent to the process. I'm afraid that your solution goes in the wrong direction. I think we have to improve the heuristic. We have to untangle per-thread and per-process bits.
Re: signal to process or posix thread
On Thu, Jun 28, 2018 at 01:54:29PM +0200, Martin Pieuchot wrote: > > It may happen that the worker thread is in the signal handler and > > also blocks the signals. > > Are you saying that the worker thread modified its mask itself, via > a syscall, or that the kernel changed `p_sigmask'? Unless SA_NODEFER is set, the kernel masks signals while the handler is running. This is done here: if ((sa->sa_flags & SA_NODEFER) == 0) sa->sa_mask |= sigmask(signum); ps->ps_catchmask[signum] = sa->sa_mask &~ sigcantmask; ... atomic_setbits_int(&p->p_sigmask, ps->ps_catchmask[signum]); > > Then all threads block them and ptsignal() > > sends it to the main thread. In the test program the main thread > > blocks them forever and the process gets stuck. > > Can you point us to the piece of code containing this logic? Is it: > > * [...]. Otherwise, mark it pending on the > * main thread. Yes. All threads mask the signal so it ends pending at the main thread. prsignal() calls ptsignal(ps_mainproc), so this thread is used if the TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) loop finds no match. > > According to POSIX any thread should process the signal when it > > unblocks. > > Does that mean we should rather mark the signal pending on the thread > will unblock it? Do we have a way to know that it will unblock it (see > my first question)? The problem is that POSIX has signals that are sent to processes and signals sent to individual threads. Our kernel does not support this properly. We can improve the heuristic. But still we don't know that the thread in the signal handler will unblock the signal first. It is the case for the programs in posixtestsuite, so your suggestion could work. But I think it is less general than my solution. Also I don't know how to figure out whether a thread is currently processing a signal handler. And still this does not mean it will finish and unblock. My solution has the drawback that signals sent to the main thread may be handled like signals sent to the process. bluhm
Re: signal to process or posix thread
On 22/06/18(Fri) 22:37, Alexander Bluhm wrote: > Hi, > > Since the recent futex(2) changes the posixtestsuite regress does > not finish within the given time frame. Depending on some races > tests hang, e.g. this one: > > /usr/local/libexec/posixtestsuite/conformance/interfaces/pthread_atfork/3-3.test > > It spans a worker thread that allows to handle SIGUSR1 or SIGUSR2. > Two signal sending threads are created that kill the process with > SIGUSR1 or SIGUSR2 repectively. The main thread and the signal > sending threads block these signals. The signal handler in the > worker thread uses semaphores to acknowledge signals and then the > sending threads kill again. > > It may happen that the worker thread is in the signal handler and > also blocks the signals. Are you saying that the worker thread modified its mask itself, via a syscall, or that the kernel changed `p_sigmask'? > Then all threads block them and ptsignal() > sends it to the main thread. In the test program the main thread > blocks them forever and the process gets stuck. Can you point us to the piece of code containing this logic? Is it: * [...]. Otherwise, mark it pending on the * main thread. > According to POSIX any thread should process the signal when it > unblocks. Does that mean we should rather mark the signal pending on the thread will unblock it? Do we have a way to know that it will unblock it (see my first question)?
