Implement pthread_rwlock functions for NaCl newlib.

src/untrusted/pthread/nc_rwlock.c

Index: src/untrusted/pthread/nc_rwlock.c
diff --git a/src/untrusted/pthread/nc_rwlock.c b/src/untrusted/pthread/nc_rwlock.c
new file mode 100644
index 0000000000000000000000000000000000000000..09ca5e534c60ed8686fcb7268ef753df2dee6302
--- /dev/null
+++ b/src/untrusted/pthread/nc_rwlock.c
@@ -0,0 +1,269 @@
+/*
+ * Copyright 2014 The Native Client Authors. All rights reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+/*
+ * Native Client rwlock implementation
+ *
+ * This implementation is a 'write-prefering' reader-writer lock which

[Mark Seaborn, 2015/01/05 16:58:53]

"preferring"

[Sam Clegg, 2015/01/06 19:06:24]

Done.

+ * avoids writer starvation by preventing readers from acquiring the lock
+ * while there are waiting writers. See:
+ * (http://en.wikipedia.org/wiki/Readers%E2%80%93writer_lock)

[Mark Seaborn, 2015/01/05 16:58:53]

Nit: remove () brackets

[Sam Clegg, 2015/01/06 19:06:25]

Done.

+ *
+ * The thundering herd problem is avoided by only waking a single
+ * waiter (either a single writer or a single reader) when the
+ * lock is released.
+ */
+
+#include <errno.h>
+#include <unistd.h>

[Mark Seaborn, 2015/01/05 16:58:53]

Is this used?

[Sam Clegg, 2015/01/06 19:06:25]

Done.

+
+#include "native_client/src/include/nacl_compiler_annotations.h"

[Mark Seaborn, 2015/01/05 16:58:53]

Is this used?

[Sam Clegg, 2015/01/06 19:06:24]

Done.

+#include "native_client/src/untrusted/nacl/nacl_irt.h"

[Mark Seaborn, 2015/01/05 16:58:53]

Nit: Not used, AFAICT

[Sam Clegg, 2015/01/06 19:06:24]

Done.

