libc: use monitor for select

This patch simplifies the control flow within the libc kernel by
facilitating the monitor mechanism for the implementation of 'select'.

Issue #3874

repos/libports/src/lib/libc/internal/init.h

@@ -72,7 +72,7 @@ namespace Libc {
 	/**
 	 * Select support
 	 */
-	void init_select(Suspend &, Resume &, Select &, Signal &);
+	void init_select(Select &, Signal &, Monitor &);
 
 	/**
 	 * Support for querying available RAM quota in sysctl functions

repos/libports/src/lib/libc/internal/kernel.h

@@ -209,7 +209,7 @@ struct Libc::Kernel final : Vfs::Io_response_handler,
 		bool    _dispatch_pending_io_signals = false;
 
 		/* io_progress_handler marker */
-		bool _io_ready { false };
+		bool _io_progressed { false };
 
 		Thread &_myself { *Thread::myself() };
 
@@ -285,11 +285,15 @@ struct Libc::Kernel final : Vfs::Io_response_handler,
 		Reconstructible<Io_signal_handler<Kernel>> _execute_monitors {
 			_env.ep(), *this, &Kernel::_monitors_handler };
 
-		bool _execute_monitors_pending { false };
+		Monitor::Pool::State _execute_monitors_pending = Monitor::Pool::State::ALL_COMPLETE;
+
+		Constructible<Main_job> _main_monitor_job { };
 
 		void _monitors_handler()
 		{
-			 /* used to leave I/O-signal dispatcher only - handled afterwards */
+			/* mark monitors for execution when running in kernel only */
+			_execute_monitors_pending = Monitor::Pool::State::JOBS_PENDING;
+			_io_progressed = true;
 		}
 
 		Constructible<Clone_connection> _clone_connection { };
@@ -440,29 +444,86 @@ struct Libc::Kernel final : Vfs::Io_response_handler,
 					/* the 'kernel_routine' may install another kernel routine */
 					_kernel_routine = nullptr;
 					routine.execute_in_kernel();
-					if (!_kernel_routine)
+
+					if (!_kernel_routine) {
 						_switch_to_user();
-				}
+					}
 
-				if (_execute_monitors_pending) {
-
-					_execute_monitors_pending = false;
-					_monitors.execute_monitors();
-
-				} else {
-
-					if (_dispatch_pending_io_signals) {
-						/* dispatch pending signals but don't block */
-						while (_env.ep().dispatch_pending_io_signal()) ;
-					} else {
-						/* block for signals */
+					if (_kernel_routine) {
 						_env.ep().wait_and_dispatch_one_io_signal();
-						handle_io_progress();
 					}
 				}
 
-				if (!_kernel_routine && _resume_main_once && !_setjmp(_kernel_context))
+				/*
+				 * Dispatch all pending I/O signals at once and execute
+				 * monitors that may now become able to complete.
+				 */
+				auto dispatch_all_pending_io_signals = [&] ()
+				{
+					while (_env.ep().dispatch_pending_io_signal());
+				};
+
+				dispatch_all_pending_io_signals();
+
+				if (_io_progressed)
+					Kernel::resume_all();
+
+				_io_progressed = false;
+
+				/*
+				 * Execute monitors on kernel entry regardless of any I/O
+				 * because the monitor function may be unrelated to I/O.
+				 */
+				if (_execute_monitors_pending == Monitor::Pool::State::JOBS_PENDING)
+					_execute_monitors_pending = _monitors.execute_monitors();
+
+				/*
+				 * Process I/O signals without returning to the application
+				 * as long as the main thread depends on I/O.
+				 */
+
+				auto main_blocked_in_monitor = [&] ()
+				{
+					/*
+					 * In general, 'resume_all()' only flags the main state but
+					 * does not alter the main monitor job. For exmaple in case
+					 * of a sleep timeout, main is resumed by 'resume_main()'
+					 * in 'Main_blockade::wakeup()' but did not yet return from
+					 * 'suspend()'. The expired state in the main job is set
+					 * only after 'suspend()' returned.
+					 */
+					if (_resume_main_once)
+						return false;
+
+					return _main_monitor_job.constructed()
+					   && !_main_monitor_job->completed()
+					   && !_main_monitor_job->expired();
+				};
+
+				auto main_suspended_for_io = [&] {
+					return _resume_main_once == false; };
+
+				while (main_blocked_in_monitor() || main_suspended_for_io()) {
+
+					/*
+					 * Block for one I/O signal and process all pending ones
+					 * before executing the monitor functions. This avoids
+					 * superflous executions of the monitor functions when
+					 * receiving bursts of I/O signals.
+					 */
+					_env.ep().wait_and_dispatch_one_io_signal();
+
+					dispatch_all_pending_io_signals();
+
+					handle_io_progress();
+				}
+
+				/*
+				 * Return to the application
+				 */
+				if (!_kernel_routine && _resume_main_once && !_setjmp(_kernel_context)) {
 					_switch_to_user();
+				}
 			}
 		}
 
