libc: suspend/resume in pthread mutex lock/unlock

Issue #3550

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

@@ -1,7 +1,9 @@
 <runtime ram="72M" caps="1000" binary="init">
 
+	<requires> <timer/> </requires>
+
 	<events>
-		<timeout meaning="failed" sec="60" />
+		<timeout meaning="failed" sec="70" />
 		<log meaning="succeeded">--- returning from main ---</log>
 		<log meaning="failed">Error: </log>
 		<log meaning="failed">child "test-pthread" exited</log>
@@ -22,6 +24,7 @@
 			<service name="PD"/>
 			<service name="CPU"/>
 			<service name="LOG"/>
+			<service name="Timer"/>
 		</parent-provides>
 		<default-route>
 			<any-service> <parent/> <any-child/> </any-service>

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

@@ -87,7 +87,7 @@ void Libc::Pthread::join(void **retval)
 		Pthread &_thread;
 
 		Check(Pthread &thread) : _thread(thread) { }
-		
+
 		bool suspend() override
 		{
 			retry = !_thread._exiting;
@@ -199,11 +199,24 @@ struct pthread_mutex_attr { pthread_mutextype type; };
  */
 struct pthread_mutex
 {
-	Lock      _lock; /* actual lock for blocking/deblocking */
+	pthread_t _owner { nullptr };
+	Lock      _data_mutex;
 
-	pthread_t _owner      { nullptr };
-	unsigned  _lock_count { 0 };
-	Lock      _owner_and_counter_mutex;
+	struct Missing_call_of_init_pthread_support : Exception { };
+
+	void _suspend(Suspend_functor &func)
+	{
+		if (!_suspend_ptr)
+			throw Missing_call_of_init_pthread_support();
+		_suspend_ptr->suspend(func);
+	}
+
+	void _resume_all()
+	{
+		if (!_resume_ptr)
+			throw Missing_call_of_init_pthread_support();
+		_resume_ptr->resume_all();
+	}
 
 	pthread_mutex() { }
 
@@ -224,25 +237,32 @@ 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();
-		}
+		struct Try_lock : Suspend_functor
+		{
+			bool retry { false }; /* have to try after resume */
+
+			Pthread_mutex_normal &_mutex;
+
+			Try_lock(Pthread_mutex_normal &mutex) : _mutex(mutex) { }
+
+			bool suspend() override
+			{
+				retry = _mutex.trylock() == EBUSY;
+				return retry;
+			}
+		} try_lock(*this);
+
+		do { _suspend(try_lock); } while (try_lock.retry);
 
 		return 0;
 	}
 
 	int trylock() override final
 	{
-		Lock::Guard lock_guard(_owner_and_counter_mutex);
+		Lock::Guard lock_guard(_data_mutex);
 
 		if (!_owner) {
 			_owner = pthread_self();
-			_lock.lock(); /* always succeeds */
 			return 0;
 		}
 
@@ -251,13 +271,13 @@ struct Libc::Pthread_mutex_normal : pthread_mutex
 
 	int unlock() override final
 	{
-		Lock::Guard lock_guard(_owner_and_counter_mutex);
+		Lock::Guard lock_guard(_data_mutex);
 
 		if (_owner != pthread_self())
 			return EPERM;
 
 		_owner = nullptr;
-		_lock.unlock();
+		_resume_all();
 
 		return 0;
 	}
@@ -272,31 +292,45 @@ struct Libc::Pthread_mutex_errorcheck : pthread_mutex
 		 * We can't use trylock() as it returns EBUSY also for the
 		 * EDEADLK case.
 		 */
-		while (true) {
-			{
-				Lock::Guard lock_guard(_owner_and_counter_mutex);
+		struct Try_lock : Suspend_functor
+		{
+			bool retry  { false }; /* have to try after resume */
+			int  result { 0 };
 
-				if (!_owner) {
-					_owner = pthread_self();
-					_lock.lock(); /* always succeeds */
-					return 0;
-				} else if (_owner == pthread_self()) {
-					return EDEADLK;
+			Pthread_mutex_errorcheck &_mutex;
+
+			Try_lock(Pthread_mutex_errorcheck &mutex) : _mutex(mutex) { }
+
+			bool suspend() override
+			{
+				Lock::Guard lock_guard(_mutex._data_mutex);
+
+				if (!_mutex._owner) {
+					_mutex._owner = pthread_self();
+					retry         = false;
+					result        = 0;
+				} else if (_mutex._owner == pthread_self()) {
+					retry  = false;
+					result = EDEADLK;
+				} else {
+					retry = true;
 				}
+
+				return retry;
 			}
-			/* mutex has another owner, so, yield the CPU and retry */
-			_lock.lock();
-			_lock.unlock();
-		}
+		} try_lock(*this);
+
+		do { _suspend(try_lock); } while (try_lock.retry);
+
+		return try_lock.result;
 	}
 
 	int trylock() override final
 	{
-		Lock::Guard lock_guard(_owner_and_counter_mutex);
+		Lock::Guard lock_guard(_data_mutex);
 
 		if (!_owner) {
 			_owner = pthread_self();
-			_lock.lock(); /* always succeeds */
 			return 0;
 		}
 
@@ -305,13 +339,13 @@ struct Libc::Pthread_mutex_errorcheck : pthread_mutex
 
 	int unlock() override final
 	{
-		Lock::Guard lock_guard(_owner_and_counter_mutex);
+		Lock::Guard lock_guard(_data_mutex);
 
 		if (_owner != pthread_self())
 			return EPERM;
 
 		_owner = nullptr;
-		_lock.unlock();
+		_resume_all();
 
 		return 0;
 	}
@@ -320,28 +354,40 @@ struct Libc::Pthread_mutex_errorcheck : pthread_mutex
 
 struct Libc::Pthread_mutex_recursive : pthread_mutex
 {
+	unsigned _nesting_level { 0 };
+
 	int lock() override final
 	{
-		while (trylock() == EBUSY) {
-			/* mutex has another owner, so, yield the CPU and retry */
-			_lock.lock();
-			_lock.unlock();
-		}
+		struct Try_lock : Suspend_functor
+		{
+			bool retry { false }; /* have to try after resume */
+
+			Pthread_mutex_recursive &_mutex;
+
+			Try_lock(Pthread_mutex_recursive &mutex) : _mutex(mutex) { }
+
+			bool suspend() override
+			{
+				retry = _mutex.trylock() == EBUSY;
+				return retry;
+			}
+		} try_lock(*this);
+
+		do { _suspend(try_lock); } while (try_lock.retry);
 
 		return 0;
 	}
 
 	int trylock() override final
 	{
-		Lock::Guard lock_guard(_owner_and_counter_mutex);
+		Lock::Guard lock_guard(_data_mutex);
 
 		if (!_owner) {
-			_owner      = pthread_self();
-			_lock_count = 1;
-			_lock.lock(); /* always succeeds */
+			_owner         = pthread_self();
+			_nesting_level = 1;
 			return 0;
 		} else if (_owner == pthread_self()) {
-			++_lock_count;
+			++_nesting_level;
 			return 0;
 		}
 
@@ -350,15 +396,15 @@ struct Libc::Pthread_mutex_recursive : pthread_mutex
 
 	int unlock() override final
 	{
-		Lock::Guard lock_guard(_owner_and_counter_mutex);
+		Lock::Guard lock_guard(_data_mutex);
 
 		if (_owner != pthread_self())
 			return EPERM;
 
-		--_lock_count;
-		if (_lock_count == 0) {
+		--_nesting_level;
+		if (_nesting_level == 0) {
 			_owner = nullptr;
-			_lock.unlock();
+			_resume_all();
 		}
 
 		return 0;

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

@@ -18,6 +18,7 @@
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <unistd.h>
 
 /* Genode includes */
 #include <base/log.h>
@@ -470,6 +471,77 @@ static void test_mutex_stress()
 };
 
 
+/*
+ * Test if the main thread resumes sleeping lock holders when it itself is
+ * waiting for the lock.
+ */
+
+template <pthread_mutextype MUTEX_TYPE>
+struct Test_lock_and_sleep
+{
+	sem_t             _startup;
+	Mutex<MUTEX_TYPE> _mutex;
+
+	enum { SLEEP_MS = 500 };
+
+	static void *thread_fn(void *arg)
+	{
+		((Test_lock_and_sleep *)arg)->sleeper();
+		return nullptr;
+	}
+
+	void sleeper()
+	{
+		printf("sleeper: aquire mutex\n");
+		pthread_mutex_lock(_mutex.mutex());
+
+		printf("sleeper: about to wake up main thread\n");
+		sem_post(&_startup);
+
+		printf("sleeper: sleep %u ms\n", SLEEP_MS);
+		usleep(SLEEP_MS*1000);
+
+		printf("sleeper: woke up, now release mutex\n");
+		pthread_mutex_unlock(_mutex.mutex());
+	}
+
+	Test_lock_and_sleep()
+	{
+		sem_init(&_startup, 0, 0);
+
+		printf("main thread: start %s test\n", _mutex.type_string());
+
+		pthread_t id;
+		if (pthread_create(&id, 0, thread_fn, this) != 0) {
+			printf("error: pthread_create() failed\n");
+			exit(-1);
+		}
+
+		sem_wait(&_startup);
+
+		printf("main thread: sleeper woke me up, now aquire mutex (which blocks)\n");
+		pthread_mutex_lock(_mutex.mutex());
+
+		printf("main thread: aquired mutex, now release mutex and finish\n");
+		pthread_mutex_unlock(_mutex.mutex());
+
+		printf("main thread: finished %s test\n", _mutex.type_string());
+	}
+};
+
+
+static void test_lock_and_sleep()
+{
+	printf("main thread: test resume in contended lock\n");
+
+	{ Test_lock_and_sleep<PTHREAD_MUTEX_NORMAL>     test_normal; }
+	{ Test_lock_and_sleep<PTHREAD_MUTEX_ERRORCHECK> test_errorcheck; }
+	{ Test_lock_and_sleep<PTHREAD_MUTEX_RECURSIVE>  test_recursive; }
+
+	printf("main thread: resume in contended lock testing done\n");
+}
+
+
 static void test_interplay()
 {
 	enum { NUM_THREADS = 2 };
@@ -562,6 +634,7 @@ int main(int argc, char **argv)
 	test_self_destruct();
 	test_mutex();
 	test_mutex_stress();
+	test_lock_and_sleep();
 
 	printf("--- returning from main ---\n");
 	return 0;