From d052239a7a5600b4cb3a468438c4f3d47c6aac34 Mon Sep 17 00:00:00 2001
From: Markus Armbruster <armbru@pond.sub.org>
Date: Sun, 26 Apr 2009 23:48:05 +0200
Subject: [PATCH] Make empth_rwlock_t prefer writers

LWP and Windows implementations already did that.  Rewrite the
pthreads implementation.

The write-bias makes the stupid play_wrlock_wanted busy wait in
dispatch() unnecessary.  Remove it.
---
 include/empthread.h         | 12 ++++-----
 src/lib/empthread/pthread.c | 53 ++++++++++++++++++++++++++++---------
 src/lib/player/dispatch.c   | 10 -------
 3 files changed, 47 insertions(+), 28 deletions(-)

diff --git a/include/empthread.h b/include/empthread.h
index d06efb10a..219ddb3ad 100644
--- a/include/empthread.h
+++ b/include/empthread.h
@@ -56,10 +56,10 @@
 
 /* Abstract data types */
 
-/* empth_t * represents a thread.  */
+/* A thread.  */
 typedef struct lwpProc empth_t;
 
-/* empth_rwlock_t * represents a read-write lock */
+/* A read-write lock, perferring writers */
 typedef struct lwp_rwlock empth_rwlock_t;
 
 /* Flags for empth_select(): whether to sleep on input or output */
@@ -189,7 +189,7 @@ int empth_wait_for_signal(void);
 /*
  * Create a read-write lock.
  * NAME is its name, it is used for debugging.
- * Return the reade-write lock, or NULL on error.
+ * Return the read-write lock, or NULL on error.
  */
 empth_rwlock_t *empth_rwlock_create(char *name);
 
@@ -209,9 +209,9 @@ void empth_rwlock_wrlock(empth_rwlock_t *rwlock);
 /*
  * Lock RWLOCK for reading.
  * A read-write lock can be locked for reading only when it is not
- * locked for writing.  If this is not the case, put the current
- * thread to sleep until it is.  Must not starve writers, and may
- * sleep to avoid that.
+ * locked for writing, and no other thread is attempting to lock it
+ * for writing.  If this is not the case, put the current thread to
+ * sleep until it is.
  */
 void empth_rwlock_rdlock(empth_rwlock_t *rwlock);
 
diff --git a/src/lib/empthread/pthread.c b/src/lib/empthread/pthread.c
index 6d08611f5..3378b50bc 100644
--- a/src/lib/empthread/pthread.c
+++ b/src/lib/empthread/pthread.c
@@ -62,8 +62,12 @@ struct empth_t {
 };
 
 struct empth_rwlock_t {
+    /* Can't use pthread_rwlock_t, because it needn't prefer writers */
     char *name;
-    pthread_rwlock_t lock;
+    int nread;			/* #active readers */
+    int nwrite;			/* total #writers (active and waiting) */
+    pthread_cond_t can_read;
+    pthread_cond_t can_write;
 };
 
 /* Thread-specific data key */
@@ -397,19 +401,22 @@ empth_rwlock_create(char *name)
     if (!rwlock)
 	return NULL;
 
-    if (pthread_rwlock_init(&rwlock->lock, NULL) != 0) {
+    if (pthread_cond_init(&rwlock->can_read, NULL) != 0
+	|| pthread_cond_init(&rwlock->can_write, NULL) != 0) {
 	free(rwlock);
 	return NULL;
     }
 
     rwlock->name = strdup(name);
+    rwlock->nread = rwlock->nwrite = 0;
     return rwlock;
 }
 
 void
 empth_rwlock_destroy(empth_rwlock_t *rwlock)
 {
-    pthread_rwlock_destroy(&rwlock->lock);
+    pthread_cond_destroy(&rwlock->can_read);
+    pthread_cond_destroy(&rwlock->can_write);
     free(rwlock->name);
     free(rwlock);
 }
@@ -417,23 +424,45 @@ empth_rwlock_destroy(empth_rwlock_t *rwlock)
 void
 empth_rwlock_wrlock(empth_rwlock_t *rwlock)
 {
-    pthread_mutex_unlock(&mtx_ctxsw);
-    pthread_rwlock_wrlock(&rwlock->lock);
-    pthread_mutex_lock(&mtx_ctxsw);
-    empth_restorectx();
+    empth_status("wrlock %s %d %d",
+		 rwlock->name, rwlock->nread, rwlock->nwrite);
+    rwlock->nwrite++;
+    while (rwlock->nread != 0 || rwlock->nwrite != 1) {
+	empth_status("waiting for wrlock %s", rwlock->name);
+	pthread_cond_wait(&rwlock->can_write, &mtx_ctxsw);
+	empth_status("got wrlock %s %d %d",
+		 rwlock->name, rwlock->nread, rwlock->nwrite);
+	empth_restorectx();
+    }
 }
 
 void
 empth_rwlock_rdlock(empth_rwlock_t *rwlock)
 {
-    pthread_mutex_unlock(&mtx_ctxsw);
-    pthread_rwlock_rdlock(&rwlock->lock);
-    pthread_mutex_lock(&mtx_ctxsw);
-    empth_restorectx();
+    empth_status("rdlock %s %d %d",
+		 rwlock->name, rwlock->nread, rwlock->nwrite);
+    while (rwlock->nwrite) {
+	empth_status("waiting for rdlock %s", rwlock->name);
+	pthread_cond_wait(&rwlock->can_read, &mtx_ctxsw);
+	empth_status("got rdlock %s %d %d",
+		     rwlock->name, rwlock->nread, rwlock->nwrite);
+	empth_restorectx();
+    }
+    rwlock->nread++;
 }
 
 void
 empth_rwlock_unlock(empth_rwlock_t *rwlock)
 {
-    pthread_rwlock_unlock(&rwlock->lock);
+    if (CANT_HAPPEN(!rwlock->nread && !rwlock->nwrite))
+	return;
+    if (rwlock->nread) {	/* holding read lock */
+	if (!--rwlock->nread)
+	    pthread_cond_signal(&rwlock->can_write);
+    } else {
+	rwlock->nwrite--;
+	pthread_cond_signal(&rwlock->can_write);
+    }
+    if (rwlock->nwrite == 0)
+	pthread_cond_broadcast(&rwlock->can_read);
 }
diff --git a/src/lib/player/dispatch.c b/src/lib/player/dispatch.c
index 90a75fdb9..157ee1448 100644
--- a/src/lib/player/dispatch.c
+++ b/src/lib/player/dispatch.c
@@ -85,16 +85,6 @@ dispatch(char *buf, char *redir)
 	pr("Command not implemented\n");
 	return 0;
     }
-    /*
-     * Rwlocks can hand out read locks while a write lock is wanted.
-     * Unfair to writers, but possible.  This lets commands run with
-     * !player->aborted, which can then block on input and thus delay
-     * the update indefinitely.  We could avoid that by setting
-     * player->aborted here, but spinning until the update is done is
-     * nicer to players.
-     */
-    while (play_wrlock_wanted)
-	empth_yield();
     player->command = command;
     empth_rwlock_rdlock(play_lock);
     if (redir) {
-- 
2.43.0