changing _lwp_park to return unslept time.

[christos%zoulas.com@localhost (Christos Zoulas)]

Hi,

The following is a proposal to change _lwp_park() to return the time
left to sleep in the "ts" argument when given a relative time to sleep.

This prevents the naive loop:

	while (_lwp_park(CLOCK_*, TIMER_RELTIME, &ts, ...) == -1 
	    && errno == EINTR)
	    continue;

from looping forever. The example that brought this up was from
golang, where the profiling timer kept interrupting _lwp_park
syscall leading to an infinite loop. The question was, how much
time should I sleep after EINTR? Yes, I can take the time before
the call, and then take the time after the call, and subtract them,
and then figure out how much time I have left, but why do that
since the kernel already knows? In the golang case, I decided to
arbitrarly half the time to sleep after EINTR since it does not
matter too much, but why not pass the information back to the caller
and let them decide?

The same goes for pselect(2), but I am not proposing to change
that, since this is a much more common API that assumes a const
timespec, but even fixing that is just a one line change to copyout
the timespec since the call already does all the work.

Here are the changes and unit test (minus the header file and the
man page).

Opinions?

christos


Index: kern/subr_time.c
===================================================================
RCS file: /cvsroot/src/sys/kern/subr_time.c,v
retrieving revision 1.19
diff -u -p -u -r1.19 subr_time.c
--- kern/subr_time.c	5 Jan 2017 23:29:14 -0000	1.19
+++ kern/subr_time.c	5 Dec 2017 16:45:43 -0000
@@ -227,6 +227,17 @@ gettimeleft(struct timespec *ts, struct 
 	return tstohz(ts);
 }
 