@@ -508,11 +569,17 @@ struct Libc::Kernel final : Vfs::Io_response_handler,
 		Monitor::Result _monitor(Mutex &mutex, Function &fn, uint64_t timeout_ms) override
 		{
 			if (_main_context()) {
-				Main_job job { fn, timeout_ms };
 
-				_monitors.monitor(mutex, job);
-				return job.completed() ? Monitor::Result::COMPLETE
-				                       : Monitor::Result::TIMEOUT;
+				_main_monitor_job.construct(fn, timeout_ms);
+
+				_monitors.monitor(mutex, *_main_monitor_job);
+
+				Monitor::Result const job_result = _main_monitor_job->completed()
+				                                 ? Monitor::Result::COMPLETE
+				                                 : Monitor::Result::TIMEOUT;
+				_main_monitor_job.destruct();
+
+				return job_result;
 
 			} else {
 				Pthread_job job { fn, _timer_accessor, timeout_ms };
@@ -523,13 +590,12 @@ struct Libc::Kernel final : Vfs::Io_response_handler,
 			}
 		}
 
-		void _charge_monitors() override
+		void _trigger_monitor_examination() override
 		{
-			if (!_execute_monitors_pending) {
-				_execute_monitors_pending = true;
-				if (!_main_context())
-					Signal_transmitter(*_execute_monitors).submit();
-			}
+			if (_main_context())
+				_monitors_handler();
+			else
+				Signal_transmitter(*_execute_monitors).submit();
 		}
 
 		/**
@@ -600,7 +666,7 @@ struct Libc::Kernel final : Vfs::Io_response_handler,
 		/**
 		 * Cwd interface
 		 */
-		Absolute_path &cwd() { return _cwd; }
+		Absolute_path &cwd() override { return _cwd; }
 
 
 		/*********************************
@@ -625,11 +691,8 @@ struct Libc::Kernel final : Vfs::Io_response_handler,
 		 ** Vfs::Io_response_handler interface **
 		 ****************************************/
 
-		void read_ready_response() override {
-			_io_ready = true; }
-
-		void io_progress_response() override {
-			_io_ready = true; }
+		void read_ready_response() override  { _io_progressed = true; }
+		void io_progress_response() override { _io_progressed = true; }
 
 
 		/**********************************************

repos/libports/src/lib/libc/internal/monitor.h

@@ -60,7 +60,7 @@ class Libc::Monitor : Interface
 	protected:
 
 		virtual Result _monitor(Mutex &, Function &, uint64_t) = 0;
-		virtual void _charge_monitors() = 0;
+		virtual void _trigger_monitor_examination() = 0;
 
 	public:
 
@@ -68,7 +68,7 @@ class Libc::Monitor : Interface
 		 * Block until monitored execution completed or timeout expires
 		 *
 		 * The mutex must be locked when calling the monitor. It is released
-		 * during wait for completion and re-aquired before the function
+		 * during wait for completion and re-acquired before the function
 		 * returns. This behavior is comparable to condition variables.
 		 *
 		 * Returns true if execution completed, false on timeout.
@@ -83,14 +83,13 @@ class Libc::Monitor : Interface
 				_Function(FN const &fn) : fn(fn) { }
 			} function { fn };
 
-			_charge_monitors();
 			return _monitor(mutex, function, timeout_ms);
 		}
 
 		/**
-		 * Charge monitor to execute the monitored function
+		 * Trigger examination of monitored functions
 		 */
