# HG changeset patch # User Taylor R Campbell # Date 1588449831 0 # Sat May 02 20:03:51 2020 +0000 # Branch trunk # Node ID 72dc7e9c7c0f542f92b2b4b391a18b2b177ff525 # Parent 5cadb8060b769e8f571eb05ad8f78fa94acb7d77 Fix edge cases in cv_timedwaitbt, cv_timedwaitbt_sig. - If the timeout is exactly zero, fail immediately with EWOULDBLOCK. - If the timeout is just so small it would be rounded to zero ticks, make sure to wait at least one tick. - Make sure we never return with a negative timeout left. diff -r 5cadb8060b76 -r 72dc7e9c7c0f sys/kern/kern_condvar.c --- a/sys/kern/kern_condvar.c Fri May 01 17:58:48 2020 +0000 +++ b/sys/kern/kern_condvar.c Sat May 02 20:03:51 2020 +0000 @@ -334,23 +334,45 @@ cv_timedwaitbt(kcondvar_t *cv, kmutex_t { struct bintime slept; unsigned start, end; + int timo; int error; KASSERTMSG(bt->sec >= 0, "negative timeout"); KASSERTMSG(epsilon != NULL, "specify maximum requested delay"); + /* If there's nothing left to wait, time out. */ + if (bt->sec == 0 && bt->frac == 0) + return EWOULDBLOCK; + + /* Convert to ticks, but clamp to be >=1. */ + timo = bintime2timo(bt); + KASSERTMSG(timo >= 0, "negative ticks: %d", timo); + if (timo == 0) + timo = 1; + /* * getticks() is technically int, but nothing special * happens instead of overflow, so we assume two's-complement * wraparound and just treat it as unsigned. */ start = getticks(); - error = cv_timedwait(cv, mtx, bintime2timo(bt)); + error = cv_timedwait(cv, mtx, timo); end = getticks(); + /* + * Set it to the time left, or zero, whichever is larger. We + * do not fail with EWOULDBLOCK here because this may have been + * an explicit wakeup, so the caller needs to check before they + * give up or else cv_signal would be lost. + */ slept = timo2bintime(end - start); - /* bt := bt - slept */ - bintime_sub(bt, &slept); + if (bintimecmp(bt, &slept, <=)) { + bt->sec = 0; + bt->frac = 0; + } else { + /* bt := bt - slept */ + bintime_sub(bt, &slept); + } return error; } @@ -377,23 +399,45 @@ cv_timedwaitbt_sig(kcondvar_t *cv, kmute { struct bintime slept; unsigned start, end; + int timo; int error; KASSERTMSG(bt->sec >= 0, "negative timeout"); KASSERTMSG(epsilon != NULL, "specify maximum requested delay"); + /* If there's nothing left to wait, time out. */ + if (bt->sec == 0 && bt->frac == 0) + return EWOULDBLOCK; + + /* Convert to ticks, but clamp to be >=1. */ + timo = bintime2timo(bt); + KASSERTMSG(timo >= 0, "negative ticks: %d", timo); + if (timo == 0) + timo = 1; + /* * getticks() is technically int, but nothing special * happens instead of overflow, so we assume two's-complement * wraparound and just treat it as unsigned. */ start = getticks(); - error = cv_timedwait_sig(cv, mtx, bintime2timo(bt)); + error = cv_timedwait_sig(cv, mtx, timo); end = getticks(); + /* + * Set it to the time left, or zero, whichever is larger. We + * do not fail with EWOULDBLOCK here because this may have been + * an explicit wakeup, so the caller needs to check before they + * give up or else cv_signal would be lost. + */ slept = timo2bintime(end - start); - /* bt := bt - slept */ - bintime_sub(bt, &slept); + if (bintimecmp(bt, &slept, <=)) { + bt->sec = 0; + bt->frac = 0; + } else { + /* bt := bt - slept */ + bintime_sub(bt, &slept); + } return error; } # HG changeset patch # User Taylor R Campbell # Date 1588449909 0 # Sat May 02 20:05:09 2020 +0000 # Branch trunk # Node ID 444590139ff00ce5fa72dac452af48dee6dc0038 # Parent 72dc7e9c7c0f542f92b2b4b391a18b2b177ff525 Define DEFAULT_TIMEOUT_EPSILON for cv_timedwaitbt. For now, it is roughly one bintime's worth of ticks. This is about the smallest difference between wait periods we can rely on at the moment; in the glorious tickless future we can revisit this. diff -r 72dc7e9c7c0f -r 444590139ff0 sys/sys/timevar.h --- a/sys/sys/timevar.h Sat May 02 20:03:51 2020 +0000 +++ b/sys/sys/timevar.h Sat May 02 20:05:09 2020 +0000 @@ -195,6 +195,12 @@ bool time_wraps(struct timespec *, struc extern volatile time_t time_second; /* current second in the epoch */ extern volatile time_t time_uptime; /* system uptime in seconds */ +#define DEFAULT_TIMEOUT_EPSILON \ + (&(const struct bintime) { \ + .sec = 0, \ + .frac = ((uint64_t)1 << 32)/hz << 32, \ + }) + static __inline time_t time_mono_to_wall(time_t t) { # HG changeset patch # User Taylor R Campbell # Date 1588450222 0 # Sat May 02 20:10:22 2020 +0000 # Branch trunk # Node ID 0542d615765aa02bb6b94ae9323c7f878de3d59b # Parent 444590139ff00ce5fa72dac452af48dee6dc0038 New cv_timedwaitclock, cv_timedwaitclock_sig. Usage: given a struct timespec timeout copied from userland, along with a clockid and TIMER_* flags, error = cv_timedwaitclock(cv, lock, timeout, clockid, flags, DEFAULT_TIMEOUT_EPSILON); if (error) /* fail */ If flags is relative (i.e., (flags & TIMER_ABSTIME) == 0), then this deducts the time spent waiting from timeout, so you can run it in a loop: struct timespec timeout; error = copyin(SCARG(uap, timeout), &timeout, sizeof timeout); if (error) return error; mutex_enter(lock); while (!ready()) { error = cv_timedwaitclock_sig(cv, lock, &timeout, SCARG(uap, clockid), SCARG(uap, flags), DEFAULT_TIMEOUT_EPSILON); if (error) break; } mutex_exit(lock); CAVEAT: If the system call is interrupted by a signal with SA_RESTART so cv_timedwaitclock_sig fails with ERESTART, then the system call will be restarted with the _original_ relative timeout, not counting the time that was already spent waiting. This is a problem but it's not a problem I want to deal with at the moment. diff -r 444590139ff0 -r 0542d615765a sys/kern/kern_condvar.c --- a/sys/kern/kern_condvar.c Sat May 02 20:05:09 2020 +0000 +++ b/sys/kern/kern_condvar.c Sat May 02 20:10:22 2020 +0000 @@ -244,6 +244,191 @@ cv_timedwait_sig(kcondvar_t *cv, kmutex_ return error; } +struct timedwaitclock { + struct timespec *timeout; + clockid_t clockid; + int flags; + const struct bintime *epsilon; + struct timespec starttime; +}; + +static int +cv_timedwaitclock_begin(struct timedwaitclock *T, int *timo) +{ + struct timespec delta; + const struct timespec *deltap; + int error; + + /* Sanity-check timeout -- may have come from userland. */ + if (T->timeout->tv_nsec < 0 || T->timeout->tv_nsec >= 1000000000L) + return EINVAL; + + /* + * Compute the time delta. + */ + if ((T->flags & TIMER_ABSTIME) == TIMER_ABSTIME) { + /* Check our watch. */ + error = clock_gettime1(T->clockid, &T->starttime); + if (error) + return error; + + /* If the deadline has passed, we're done. */ + if (timespeccmp(T->timeout, &T->starttime, <=)) + return ETIMEDOUT; + + /* Count how much time is left. */ + timespecsub(T->timeout, &T->starttime, &delta); + deltap = δ + } else { + /* The user specified how much time is left. */ + deltap = T->timeout; + + /* If there's none left, we've timed out. */ + if (deltap->tv_sec == 0 && deltap->tv_nsec == 0) + return ETIMEDOUT; + } + + /* + * Convert to ticks, but clamp to be >=1. + * + * XXX In the tickless future, use a high-resolution timer if + * timo would round to zero. + */ + *timo = tstohz(deltap); + KASSERTMSG(*timo >= 0, "negative ticks: %d", *timo); + if (*timo == 0) + *timo = 1; + + /* Success! */ + return 0; +} + +static void +cv_timedwaitclock_end(struct timedwaitclock *T) +{ + struct timespec endtime, delta; + + /* If the timeout is absolute, nothing to do. */ + if ((T->flags & TIMER_ABSTIME) == TIMER_ABSTIME) + return; + + /* + * Check our watch. If anything goes wrong with it, make sure + * that the next time we immediately time out rather than fail + * to deduct the time elapsed. + */ + if (clock_gettime1(T->clockid, &endtime)) { + T->timeout->tv_sec = 0; + T->timeout->tv_nsec = 0; + return; + } + + /* Find how much time elapsed while we waited. */ + timespecsub(&endtime, &T->starttime, &delta); + + /* + * Paranoia: If the clock went backwards, treat it as if no + * time elapsed at all rather than adding anything. + */ + if (delta.tv_sec < 0 || + (delta.tv_sec == 0 && delta.tv_nsec < 0)) { + delta.tv_sec = 0; + delta.tv_nsec = 0; + } + + /* + * Set it to the time left, or zero, whichever is larger. We + * do not fail with EWOULDBLOCK here because this may have been + * an explicit wakeup, so the caller needs to check before they + * give up or else cv_signal would be lost. + */ + if (timespeccmp(T->timeout, &delta, <=)) { + T->timeout->tv_sec = 0; + T->timeout->tv_nsec = 0; + } else { + timespecsub(T->timeout, &delta, T->timeout); + } +} + +/* + * cv_timedwaitclock: + * + * Wait on a condition variable until awoken normally, or the + * specified timeout expires according to the provided clock. + * Returns zero if awoken normally or EWOULDBLOCK if the timeout + * expired. For relative timeouts ((flags & TIMER_ABSTIME) == 0), + * updates timeout with the time left. + * + * timeout == NULL specifies an infinite timeout. epsilon is a + * requested maximum error in timeout (excluding spurious + * wakeups). + */ +int +cv_timedwaitclock(kcondvar_t *cv, kmutex_t *mtx, struct timespec *timeout, + clockid_t clockid, int flags, const struct bintime *epsilon) +{ + struct timedwaitclock T = { + .timeout = timeout, + .clockid = clockid, + .flags = flags, + .epsilon = epsilon, + }; + int timo; + int error; + + if (timeout == NULL) { + cv_wait(cv, mtx); + return 0; + } + + error = cv_timedwaitclock_begin(&T, &timo); + if (error) + return error; + error = cv_timedwait(cv, mtx, timo); + cv_timedwaitclock_end(&T); + return error; +} + +/* + * cv_timedwaitclock_sig: + * + * Wait on a condition variable until awoken normally, interrupted + * by a signal, or the specified timeout expires according to the + * provided clock. Returns zero if awoken normally, + * EINTR/ERESTART if interrupted by a signal, or EWOULDBLOCK if + * the timeout expired. For relative timeouts ((flags & + * TIMER_ABSTIME) == 0), updates timeout with the time left. + * + * timeout == NULL specifies an infinite timeout. epsilon is a + * requested maximum error in timeout (excluding spurious + * wakeups). + */ +int +cv_timedwaitclock_sig(kcondvar_t *cv, kmutex_t *mtx, struct timespec *timeout, + clockid_t clockid, int flags, const struct bintime *epsilon) +{ + struct timedwaitclock T = { + .timeout = timeout, + .clockid = clockid, + .flags = flags, + .epsilon = epsilon, + }; + int timo; + int error; + + if (timeout == NULL) { + cv_wait(cv, mtx); + return 0; + } + + error = cv_timedwaitclock_begin(&T, &timo); + if (error) + return error; + error = cv_timedwait_sig(cv, mtx, timo); + cv_timedwaitclock_end(&T); + return error; +} + /* * Given a number of seconds, sec, and 2^64ths of a second, frac, we * want a number of ticks for a timeout: diff -r 444590139ff0 -r 0542d615765a sys/sys/condvar.h --- a/sys/sys/condvar.h Sat May 02 20:05:09 2020 +0000 +++ b/sys/sys/condvar.h Sat May 02 20:10:22 2020 +0000 @@ -40,6 +40,7 @@ typedef struct kcondvar { struct bintime; struct kmutex; +struct timespec; void cv_init(kcondvar_t *, const char *); void cv_destroy(kcondvar_t *); @@ -48,6 +49,10 @@ void cv_wait(kcondvar_t *, struct kmutex int cv_wait_sig(kcondvar_t *, struct kmutex *); int cv_timedwait(kcondvar_t *, struct kmutex *, int); int cv_timedwait_sig(kcondvar_t *, struct kmutex *, int); +int cv_timedwaitclock(kcondvar_t *, struct kmutex *, struct timespec *, + clockid_t, int, const struct bintime *); +int cv_timedwaitclock_sig(kcondvar_t *, struct kmutex *, struct timespec *, + clockid_t, int, const struct bintime *); int cv_timedwaitbt(kcondvar_t *, struct kmutex *, struct bintime *, const struct bintime *); int cv_timedwaitbt_sig(kcondvar_t *, struct kmutex *, struct bintime *, # HG changeset patch # User Taylor R Campbell # Date 1588450295 0 # Sat May 02 20:11:35 2020 +0000 # Branch trunk # Node ID 3a730e764ef727898529ce422f1472fdb77aa42a # Parent 0542d615765aa02bb6b94ae9323c7f878de3d59b Use cv_timedwaitclock_sig in futex. Possible fix for hangs observed with Java under Linux emulation. diff -r 0542d615765a -r 3a730e764ef7 sys/kern/sys_futex.c --- a/sys/kern/sys_futex.c Sat May 02 20:10:22 2020 +0000 +++ b/sys/kern/sys_futex.c Sat May 02 20:11:35 2020 +0000 @@ -916,17 +916,18 @@ futex_wait_abort(struct futex_wait *fw) } /* - * futex_wait(fw, deadline, clkid) + * futex_wait(fw, timeout, clkid, clkflags) * - * fw must be a waiter on a futex's queue. Wait until deadline on - * the clock clkid, or forever if deadline is NULL, for a futex - * wakeup. Return 0 on explicit wakeup or destruction of futex, - * ETIMEDOUT on timeout, EINTR/ERESTART on signal. Either way, fw - * will no longer be on a futex queue on return. + * fw must be a waiter on a futex's queue. Wait for timeout on + * the clock clkid according to clkflags (TIMER_*), or forever if + * timeout is NULL, for a futex wakeup. Return 0 on explicit + * wakeup or destruction of futex, ETIMEDOUT on timeout, + * EINTR/ERESTART on signal. Either way, fw will no longer be on + * a futex queue on return. */ static int -futex_wait(struct futex_wait *fw, const struct timespec *deadline, - clockid_t clkid) +futex_wait(struct futex_wait *fw, struct timespec *timeout, clockid_t clkid, + int clkflags) { int error = 0; @@ -934,7 +935,7 @@ futex_wait(struct futex_wait *fw, const mutex_enter(&fw->fw_lock); for (;;) { - /* If we're done yet, stop and report success. */ + /* If we're done already, stop and report success. */ if (fw->fw_bitset == 0 || fw->fw_futex == NULL) { error = 0; break; @@ -945,30 +946,8 @@ futex_wait(struct futex_wait *fw, const break; /* Not done yet. Wait. */ - if (deadline) { - struct timespec ts; - - /* Check our watch. */ - error = clock_gettime1(clkid, &ts); - if (error) - break; - - /* If we're past the deadline, ETIMEDOUT. */ - if (timespeccmp(deadline, &ts, <=)) { - error = ETIMEDOUT; - break; - } - - /* Count how much time is left. */ - timespecsub(deadline, &ts, &ts); - - /* Wait for that much time, allowing signals. */ - error = cv_timedwait_sig(&fw->fw_cv, &fw->fw_lock, - tstohz(&ts)); - } else { - /* Wait indefinitely, allowing signals. */ - error = cv_wait_sig(&fw->fw_cv, &fw->fw_lock); - } + error = cv_timedwaitclock_sig(&fw->fw_cv, &fw->fw_lock, + timeout, clkid, clkflags, DEFAULT_TIMEOUT_EPSILON); } /* @@ -1188,13 +1167,11 @@ futex_queue_unlock2(struct futex *f, str */ static int futex_func_wait(bool shared, int *uaddr, int val, int val3, - const struct timespec *timeout, clockid_t clkid, int clkflags, + struct timespec *timeout, clockid_t clkid, int clkflags, register_t *retval) { struct futex *f; struct futex_wait wait, *fw = &wait; - struct timespec ts; - const struct timespec *deadline; int error; /* @@ -1208,17 +1185,6 @@ futex_func_wait(bool shared, int *uaddr, if (!futex_test(uaddr, val)) return EAGAIN; - /* Determine a deadline on the specified clock. */ - if (timeout == NULL || (clkflags & TIMER_ABSTIME) == TIMER_ABSTIME) { - deadline = timeout; - } else { - error = clock_gettime1(clkid, &ts); - if (error) - return error; - timespecadd(&ts, timeout, &ts); - deadline = &ts; - } - /* Get the futex, creating it if necessary. */ error = futex_lookup_create(uaddr, shared, &f); if (error) @@ -1255,7 +1221,7 @@ futex_func_wait(bool shared, int *uaddr, f = NULL; /* Wait. */ - error = futex_wait(fw, deadline, clkid); + error = futex_wait(fw, timeout, clkid, clkflags); if (error) goto out; @@ -1580,8 +1546,8 @@ out: * parsed out. */ int -do_futex(int *uaddr, int op, int val, const struct timespec *timeout, - int *uaddr2, int val2, int val3, register_t *retval) +do_futex(int *uaddr, int op, int val, struct timespec *timeout, int *uaddr2, + int val2, int val3, register_t *retval) { const bool shared = (op & FUTEX_PRIVATE_FLAG) ? false : true; const clockid_t clkid = (op & FUTEX_CLOCK_REALTIME) ? CLOCK_REALTIME diff -r 0542d615765a -r 3a730e764ef7 sys/sys/futex.h --- a/sys/sys/futex.h Sat May 02 20:10:22 2020 +0000 +++ b/sys/sys/futex.h Sat May 02 20:11:35 2020 +0000 @@ -175,8 +175,8 @@ struct lwp; int futex_robust_head_lookup(struct lwp *, lwpid_t, void **); void futex_release_all_lwp(struct lwp *, lwpid_t); -int do_futex(int *, int, int, const struct timespec *, int *, int, - int, register_t *); +int do_futex(int *, int, int, struct timespec *, int *, int, int, + register_t *); void futex_sys_init(void); void futex_sys_fini(void); #endif /* _KERNEL */