Add Io_progress_handler to Entrypoint interface

The "schedule_post_signal_hook" method of the Genode::Entrypoint class
is problematic because the signal hook can be scheduled and replaced
multiple times during the signal dispatch cycle. Add an alternative to
this method with "register_io_progress_handler" and the "Post_signal_
hook" class with "Io_progress_handler". The difference being an
"Io_progress_handler" may be registered once during the lifetime of an
entrypoint to prevent arbitrary libraries from replacing a pending hook.

The "register_io_progress_handler" remains as a deprecated API, and is
now invoked for every I/O signal received and only for I/O signals
rather than for any signal.

Ref #3132

repos/base/include/base/entrypoint.h

@@ -5,7 +5,7 @@
  */
 
 /*
- * Copyright (C) 2015-2017 Genode Labs GmbH
+ * Copyright (C) 2015-2019 Genode Labs GmbH
  *
  * This file is part of the Genode OS framework, which is distributed
  * under the terms of the GNU Affero General Public License version 3.
@@ -31,8 +31,28 @@ class Genode::Entrypoint : Noncopyable
 {
 	public:
 
+		/**
+		 * Functor for post I/O signal progress handling
+		 *
+		 * This mechanism is for processing I/O events
+		 * deferred during signal dispatch. This is the
+		 * case when the application is blocked by I/O
+		 * but should not be resumed during signal
+		 * dispatch.
+		 */
+		struct Io_progress_handler : Interface
+		{
+			virtual ~Io_progress_handler() {
+				error("Io_progress_handler subclass cannot be safely destroyed!"); }
+
+			virtual void handle_io_progress() = 0;
+		};
+
 		/**
 		 * Functor for post signal-handler hook
+		 *
+		 * \deprecated
+		 * \noapi
 		 */
 		struct Post_signal_hook : Interface { virtual void function() = 0; };
 
@@ -96,7 +116,15 @@ class Genode::Entrypoint : Noncopyable
 		int               _signal_recipient   { NONE };
 		Genode::Lock      _signal_pending_lock     { };
 		Genode::Lock      _signal_pending_ack_lock { };
-		Post_signal_hook *_post_signal_hook = nullptr;
+
+		Io_progress_handler *_io_progress_handler { nullptr };
+		Post_signal_hook    *_post_signal_hook    { nullptr };
+
+		void _handle_io_progress()
+		{
+			if (_io_progress_handler != nullptr)
+				_io_progress_handler->handle_io_progress();
+		}
 
 		void _execute_post_signal_hook()
 		{
@@ -220,8 +248,23 @@ class Genode::Entrypoint : Noncopyable
 		 */
 		void schedule_suspend(void (*suspended)(), void (*resumed)());
 
+		/**
+		 * Register hook functor to be called after I/O signals are dispatched
+		 */
+		void register_io_progress_handler(Io_progress_handler &handler)
+		{
+			if (_io_progress_handler != nullptr) {
+				error("cannot call ", __func__, " twice!");
+				throw Exception();
+			}
+			_io_progress_handler = &handler;
+		}
+
 		/**
 		 * Register hook functor to be called after signal was handled
+		 *
+		 * \deprecated
+		 * \noapi
 		 */
 		void schedule_post_signal_hook(Post_signal_hook *hook)
 		{

repos/base/src/lib/base/entrypoint.cc

@@ -6,7 +6,7 @@
  */
 
 /*
- * Copyright (C) 2015-2017 Genode Labs GmbH
+ * Copyright (C) 2015-2019 Genode Labs GmbH
  *
  * This file is part of the Genode OS framework, which is distributed
  * under the terms of the GNU Affero General Public License version 3.
@@ -43,14 +43,30 @@ static char const *initial_ep_name() { return "ep"; }
 
 void Entrypoint::Signal_proxy_component::signal()
 {
-	/* XXX introduce while-pending loop */
+	ep._process_deferred_signals();
+
+	bool io_progress = false;
+
+	/*
+	 * Try to dispatch one pending signal picked-up by the signal-proxy thread.
+	 * Note, we handle only one signal here to ensure fairness between RPCs and
+	 * signals.
+	 */
 	try {
 		Signal sig = ep._sig_rec->pending_signal();
 		ep._dispatch_signal(sig);
+
+		if (sig.context()->level() == Signal_context::Level::Io) {
+			/* trigger the progress handler */
+			io_progress = true;
+
+			/* execute deprecated per-signal hook */
+			ep._execute_post_signal_hook();
+		}
 	} catch (Signal_receiver::Signal_not_pending) { }
 
-	ep._execute_post_signal_hook();
+	if (io_progress)
-	ep._process_deferred_signals();
+		ep._handle_io_progress();
 }
 
 
@@ -198,6 +214,7 @@ bool Entrypoint::_wait_and_dispatch_one_io_signal(bool const dont_block)
 			}
 
 			_dispatch_signal(sig);
+			_execute_post_signal_hook();
 			break;
 
 		} catch (Signal_receiver::Signal_not_pending) {
@@ -211,7 +228,7 @@ bool Entrypoint::_wait_and_dispatch_one_io_signal(bool const dont_block)
 		}
 	}
 
