From owner-svn-src-all@FreeBSD.ORG Wed Apr 17 11:00:29 2013 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id C820C6C6; Wed, 17 Apr 2013 11:00:29 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) by mx1.freebsd.org (Postfix) with ESMTP id 760EAC35; Wed, 17 Apr 2013 11:00:29 +0000 (UTC) Received: from ralph.baldwin.cx (c-68-39-198-164.hsd1.de.comcast.net [68.39.198.164]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id B5809B9B9; Wed, 17 Apr 2013 07:00:28 -0400 (EDT) From: John Baldwin To: Jilles Tjoelker Subject: Re: svn commit: r249566 - in head: lib/libc/gen sys/kern Date: Wed, 17 Apr 2013 06:59:58 -0400 User-Agent: KMail/1.13.7 (FreeBSD/9.1-STABLE; KDE/4.8.4; amd64; ; ) References: <201304162026.r3GKQVgs093874@svn.freebsd.org> <20130416211503.GA1025@stack.nl> In-Reply-To: <20130416211503.GA1025@stack.nl> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201304170659.58569.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Wed, 17 Apr 2013 07:00:28 -0400 (EDT) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Apr 2013 11:00:29 -0000 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