+#include "native_client/src/untrusted/pthread/pthread.h"
+#include "native_client/src/untrusted/pthread/pthread_internal.h"
+#include "native_client/src/untrusted/pthread/pthread_types.h"
+
+int pthread_rwlockattr_init(pthread_rwlockattr_t *attr) {
+  attr->type = PTHREAD_PROCESS_PRIVATE;
+  return 0;
+}
+
+int pthread_rwlockattr_destroy(pthread_rwlockattr_t *attr) {
+  return 0;
+}
+
+int pthread_rwlock_init(pthread_rwlock_t *rwlock,
+                        const pthread_rwlockattr_t *attr) {
+  rwlock->writer_thread_id = NACL_PTHREAD_ILLEGAL_THREAD_ID;
+  rwlock->writers_waiting = 0;
+  rwlock->reader_count = 0;
+  int rc = pthread_mutex_init(&rwlock->mutex, NULL);
+  if (rc != 0)
+    return rc;
+  rc = pthread_cond_init(&rwlock->write_possible, NULL);
+  if (rc != 0)
+    return rc;
+  return pthread_cond_init(&rwlock->read_possible, NULL);
+}
+
+/*
+ * Helper function used by waiting writers to determine if they can take
+ * the lock. The rwlock->mutex must be held when calling this function.
+ * Returns 1 if the write lock can be taken, 0 it it can't.

[Mark Seaborn, 2015/01/05 16:58:53]

"if it"

[Sam Clegg, 2015/01/06 19:06:24]

Done.

+ */
+static inline int write_lock_available(pthread_rwlock_t *rwlock) {
+  /*
+   * Write lock is available if there is no current writer and no current
+   * readers.
+   */
+  if (rwlock->writer_thread_id != NACL_PTHREAD_ILLEGAL_THREAD_ID)
+    return 0;
+  if (rwlock->reader_count > 0)
+    return 0;
+  return 1;
+}
+
+/*
+ * Helper function used by waiting readers to determine if they can take
+ * the lock. The rwlock->mutex must be held when calling this function.
+ * Returns 1 if the write lock can be taken, 0 it it can't.

[Mark Seaborn, 2015/01/05 16:58:53]

Ditto

[Sam Clegg, 2015/01/06 19:06:24]

Done.

+ */
+static inline int read_lock_available(pthread_rwlock_t *rwlock) {
+  /*
+   * Read lock is available if there is no current writer and no *waiting*
+   * writers.
+   */
+  if (rwlock->writer_thread_id != NACL_PTHREAD_ILLEGAL_THREAD_ID)
+    return 0;
+  if (rwlock->writers_waiting > 0)
+    return 0;
+  return 1;
+}
+
+/*
+ * Internal function used to acquire the read lock.
+ * This operates is three differnet ways in order to implement the three public

[Mark Seaborn, 2015/01/05 16:58:53]

"operates in", "different"

[Sam Clegg, 2015/01/06 19:06:24]

Done.

+ * functions:
+ *  pthread_rwlock_rdlock
+ *  pthread_rwlock_tryrdlock
+ *  pthread_rwlock_timedrdlock
+ */
+static int rdlock_internal(pthread_rwlock_t *rwlock,
+                           const struct timespec *abs_timeout,
+                           int try_only) {
+  int rc = pthread_mutex_lock(&rwlock->mutex);
+  if (rc != 0)
+    return rc;
+
+  /*
+   * Wait repeatedly until the write preconditions are met.
+   * In theory this loop should only execute once because the preconditions
+   * should always be true when the condition is signaled.
+   */
+  while (!read_lock_available(rwlock)) {
+    if (try_only) {
+      rc = EBUSY;
+    } else if (abs_timeout != NULL) {
+      rc = pthread_cond_timedwait(&rwlock->read_possible,
+                                  &rwlock->mutex,
+                                  abs_timeout);
+

[Mark Seaborn, 2015/01/05 16:58:53]

Remove empty line

[Sam Clegg, 2015/01/06 19:06:24]

Done.

+    } else {
+      rc = pthread_cond_wait(&rwlock->read_possible, &rwlock->mutex);
+    }
+    if (rc != 0)
+      goto done;
+  }
+
+  /* Acquire the read lock. */
+  rwlock->reader_count++;
+done:
+  pthread_mutex_unlock(&rwlock->mutex);
+  return rc;
+}
+
+/*
+ * Internal function used to acquire the write lock.
+ * This operates is three differnet ways in order to implement the three public

[Mark Seaborn, 2015/01/05 16:58:53]

Same here

[Sam Clegg, 2015/01/06 19:06:24]

Done.

+ * functions:
+ *  pthread_rwlock_wrlock
+ *  pthread_rwlock_trywrlock
+ *  pthread_rwlock_timedwrlock
+ */
+static int rwlock_internal(pthread_rwlock_t *rwlock,
+                           const struct timespec *abs_timeout,
+                           int try_only) {
+  int rc = pthread_mutex_lock(&rwlock->mutex);
+  if (rc != 0)
+    return rc;
+
+  /* Wait repeatedly until the write preconditions are met */
+  while (!write_lock_available(rwlock)) {
+    if (try_only) {
+      rc = EBUSY;
+    } else {
+      /*
+       * Before waiting (and releasing the lock) we increment the
+       * waiting_writers count so the unlocking code knows to wake
+       * us first (before any waiting readers).
+       */
+      rwlock->writers_waiting++;
+      if (abs_timeout) {

[Mark Seaborn, 2015/01/05 16:58:53]

Style nit: Add "!= NULL"

[Sam Clegg, 2015/01/06 19:06:24]

Done.

+        rc = pthread_cond_timedwait(&rwlock->write_possible,
+                                    &rwlock->mutex,
+                                    abs_timeout);
+      } else {
+        rc = pthread_cond_wait(&rwlock->write_possible,
+                               &rwlock->mutex);
+      }
+      rwlock->writers_waiting--;
+    }
+    if (rc != 0)
+      goto done;
+  }
+
+  /* Acquire the write lock. */
+  rwlock->writer_thread_id = pthread_self();
+done:
+  pthread_mutex_unlock(&rwlock->mutex);

[Mark Seaborn, 2015/01/05 16:58:53]

You should really check the return value here too.

[Sam Clegg, 2015/01/06 19:06:25]

Done.

+  return rc;
+}
+
+int pthread_rwlock_timedrdlock(pthread_rwlock_t *rwlock,
+                               const struct timespec *abs_timeout) {
+  return rdlock_internal(rwlock, abs_timeout, 0);
+}
+
+int pthread_rwlock_rdlock(pthread_rwlock_t *rwlock) {
+  return rdlock_internal(rwlock, NULL, 0);
+}
+
+int pthread_rwlock_tryrdlock(pthread_rwlock_t *rwlock) {
+  return rdlock_internal(rwlock, NULL, 1);
+}
+
+int pthread_rwlock_wrlock(pthread_rwlock_t *rwlock) {
+  return rwlock_internal(rwlock, NULL, 0);
+}
+
+int pthread_rwlock_timedwrlock(pthread_rwlock_t *rwlock,
+                               const struct timespec *abs_timeout) {
+  return rwlock_internal(rwlock, abs_timeout, 0);
+}
+
+int pthread_rwlock_trywrlock(pthread_rwlock_t *rwlock) {
+  return rwlock_internal(rwlock, NULL, 1);
+}
+
+int pthread_rwlock_unlock(pthread_rwlock_t *rwlock) {
+  int rc = pthread_mutex_lock(&rwlock->mutex);
+  if (rc != 0)
+    return rc;
+
+  if (rwlock->writer_thread_id != NACL_PTHREAD_ILLEGAL_THREAD_ID) {
+    /* The write lock is held. Ensure its us that holds it. */

[Mark Seaborn, 2015/01/05 16:58:53]

"it's"

"us" is vague.  How about "Ensure it's the current thread that holds it"?

[Sam Clegg, 2015/01/06 19:06:24]

Done.

+    if (rwlock->writer_thread_id != pthread_self()) {
+      rc = EPERM;
+      goto done;
+    }
+
+    /* Release write lock. */
+    rwlock->writer_thread_id = NACL_PTHREAD_ILLEGAL_THREAD_ID;
+    if (rwlock->writers_waiting > 0) {
+      /* Wake a waiting writer if there is one. */
+      rc = pthread_cond_signal(&rwlock->write_possible);
+    } else {
+      /* Otherwise wake a waiting reader. */
+      rc = pthread_cond_signal(&rwlock->read_possible);
+    }
+  } else {
+    if (rwlock->reader_count == 0) {
+      rc = EPERM;
+      goto done;
+    }
+
+    /* Release read lock. */
+    rwlock->reader_count--;
+    if (rwlock->reader_count == 0 && rwlock->writers_waiting > 0) {
+      /* Wake a waiting writer. */
+      rc = pthread_cond_signal(&rwlock->write_possible);
+    }
+  }
+
+done:
+  pthread_mutex_unlock(&rwlock->mutex);

[Mark Seaborn, 2015/01/05 16:58:53]

Ditto: check return value

[Sam Clegg, 2015/01/06 19:06:25]

Done.

+  return rc;
+}
+
+int pthread_rwlock_destroy(pthread_rwlock_t *rwlock) {
+  /* Return EBUSY if anther thread hold the mutex */

[Mark Seaborn, 2015/01/05 16:58:53]

"another", "holds".  (Can you proof-read your comments, please?)

[Sam Clegg, 2015/01/06 19:06:24]

Done.

+  int rc = pthread_mutex_trylock(&rwlock->mutex);
+  if (rc != 0) {
+    return rc;
+  }
+
+  /* Return EBUSY if there are current readers or a current writer */
+  if (rwlock->reader_count != 0) {
+    pthread_mutex_unlock(&rwlock->mutex);
+    return EBUSY;
+  }
+  if (rwlock->writer_thread_id != NACL_PTHREAD_ILLEGAL_THREAD_ID) {
+    pthread_mutex_unlock(&rwlock->mutex);
+    return EBUSY;
+  }
+
+  int rc1 = pthread_cond_destroy(&rwlock->write_possible);
+  int rc2 = pthread_cond_destroy(&rwlock->read_possible);
+
+  /* Finally unlock the mutex and destroy it */
+  pthread_mutex_unlock(&rwlock->mutex);
+  int rc3 = pthread_mutex_destroy(&rwlock->mutex);
+  if (rc1 != 0)
+    return rc1;
+  if (rc2 != 0)
+    return rc2;
+  return rc3;
+}