mike-jumper commented on code in PR #645:
URL: https://github.com/apache/guacamole-server/pull/645#discussion_r2936158952
##########
src/libguac/rwlock.c:
##########
@@ -38,123 +41,173 @@
*/
#define GUAC_REENTRANT_LOCK_WRITE_LOCK 2
-void guac_rwlock_init(guac_rwlock* lock) {
+/**
+ * The maximum number of distinct guac_rwlock instances a single thread may
+ * hold simultaneously.
+ */
+#define GUAC_RWLOCK_MAX_HELD 16
- /* Configure to allow sharing this lock with child processes */
- pthread_rwlockattr_t lock_attributes;
- pthread_rwlockattr_init(&lock_attributes);
- pthread_rwlockattr_setpshared(&lock_attributes, PTHREAD_PROCESS_SHARED);
+/**
+ * Per-lock state tracked for a single thread. One slot is allocated per
+ * distinct lock that the thread currently holds.
+ */
+typedef struct guac_rwlock_thread_state {
- /* Initialize the rwlock */
- pthread_rwlock_init(&(lock->lock), &lock_attributes);
+ /**
+ * The lock this slot describes. NULL indicates the slot is empty.
+ */
+ guac_rwlock* lock;
- /* Initialize the flags to 0, as threads won't have acquired it yet */
- pthread_key_create(&(lock->key), (void *) 0);
+ /**
+ * Which lock the current thread holds: GUAC_REENTRANT_LOCK_NO_LOCK,
+ * GUAC_REENTRANT_LOCK_READ_LOCK, or GUAC_REENTRANT_LOCK_WRITE_LOCK.
+ */
+ int flag;
-}
+ /**
+ * The reentrant depth, representing the number of times the current thread
+ * has acquired this lock without a corresponding release.
+ */
+ unsigned int count;
-void guac_rwlock_destroy(guac_rwlock* lock) {
+} guac_rwlock_thread_state;
- /* Destroy the rwlock */
- pthread_rwlock_destroy(&(lock->lock));
+/**
+ * A single process wide key whose per-thread value is a pointer to that
+ * thread's array of guac_rwlock_thread_state entries. Created exactly once
+ * via pthread_once.
+ */
+static pthread_key_t guac_rwlock_key;
+static pthread_once_t guac_rwlock_key_init = PTHREAD_ONCE_INIT;
+
+/**
+ * Destructor called by pthreads when a thread exits. Frees the per-thread
+ * state array.
+ */
+static void guac_rwlock_free_pointer(void* pointer) {
- /* Destroy the thread-local key */
- pthread_key_delete(lock->key);
+ guac_mem_free(pointer);
}
/**
- * Clean up and destroy the provided guac reentrant rwlock.
- *
- * @param lock
- * The guac reentrant rwlock to be destroyed.
+ * Creates the single global thread-local key. Invoked exactly once via
+ * pthread_once.
*/
-void guac_rwlock_destroy(guac_rwlock* lock);
+static void guac_rwlock_create_key(void) {
+
+ pthread_key_create(&guac_rwlock_key, guac_rwlock_free_pointer);
+
+}
/**
- * Extract and return the flag indicating which lock is held, if any, from the
- * provided key value. The flag is always stored in the least-significant
- * nibble of the value.
- *
- * @param value
- * The key value containing the flag.
+ * Returns the per-thread state array, allocating and registering it on first
+ * use.
*
* @return
- * The flag indicating which lock is held, if any.
+ * A pointer to the calling thread's guac_rwlock_thread_state array, or
+ * NULL if allocation fails.
*/
-static uintptr_t get_lock_flag(uintptr_t value) {
- return value & 0xF;
+static guac_rwlock_thread_state* guac_rwlock_get_thread_states(void) {
+
+ pthread_once(&guac_rwlock_key_init, guac_rwlock_create_key);
+
+ guac_rwlock_thread_state* states = pthread_getspecific(guac_rwlock_key);
+ if (states == NULL) {
+ states = guac_mem_zalloc(sizeof(guac_rwlock_thread_state),
GUAC_RWLOCK_MAX_HELD);
+ pthread_setspecific(guac_rwlock_key, states);
+ }
+
+ return states;
+
}
/**
- * Extract and return the lock count from the provided key. This returned value
- * is the difference between the number of lock and unlock requests made by the
- * current thread. This count is always stored in the remaining value after the
- * least-significant nibble where the flag is stored.
- *
- * @param value
- * The key value containing the count.
- *
- * @return
- * The difference between the number of lock and unlock requests made by
- * the current thread.
+ * Returns the state slot for the given lock for the current thread, or NULL
+ * if the current thread does not hold that lock.
*/
-static uintptr_t get_lock_count(uintptr_t value) {
- return value >> 4;
+static guac_rwlock_thread_state* guac_rwlock_state_get(guac_rwlock* lock) {
Review Comment:
Please document the parameter and return value.
##########
src/libguac/rwlock.c:
##########
@@ -38,123 +41,173 @@
*/
#define GUAC_REENTRANT_LOCK_WRITE_LOCK 2
-void guac_rwlock_init(guac_rwlock* lock) {
+/**
+ * The maximum number of distinct guac_rwlock instances a single thread may
+ * hold simultaneously.
+ */
+#define GUAC_RWLOCK_MAX_HELD 16
- /* Configure to allow sharing this lock with child processes */
- pthread_rwlockattr_t lock_attributes;
- pthread_rwlockattr_init(&lock_attributes);
- pthread_rwlockattr_setpshared(&lock_attributes, PTHREAD_PROCESS_SHARED);
+/**
+ * Per-lock state tracked for a single thread. One slot is allocated per
+ * distinct lock that the thread currently holds.
+ */
+typedef struct guac_rwlock_thread_state {
- /* Initialize the rwlock */
- pthread_rwlock_init(&(lock->lock), &lock_attributes);
+ /**
+ * The lock this slot describes. NULL indicates the slot is empty.
+ */
+ guac_rwlock* lock;
- /* Initialize the flags to 0, as threads won't have acquired it yet */
- pthread_key_create(&(lock->key), (void *) 0);
+ /**
+ * Which lock the current thread holds: GUAC_REENTRANT_LOCK_NO_LOCK,
+ * GUAC_REENTRANT_LOCK_READ_LOCK, or GUAC_REENTRANT_LOCK_WRITE_LOCK.
+ */
+ int flag;
-}
+ /**
+ * The reentrant depth, representing the number of times the current thread
+ * has acquired this lock without a corresponding release.
+ */
+ unsigned int count;
-void guac_rwlock_destroy(guac_rwlock* lock) {
+} guac_rwlock_thread_state;
- /* Destroy the rwlock */
- pthread_rwlock_destroy(&(lock->lock));
+/**
+ * A single process wide key whose per-thread value is a pointer to that
+ * thread's array of guac_rwlock_thread_state entries. Created exactly once
+ * via pthread_once.
+ */
+static pthread_key_t guac_rwlock_key;
+static pthread_once_t guac_rwlock_key_init = PTHREAD_ONCE_INIT;
+
+/**
+ * Destructor called by pthreads when a thread exits. Frees the per-thread
+ * state array.
+ */
+static void guac_rwlock_free_pointer(void* pointer) {
Review Comment:
Please document the `pointer` parameter.
##########
src/libguac/rwlock.c:
##########
@@ -38,123 +41,173 @@
*/
#define GUAC_REENTRANT_LOCK_WRITE_LOCK 2
-void guac_rwlock_init(guac_rwlock* lock) {
+/**
+ * The maximum number of distinct guac_rwlock instances a single thread may
+ * hold simultaneously.
+ */
+#define GUAC_RWLOCK_MAX_HELD 16
- /* Configure to allow sharing this lock with child processes */
- pthread_rwlockattr_t lock_attributes;
- pthread_rwlockattr_init(&lock_attributes);
- pthread_rwlockattr_setpshared(&lock_attributes, PTHREAD_PROCESS_SHARED);
+/**
+ * Per-lock state tracked for a single thread. One slot is allocated per
+ * distinct lock that the thread currently holds.
+ */
+typedef struct guac_rwlock_thread_state {
- /* Initialize the rwlock */
- pthread_rwlock_init(&(lock->lock), &lock_attributes);
+ /**
+ * The lock this slot describes. NULL indicates the slot is empty.
+ */
+ guac_rwlock* lock;
- /* Initialize the flags to 0, as threads won't have acquired it yet */
- pthread_key_create(&(lock->key), (void *) 0);
+ /**
+ * Which lock the current thread holds: GUAC_REENTRANT_LOCK_NO_LOCK,
+ * GUAC_REENTRANT_LOCK_READ_LOCK, or GUAC_REENTRANT_LOCK_WRITE_LOCK.
+ */
+ int flag;
-}
+ /**
+ * The reentrant depth, representing the number of times the current thread
+ * has acquired this lock without a corresponding release.
+ */
+ unsigned int count;
-void guac_rwlock_destroy(guac_rwlock* lock) {
+} guac_rwlock_thread_state;
- /* Destroy the rwlock */
- pthread_rwlock_destroy(&(lock->lock));
+/**
+ * A single process wide key whose per-thread value is a pointer to that
+ * thread's array of guac_rwlock_thread_state entries. Created exactly once
+ * via pthread_once.
+ */
+static pthread_key_t guac_rwlock_key;
+static pthread_once_t guac_rwlock_key_init = PTHREAD_ONCE_INIT;
+
+/**
+ * Destructor called by pthreads when a thread exits. Frees the per-thread
+ * state array.
+ */
+static void guac_rwlock_free_pointer(void* pointer) {
- /* Destroy the thread-local key */
- pthread_key_delete(lock->key);
+ guac_mem_free(pointer);
}
/**
- * Clean up and destroy the provided guac reentrant rwlock.
- *
- * @param lock
- * The guac reentrant rwlock to be destroyed.
+ * Creates the single global thread-local key. Invoked exactly once via
+ * pthread_once.
*/
-void guac_rwlock_destroy(guac_rwlock* lock);
+static void guac_rwlock_create_key(void) {
+
+ pthread_key_create(&guac_rwlock_key, guac_rwlock_free_pointer);
+
+}
/**
- * Extract and return the flag indicating which lock is held, if any, from the
- * provided key value. The flag is always stored in the least-significant
- * nibble of the value.
- *
- * @param value
- * The key value containing the flag.
+ * Returns the per-thread state array, allocating and registering it on first
+ * use.
*
* @return
- * The flag indicating which lock is held, if any.
+ * A pointer to the calling thread's guac_rwlock_thread_state array, or
+ * NULL if allocation fails.
*/
-static uintptr_t get_lock_flag(uintptr_t value) {
- return value & 0xF;
+static guac_rwlock_thread_state* guac_rwlock_get_thread_states(void) {
+
+ pthread_once(&guac_rwlock_key_init, guac_rwlock_create_key);
+
+ guac_rwlock_thread_state* states = pthread_getspecific(guac_rwlock_key);
+ if (states == NULL) {
+ states = guac_mem_zalloc(sizeof(guac_rwlock_thread_state),
GUAC_RWLOCK_MAX_HELD);
+ pthread_setspecific(guac_rwlock_key, states);
+ }
+
+ return states;
+
}
/**
- * Extract and return the lock count from the provided key. This returned value
- * is the difference between the number of lock and unlock requests made by the
- * current thread. This count is always stored in the remaining value after the
- * least-significant nibble where the flag is stored.
- *
- * @param value
- * The key value containing the count.
- *
- * @return
- * The difference between the number of lock and unlock requests made by
- * the current thread.
+ * Returns the state slot for the given lock for the current thread, or NULL
+ * if the current thread does not hold that lock.
*/
-static uintptr_t get_lock_count(uintptr_t value) {
- return value >> 4;
+static guac_rwlock_thread_state* guac_rwlock_state_get(guac_rwlock* lock) {
+
+ guac_rwlock_thread_state* states = guac_rwlock_get_thread_states();
+
+ for (int i = 0; i < GUAC_RWLOCK_MAX_HELD; i++) {
+ if (states[i].lock == lock)
+ return &states[i];
+ }
+
+ return NULL;
+
}
/**
- * Given a flag indicating if and how the current thread controls a lock, and
- * a count of the depth of lock requests, return a value containing the flag
- * in the least-significant nibble, and the count in the rest.
- *
- * @param flag
- * A flag indicating which lock, if any, is held by the current thread.
- *
- * @param count
- * The depth of the lock attempt by the current thread, i.e. the number of
- * lock requests minus unlock requests.
- *
- * @return
- * A value containing the flag in the least-significant nibble, and the
- * count in the rest, cast to a void* for thread-local storage.
+ * Returns the state slot for the given lock for the current thread, creating
+ * and initializing a new slot if one does not already exist. Returns NULL if
+ * all slots are in use.
*/
-static void* get_value_from_flag_and_count(
- uintptr_t flag, uintptr_t count) {
- return (void*) ((flag & 0xF) | count << 4);
+static guac_rwlock_thread_state* guac_rwlock_state_get_or_create(guac_rwlock*
lock) {
+
+ guac_rwlock_thread_state* states = guac_rwlock_get_thread_states();
+
+ guac_rwlock_thread_state* empty = NULL;
+ for (int i = 0; i < GUAC_RWLOCK_MAX_HELD; i++) {
+ if (states[i].lock == lock)
+ return &states[i];
+ if (empty == NULL && states[i].lock == NULL)
+ empty = &states[i];
+ }
+
+ if (empty != NULL) {
+ empty->lock = lock;
+ empty->flag = GUAC_REENTRANT_LOCK_NO_LOCK;
+ empty->count = 0;
+ }
+
+ return empty;
+
}
/**
- * Return zero if adding one to the current count would overflow the storage
- * allocated to the count, or a non-zero value otherwise.
- *
- * @param current_count
- * The current count for a lock that the current thread is trying to
- * reentrantly acquire.
- *
- * @return
- * Zero if adding one to the current count would overflow the storage
- * allocated to the count, or a non-zero value otherwise.
+ * Clears a state slot, marking it as empty so it can be reused.
*/
-static int would_overflow_count(uintptr_t current_count) {
+static void guac_rwlock_state_clear(guac_rwlock_thread_state* state) {
Review Comment:
Please document the `state` parameter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
