On Sat, Jan 25, 2003 at 05:25:25PM +0100, Jaromir Dolecek wrote:

 > it occurred to me sigwait(2) could be useful now that we have threads.
 > So I implemented one, patch is appended to this e-mail[*]
 > It should work according to how sigwait(2) is specified in SUSv3.
 > There are two potential issues tho.

A couple of comments:

	* Please implement it in terms of sigwaitinfo(), i.e.
	  do not use a second system call entry point.

	* For a threaded program, sigwait()/sigwaitinfo() need to
	  be implemented entirely within libpthread, much like the
	  rest of signal delivery is implemented entirely within
	  libpthread for threaded programs.

 > Second one is potentially more dangerous. If thread sigwaits for
 > more than one signal and more than one signal actually occurs before
 > the thread has a chance to pick them up (say, SIGSEGV and SIGBUS,
 > from interrupt context), only one of the signals would get returned
 > via sigwait(). The other signal(s) would stay on the ps_sigwaited
 > list, and not acted upon by normal signal delivery, as they are
 > supposed to. If the process would call sigwait(2) again with same
 > args, it would pick up the 'other' signal(s)s from ps_sigwaited list, so they
 > eventually _would_ be delivered.  I wonder how big is this a problem.

Err, when the sigwait()/sigwaitinfo() call returns, you should simply
clear the bits from the list.

Do not attempt to put any multi-threaded awareness into the kernel's
signal delivery mechanism -- that is all for single-threaded processes;
the thread library deals with the complexity of signals for multi-threaded
applications.

 > Also, current implementation lacks any locks. This should
 > work as far as our high kernel is biglock+nonpreemptive.

Well, just like the rest of the signal code.

