From b215ceaaec905918e672fd257340e2fd3960e201 Mon Sep 17 00:00:00 2001 From: Eric van Gyzen Date: Thu, 23 Feb 2017 19:36:38 +0000 Subject: [PATCH] Add sem_clockwait_np() This function allows the caller to specify the reference clock and choose between absolute and relative mode. In relative mode, the remaining time can be returned. The API is similar to clock_nanosleep(3). Thanks to Ed Schouten for that suggestion. While I'm here, reduce the sleep time in the semaphore "child" test to greatly reduce its runtime. Also add a reasonable timeout. Reviewed by: ed (userland) MFC after: 2 weeks Relnotes: yes Sponsored by: Dell EMC Differential Revision: https://reviews.freebsd.org/D9656 --- contrib/netbsd-tests/lib/librt/t_sem.c | 149 ++++++++++++++++++++++++- include/semaphore.h | 2 + lib/libc/gen/Makefile.inc | 1 + lib/libc/gen/Symbol.map | 1 + lib/libc/gen/sem_new.c | 53 ++++++--- lib/libc/gen/sem_timedwait.3 | 61 ++++++++-- lib/libc/include/namespace.h | 1 + lib/libc/include/un-namespace.h | 1 + share/man/man3/pthread_testcancel.3 | 7 +- sys/kern/kern_umtx.c | 66 ++++++++--- 10 files changed, 299 insertions(+), 43 deletions(-) diff --git a/contrib/netbsd-tests/lib/librt/t_sem.c b/contrib/netbsd-tests/lib/librt/t_sem.c index 0541ae57113a..4bf379bd1fb1 100644 --- a/contrib/netbsd-tests/lib/librt/t_sem.c +++ b/contrib/netbsd-tests/lib/librt/t_sem.c @@ -60,18 +60,24 @@ __COPYRIGHT("@(#) Copyright (c) 2008, 2010\ The NetBSD Foundation, inc. All rights reserved."); __RCSID("$NetBSD: t_sem.c,v 1.3 2017/01/14 20:58:20 christos Exp $"); +#include #include #include #include #include +#include #include +#include #include #include #define NCHILDREN 10 +#define SEM_REQUIRE(x) \ + ATF_REQUIRE_EQ_MSG(x, 0, "%s", strerror(errno)) + ATF_TC_WITH_CLEANUP(basic); ATF_TC_HEAD(basic, tc) { @@ -118,6 +124,7 @@ ATF_TC_HEAD(child, tc) { atf_tc_set_md_var(tc, "descr", "Checks using semaphores to synchronize " "parent with multiple child processes"); + atf_tc_set_md_var(tc, "timeout", "5"); } ATF_TC_BODY(child, tc) { @@ -153,7 +160,7 @@ ATF_TC_BODY(child, tc) } for (i = 0; i < NCHILDREN; i++) { - sleep(1); + usleep(100000); printf("main loop %d: posting...\n", j); ATF_REQUIRE_EQ(sem_post(sem_a), 0); } @@ -173,11 +180,151 @@ ATF_TC_CLEANUP(child, tc) (void)sem_unlink("/sem_a"); } +static inline void +timespec_add_ms(struct timespec *ts, int ms) +{ + ts->tv_nsec += ms * 1000*1000; + if (ts->tv_nsec > 1000*1000*1000) { + ts->tv_sec++; + ts->tv_nsec -= 1000*1000*1000; + } +} + +volatile sig_atomic_t got_sigalrm = 0; + +static void +sigalrm_handler(int sig __unused) +{ + got_sigalrm = 1; +} + +ATF_TC(timedwait); +ATF_TC_HEAD(timedwait, tc) +{ + atf_tc_set_md_var(tc, "descr", "Tests sem_timedwait(3)" +#ifdef __FreeBSD__ + " and sem_clockwait_np(3)" +#endif + ); + atf_tc_set_md_var(tc, "timeout", "20"); +} +ATF_TC_BODY(timedwait, tc) +{ + struct timespec ts; + sem_t sem; + int result; + + SEM_REQUIRE(sem_init(&sem, 0, 0)); + SEM_REQUIRE(sem_post(&sem)); + ATF_REQUIRE_MSG(clock_gettime(CLOCK_REALTIME, &ts) == 0, + "%s", strerror(errno)); + timespec_add_ms(&ts, 100); + SEM_REQUIRE(sem_timedwait(&sem, &ts)); + ATF_REQUIRE_ERRNO(ETIMEDOUT, sem_timedwait(&sem, &ts)); + ts.tv_sec--; + ATF_REQUIRE_ERRNO(ETIMEDOUT, sem_timedwait(&sem, &ts)); + SEM_REQUIRE(sem_post(&sem)); + SEM_REQUIRE(sem_timedwait(&sem, &ts)); + + /* timespec validation, in the past */ + ts.tv_nsec += 1000*1000*1000; + ATF_REQUIRE_ERRNO(EINVAL, sem_timedwait(&sem, &ts)); + ts.tv_nsec = -1; + ATF_REQUIRE_ERRNO(EINVAL, sem_timedwait(&sem, &ts)); + /* timespec validation, in the future */ + ATF_REQUIRE_MSG(clock_gettime(CLOCK_REALTIME, &ts) == 0, + "%s", strerror(errno)); + ts.tv_sec++; + ts.tv_nsec = 1000*1000*1000; + ATF_REQUIRE_ERRNO(EINVAL, sem_timedwait(&sem, &ts)); + ts.tv_nsec = -1; + ATF_REQUIRE_ERRNO(EINVAL, sem_timedwait(&sem, &ts)); + + /* EINTR */ + struct sigaction act = { + .sa_handler = sigalrm_handler, + .sa_flags = 0 /* not SA_RESTART */ + }; + ATF_REQUIRE_MSG(sigemptyset(&act.sa_mask) == 0, + "%s", strerror(errno)); + ATF_REQUIRE_MSG(sigaction(SIGALRM, &act, NULL) == 0, + "%s", strerror(errno)); + struct itimerval it = { + .it_value.tv_usec = 50*1000 + }; + ATF_REQUIRE_MSG(setitimer(ITIMER_REAL, &it, NULL) == 0, + "%s", strerror(errno)); + ATF_REQUIRE_MSG(clock_gettime(CLOCK_REALTIME, &ts) == 0, + "%s", strerror(errno)); + timespec_add_ms(&ts, 100); + ATF_REQUIRE_ERRNO(EINTR, sem_timedwait(&sem, &ts)); + ATF_REQUIRE_MSG(got_sigalrm, "did not get SIGALRM"); + +#ifdef __FreeBSD__ + /* CLOCK_MONOTONIC, absolute */ + SEM_REQUIRE(sem_post(&sem)); + ATF_REQUIRE_MSG(clock_gettime(CLOCK_MONOTONIC, &ts) == 0, + "%s", strerror(errno)); + timespec_add_ms(&ts, 100); + SEM_REQUIRE(sem_clockwait_np(&sem, CLOCK_MONOTONIC, TIMER_ABSTIME, + &ts, NULL)); + ATF_REQUIRE_ERRNO(ETIMEDOUT, + sem_clockwait_np(&sem, CLOCK_MONOTONIC, TIMER_ABSTIME, &ts, NULL)); + + /* CLOCK_MONOTONIC, relative */ + SEM_REQUIRE(sem_post(&sem)); + ts.tv_sec = 0; + ts.tv_nsec = 100*1000*1000; + SEM_REQUIRE(sem_clockwait_np(&sem, CLOCK_MONOTONIC, 0, + &ts, NULL)); + ATF_REQUIRE_ERRNO(ETIMEDOUT, + sem_clockwait_np(&sem, CLOCK_MONOTONIC, 0, &ts, NULL)); + + /* absolute does not update remaining time on EINTR */ + struct timespec remain = {42, 1000*1000*1000}; + got_sigalrm = 0; + it.it_value.tv_usec = 50*1000; + ATF_REQUIRE_MSG(setitimer(ITIMER_REAL, &it, NULL) == 0, + "%s", strerror(errno)); + ATF_REQUIRE_MSG(clock_gettime(CLOCK_MONOTONIC, &ts) == 0, + "%s", strerror(errno)); + timespec_add_ms(&ts, 100); + ATF_REQUIRE_ERRNO(EINTR, sem_clockwait_np(&sem, CLOCK_MONOTONIC, + TIMER_ABSTIME, &ts, &remain)); + ATF_REQUIRE_MSG(got_sigalrm, "did not get SIGALRM"); + ATF_REQUIRE_MSG(remain.tv_sec == 42 && remain.tv_nsec == 1000*1000*1000, + "an absolute clockwait modified the remaining time on EINTR"); + + /* relative updates remaining time on EINTR */ + remain.tv_sec = 42; + remain.tv_nsec = 1000*1000*1000; + got_sigalrm = 0; + it.it_value.tv_usec = 50*1000; + ATF_REQUIRE_MSG(setitimer(ITIMER_REAL, &it, NULL) == 0, + "%s", strerror(errno)); + ts.tv_sec = 0; + ts.tv_nsec = 100*1000*1000; + ATF_REQUIRE_ERRNO(EINTR, sem_clockwait_np(&sem, CLOCK_MONOTONIC, 0, &ts, + &remain)); + ATF_REQUIRE_MSG(got_sigalrm, "did not get SIGALRM"); + /* + * If this nsec comparison turns out to be unreliable due to timing, + * it could simply check that nsec < 100 ms. + */ + ATF_REQUIRE_MSG(remain.tv_sec == 0 && + remain.tv_nsec >= 25*1000*1000 && + remain.tv_nsec <= 75*1000*1000, + "the remaining time was not as expected when a relative clockwait" + " got EINTR" ); +#endif +} + ATF_TP_ADD_TCS(tp) { ATF_TP_ADD_TC(tp, basic); ATF_TP_ADD_TC(tp, child); + ATF_TP_ADD_TC(tp, timedwait); return atf_no_error(); } diff --git a/include/semaphore.h b/include/semaphore.h index c42a8d230ec2..e220fd2c4dad 100644 --- a/include/semaphore.h +++ b/include/semaphore.h @@ -59,6 +59,8 @@ int sem_init(sem_t *, int, unsigned int); sem_t *sem_open(const char *, int, ...); int sem_post(sem_t *); int sem_timedwait(sem_t * __restrict, const struct timespec * __restrict); +int sem_clockwait_np(sem_t * __restrict, __clockid_t, int, + const struct timespec *, struct timespec *); int sem_trywait(sem_t *); int sem_unlink(const char *); int sem_wait(sem_t *); diff --git a/lib/libc/gen/Makefile.inc b/lib/libc/gen/Makefile.inc index 460d09a9c4e2..ac6e077207a8 100644 --- a/lib/libc/gen/Makefile.inc +++ b/lib/libc/gen/Makefile.inc @@ -463,6 +463,7 @@ MLINKS+=scandir.3 alphasort.3 MLINKS+=sem_open.3 sem_close.3 \ sem_open.3 sem_unlink.3 MLINKS+=sem_wait.3 sem_trywait.3 +MLINKS+=sem_timedwait.3 sem_clockwait_np.3 MLINKS+=send.2 sendmmsg.2 MLINKS+=setjmp.3 _longjmp.3 \ setjmp.3 _setjmp.3 \ diff --git a/lib/libc/gen/Symbol.map b/lib/libc/gen/Symbol.map index 4b07a0067be9..0984febf9c70 100644 --- a/lib/libc/gen/Symbol.map +++ b/lib/libc/gen/Symbol.map @@ -415,6 +415,7 @@ FBSD_1.4 { FBSD_1.5 { basename; dirname; + sem_clockwait_np; }; FBSDprivate_1.0 { diff --git a/lib/libc/gen/sem_new.c b/lib/libc/gen/sem_new.c index 8fe027f12449..d36d1e25f7ce 100644 --- a/lib/libc/gen/sem_new.c +++ b/lib/libc/gen/sem_new.c @@ -56,6 +56,7 @@ __weak_reference(_sem_init, sem_init); __weak_reference(_sem_open, sem_open); __weak_reference(_sem_post, sem_post); __weak_reference(_sem_timedwait, sem_timedwait); +__weak_reference(_sem_clockwait_np, sem_clockwait_np); __weak_reference(_sem_trywait, sem_trywait); __weak_reference(_sem_unlink, sem_unlink); __weak_reference(_sem_wait, sem_wait); @@ -345,23 +346,34 @@ usem_wake(struct _usem2 *sem) } static __inline int -usem_wait(struct _usem2 *sem, const struct timespec *abstime) +usem_wait(struct _usem2 *sem, clockid_t clock_id, int flags, + const struct timespec *rqtp, struct timespec *rmtp) { - struct _umtx_time *tm_p, timeout; + struct { + struct _umtx_time timeout; + struct timespec remain; + } tms; + void *tm_p; size_t tm_size; + int retval; - if (abstime == NULL) { + if (rqtp == NULL) { tm_p = NULL; tm_size = 0; } else { - timeout._clockid = CLOCK_REALTIME; - timeout._flags = UMTX_ABSTIME; - timeout._timeout = *abstime; - tm_p = &timeout; - tm_size = sizeof(timeout); + tms.timeout._clockid = clock_id; + tms.timeout._flags = (flags & TIMER_ABSTIME) ? UMTX_ABSTIME : 0; + tms.timeout._timeout = *rqtp; + tm_p = &tms; + tm_size = sizeof(tms); } - return _umtx_op(sem, UMTX_OP_SEM2_WAIT, 0, - (void *)tm_size, __DECONST(void*, tm_p)); + retval = _umtx_op(sem, UMTX_OP_SEM2_WAIT, 0, (void *)tm_size, tm_p); + if (retval == -1 && errno == EINTR && (flags & TIMER_ABSTIME) == 0 && + rqtp != NULL && rmtp != NULL) { + *rmtp = tms.remain; + } + + return (retval); } int @@ -381,8 +393,8 @@ _sem_trywait(sem_t *sem) } int -_sem_timedwait(sem_t * __restrict sem, - const struct timespec * __restrict abstime) +_sem_clockwait_np(sem_t * __restrict sem, clockid_t clock_id, int flags, + const struct timespec *rqtp, struct timespec *rmtp) { int val, retval; @@ -393,7 +405,8 @@ _sem_timedwait(sem_t * __restrict sem, _pthread_testcancel(); for (;;) { while (USEM_COUNT(val = sem->_kern._count) > 0) { - if (atomic_cmpset_acq_int(&sem->_kern._count, val, val - 1)) + if (atomic_cmpset_acq_int(&sem->_kern._count, val, + val - 1)) return (0); } @@ -406,19 +419,27 @@ _sem_timedwait(sem_t * __restrict sem, * The timeout argument is only supposed to * be checked if the thread would have blocked. */ - if (abstime != NULL) { - if (abstime->tv_nsec >= 1000000000 || abstime->tv_nsec < 0) { + if (rqtp != NULL) { + if (rqtp->tv_nsec >= 1000000000 || rqtp->tv_nsec < 0) { errno = EINVAL; return (-1); } } _pthread_cancel_enter(1); - retval = usem_wait(&sem->_kern, abstime); + retval = usem_wait(&sem->_kern, clock_id, flags, rqtp, rmtp); _pthread_cancel_leave(0); } return (retval); } +int +_sem_timedwait(sem_t * __restrict sem, + const struct timespec * __restrict abstime) +{ + return (_sem_clockwait_np(sem, CLOCK_REALTIME, TIMER_ABSTIME, abstime, + NULL)); +}; + int _sem_wait(sem_t *sem) { diff --git a/lib/libc/gen/sem_timedwait.3 b/lib/libc/gen/sem_timedwait.3 index 16d107369ea0..3a487993b13c 100644 --- a/lib/libc/gen/sem_timedwait.3 +++ b/lib/libc/gen/sem_timedwait.3 @@ -34,11 +34,12 @@ .\" .\" $FreeBSD$ .\" -.Dd August 17, 2016 +.Dd February 23, 2017 .Dt SEM_TIMEDWAIT 3 .Os .Sh NAME -.Nm sem_timedwait +.Nm sem_timedwait , +.Nm sem_clockwait_np .Nd "lock a semaphore" .Sh LIBRARY .Lb libc @@ -46,6 +47,8 @@ .In semaphore.h .Ft int .Fn sem_timedwait "sem_t * restrict sem" "const struct timespec * restrict abs_timeout" +.Ft int +.Fn sem_clockwait_np "sem_t * restrict sem" "clockid_t clock_id" "int flags" "const struct timespec * rqtp" "struct timespec * rmtp" .Sh DESCRIPTION The .Fn sem_timedwait @@ -77,10 +80,40 @@ clock. The validity of the .Fa abs_timeout is not checked if the semaphore can be locked immediately. -.Sh RETURN VALUES +.Pp The -.Fn sem_timedwait -function returns zero if the calling process successfully performed the +.Fn sem_clockwait_np +function is a more flexible variant of +.Fn sem_timedwait . +The +.Fa clock_id +parameter specifies the reference clock. +If the +.Fa flags +parameter contains +.Dv TIMER_ABSTIME , +then the requested timeout +.Pq Fa rqtp +is an absolute timeout; otherwise, +the timeout is relative. +If this function fails with +.Er EINTR +and the timeout is relative, +a non-NULL +.Fa rmtp +will be updated to contain the amount of time remaining in the interval +.Po +the requested time minus the time actually slept +.Pc . +An absolute timeout has no effect on +.Fa rmtp . +A single structure can be used for both +.Fa rqtp +and +.Fa rmtp . +.Sh RETURN VALUES +These +functions return zero if the calling process successfully performed the semaphore lock operation on the semaphore designated by .Fa sem . If the call was unsuccessful, the state of the semaphore is unchanged, @@ -88,9 +121,7 @@ and the function returns a value of \-1 and sets the global variable .Va errno to indicate the error. .Sh ERRORS -The -.Fn sem_timedwait -function will fail if: +These functions will fail if: .Bl -tag -width Er .It Bq Er EINVAL The @@ -114,6 +145,18 @@ The .Fn sem_timedwait function conforms to .St -p1003.1-2004 . +The +.Fn sem_clockwait_np +function is not specified by any standard; +it exists only on +.Fx +at the time of this writing. .Sh HISTORY -The function first appeared in +The +.Fn sem_timedwait +function first appeared in .Fx 5.0 . +The +.Fn sem_clockwait_np +function first appeared in +.Fx 12.0 . diff --git a/lib/libc/include/namespace.h b/lib/libc/include/namespace.h index c95829e401f1..52e9b2558097 100644 --- a/lib/libc/include/namespace.h +++ b/lib/libc/include/namespace.h @@ -217,6 +217,7 @@ #define sem_open _sem_open #define sem_post _sem_post #define sem_timedwait _sem_timedwait +#define sem_clockwait_np _sem_clockwait_np #define sem_trywait _sem_trywait #define sem_unlink _sem_unlink #define sem_wait _sem_wait diff --git a/lib/libc/include/un-namespace.h b/lib/libc/include/un-namespace.h index 023334858572..e40e6fd1e656 100644 --- a/lib/libc/include/un-namespace.h +++ b/lib/libc/include/un-namespace.h @@ -198,6 +198,7 @@ #undef sem_open #undef sem_post #undef sem_timedwait +#undef sem_clockwait_np #undef sem_trywait #undef sem_unlink #undef sem_wait diff --git a/share/man/man3/pthread_testcancel.3 b/share/man/man3/pthread_testcancel.3 index 1dfc96483445..8d0204da32d3 100644 --- a/share/man/man3/pthread_testcancel.3 +++ b/share/man/man3/pthread_testcancel.3 @@ -1,5 +1,5 @@ .\" $FreeBSD$ -.Dd August 16, 2016 +.Dd February 17, 2017 .Dt PTHREAD_TESTCANCEL 3 .Os .Sh NAME @@ -120,9 +120,9 @@ is The .Fn kevent function is a cancellation point if it is potentially blocking, -i.e. when the +such as when the .Fa nevents -argument is non-zero. +argument is non-zero. .It Fn mq_receive .It Fn mq_send .It Fn mq_timedreceive @@ -146,6 +146,7 @@ argument is non-zero. .It Fn recvmsg .It Fn select .It Fn sem_timedwait +.It Fn sem_clockwait_np .It Fn sem_wait .It Fn send .It Fn sendmsg diff --git a/sys/kern/kern_umtx.c b/sys/kern/kern_umtx.c index 2bdc2bf8d81c..74cdd8f04f34 100644 --- a/sys/kern/kern_umtx.c +++ b/sys/kern/kern_umtx.c @@ -3219,10 +3219,16 @@ do_sem2_wait(struct thread *td, struct _usem2 *sem, struct _umtx_time *timeout) error = 0; else { umtxq_remove(uq); - /* A relative timeout cannot be restarted. */ - if (error == ERESTART && timeout != NULL && - (timeout->_flags & UMTX_ABSTIME) == 0) - error = EINTR; + if (timeout != NULL && (timeout->_flags & UMTX_ABSTIME) == 0) { + /* A relative timeout cannot be restarted. */ + if (error == ERESTART) + error = EINTR; + if (error == EINTR) { + abs_timeout_update(&timo); + timeout->_timeout = timo.end; + timespecsub(&timeout->_timeout, &timo.cur); + } + } } umtxq_unlock(&uq->uq_key); umtx_key_release(&uq->uq_key); @@ -3585,19 +3591,33 @@ static int __umtx_op_sem2_wait(struct thread *td, struct _umtx_op_args *uap) { struct _umtx_time *tm_p, timeout; + size_t uasize; int error; /* Allow a null timespec (wait forever). */ - if (uap->uaddr2 == NULL) + if (uap->uaddr2 == NULL) { + uasize = 0; tm_p = NULL; - else { - error = umtx_copyin_umtx_time( - uap->uaddr2, (size_t)uap->uaddr1, &timeout); + } else { + uasize = (size_t)uap->uaddr1; + error = umtx_copyin_umtx_time(uap->uaddr2, uasize, &timeout); if (error != 0) return (error); tm_p = &timeout; } - return (do_sem2_wait(td, uap->obj, tm_p)); + error = do_sem2_wait(td, uap->obj, tm_p); + if (error == EINTR && uap->uaddr2 != NULL && + (timeout._flags & UMTX_ABSTIME) == 0 && + uasize >= sizeof(struct _umtx_time) + sizeof(struct timespec)) { + error = copyout(&timeout._timeout, + (struct _umtx_time *)uap->uaddr2 + 1, + sizeof(struct timespec)); + if (error == 0) { + error = EINTR; + } + } + + return (error); } static int @@ -4194,19 +4214,37 @@ static int __umtx_op_sem2_wait_compat32(struct thread *td, struct _umtx_op_args *uap) { struct _umtx_time *tm_p, timeout; + size_t uasize; int error; /* Allow a null timespec (wait forever). */ - if (uap->uaddr2 == NULL) + if (uap->uaddr2 == NULL) { + uasize = 0; tm_p = NULL; - else { - error = umtx_copyin_umtx_time32(uap->uaddr2, - (size_t)uap->uaddr1, &timeout); + } else { + uasize = (size_t)uap->uaddr1; + error = umtx_copyin_umtx_time32(uap->uaddr2, uasize, &timeout); if (error != 0) return (error); tm_p = &timeout; } - return (do_sem2_wait(td, uap->obj, tm_p)); + error = do_sem2_wait(td, uap->obj, tm_p); + if (error == EINTR && uap->uaddr2 != NULL && + (timeout._flags & UMTX_ABSTIME) == 0 && + uasize >= sizeof(struct umtx_time32) + sizeof(struct timespec32)) { + struct timespec32 remain32 = { + .tv_sec = timeout._timeout.tv_sec, + .tv_nsec = timeout._timeout.tv_nsec + }; + error = copyout(&remain32, + (struct umtx_time32 *)uap->uaddr2 + 1, + sizeof(struct timespec32)); + if (error == 0) { + error = EINTR; + } + } + + return (error); } static int