From: Matthew Dempsky <mdempsky@google.com>
Date: Wed, 10 Sep 2014 00:41:48 +0000 (-0700)
Subject: runtime: cleanup openbsd semasleep implementation
X-Git-Tag: go1.4beta1~449
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=d955dfb0075addb844d1d683a96967bb7ea0dea7;p=gostls13.git

runtime: cleanup openbsd semasleep implementation

The previous implementation had several subtle issues.  It's not
clear if any of these could actually be causing the flakiness
problems on openbsd/386, but fixing them should only help.

1. thrsleep() is implemented internally as unlock, then test *abort
(if abort != nil), then tsleep().  Under the current code, that makes
it theoretically possible that semasleep()/thrsleep() could release
waitsemalock, then a racing semawakeup() could acquire the lock,
increment waitsemacount, and call thrwakeup()/wakeup() before
thrsleep() reaches tsleep().  (In practice, OpenBSD's big kernel lock
seems unlikely to let this actually happen.)

The proper way to avoid this is to pass &waitsemacount as the abort
pointer to thrsleep so thrsleep knows to re-check it before going to
sleep, and to wakeup if it's non-zero.  Then we avoid any races.
(I actually suspect openbsd's sema{sleep,wakeup}() could be further
simplified using cas/xadd instead of locks, but I don't want to be
more intrusive than necessary so late in the 1.4 release cycle.)

2. semasleep() takes a relative sleep duration, but thrsleep() needs
an absolute sleep deadline.  Instead of recomputing the deadline each
iteration, compute it once up front and use (*Timespec)(nil) to signify
no deadline.  Ensures we retry properly if there's a spurious wakeup.

3. Instead of assuming if thrsleep() woke up and waitsemacount wasn't
available that we must have hit the deadline, check that the system
call returned EWOULDBLOCK.

4. Instead of assuming that 64-bit systems are little-endian, compute
timediv() using a temporary int32 nsec and then assign it to tv_nsec.

LGTM=iant
R=jsing, iant
CC=golang-codereviews
https://golang.org/cl/137960043
---

diff --git a/src/runtime/os_openbsd.c b/src/runtime/os_openbsd.c
index 91bd9449a1..eebaa13eea 100644
--- a/src/runtime/os_openbsd.c
+++ b/src/runtime/os_openbsd.c
@@ -12,6 +12,8 @@
 enum
 {
 	ESRCH = 3,
+	EAGAIN = 35,
+	EWOULDBLOCK = EAGAIN,
 	ENOTSUP = 91,
 
 	// From OpenBSD's sys/time.h
@@ -65,32 +67,24 @@ runtime·semacreate(void)
 int32
 runtime·semasleep(int64 ns)
 {
-	Timespec ts;
-
-	// spin-mutex lock
-	while(runtime·xchg(&g->m->waitsemalock, 1))
-		runtime·osyield();
+	Timespec ts, *tsp = nil;
+
+	// Compute sleep deadline.
+	if(ns >= 0) {
+		int32 nsec;
+		ns += runtime·nanotime();
+		ts.tv_sec = runtime·timediv(ns, 1000000000, &nsec);
+		ts.tv_nsec = nsec; // tv_nsec is int64 on amd64
+		tsp = &ts;
+	}
 
 	for(;;) {
-		// lock held
-		if(g->m->waitsemacount == 0) {
-			// sleep until semaphore != 0 or timeout.
-			// thrsleep unlocks m->waitsemalock.
-			if(ns < 0)
-				runtime·thrsleep(&g->m->waitsemacount, 0, nil, &g->m->waitsemalock, nil);
-			else {
-				ns += runtime·nanotime();
-				// NOTE: tv_nsec is int64 on amd64, so this assumes a little-endian system.
-				ts.tv_nsec = 0;
-				ts.tv_sec = runtime·timediv(ns, 1000000000, (int32*)&ts.tv_nsec);
-				runtime·thrsleep(&g->m->waitsemacount, CLOCK_MONOTONIC, &ts, &g->m->waitsemalock, nil);
-			}
-			// reacquire lock
-			while(runtime·xchg(&g->m->waitsemalock, 1))
-				runtime·osyield();
-		}
+		int32 ret;
+
+		// spin-mutex lock
+		while(runtime·xchg(&g->m->waitsemalock, 1))
+			runtime·osyield();
 
-		// lock held (again)
 		if(g->m->waitsemacount != 0) {
 			// semaphore is available.
 			g->m->waitsemacount--;
@@ -99,17 +93,12 @@ runtime·semasleep(int64 ns)
 			return 0;  // semaphore acquired
 		}
 
-		// semaphore not available.
-		// if there is a timeout, stop now.
-		// otherwise keep trying.
-		if(ns >= 0)
-			break;
+		// sleep until semaphore != 0 or timeout.
+		// thrsleep unlocks m->waitsemalock.
+		ret = runtime·thrsleep(&g->m->waitsemacount, CLOCK_MONOTONIC, tsp, &g->m->waitsemalock, (int32 *)&g->m->waitsemacount);
+		if(ret == EWOULDBLOCK)
+			return -1;
 	}
-
-	// lock held but giving up
-	// spin-mutex unlock
-	runtime·atomicstore(&g->m->waitsemalock, 0);
-	return -1;
 }
 
 static void badsemawakeup(void);