-	_execute_post_signal_hook();
+	_handle_io_progress();
 
 	/* initiate potential deferred-signal handling in entrypoint */
 	if (_deferred_signals.first()) {

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

@@ -6,7 +6,7 @@
  */
 
 /*
- * Copyright (C) 2016-2018 Genode Labs GmbH
+ * Copyright (C) 2016-2019 Genode Labs GmbH
  *
  * This file is part of the Genode OS framework, which is distributed
  * under the terms of the GNU Affero General Public License version 3.
@@ -20,8 +20,7 @@
 #include <base/rpc_client.h>
 #include <base/heap.h>
 #include <base/attached_rom_dataspace.h>
-#include <vfs/file_system_factory.h>
+#include <vfs/simple_env.h>
-#include <vfs/dir_file_system.h>
 #include <timer_session/connection.h>
 
 /* libc includes */
@@ -40,60 +39,17 @@ extern char **environ;
 
 namespace Libc {
 	class Env_implementation;
-	class Vfs_env;
 	class Kernel;
 	class Pthreads;
 	class Timer;
 	class Timer_accessor;
 	class Timeout;
 	class Timeout_handler;
-	class Io_response_handler;
 
 	using Genode::Microseconds;
 }
 
 
-class Libc::Vfs_env : public Vfs::Env
-{
-	private:
-
-		Genode::Env       &_env;
-		Genode::Allocator &_alloc;
-
-		Vfs::Io_response_handler &_io_handler;
-
-		struct Watch_response_stub : Vfs::Watch_response_handler {
-			void handle_watch_response(Vfs::Vfs_watch_handle::Context*) override { };
-		} _watch_stub { };
-
-		Vfs::Global_file_system_factory _global_file_system_factory { _alloc };
-
-		Vfs::Dir_file_system _root_dir;
-
-	public:
-
-		Vfs_env(Genode::Env &env,
-		        Genode::Allocator &alloc,
-		        Genode::Xml_node config,
-		        Vfs::Io_response_handler &io_handler)
-		: _env(env), _alloc(alloc), _io_handler(io_handler),
-		  _root_dir(*this, config, _global_file_system_factory)
-		{ }
-
-		Genode::Env &env() override { return _env; }
-
-		Genode::Allocator &alloc() override { return _alloc; }
-
-		Vfs::File_system &root_dir() override { return _root_dir; }
-
-		Vfs::Io_response_handler &io_handler() override {
-			return _io_handler; }
-
-		Vfs::Watch_response_handler &watch_handler() override {
-			return _watch_stub; }
-};
-
-
 class Libc::Env_implementation : public Libc::Env
 {
 	private:
@@ -126,20 +82,15 @@ class Libc::Env_implementation : public Libc::Env
 			return Genode::Xml_node("<libc/>");
 		}
 
-		Vfs::Global_file_system_factory _file_system_factory;
+		Vfs::Simple_env _vfs_env;
-		Vfs_env _vfs_env;
 
 		Genode::Xml_node _config_xml() const override {
 			return _config.xml(); };
 
 	public:
 
-		Env_implementation(Genode::Env &env, Genode::Allocator &alloc,
+		Env_implementation(Genode::Env &env, Genode::Allocator &alloc)
-		                   Vfs::Io_response_handler &io_response_handler)
+		: _env(env), _vfs_env(_env, alloc, _vfs_config()) { }
-		:
-			_env(env), _file_system_factory(alloc),
-			_vfs_env(_env, alloc, _vfs_config(), io_response_handler)
-		{ }
 
 
 		/*************************
@@ -371,19 +322,6 @@ struct Libc::Pthreads
 
 extern void (*libc_select_notify)();
 
-struct Libc::Io_response_handler : Vfs::Io_response_handler
-{
-	void handle_io_response(Vfs::Vfs_handle::Context *) override
-	{
-		/* some contexts may have been deblocked from select() */
-		if (libc_select_notify)
-			libc_select_notify();
-
-		/* resume all as any context may have been deblocked from blocking I/O */
-		Libc::resume_all();
-	}
-};
-
 
 /* internal utility */
 static void resumed_callback();
@@ -400,14 +338,14 @@ static void suspended_callback();
  * secondary stack for the application task. Context switching uses
  * setjmp/longjmp.
  */
-struct Libc::Kernel
+struct Libc::Kernel final : Genode::Entrypoint::Io_progress_handler
 {
 	private:
 
 		Genode::Env         &_env;
 		Genode::Allocator   &_heap;
-		Io_response_handler  _io_response_handler;
+
-		Env_implementation   _libc_env { _env, _heap, _io_response_handler };
+		Env_implementation   _libc_env { _env, _heap };
 		Vfs_plugin           _vfs { _libc_env, _heap };
 
 		Genode::Reconstructible<Genode::Io_signal_handler<Kernel>> _resume_main_handler {
@@ -619,7 +557,10 @@ struct Libc::Kernel
 	public:
 
 		Kernel(Genode::Env &env, Genode::Allocator &heap)
-		: _env(env), _heap(heap) { }
+		: _env(env), _heap(heap)
+		{
+			_env.ep().register_io_progress_handler(*this);
+		}
 
 		~Kernel() { Genode::error(__PRETTY_FUNCTION__, " should not be executed!"); }
 
@@ -821,6 +762,22 @@ struct Libc::Kernel
 				_switch_to_user();
 			}
 		}