+void
+clock_timeleft(clockid_t clockid, struct timespec *ts, struct timespec *sleepts)
+{
+	struct timespec sleptts;
+
+	clock_gettime1(clockid, &sleptts);
+	timespecadd(ts, sleepts, ts);
+	timespecsub(ts, &sleptts, ts);
+	*sleepts = sleptts;
+}
+
 static void
 ticks2ts(uint64_t ticks, struct timespec *ts)
 {
Index: kern/sys_lwp.c
===================================================================
RCS file: /cvsroot/src/sys/kern/sys_lwp.c,v
retrieving revision 1.61
diff -u -p -u -r1.61 sys_lwp.c
--- kern/sys_lwp.c	1 Jun 2017 02:45:13 -0000	1.61
+++ kern/sys_lwp.c	5 Dec 2017 16:45:44 -0000
@@ -542,10 +542,13 @@ lwp_park(clockid_t clock_id, int flags, 
 	kmutex_t *mp;
 	wchan_t wchan;
 	int timo, error;
+	struct timespec start;
 	lwp_t *l;
+	bool reltime = !(flags & TIMER_ABSTIME);
 
 	if (ts != NULL) {
-		if ((error = ts2timo(clock_id, flags, ts, &timo, NULL)) != 0)
+		if ((error = ts2timo(clock_id, flags, ts, &timo, 
+		    reltime ? &start : NULL)) != 0)
 			return error;
 		KASSERT(timo != 0);
 	} else {
@@ -575,12 +578,15 @@ lwp_park(clockid_t clock_id, int flags, 
 	switch (error) {
 	case EWOULDBLOCK:
 		error = ETIMEDOUT;
+		if (reltime)
+			memset(ts, 0, sizeof(*ts));
 		break;
 	case ERESTART:
 		error = EINTR;
-		break;
+		/* FALLTHROUGH*/
 	default:
-		/* nothing */
+		if (reltime)
+			clock_timeleft(clock_id, ts, &start);
 		break;
 	}
 	return error;
@@ -598,7 +604,7 @@ sys____lwp_park60(struct lwp *l, const s
 	/* {
 		syscallarg(clockid_t)			clock_id;
 		syscallarg(int)				flags;
-		syscallarg(const struct timespec *)	ts;
+		syscallarg(struct timespec *)		ts;
 		syscallarg(lwpid_t)			unpark;
 		syscallarg(const void *)		hint;
 		syscallarg(const void *)		unparkhint;
@@ -621,8 +627,11 @@ sys____lwp_park60(struct lwp *l, const s
 			return error;
 	}
 
-	return lwp_park(SCARG(uap, clock_id), SCARG(uap, flags), tsp,
+	error = lwp_park(SCARG(uap, clock_id), SCARG(uap, flags), tsp,
 	    SCARG(uap, hint));
+	if (SCARG(uap, ts) != NULL && (SCARG(uap, flags) & TIMER_ABSTIME) == 0)
+		(void)copyout(tsp, SCARG(uap, ts), sizeof(*tsp));
+	return error;
 }
 
 int
Index: kern/syscalls.master
===================================================================
RCS file: /cvsroot/src/sys/kern/syscalls.master,v
retrieving revision 1.286
diff -u -p -u -r1.286 syscalls.master
--- kern/syscalls.master	2 Nov 2016 00:11:59 -0000	1.286
+++ kern/syscalls.master	5 Dec 2017 16:45:44 -0000
@@ -975,7 +975,7 @@
 			    int flags, const struct timespec *rqtp, \
 			    struct timespec *rmtp); }
 478	STD 		{ int|sys|60|_lwp_park(clockid_t clock_id, int flags, \
-			    const struct timespec *ts, lwpid_t unpark, \
+			    struct timespec *ts, lwpid_t unpark, \
 			    const void *hint, const void *unparkhint); }
 479	NOERR	RUMP	{ int|sys||posix_fallocate(int fd, int PAD, off_t pos, \
 			    off_t len); }
Index: sys/timevar.h
===================================================================
RCS file: /cvsroot/src/sys/sys/timevar.h,v
retrieving revision 1.36
diff -u -p -u -r1.36 timevar.h
--- sys/timevar.h	8 Mar 2016 05:02:55 -0000	1.36
+++ sys/timevar.h	5 Dec 2017 16:45:44 -0000
@@ -151,6 +151,7 @@ void	adjtime1(const struct timeval *, st
 int	clock_getres1(clockid_t, struct timespec *);
 int	clock_gettime1(clockid_t, struct timespec *);
 int	clock_settime1(struct proc *, clockid_t, const struct timespec *, bool);
+void	clock_timeleft(clockid_t, struct timespec *, struct timespec *);
 int	dogetitimer(struct proc *, int, struct itimerval *);
 int	dosetitimer(struct proc *, int, struct itimerval *);
 int	dotimer_gettime(int, struct proc *, struct itimerspec *);

--- /dev/null	2017-12-05 11:41:16.766347089 -0500
+++ /usr/src/tests/lib/libc/sys/t_timeleft.c	2017-12-05 11:48:12.899852409 -0500
@@ -0,0 +1,136 @@
+/* $NetBSD: t_write.c,v 1.6 2017/07/09 22:18:43 christos Exp $ */
+
+/*-
+ * Copyright (c) 2017 The NetBSD Foundation, Inc.
+ * All rights reserved.
+ *
+ * This code is derived from software contributed to The NetBSD Foundation
+ * by Christos Zoulas.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTORS
+ * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE FOUNDATION OR CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <sys/cdefs.h>
+__COPYRIGHT("@(#) Copyright (c) 2008\
+ The NetBSD Foundation, inc. All rights reserved.");
+__RCSID("$NetBSD: t_write.c,v 1.6 2017/07/09 22:18:43 christos Exp $");
+
+#include <sys/types.h>
+#include <sys/select.h>
+
+#include <atf-c.h>
+#include <time.h>
+#include <errno.h>
+#include <lwp.h>
+#include <signal.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <sched.h>
+#include <unistd.h>
+
+static void
+sighandler(int signo __unused)
+{
+}
+
+struct info {
+	void (*fun)(struct timespec *);
+	struct timespec ts;
+};
+
+static void
+timeleft__lwp_park(struct timespec *ts)
+{
+	ATF_REQUIRE_ERRNO(EINTR, _lwp_park(CLOCK_MONOTONIC, TIMER_RELTIME,
+	    ts, 0, ts, NULL) == -1);
+}
+
+#if 0
+static void
+timeleft_pselect(struct timespec *ts)
+{
+	ATF_REQUIRE_ERRNO(EINTR, pselect(1, NULL, NULL, NULL, ts, NULL));
+}
+#endif
+
+static void *
+runner(void *arg)
+{
+	struct info *i = arg;
+	(*i->fun)(&i->ts);
+	return NULL;
+}
+
+static void
+tester(void (*fun)(struct timespec *))
+{
+	const struct timespec ts = { 5, 0 };
+	const struct timespec sts = { 0, 1000000 };
+	struct info i = { fun, ts };
+	pthread_t thr;
+
+	ATF_REQUIRE(signal(SIGINT, sighandler) == 0);
+	ATF_REQUIRE(pthread_create(&thr, NULL, runner, &i) == 0);
+
+	nanosleep(&sts, NULL);
+	pthread_kill(thr, SIGINT);
+	printf("Orig time %ju.%lu\n", (intmax_t)ts.tv_sec, ts.tv_nsec);
+	printf("Time left %ju.%lu\n", (intmax_t)i.ts.tv_sec, i.ts.tv_nsec);
+	ATF_REQUIRE(timespeccmp(&i.ts, &ts, <));
+}
+
+ATF_TC(timeleft__lwp_park);
+ATF_TC_HEAD(timeleft__lwp_park, tc)
+{
+	atf_tc_set_md_var(tc, "descr", "Checks that _lwp_park(2) returns "
+	    "the time left to sleep after interrupted");
+}
+
+ATF_TC_BODY(timeleft__lwp_park, tc)
+{
+	tester(timeleft__lwp_park);
+}
+
+#if 0
+ATF_TC(timeleft_pselect);
+ATF_TC_HEAD(timeleft_pselect, tc)
+{
+	atf_tc_set_md_var(tc, "descr", "Checks that pselect(2) returns "
+	    "the time left to sleep after interrupted");
+}
+
+ATF_TC_BODY(timeleft_pselect, tc)
+{
+	tester(timeleft_pselect);
+}
+#endif
+
+ATF_TP_ADD_TCS(tp)
+{
+
+	ATF_TP_ADD_TC(tp, timeleft__lwp_park);
+#if 0
+	ATF_TP_ADD_TC(tp, timeleft_pselect);
+#endif
+
+	return atf_no_error();
+}