svn commit: r249566 - in head: lib/libc/gen sys/kern

[John Baldwin]

On Tuesday, April 16, 2013 05:15:03 PM Jilles Tjoelker wrote:
> On Tue, Apr 16, 2013 at 08:26:31PM +0000, John Baldwin wrote:
> > Author: jhb
> > Date: Tue Apr 16 20:26:31 2013
> > New Revision: 249566
> > URL: http://svnweb.freebsd.org/changeset/base/249566
> > 
> > Log:
> >   - Document that sem_wait() can fail with EINTR if it is interrupted by
> >   a
> >   
> >     signal.
> >   
> >   - Fix the old ksem implementation for POSIX semaphores to not restart
> >   
> >     sem_wait() or sem_timedwait() if interrupted by a signal.
> >   
> >   MFC after:	1 week
> > 
> > Modified:
> >   head/lib/libc/gen/sem_wait.3
> >   head/sys/kern/uipc_sem.c
> > 
> > Modified: head/lib/libc/gen/sem_wait.3
> > =========================================================================
> > ===== --- head/lib/libc/gen/sem_wait.3	Tue Apr 16 20:21:02 2013	
(r249565)
> > +++ head/lib/libc/gen/sem_wait.3	Tue Apr 16 20:26:31 2013	(r249566) 
@@
> > -27,7 +27,7 @@
> > 
> >  .\"
> >  .\" $FreeBSD$
> >  .\"
> > 
> > -.Dd February 15, 2000
> > +.Dd April 16, 2013
> > 
> >  .Dt SEM_WAIT 3
> >  .Os
> >  .Sh NAME
> > 
> > @@ -75,6 +75,14 @@ points to an invalid semaphore.
> > 
> >  .El
> >  .Pp
> >  Additionally,
> > 
> > +.Fn sem_wait
> > +will fail if:
> > +.Bl -tag -width Er
> > +.Pp
> > +.It Bq Er EINTR
> > +A signal interrupted this function.
> > +.El
> > +Additionally,
> > 
> >  .Fn sem_trywait
> >  will fail if:
> >  .Bl -tag -width Er
> > 
> > Modified: head/sys/kern/uipc_sem.c
> > =========================================================================
> > ===== --- head/sys/kern/uipc_sem.c	Tue Apr 16 20:21:02 2013	(r249565)
> > +++ head/sys/kern/uipc_sem.c	Tue Apr 16 20:26:31 2013	(r249566)
> > @@ -846,6 +846,8 @@ kern_sem_wait(struct thread *td, semid_t
> > 
> >  err:
> >  	mtx_unlock(&sem_lock);
> >  	fdrop(fp, td);
> > 
> > +	if (error == ERESTART)
> > +		error = EINTR;
> > 
> >  	DP(("<<< kern_sem_wait leaving, pid=%d, error = %d\n",
> >  	
> >  	    (int)td->td_proc->p_pid, error));
> >  	
> >  	return (error);
> 
> It would normally be expected (and required by POSIX) that a signal with
> SA_RESTART set does not cause a function to fail with [EINTR], so more
> rationale is needed here.
> 
> Also, the timeouts passed to these functions are absolute and also are
> at the system call level except UMTX_OP_SEM_WAIT which can take a
> relative or an absolute timeout but libc passed it an absolute timeout.
> So there is no need for a caller to see [EINTR] to avoid overlong
> timeouts.
> 
> Given the number of times people call sem_wait() without checking the
> return value, call sem_wait() and assume any error is fatal or call
> sem_timedwait() and assume any error is a timeout, I think it may be
> better to go the other way and never fail with [EINTR], just like the
> pthread functions. POSIX explicitly permits this: the [EINTR] error for
> sem_wait(), sem_trywait() and sem_timedwait() is "may fail" rather than
> the usual "shall fail" (note that SA_RESTART overrides such a "shall
> fail" unless there is a relative timeout or it is explicitly excluded
> with the function).
> 
> I realize that this reduces functionality, but relying on [EINTR] here
> introduces a race condition anyway (because the signal handler may be
> called just before sem_wait() blocks). I suggest pthread cancellation,
> not returning from the signal handler or leaving the default
> action in place (if it is termination). The racy functionality can be
> kept by leaving [EINTR] and [ERESTART] from the sleep function as they
> are except for changing [ERESTART] to [EINTR] when UMTX_OP_SEM_WAIT was
> passed a relative timeout. Breakage in programs that check sem_wait()'s
> return value incorrectly will then be less frequent and only present
> when POSIX permits it.

Hmm, this merely documents how our current semaphores in 9.x and later behave
and adjusts the semaphores in 8.x to behave the same way.  For mutexes it
seems we restart non-timed lock attempts and do not restart timed lock 
attempts in the umtx code (whereas all arbitrary waits in do_wait() are not
restarted).  If you want to fix the current semaphores to not fail with EINTR
that would be fine with me, but our two implementations should be consistent
(as should the manpages: sem_timedwait() documented EINTR but sem_wait() did
not).

-- 
John Baldwin