+
+		/**
+		 * Entrypoint::Io_progress_handler interface
+		 */
+		void handle_io_progress() override
+		{
+			/* some contexts may have been deblocked from select() */
+			if (libc_select_notify)
+					libc_select_notify();
+
+			/*
+			 * resume all as any VFS context may have
+			 * been deblocked from blocking I/O
+			 */
+			Kernel::resume_all();
+		}
 };
 
 

repos/os/src/lib/vfs/fs_file_system.h

@@ -370,8 +370,7 @@ class Vfs::Fs_file_system : public File_system
 			Io_response_handler       &_io_handler;
 			Watch_response_handler    &_watch_handler;
 			List<Vfs_handle::Context>  _context_list { };
-			List<Vfs_watch_handle::Context>
+
-			                           _watch_context_list { };
 			Lock                       _list_lock    { };
 			bool                       _notify_all   { false };
 
@@ -406,27 +405,6 @@ class Vfs::Fs_file_system : public File_system
 				_ep.schedule_post_signal_hook(this);
 			}
 
-			void arm_watch_event(Vfs_watch_handle::Context &context)
-			{
-				{
-					Lock::Guard list_guard(_list_lock);
-
-					for (Vfs_watch_handle::Context *list_context = _watch_context_list.first();
-					     list_context;
-					     list_context = list_context->next())
-					{
-						if (list_context == &context) {
-							/* already in list */
-							return;
-						}
-					}
-
-					_watch_context_list.insert(&context);
-				}
-
-				_ep.schedule_post_signal_hook(this);
-			}
-
 			void function() override
 			{
 				Vfs_handle::Context *context = nullptr;
@@ -451,18 +429,6 @@ class Vfs::Fs_file_system : public File_system
 
 					/* done if no contexts and all notified */
 				} while (context);
-
-				for (;;) {
-					Vfs_watch_handle::Context *context = nullptr;
-					{
-						Lock::Guard list_guard(_list_lock);
-
-						context = _watch_context_list.first();
-						if (!context) break;
-						_watch_context_list.remove(context);
-						_watch_handler.handle_watch_response(context);
-					}
-				}
 			}
 		};
 
@@ -609,9 +575,14 @@ class Vfs::Fs_file_system : public File_system
 
 				try {
 					if (packet.operation() == Packet_descriptor::CONTENT_CHANGED) {
+						/*
+						 * Trigger the watch response during signal dispatch.
+						 * This is incompatible with the Libc I/O handling
+						 * but the Libc does not open watch handles and shall
+						 * not use them before Post_signal_hook is removed.
+						 */
 						_watch_handle_space.apply<Fs_vfs_watch_handle>(id, [&] (Fs_vfs_watch_handle &handle) {
-							if (auto *ctx = handle.context())
+							_env.watch_handler().handle_watch_response(handle.context()); });
-								_post_signal_hook.arm_watch_event(*ctx); });
 					} else {
 						_handle_space.apply<Fs_vfs_handle>(id, handle_read);
 					}

repos/os/src/server/vfs/main.cc

@@ -712,8 +712,9 @@ class Vfs_server::Session_component : private Session_resources,
 		void control(Node_handle, Control) override { }
 };
 
+
 /**
- * Global I/O event handler
+ * Global VFS event handler
  */
 struct Vfs_server::Io_response_handler : Vfs::Io_response_handler
 {
@@ -751,6 +752,7 @@ struct Vfs_server::Io_response_handler : Vfs::Io_response_handler
 	}
 };
 
+
 /**
  * Global VFS watch handler
  */
@@ -822,6 +824,34 @@ class Vfs_server::Root : public Genode::Root_component<Session_component>
 
 		Vfs_env _vfs_env { _env, vfs_config(), _session_registry };
 
+		/**
+		 * Global I/O event handler
+		 *
+		 * This is a safe and slow intermediate implementation
+		 * to be replaced with one that only processes handles
+		 * and sessions that await progress.
+		 */
+		struct Io_progress_handler : Genode::Entrypoint::Io_progress_handler
+		{
+			Session_registry &_session_registry;
+
+			Io_progress_handler(Genode::Entrypoint &ep,
+			                    Session_registry   &session_registry)
+			: _session_registry(session_registry)
+			{
+				ep.register_io_progress_handler(*this);
+			}
+
+			/**
+			 * Entrypoint::Io_progress_handler interface
+			 */
+			void handle_io_progress() override
+			{
+				_session_registry.for_each([ ] (Registered_session &r) {
+					r.handle_general_io(); });
+			}
+		} _io_progress_handler { _env.ep(), _session_registry };
+
 		Genode::Signal_handler<Root> _config_handler {
 			_env.ep(), *this, &Root::_config_update };