libc: fix and cleanup pthread mutexes

Issue #3503
Fixes #3504

repos/libports/recipes/pkg/test-pthread/runtime

@@ -1,7 +1,7 @@
 <runtime ram="72M" caps="1000" binary="init">
 
 	<events>
-		<timeout meaning="failed" sec="30" />
+		<timeout meaning="failed" sec="60" />
 		<log meaning="succeeded">--- returning from main ---</log>
 		<log meaning="failed">Error: </log>
 		<log meaning="failed">child "test-pthread" exited</log>
@@ -19,11 +19,7 @@
 	<config>
 		<parent-provides>
 			<service name="ROM"/>
-			<service name="IRQ"/>
-			<service name="IO_MEM"/>
-			<service name="IO_PORT"/>
 			<service name="PD"/>
-			<service name="RM"/>
 			<service name="CPU"/>
 			<service name="LOG"/>
 		</parent-provides>

repos/libports/src/lib/libc/pthread.cc

@@ -175,6 +175,197 @@ Libc::Pthread_registry &pthread_registry()
 }
 
 
+/***********
+ ** Mutex **
+ ***********/
+
+namespace Libc {
+	struct Pthread_mutex_normal;
+	struct Pthread_mutex_errorcheck;
+	struct Pthread_mutex_recursive;
+}
+
+/*
+ * This class is named 'struct pthread_mutex_attr' because the
+ * 'pthread_mutexattr_t' type is defined as 'struct pthread_mutex_attr *'
+ * in '_pthreadtypes.h'
+ */
+struct pthread_mutex_attr { pthread_mutextype type; };
+
+
+/*
+ * This class is named 'struct pthread_mutex' because the 'pthread_mutex_t'
+ * type is defined as 'struct pthread_mutex *' in '_pthreadtypes.h'
+ */
+struct pthread_mutex
+{
+	Lock      _lock; /* actual lock for blocking/deblocking */
+
+	pthread_t _owner      { nullptr };
+	unsigned  _lock_count { 0 };
+	Lock      _owner_and_counter_mutex;
+
+	pthread_mutex() { }
+
+	virtual ~pthread_mutex() { }
+
+	/*
+	 * The behavior of the following function follows the "robust mutex"
+	 * described IEEE Std 1003.1 POSIX.1-2017
+	 * https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutex_lock.html
+	 */
+	virtual int lock()    = 0;
+	virtual int trylock() = 0;
+	virtual int unlock()  = 0;
+};
+
+
+struct Libc::Pthread_mutex_normal : pthread_mutex
+{
+	int lock() override final
+	{
+		while (trylock() == EBUSY) {
+			/*
+			 * We did not get the lock, so, yield the CPU and retry. This
+			 * may implicitly dead-lock if we are already the lock owner.
+			 */
+			_lock.lock();
+			_lock.unlock();
+		}
+
+		return 0;
+	}
+
+	int trylock() override final
+	{
+		Lock::Guard lock_guard(_owner_and_counter_mutex);
+
+		if (!_owner) {
+			_owner = pthread_self();
+			_lock.lock(); /* always succeeds */
+			return 0;
+		}
+
+		return EBUSY;
+	}
+
+	int unlock() override final
+	{
+		Lock::Guard lock_guard(_owner_and_counter_mutex);
+
+		if (_owner != pthread_self())
+			return EPERM;
+
+		_owner = nullptr;
+		_lock.unlock();
+
+		return 0;
+	}
+};
+
+
+struct Libc::Pthread_mutex_errorcheck : pthread_mutex
+{
+	int lock() override final
+	{
+		/*
+		 * We can't use trylock() as it returns EBUSY also for the
+		 * EDEADLK case.
+		 */
+		while (true) {
+			{
+				Lock::Guard lock_guard(_owner_and_counter_mutex);
+
+				if (!_owner) {
+					_owner = pthread_self();
+					_lock.lock(); /* always succeeds */
+					return 0;
+				} else if (_owner == pthread_self()) {
+					return EDEADLK;
+				}
+			}
+			/* mutex has another owner, so, yield the CPU and retry */
+			_lock.lock();
+			_lock.unlock();
+		}
+	}
+
+	int trylock() override final
+	{
+		Lock::Guard lock_guard(_owner_and_counter_mutex);
+
+		if (!_owner) {
+			_owner = pthread_self();
+			_lock.lock(); /* always succeeds */
+			return 0;
+		}
+
+		return EBUSY;
+	}
+
+	int unlock() override final
+	{
+		Lock::Guard lock_guard(_owner_and_counter_mutex);
+
+		if (_owner != pthread_self())
+			return EPERM;
+
+		_owner = nullptr;
+		_lock.unlock();
+
+		return 0;
+	}
+};
+
+
+struct Libc::Pthread_mutex_recursive : pthread_mutex
+{
+	int lock() override final
+	{
+		while (trylock() == EBUSY) {
+			/* mutex has another owner, so, yield the CPU and retry */
+			_lock.lock();
+			_lock.unlock();
+		}
+
+		return 0;
+	}
+
+	int trylock() override final
+	{
+		Lock::Guard lock_guard(_owner_and_counter_mutex);
+
+		if (!_owner) {
+			_owner      = pthread_self();
+			_lock_count = 1;
+			_lock.lock(); /* always succeeds */
+			return 0;
+		} else if (_owner == pthread_self()) {
+			++_lock_count;
+			return 0;
+		}
+
+		return EBUSY;
+	}
+
+	int unlock() override final
+	{
+		Lock::Guard lock_guard(_owner_and_counter_mutex);
+
+		if (_owner != pthread_self())
+			return EPERM;
+
+		--_lock_count;
+		if (_lock_count == 0) {
+			_owner = nullptr;
+			_lock.unlock();
+		}
+
+		return 0;
+	}
+};
+
+
 extern "C" {
 
 	/* Thread */
@@ -345,176 +536,13 @@ extern "C" {
 
 	/* Mutex */
 
-
-	struct pthread_mutex_attr
-	{
-		int type;
-
-		pthread_mutex_attr() : type(PTHREAD_MUTEX_NORMAL) { }
-	};
-
-
-	struct pthread_mutex
-	{
-		pthread_mutex_attr mutexattr;
-
-		Lock mutex_lock;
-
-		pthread_t owner;
-		int       lock_count;
-		Lock      owner_and_counter_lock;
-
-		pthread_mutex(const pthread_mutexattr_t *__restrict attr)
-		: owner(0),
-		  lock_count(0)
-		{
-			if (attr && *attr)
-				mutexattr = **attr;
-		}
-
-		int lock()
-		{
-			if (mutexattr.type == PTHREAD_MUTEX_RECURSIVE) {
-
-				Lock::Guard lock_guard(owner_and_counter_lock);
-
-				if (lock_count == 0) {
-					owner = pthread_self();
-					lock_count++;
-					mutex_lock.lock();
-					return 0;
-				}
-
-				/* the mutex is already locked */
-				if (pthread_self() == owner) {
-					lock_count++;
-					return 0;
-				} else {
-					mutex_lock.lock();
-					return 0;
-				}
-			}
-
-			if (mutexattr.type == PTHREAD_MUTEX_ERRORCHECK) {
-
-				Lock::Guard lock_guard(owner_and_counter_lock);
-
-				if (lock_count == 0) {
-					owner = pthread_self();
-					mutex_lock.lock();
-					return 0;
-				}
-
-				/* the mutex is already locked */
-				if (pthread_self() != owner) {
-					mutex_lock.lock();
-					return 0;
-				} else
-					return EDEADLK;
-			}
-
-			/* PTHREAD_MUTEX_NORMAL or PTHREAD_MUTEX_DEFAULT */
-			mutex_lock.lock();
-			return 0;
-		}
-
-		int trylock()
-		{
-			if (mutexattr.type == PTHREAD_MUTEX_RECURSIVE) {
-
-				Lock::Guard lock_guard(owner_and_counter_lock);
-
-				if (lock_count == 0) {
-					owner = pthread_self();
-					lock_count++;
-					mutex_lock.lock();
-					return 0;
-				}
-
-				/* the mutex is already locked */
-				if (pthread_self() == owner) {
-					lock_count++;
-					return 0;
-				} else {
-					return EBUSY;
-				}
-			}
-
-			if (mutexattr.type == PTHREAD_MUTEX_ERRORCHECK) {
-
-				Lock::Guard lock_guard(owner_and_counter_lock);
-
-				if (lock_count == 0) {
-					owner = pthread_self();
-					mutex_lock.lock();
-					return 0;
-				}
-
-				/* the mutex is already locked */
-				if (pthread_self() != owner) {
-					return EBUSY;
-				} else
-					return EDEADLK;
-			}
-
-			/* PTHREAD_MUTEX_NORMAL or PTHREAD_MUTEX_DEFAULT */
-			Lock::Guard lock_guard(owner_and_counter_lock);
-
-			if (lock_count == 0) {
-				owner = pthread_self();
-				mutex_lock.lock();
-				return 0;
-			}
-
-			return EBUSY;
-		}
-
-		int unlock()
-		{
-
-			if (mutexattr.type == PTHREAD_MUTEX_RECURSIVE) {
-
-				Lock::Guard lock_guard(owner_and_counter_lock);
-
-				if (pthread_self() != owner)
-					return EPERM;
-
-				lock_count--;
-
-				if (lock_count == 0) {
-					owner = 0;
-					mutex_lock.unlock();
-				}
-
-				return 0;
-			}
-
-			if (mutexattr.type == PTHREAD_MUTEX_ERRORCHECK) {
-
-				Lock::Guard lock_guard(owner_and_counter_lock);
-
-				if (pthread_self() != owner)
-					return EPERM;
-
-				owner = 0;
-				mutex_lock.unlock();
-				return 0;
-			}
-
-			/* PTHREAD_MUTEX_NORMAL or PTHREAD_MUTEX_DEFAULT */
-			mutex_lock.unlock();
-			return 0;
-		}
-	};
-
-
 	int pthread_mutexattr_init(pthread_mutexattr_t *attr)
 	{
 		if (!attr)
 			return EINVAL;
 
 		Libc::Allocator alloc { };
-		*attr = new (alloc) pthread_mutex_attr;
+		*attr = new (alloc) pthread_mutex_attr { PTHREAD_MUTEX_NORMAL };
 
 		return 0;
 	}
@@ -527,7 +555,7 @@ extern "C" {
 
 		Libc::Allocator alloc { };
 		destroy(alloc, *attr);
-		*attr = 0;
+		*attr = nullptr;
 
 		return 0;
 	}
@@ -538,20 +566,32 @@ extern "C" {
 		if (!attr || !*attr)
 			return EINVAL;
 
-		(*attr)->type = type;
+		(*attr)->type = (pthread_mutextype)type;
 
 		return 0;
 	}
 
 
-	int pthread_mutex_init(pthread_mutex_t *__restrict mutex,
+	int pthread_mutex_init(pthread_mutex_t *mutex,
-	                       const pthread_mutexattr_t *__restrict attr)
+	                       pthread_mutexattr_t const *attr)
 	{
 		if (!mutex)
 			return EINVAL;
 
+
 		Libc::Allocator alloc { };
-		*mutex = new (alloc) pthread_mutex(attr);
+
+		pthread_mutextype const type = (!attr || !*attr)
+		                             ? PTHREAD_MUTEX_NORMAL : (*attr)->type;
+		switch (type) {
+		case PTHREAD_MUTEX_NORMAL:     *mutex = new (alloc) Pthread_mutex_normal; break;
+		case PTHREAD_MUTEX_ERRORCHECK: *mutex = new (alloc) Pthread_mutex_errorcheck; break;
+		case PTHREAD_MUTEX_RECURSIVE:  *mutex = new (alloc) Pthread_mutex_recursive; break;
+
+		default:
+			*mutex = nullptr;
+			return EINVAL;
+		}
 
 		return 0;
 	}
@@ -576,11 +616,9 @@ extern "C" {
 			return EINVAL;
 
 		if (*mutex == PTHREAD_MUTEX_INITIALIZER)
-			pthread_mutex_init(mutex, 0);
+			pthread_mutex_init(mutex, nullptr);
 
-		(*mutex)->lock();
+		return (*mutex)->lock();
-
-		return 0;
 	}
 
 
@@ -590,7 +628,7 @@ extern "C" {
 			return EINVAL;
 
 		if (*mutex == PTHREAD_MUTEX_INITIALIZER)
-			pthread_mutex_init(mutex, 0);
+			pthread_mutex_init(mutex, nullptr);
 
 		return (*mutex)->trylock();
 	}
@@ -602,11 +640,9 @@ extern "C" {
 			return EINVAL;
 
 		if (*mutex == PTHREAD_MUTEX_INITIALIZER)
-			pthread_mutex_init(mutex, 0);
+			return EINVAL;
 
-		(*mutex)->unlock();
+		return (*mutex)->unlock();
-
-		return 0;
 	}
 
 
@@ -967,30 +1003,28 @@ extern "C" {
 	{
 		if (!once || ((once->state != PTHREAD_NEEDS_INIT) &&
 		              (once->state != PTHREAD_DONE_INIT)))
-			return EINTR;
+			return EINVAL;
 
 		if (!once->mutex) {
-			Libc::Allocator alloc { };
+			pthread_mutex_t p;
-			pthread_mutex_t p = new (alloc) pthread_mutex(0);
+			pthread_mutex_init(&p, nullptr);
-			/* be paranoid */
+			if (!p) return EINVAL;
-			if (!p)
-				return EINTR;
 
+			{
 				static Lock lock;
+				Lock::Guard guard(lock);
 
-			lock.lock();
 				if (!once->mutex) {
 					once->mutex = p;
 					p = nullptr;
 				}
-			lock.unlock();
+			}
 
 			/*
 			 * If another thread concurrently allocated a mutex and was faster,
 			 * free our mutex since it is not used.
 			 */
-			if (p)
+			if (p) pthread_mutex_destroy(&p);
-				destroy(alloc, p);
 		}
 
 		once->mutex->lock();

repos/libports/src/lib/libc/semaphore.cc

@@ -32,7 +32,7 @@ extern "C" {
 	 * This class is named 'struct sem' because the 'sem_t' type is
 	 * defined as 'struct sem*' in 'semaphore.h'
 	 */
-	struct sem : Semaphore
+	struct sem : Genode::Semaphore
 	{
 		sem(int value) : Semaphore(value) { }
 	};

repos/libports/src/test/pthread/main.cc

@@ -1,6 +1,7 @@
 /*
  * \brief  POSIX thread and semaphore test
  * \author Christian Prochaska
+ * \author Christian Helmuth
  * \date   2012-04-04
  */
 
@@ -17,6 +18,9 @@
 #include <stdio.h>
 #include <stdlib.h>
 
+#include <base/log.h>
+#include <base/thread.h>
+
 
 enum { NUM_THREADS = 2 };
 
@@ -125,9 +129,15 @@ struct Test_mutex_data
 		pthread_mutex_init(&errorcheck_mutex, &errorcheck_mutex_attr);
 		pthread_mutexattr_destroy(&errorcheck_mutex_attr);
 	}
+
+	~Test_mutex_data()
+	{
+		pthread_mutex_destroy(&errorcheck_mutex);
+		pthread_mutex_destroy(&recursive_mutex);
+	}
 };
 
-void *thread_mutex_func(void *arg)
+static void *thread_mutex_func(void *arg)
 {
 	Test_mutex_data *test_mutex_data = (Test_mutex_data*)arg;
 
@@ -251,17 +261,17 @@ void *thread_mutex_func(void *arg)
 	/* wake up main thread */
 	sem_post(&test_mutex_data->test_thread_ready_sem);
 
-	return 0;
+	return nullptr;
 }
 
-void test_mutex()
+static void test_mutex()
 {
 	pthread_t t;
 
 	Test_mutex_data test_mutex_data;
 
 	if (pthread_create(&t, 0, thread_mutex_func, &test_mutex_data) != 0) {
-		printf("error: pthread_create() failed\n");
+		printf("Error: pthread_create() failed\n");
 		exit(-1);
 	}
 
@@ -317,6 +327,134 @@ void test_mutex()
 	pthread_join(t, NULL);
 }
 
+
+template <pthread_mutextype MUTEX_TYPE>
+struct Test_mutex_stress
+{
+	static const char *type_string(pthread_mutextype t)
+	{
+		switch (t) {
+		case PTHREAD_MUTEX_NORMAL:     return "PTHREAD_MUTEX_NORMAL";
+		case PTHREAD_MUTEX_ERRORCHECK: return "PTHREAD_MUTEX_ERRORCHECK";
+		case PTHREAD_MUTEX_RECURSIVE:  return "PTHREAD_MUTEX_RECURSIVE";
+
+		default: break;
+		}
+		return "<unexpected mutex type>";
+	};
+
+	struct Data
+	{
+		pthread_mutexattr_t _attr;
+		pthread_mutex_t     _mutex;
+
+		Data()
+		{
+			pthread_mutexattr_init(&_attr);
+			pthread_mutexattr_settype(&_attr, MUTEX_TYPE);
+			pthread_mutex_init(&_mutex, &_attr);
+			pthread_mutexattr_destroy(&_attr);
+		}
+
+		~Data()
+		{
+			pthread_mutex_destroy(&_mutex);
+		}
+
+		pthread_mutex_t * mutex() { return &_mutex; }
+	} data;
+
+	struct Thread
+	{
+		pthread_mutex_t *_mutex;
+		sem_t            _startup_sem;
+		pthread_t        _thread;
+
+		static void * _entry_trampoline(void *arg)
+		{
+			Thread *t = (Thread *)arg;
+			t->_entry();
+			return nullptr;
+		}
+
+		void _lock()
+		{
+			if (int const err = pthread_mutex_lock(_mutex))
+				Genode::error("lock() returned ", err);
+		}
+
+		void _unlock()
+		{
+			if (int const err = pthread_mutex_unlock(_mutex))
+				Genode::error("unlock() returned ", err);
+		}
+
+		void _entry()
+		{
+			sem_wait(&_startup_sem);
+
+			enum { ROUNDS = 800 };
+
+			for (unsigned i = 0; i < ROUNDS; ++i) {
+				_lock();
+				if (MUTEX_TYPE == PTHREAD_MUTEX_RECURSIVE) {
+					_lock();
+					_lock();
+				}
+
+				/* stay in mutex for some time */
+				for (unsigned volatile d = 0; d < 30000; ++d) ;
+
+				if (MUTEX_TYPE == PTHREAD_MUTEX_RECURSIVE) {
+					_unlock();
+					_unlock();
+				}
+				_unlock();
+			}
+			Genode::log("thread ", this, ": ", (int)ROUNDS, " rounds done");
+		}
+
+		Thread(pthread_mutex_t *mutex) : _mutex(mutex)
+		{
+			sem_init(&_startup_sem, 0, 0);
+
+			if (pthread_create(&_thread, 0, _entry_trampoline, this) != 0) {
+				printf("Error: pthread_create() failed\n");
+				exit(-1);
+			}
+		}
+
+		void start() { sem_post(&_startup_sem); }
+		void join()  { pthread_join(_thread, nullptr); }
+	} threads[10] = {
+		data.mutex(), data.mutex(), data.mutex(), data.mutex(), data.mutex(),
+		data.mutex(), data.mutex(), data.mutex(), data.mutex(), data.mutex(),
+	};
+
+	Test_mutex_stress()
+	{
+		printf("main thread: start %s stress test\n", type_string(MUTEX_TYPE));
+		for (Thread &t : threads) t.start();
+		for (Thread &t : threads) t.join();
+		printf("main thread: finished %s stress test\n", type_string(MUTEX_TYPE));
+	}
+};
+
+
+extern "C" void wait_for_continue();
+
+static void test_mutex_stress()
+{
+	printf("main thread: stressing mutexes\n");
+
+	{ Test_mutex_stress<PTHREAD_MUTEX_NORMAL>     test_normal; }
+	{ Test_mutex_stress<PTHREAD_MUTEX_ERRORCHECK> test_errorcheck; }
+	{ Test_mutex_stress<PTHREAD_MUTEX_RECURSIVE>  test_recursive; }
+
+	printf("main thread: mutex stress testing done\n");
+};
+
+
 int main(int argc, char **argv)
 {
 	printf("--- pthread test ---\n");
@@ -399,7 +537,7 @@ int main(int argc, char **argv)
 	for (int i = 0; i < NUM_THREADS; i++)
 		sem_destroy(&thread[i].thread_args.thread_finished_sem);
 
-	printf("main thread: create pthreads which self de-struct\n");
+	printf("main thread: create pthreads which self destruct\n");
 
 	test_self_destruct(thread_func_self_destruct, 100);
 
@@ -407,6 +545,8 @@ int main(int argc, char **argv)
 
 	test_mutex();
 
+	test_mutex_stress();
+
 	printf("--- returning from main ---\n");
 	return 0;
 }