posix: use sys_sem instead of k_spinlock for pool synch

Based on Andy's talk at eoss 2024, use the sys/sem.h api instead
of the spinlock.h api to synchronize pooled elements since it
has minimal overhead like semaphores but also works from
userspace.

Signed-off-by: Chris Friedt <[email protected]>

Author: cfriedt

lib/posix/options/key.c

@@ -11,6 +11,7 @@
 #include <zephyr/posix/pthread.h>
 #include <zephyr/sys/bitarray.h>
 #include <zephyr/sys/__assert.h>
+#include <zephyr/sys/sem.h>
 
 struct pthread_key_data {
 	sys_snode_t node;
@@ -19,7 +20,7 @@ struct pthread_key_data {
 
 LOG_MODULE_REGISTER(pthread_key, CONFIG_PTHREAD_KEY_LOG_LEVEL);
 
-static struct k_spinlock pthread_key_lock;
+static SYS_SEM_DEFINE(pthread_key_lock, 1, 1);
 
 /* This is non-standard (i.e. an implementation detail) */
 #define PTHREAD_KEY_INITIALIZER (-1)
@@ -128,42 +129,40 @@ int pthread_key_create(pthread_key_t *key,
 int pthread_key_delete(pthread_key_t key)
 {
 	size_t bit;
-	__unused int ret;
-	pthread_key_obj *key_obj;
+	int ret = EINVAL;
+	pthread_key_obj *key_obj = NULL;
 	struct pthread_key_data *key_data;
 	sys_snode_t *node_l, *next_node_l;
-	k_spinlock_key_t key_key;
 
-	key_key = k_spin_lock(&pthread_key_lock);
+	SYS_SEM_LOCK(&pthread_key_lock) {
+		key_obj = get_posix_key(key);
+		if (key_obj == NULL) {
+			ret = EINVAL;
+			SYS_SEM_LOCK_BREAK;
+		}
 
-	key_obj = get_posix_key(key);
-	if (key_obj == NULL) {
-		k_spin_unlock(&pthread_key_lock, key_key);
-		return EINVAL;
-	}
+		/* Delete thread-specific elements associated with the key */
+		SYS_SLIST_FOR_EACH_NODE_SAFE(&(key_obj->key_data_l), node_l, next_node_l) {
 
-	/* Delete thread-specific elements associated with the key */
-	SYS_SLIST_FOR_EACH_NODE_SAFE(&(key_obj->key_data_l),
-			node_l, next_node_l) {
+			/* Remove the object from the list key_data_l */
+			key_data = (struct pthread_key_data *)sys_slist_get(&(key_obj->key_data_l));
 
-		/* Remove the object from the list key_data_l */
-		key_data = (struct pthread_key_data *)
-			sys_slist_get(&(key_obj->key_data_l));
+			/* Deallocate the object's memory */
+			k_free((void *)key_data);
+			LOG_DBG("Freed key data %p for key %x in thread %x", key_data, key,
+				pthread_self());
+		}
 
-		/* Deallocate the object's memory */
-		k_free((void *)key_data);
-		LOG_DBG("Freed key data %p for key %x in thread %x", key_data, key, pthread_self());
+		bit = posix_key_to_offset(key_obj);
+		ret = sys_bitarray_free(&posix_key_bitarray, 1, bit);
+		__ASSERT_NO_MSG(ret == 0);
 	}
 
-	bit = posix_key_to_offset(key_obj);
-	ret = sys_bitarray_free(&posix_key_bitarray, 1, bit);
-	__ASSERT_NO_MSG(ret == 0);
-
-	k_spin_unlock(&pthread_key_lock, key_key);
-
-	LOG_DBG("Deleted key %p (%x)", key_obj, key);
+	if (ret == 0) {
+		LOG_DBG("Deleted key %p (%x)", key_obj, key);
+	}
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -173,12 +172,10 @@ int pthread_key_delete(pthread_key_t key)
  */
 int pthread_setspecific(pthread_key_t key, const void *value)
 {
-	pthread_key_obj *key_obj;
+	pthread_key_obj *key_obj = NULL;
 	struct posix_thread *thread;
 	struct pthread_key_data *key_data;
-	pthread_thread_data *thread_spec_data;
-	k_spinlock_key_t key_key;
-	sys_snode_t *node_l;
+	sys_snode_t *node_l = NULL;
 	int retval = 0;
 
 	thread = to_posix_thread(pthread_self());
@@ -190,37 +187,38 @@ int pthread_setspecific(pthread_key_t key, const void *value)
 	 * If the key is already in the list, re-assign its value.
 	 * Else add the key to the thread's list.
 	 */
-	key_key = k_spin_lock(&pthread_key_lock);
-
-	key_obj = get_posix_key(key);
-	if (key_obj == NULL) {
-		k_spin_unlock(&pthread_key_lock, key_key);
-		return EINVAL;
-	}
-
-	SYS_SLIST_FOR_EACH_NODE(&(thread->key_list), node_l) {
+	SYS_SEM_LOCK(&pthread_key_lock) {
+		key_obj = get_posix_key(key);
+		if (key_obj == NULL) {
+			retval = EINVAL;
+			SYS_SEM_LOCK_BREAK;
+		}
 
-		thread_spec_data = (pthread_thread_data *)node_l;
+		SYS_SLIST_FOR_EACH_NODE(&(thread->key_list), node_l) {
+			pthread_thread_data *thread_spec_data = (pthread_thread_data *)node_l;
 
-		if (thread_spec_data->key == key_obj) {
+			if (thread_spec_data->key == key_obj) {
+				/* Key is already present so associate thread specific data */
+				thread_spec_data->spec_data = (void *)value;
+				LOG_DBG("Paired key %x to value %p for thread %x", key, value,
+					pthread_self());
+				break;
+			}
+		}
 
-			/* Key is already present so
-			 * associate thread specific data
-			 */
-			thread_spec_data->spec_data = (void *)value;
-			LOG_DBG("Paired key %x to value %p for thread %x", key, value,
-				pthread_self());
-			goto out;
+		retval = 0;
+		if (node_l != NULL) {
+			/* Key is already present, so we are done */
+			SYS_SEM_LOCK_BREAK;
 		}
-	}
 
-	if (node_l == NULL) {
+		/* Key and data need to be added */
 		key_data = k_malloc(sizeof(struct pthread_key_data));
 
 		if (key_data == NULL) {
 			LOG_DBG("Failed to allocate key data for key %x", key);
 			retval = ENOMEM;
-			goto out;
+			SYS_SEM_LOCK_BREAK;
 		}
 
 		LOG_DBG("Allocated key data %p for key %x in thread %x", key_data, key,
@@ -239,9 +237,6 @@ int pthread_setspecific(pthread_key_t key, const void *value)
 		LOG_DBG("Paired key %x to value %p for thread %x", key, value, pthread_self());
 	}
 
-out:
-	k_spin_unlock(&pthread_key_lock, key_key);
-
 	return retval;
 }
 
@@ -257,33 +252,30 @@ void *pthread_getspecific(pthread_key_t key)
 	pthread_thread_data *thread_spec_data;
 	void *value = NULL;
 	sys_snode_t *node_l;
-	k_spinlock_key_t key_key;
 
 	thread = to_posix_thread(pthread_self());
 	if (thread == NULL) {
 		return NULL;
 	}
 
-	key_key = k_spin_lock(&pthread_key_lock);
-
-	key_obj = get_posix_key(key);
-	if (key_obj == NULL) {
-		k_spin_unlock(&pthread_key_lock, key_key);
-		return NULL;
-	}
+	SYS_SEM_LOCK(&pthread_key_lock) {
+		key_obj = get_posix_key(key);
+		if (key_obj == NULL) {
+			value = NULL;
+			SYS_SEM_LOCK_BREAK;
+		}
 
-	/* Traverse the list of keys set by the thread, looking for key */
+		/* Traverse the list of keys set by the thread, looking for key */
 
-	SYS_SLIST_FOR_EACH_NODE(&(thread->key_list), node_l) {
-		thread_spec_data = (pthread_thread_data *)node_l;
-		if (thread_spec_data->key == key_obj) {
-			/* Key is present, so get the set thread data */
-			value = thread_spec_data->spec_data;
-			break;
+		SYS_SLIST_FOR_EACH_NODE(&(thread->key_list), node_l) {
+			thread_spec_data = (pthread_thread_data *)node_l;
+			if (thread_spec_data->key == key_obj) {
+				/* Key is present, so get the set thread data */
+				value = thread_spec_data->spec_data;
+				break;
+			}
 		}
 	}
 
-	k_spin_unlock(&pthread_key_lock, key_key);
-
 	return value;
 }

lib/posix/options/mutex.c

@@ -12,10 +12,11 @@
 #include <zephyr/logging/log.h>
 #include <zephyr/posix/pthread.h>
 #include <zephyr/sys/bitarray.h>
+#include <zephyr/sys/sem.h>
 
 LOG_MODULE_REGISTER(pthread_mutex, CONFIG_PTHREAD_MUTEX_LOG_LEVEL);
 
-static struct k_spinlock pthread_mutex_spinlock;
+static SYS_SEM_DEFINE(lock, 1, 1);
 
 int64_t timespec_to_timeoutms(const struct timespec *abstime);
 
@@ -106,43 +107,50 @@ struct k_mutex *to_posix_mutex(pthread_mutex_t *mu)
 
 static int acquire_mutex(pthread_mutex_t *mu, k_timeout_t timeout)
 {
-	int type;
-	size_t bit;
-	int ret = 0;
-	struct k_mutex *m;
-	k_spinlock_key_t key;
+	int type = -1;
+	size_t bit = -1;
+	int ret = EINVAL;
+	size_t lock_count = -1;
+	struct k_mutex *m = NULL;
+	struct k_thread *owner = NULL;
+
+	SYS_SEM_LOCK(&lock) {
+		m = to_posix_mutex(mu);
+		if (m == NULL) {
+			ret = EINVAL;
+			SYS_SEM_LOCK_BREAK;
+		}
 
-	key = k_spin_lock(&pthread_mutex_spinlock);
+		LOG_DBG("Locking mutex %p with timeout %llx", m, timeout.ticks);
 
-	m = to_posix_mutex(mu);
-	if (m == NULL) {
-		k_spin_unlock(&pthread_mutex_spinlock, key);
-		return EINVAL;
+		ret = 0;
+		bit = posix_mutex_to_offset(m);
+		type = posix_mutex_type[bit];
+		owner = m->owner;
+		lock_count = m->lock_count;
 	}
 
-	LOG_DBG("Locking mutex %p with timeout %llx", m, timeout.ticks);
-
-	bit = posix_mutex_to_offset(m);
-	type = posix_mutex_type[bit];
+	if (ret != 0) {
+		goto handle_error;
+	}
 
-	if (m->owner == k_current_get()) {
+	if (owner == k_current_get()) {
 		switch (type) {
 		case PTHREAD_MUTEX_NORMAL:
 			if (K_TIMEOUT_EQ(timeout, K_NO_WAIT)) {
-				k_spin_unlock(&pthread_mutex_spinlock, key);
 				LOG_DBG("Timeout locking mutex %p", m);
-				return EBUSY;
+				ret = EBUSY;
+				break;
 			}
 			/* On most POSIX systems, this usually results in an infinite loop */
-			k_spin_unlock(&pthread_mutex_spinlock, key);
 			LOG_DBG("Attempt to relock non-recursive mutex %p", m);
 			do {
 				(void)k_sleep(K_FOREVER);
 			} while (true);
 			CODE_UNREACHABLE;
 			break;
 		case PTHREAD_MUTEX_RECURSIVE:
-			if (m->lock_count >= MUTEX_MAX_REC_LOCK) {
+			if (lock_count >= MUTEX_MAX_REC_LOCK) {
 				LOG_DBG("Mutex %p locked recursively too many times", m);
 				ret = EAGAIN;
 			}
@@ -157,7 +165,6 @@ static int acquire_mutex(pthread_mutex_t *mu, k_timeout_t timeout)
 			break;
 		}
 	}
-	k_spin_unlock(&pthread_mutex_spinlock, key);
 
 	if (ret == 0) {
 		ret = k_mutex_lock(m, timeout);
@@ -171,6 +178,7 @@ static int acquire_mutex(pthread_mutex_t *mu, k_timeout_t timeout)
 		}
 	}
 
+handle_error:
 	if (ret < 0) {
 		LOG_DBG("k_mutex_unlock() failed: %d", ret);
 		ret = -ret;