Date: Tue, 16 Apr 2013 23:15:03 +0200 From: Jilles Tjoelker <jilles@stack.nl> To: John Baldwin <jhb@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r249566 - in head: lib/libc/gen sys/kern Message-ID: <20130416211503.GA1025@stack.nl> In-Reply-To: <201304162026.r3GKQVgs093874@svn.freebsd.org> References: <201304162026.r3GKQVgs093874@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. -- Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130416211503.GA1025>