Browse Source

Sleep: partially undo the change of sleep to use awake (Fixes go runtime test hangs)

This CL restores the original sleep system call, and now the go runtime
test at least completes. It still has some bugs, but not the hang.

I am doing this because the awake-based sleep has a race condition that can cause
it to hang. The hang was seen in the go runtime tests, which would never complete.
Still worse, the runtime hang was really hard to diagnose, as it occured almost
randomly -- a classic sign of a race condition somewhere.

The race was in our sleep function in libc.

The proposed sleep replacement, using awake, looked like this:

void
sleep(int32_t millisecs)
{
	int64_t wakeup;
	char c;

	wakeup = awake(millisecs);

	while(!awakened(wakeup) && /*read(0, &c, 1) && */rendezvous(&wakeup, (void*)1) == (void*)~0)
		;
}

I've added the part in /* */

The code as written has a race that's not fixable in user mode.The test for exiting the
loop is two parts: awakened and rendezvous. It's possible for the awakened test to fail,
meaning there is time left; but is is further possible for
the awake to finish, and now the rendezvous will block. Note the commented-out-read: if you
uncomment it, and hit return at various times, the rendezvous will hang *depending on the value of the timeout
variable*. This is a classic race.

This broke the to Go runtime test, in various parts, in nonrepeatable ways.

I'm leaving most of the awake-based sleep there until we decide what to do with it. It's also a
warning to all of us to review global system call patches much more carefully. The libc sleep was
broken, and the breakage should have been caught in code review. It was not.

Finally, before we continue further global changes to system calls, we need a solid workload test.
I renew my call for a moratorium on system call changes until we can verify them with a workload.

Change-Id: If4eec8125048b03517cc759a1ddffbc48cf016a2
Signed-off-by: Ronald G. Minnich <rminnich@gmail.com>
Ronald G. Minnich 8 years ago
parent
commit
1b8fa2ccab

+ 2 - 2
sys/include/libc.h

@@ -703,8 +703,8 @@ extern	int	segfree(void*, uint32_t);
 extern	int	semacquire(int32_t*, int);
 extern	int32_t	semrelease(int32_t*, int32_t);
 extern	int	sleep(int32_t);
-extern	int	stat(char*, uint8_t*, int);
-extern	int	tsemacquire(int32_t*, uint32_t);
+extern	int	stat(const char*, uint8_t*, int);
+extern	int	tsemacquire(int32_t*, uint64_t);
 extern	Waitmsg*	wait(void);
 extern	int	waitpid(void);
 extern	int32_t	write(int, void*, int32_t);

+ 1 - 1
sys/src/9/port/portdat.h

@@ -863,7 +863,7 @@ struct Proc
 
 	Lock	rlock;		/* sync sleep/wakeup with postnote */
 	Rendez	*r;		/* rendezvous point slept on */
-	Rendez	sleep;		/* place for syssleep/debug */
+	Rendez	sleep;		/* place for syssleep/debug/tsleep */
 	int	notepending;	/* note issued but not acted on */
 	int	kp;		/* true if a kernel process */
 	Proc	*palarm;	/* Next alarm time */

+ 2 - 2
sys/src/9/port/sysproc.c

@@ -730,14 +730,14 @@ void
 syssleep(Ar0* ar0, ...)
 {
 	Proc *up = externup();
-	int32_t ms;
+	int64_t ms;
 	va_list list;
 	va_start(list, ar0);
 
 	/*
 	 * int sleep(long millisecs);
 	 */
-	ms = va_arg(list, int32_t);
+	ms = va_arg(list, int64_t);
 	va_end(list);
 
 	ar0->i = 0;

+ 2 - 2
sys/src/cmd/auth/cron.c

@@ -89,8 +89,8 @@ sleepuntil(uint32_t tm)
 	
 	if (now < tm)
 		return sleep((tm - now)*1000);
-	else
-		return 0;
+
+	return 0;
 }
 
 #pragma varargck	argpos clog 1

+ 1 - 1
sys/src/cmd/aux/apm.c

@@ -995,7 +995,7 @@ eventwatch(void *v)
 		}
 		if(s)
 			sendul(cevent, -1);
-		if(sleep(750) < 0)
+		if (sleep(750) < 0)
 			break;
 	}
 }

+ 2 - 2
sys/src/cmd/pump.c

@@ -134,9 +134,9 @@ sleepunlocked(int32_t ms)
 {
 	int r;
 
-	unlock(&arithlock);
+ 	unlock(&arithlock);
 	r = sleep(ms);
-	lock(&arithlock);
+ 	lock(&arithlock);
 	return r;
 }
 

+ 0 - 1
sys/src/libc/klibc.json

@@ -107,7 +107,6 @@
 			"9syscall/semacquire.s",
 			"9syscall/semrelease.s",
 			"9syscall/sleep.s",
-			"9syscall/_stat.s",
 			"9syscall/stat.s",
 			"9syscall/sysr1.s",
 			"9syscall/tsemacquire.s",

+ 0 - 1
sys/src/libc/libc.json

@@ -107,7 +107,6 @@
 			"9syscall/semacquire.s",
 			"9syscall/semrelease.s",
 			"9syscall/sleep.s",
-			"9syscall/_stat.s",
 			"9syscall/stat.s",
 			"9syscall/sysr1.s",
 			"9syscall/tsemacquire.s",

+ 11 - 0
sys/src/sysconf.json

@@ -565,6 +565,17 @@
 				"int32_t"
 			]
 		},
+	    {
+		"#Id": 17,
+		"Args": [
+		    "int64_t"
+		],
+		"Id": 4113,
+		"Name": "sleep",
+		"Ret": [
+		    "int32_t"
+		]
+	    },
 		{
 			"#Id": 52,
 			"Args": [