-		void charge_monitors() { _charge_monitors(); }
+		void trigger_monitor_examination() { _trigger_monitor_examination(); }
 };
 
 
@@ -136,21 +135,35 @@ struct Libc::Monitor::Pool
 
 			mutex.release();
 
-			_monitor.charge_monitors();
+			_monitor.trigger_monitor_examination();
 
 			job.wait_for_completion();
 
 			mutex.acquire();
 		}
 
+		enum class State { JOBS_PENDING, ALL_COMPLETE };
+
 		/* called by the monitor context itself */
-		void execute_monitors()
+		State execute_monitors()
 		{
+			State result = State::ALL_COMPLETE;
+
 			_jobs.for_each([&] (Job &job) {
-				if (!job.completed() && !job.expired() && job.execute()) {
-					job.complete();
+
+				if (!job.completed() && !job.expired()) {
+
+					bool const completed = job.execute();
+
+					if (completed)
+						job.complete();
+
+					if (!completed)
+						result = State::JOBS_PENDING;
 				}
 			});
+
+			return result;
 		}
 };
 

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

@@ -391,28 +391,13 @@ extern void (*libc_select_notify_from_kernel)();
 
 void Libc::Kernel::handle_io_progress()
 {
-	if (_execute_monitors_pending) {
-		_execute_monitors_pending = false;
-		_monitors.execute_monitors();
-	}
+	if (_io_progressed) {
+		_io_progressed = false;
 
-	/*
-	 * TODO: make VFS I/O completion checks during
-	 * kernel time to avoid flapping between stacks
-	 */
-
-	if (_io_ready) {
-		_io_ready = false;
-
-		/* some contexts may have been deblocked from select() */
-		select_notify_from_kernel();
-
-		/*
-		 * resume all as any VFS context may have
-		 * been deblocked from blocking I/O
-		 */
 		Kernel::resume_all();
-		_monitors.execute_monitors();
+
+		if (_execute_monitors_pending == Monitor::Pool::State::JOBS_PENDING)
+			_execute_monitors_pending = _monitors.execute_monitors();
 	}
 }
 
@@ -490,7 +475,7 @@ Libc::Kernel::Kernel(Genode::Env &env, Genode::Allocator &heap)
 	init_vfs_plugin(*this, _env.rm());
 	init_file_operations(*this);
 	init_time(*this, *this);
-	init_select(*this, *this, *this, _signal);
+	init_select(*this, _signal, *this);
 	init_socket_fs(*this);
 	init_passwd(_passwd_config());
 	init_signal(_signal);

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

@@ -22,7 +22,6 @@
 #include <internal/errno.h>
 #include <internal/file.h>
 #include <internal/init.h>
-#include <internal/suspend.h>
 
 using namespace Libc;
 

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

@@ -38,10 +38,9 @@
 #include <internal/kernel.h>
 #include <internal/init.h>
 #include <internal/signal.h>
-#include <internal/suspend.h>
-#include <internal/resume.h>
 #include <internal/select.h>
 #include <internal/errno.h>
+#include <internal/monitor.h>
 
 namespace Libc {
 	struct Select_cb;
@@ -51,19 +50,16 @@ namespace Libc {
 using namespace Libc;
 
 
-static Suspend      *_suspend_ptr;
-static Resume       *_resume_ptr;
 static Select       *_select_ptr;
 static Libc::Signal *_signal_ptr;
+static Monitor      *_monitor_ptr;
 
 
-void Libc::init_select(Suspend &suspend, Resume &resume, Select &select,
-                       Signal &signal)
+void Libc::init_select(Select &select, Signal &signal, Monitor &monitor)
 {
-	_suspend_ptr = &suspend;
-	_resume_ptr  = &resume;
 	_select_ptr  = &select;
 	_signal_ptr  = &signal;
+	_monitor_ptr = &monitor;
 }
 
 
@@ -210,7 +206,6 @@ static int selscan(int nfds,
 /* this function gets called by plugin backends when file descripors become ready */
 void Libc::select_notify_from_kernel()
 {
-	bool resume_all = false;
 	fd_set tmp_readfds, tmp_writefds, tmp_exceptfds;
 
 	/* check for each waiting select() function if one of its fds is ready now
@@ -224,18 +219,8 @@ void Libc::select_notify_from_kernel()
 			scb.readfds   = tmp_readfds;
 			scb.writefds  = tmp_writefds;
 			scb.exceptfds = tmp_exceptfds;
-
-			resume_all = true;
 		}
 	});
-
-	if (resume_all) {
-		struct Missing_call_of_init_select : Exception { };
-		if (!_resume_ptr)
-			throw Missing_call_of_init_select();
-
-		_resume_ptr->resume_all();
-	}
 }
 
 
@@ -288,35 +273,14 @@ int select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 		return 0;
 	}
 
-	/* suspend as we don't have any immediate events */
-
-	struct Timeout
-	{
-		timeval const *_tv;
-		bool    const  valid    { _tv != nullptr };
-		Genode::uint64_t  duration {
-			valid ? (Genode::uint64_t)_tv->tv_sec*1000 + _tv->tv_usec/1000 : 0UL };
-
-		bool expired() const { return valid && duration == 0; };
-
-		Timeout(timeval *tv) : _tv(tv) { }
-	} timeout { tv };
-
-	struct Check : Suspend_functor
-	{
-		struct Timeout *timeout;
-		Select_cb      *select_cb;
-
-		Check(Timeout *timeout, Select_cb * select_cb)
-		: timeout(timeout), select_cb(select_cb) { }
-
-		bool suspend() override {
-			return !timeout->expired() && select_cb->nready == 0; }
-	} check ( &timeout, &*select_cb );
+	using Genode::uint64_t;
 
+	uint64_t const timeout_ms = (tv != nullptr)
+	                          ? (uint64_t)tv->tv_sec*1000 + tv->tv_usec/1000
+	                          : 0UL;
 	{
 		struct Missing_call_of_init_select : Exception { };
-		if (!_suspend_ptr || !_signal_ptr)
+		if (!_monitor_ptr || !_signal_ptr)
 			throw Missing_call_of_init_select();
 	}
 
@@ -324,25 +288,31 @@ int select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 
 	auto signal_occurred_during_select = [&] ()
 	{
-		return _signal_ptr->count() != orig_signal_count;
+		return (_signal_ptr->count() != orig_signal_count);
 	};
 
-	for (;;) {
-		if (timeout.expired())
-			break;
+	auto monitor_fn = [&] ()
+	{
+		select_notify_from_kernel();
 
 		if (select_cb->nready != 0)
-			break;
+			return Monitor::Function_result::COMPLETE;
 
 		if (signal_occurred_during_select())
-			break;
+			return Monitor::Function_result::COMPLETE;
 
-		timeout.duration = _suspend_ptr->suspend(check, timeout.duration);
-	}
+		return Monitor::Function_result::INCOMPLETE;
+	};
+
+	Mutex mutex { };
+	Mutex::Guard guard(mutex);
+
+	Monitor::Result const monitor_result =
+		_monitor_ptr->monitor(mutex, monitor_fn, timeout_ms);
 
 	select_cb_list().remove(&(*select_cb));
 
-	if (timeout.expired())
+	if (monitor_result == Monitor::Result::TIMEOUT)
 		return 0;
 
 	if (signal_occurred_during_select())
@@ -357,6 +327,7 @@ int select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 	return select_cb->nready;
 }
 
+
 extern "C" __attribute__((alias("select")))
 int __sys_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
                  struct timeval *tv);