..and now on to specifics of the code...

 > Opinions?
 > 
 > Jaromir
 > 
 >  [*] The syscall glue in syscall tables and libc is not included in patch.

 > Index: kern/kern_sig.c
 > ===================================================================
 > RCS file: /cvsroot/src/sys/kern/kern_sig.c,v
 > retrieving revision 1.130
 > diff -u -p -r1.130 kern_sig.c
 > --- kern/kern_sig.c	2003/01/18 10:06:29	1.130
 > +++ kern/kern_sig.c	2003/01/25 13:41:50
 > @@ -356,6 +356,8 @@ siginit(struct proc *p)
 >  		SIGACTION_PS(ps, signum).sa_flags = SA_RESTART;
 >  	}
 >  	sigemptyset(&p->p_sigctx.ps_sigcatch);
 > +	sigemptyset(&p->p_sigctx.ps_sigwait);
 > +	sigemptyset(&p->p_sigctx.ps_sigwaited);
 >  	p->p_flag &= ~P_NOCLDSTOP;
 >  
 >  	/*
 > @@ -402,6 +404,8 @@ execsigs(struct proc *p)
 >  		SIGACTION_PS(ps, signum).sa_flags = SA_RESTART;
 >  	}
 >  	sigemptyset(&p->p_sigctx.ps_sigcatch);
 > +	sigemptyset(&p->p_sigctx.ps_sigwait);
 > +	sigemptyset(&p->p_sigctx.ps_sigwaited);
 >  	p->p_flag &= ~P_NOCLDSTOP;
 >  
 >  	/*
 > @@ -856,6 +860,22 @@ psignal1(struct proc *p, int signum,
 >  	p->p_sigctx.ps_sigcheck = 1;
 >  
 >  	/*
 > +	 * If the signal doesn't have SA_CANTMASK (no override for SIGKILL,
 > +	 * please!), check if anything waits on it. If yes, clear the
 > +	 * pending signal from siglist set, add it to sigwaited set, and
 > +	 * wakeup any sigwaiters. The signal won't be processed further
 > +	 * here.
 > +	 */
 > +	if ((prop & SA_CANTMASK) == 0
 > +	    && sigismember(&p->p_sigctx.ps_sigwait, signum)) {
 > +		sigdelset(&p->p_sigctx.ps_siglist, signum);
 > +		sigaddset(&p->p_sigctx.ps_sigwaited, signum);
 > +
 > +		wakeup(&p->p_sigctx.ps_sigwait);

You need to handle the case where the sched_lock might be held, here:

		if (dolock)
			wakeup(&p->p_sigctx.ps_sigwait);
		else
			sched_wakeup(&p->p_sigctx.ps_sigwait);

I know it's icky, but we currently have to do this little dance.

 > +		return;
 > +	}
 > +
 > +	/*
 >  	 * Defer further processing for signals which are held,
 >  	 * except that stopped processes must be continued by SIGCONT.
 >  	 */
 > @@ -1032,7 +1052,8 @@ psignal1(struct proc *p, int signum,
 >  			goto out;
 >  		} else {
 >  			/* Else what? */
 > -			panic("psignal: Invalid process state.");
 > +			panic("psignal: Invalid process state %d.",
 > +				p->p_stat);
 >  		}
 >  	}
 >  	/*NOTREACHED*/
 > @@ -1818,6 +1839,79 @@ sys_setcontext(struct lwp *l, void *v, r
 >  	return (EJUSTRETURN);
 >  }
 >  
 > +int
 > +sys_sigwait(struct lwp *l, void *v, register_t *retval)
 > +{
 > +	struct sys_sigwait_args /* {
 > +		syscallarg(const sigset_t *) set;
 > +		syscallarg(int *) signum;
 > +	} */ *uap = v;
 > +	sigset_t waitset;
 > +	struct proc *p = l->l_proc;
 > +	int error;
 > +	int sig, signum = 0;
 > +
 > +	sigemptyset(&waitset);
 > +	if ((error = copyin(SCARG(uap, set), &waitset, sizeof(waitset))))
 > +		return (error);
 > +
 > +	/*
 > +	 * First scan siglist and check if there already is signal from
 > +	 * our waitlist pending. Ignore CANTMASK signals.
 > +	 */
 > +	for(sig=1; sig < _NSIG; sig++) {
 > +		if ((sigprop[sig] & SA_CANTMASK) == 0
 > +		    && sigismember(&waitset, sig)
 > +		    && sigismember(&p->p_sigctx.ps_siglist, sig)) {
 > +			/* found pending signal */
 > +			signum = sig;
 > +			sigdelset(&p->p_sigctx.ps_siglist, sig);
 > +			goto out;

I think you want to find a more efficient way to do this.

 > +		}
 > +	}
 > +
 > +	/*
 > +	 * Add to ps_sigwait list according to our needs.
 > +	 * XXXSMP this is only safe with BIG LOCK nonpreemptive kernel
 > +	 */
 > +	sigplusset(&waitset, &p->p_sigctx.ps_sigwait);

Go ahead and remove the XXXSMP comment there -- it is known that all
of the signal stuff only works with big-lock right now.

 > +		
 > +	/*
 > +	 * Loop until we'd either:
 > +	 * 1. found the signal we want
 > +	 * 2. would be interrupted by signal ourselves
 > +	 */
 > +	for(; error == 0;) {
 > +		/*
 > +		 * Check if a signal from our wait set has arrived.
 > +		 */
 > +		for(sig=1; sig < _NSIG; sig++) {
 > +			if (sigismember(&waitset, sig)
 > +			    && sigismember(&p->p_sigctx.ps_sigwaited, sig)) {
 > +				/* signal from our wait set has been posted */
 > +				signum = sig;
 > +
 > +				sigdelset(&p->p_sigctx.ps_sigwaited, sig);
 > +				goto outset;
 > +			}
 > +		}
 > +
 > +		/* wake us in better times */
 > +		error = tsleep(&p->p_sigctx.ps_sigwait, PUSER|PCATCH,
 > +			"sigwait", 0);

I think you want to use PPAUSE|PCATCH; see sigsuspend1().

 > +	}
 > +
 > +    outset:
 > +	/* remove our set from waited list */
 > +	sigminusset(&waitset, &p->p_sigctx.ps_sigwait);

Again, I think you simply want to sigemptyset the wait list.  A multi-threaded
program is going to handle all of this in the thread library.

 > +
 > +    out:
 > +	/* If a signal from our wait set arrived, copy it to userland. */
 > +	if (signum > 0)
 > +		error = copyout(&signum, SCARG(uap, signum), sizeof(signum));
 > +
 > +	return (error);
 > +}
 >  
 >  /*
 >   * Returns true if signal is ignored or masked for passed process.
 > Index: sys/signalvar.h
 > ===================================================================
 > RCS file: /cvsroot/src/sys/sys/signalvar.h,v
 > retrieving revision 1.37
 > diff -u -p -r1.37 signalvar.h
 > --- sys/signalvar.h	2003/01/18 09:53:20	1.37
 > +++ sys/signalvar.h	2003/01/25 13:41:50
 > @@ -75,6 +75,8 @@ struct	sigctx {
 >  	sigset_t ps_sigmask;		/* Current signal mask. */
 >  	sigset_t ps_sigignore;		/* Signals being ignored. */
 >  	sigset_t ps_sigcatch;		/* Signals being caught by user. */
 > +	sigset_t ps_sigwait;		/* Signals being waited for */
 > +	sigset_t ps_sigwaited;		/* Delivered signals from wait set */
 >  };
 >  
 >  /* signal flags */

It would also be nice if you wrote a regression test for this.

Please post an updated patch :-)

-- 
        -- Jason R. Thorpe <thorpej@wasabisystems.com>