From b811ef4331386fe4601fd751d2a826b0365a28d8 Mon Sep 17 00:00:00 2001 From: Martin Stein Date: Sun, 17 Sep 2017 12:30:20 +0200 Subject: [PATCH] signal: fix starvation by fast signal contexts In the past, a signal context, that was chosen for handling by 'Signal_receiver::pending_signal and always triggered again before the next call of 'pending_signal', caused all other contexts behind in the list to starve. This was the case because 'pending_signal' always took the first pending context in its context list. We avoid this problem now by handling pending signals in a round-robin fashion instead. Ref #2532 --- repos/base-hw/src/lib/base/signal_receiver.cc | 2 +- repos/base/include/base/signal.h | 31 ++++++++-- repos/base/src/lib/base/signal.cc | 2 +- repos/base/src/lib/base/signal_common.cc | 56 ++++++++++++++++--- 4 files changed, 75 insertions(+), 16 deletions(-) diff --git a/repos/base-hw/src/lib/base/signal_receiver.cc b/repos/base-hw/src/lib/base/signal_receiver.cc index 33df7caf1..28a5301f6 100644 --- a/repos/base-hw/src/lib/base/signal_receiver.cc +++ b/repos/base-hw/src/lib/base/signal_receiver.cc @@ -99,7 +99,7 @@ Signal_context_capability Signal_receiver::manage(Signal_context * const c) /* use signal context as imprint */ c->_cap = pd().alloc_context(_cap, (unsigned long)c); c->_receiver = this; - _contexts.insert(&c->_receiver_le); + _contexts.insert(c); return c->_cap; } catch (Out_of_ram) { ram_upgrade = Ram_quota { 1024*sizeof(long) }; } diff --git a/repos/base/include/base/signal.h b/repos/base/include/base/signal.h index 3ae7c8399..89419e8b5 100644 --- a/repos/base/include/base/signal.h +++ b/repos/base/include/base/signal.h @@ -174,6 +174,26 @@ class Genode::Signal_receiver : Noncopyable { private: + /** + * A list where the head can be moved + */ + class Context_list + { + private: + + Signal_context *_head { nullptr }; + + public: + + Signal_context *head() const { return _head; } + + void head(Signal_context *re); + + void insert(Signal_context *re); + + void remove(Signal_context const *re); + }; + /** * Semaphore used to indicate that signal(s) are ready to be picked * up. This is needed for platforms other than 'base-hw' only. @@ -189,8 +209,8 @@ class Genode::Signal_receiver : Noncopyable /** * List of associated contexts */ - Lock _contexts_lock; - List > _contexts; + Lock _contexts_lock; + Context_list _contexts; /** * Helper to dissolve given context @@ -324,7 +344,8 @@ class Genode::Signal_context /** * List element in 'Signal_receiver' */ - List_element _receiver_le; + Signal_context mutable *_next { nullptr }; + Signal_context mutable *_prev { nullptr }; /** * List element in process-global registry @@ -373,8 +394,8 @@ class Genode::Signal_context * Constructor */ Signal_context() - : _receiver_le(this), _registry_le(this), _deferred_le(this), - _receiver(0), _pending(0), _ref_cnt(0) { } + : _registry_le(this), _deferred_le(this), _receiver(0), _pending(0), + _ref_cnt(0) { } /** * Destructor diff --git a/repos/base/src/lib/base/signal.cc b/repos/base/src/lib/base/signal.cc index a6b554fc7..4bcb6bc2a 100644 --- a/repos/base/src/lib/base/signal.cc +++ b/repos/base/src/lib/base/signal.cc @@ -224,7 +224,7 @@ Signal_context_capability Signal_receiver::manage(Signal_context *context) Lock::Guard list_lock_guard(_contexts_lock); /* insert context into context list */ - _contexts.insert(&context->_receiver_le); + _contexts.insert(context); /* register context at process-wide registry */ signal_context_registry()->insert(&context->_registry_le); diff --git a/repos/base/src/lib/base/signal_common.cc b/repos/base/src/lib/base/signal_common.cc index 18443e3bc..2804bc8f0 100644 --- a/repos/base/src/lib/base/signal_common.cc +++ b/repos/base/src/lib/base/signal_common.cc @@ -163,9 +163,7 @@ Signal Signal_receiver::pending_signal() Lock::Guard list_lock_guard(_contexts_lock); /* look up the contexts for the pending signal */ - for (List_element *le = _contexts.first(); le; le = le->next()) { - - Signal_context *context = le->object(); + for (Signal_context *context = _contexts.head(); context; context = context->_next) { Lock::Guard lock_guard(context->_lock); @@ -173,6 +171,7 @@ Signal Signal_receiver::pending_signal() if (!context->_pending) continue; + _contexts.head(context); context->_pending = false; Signal::Data result = context->_curr_signal; @@ -206,8 +205,8 @@ Signal_receiver::~Signal_receiver() Lock::Guard list_lock_guard(_contexts_lock); /* disassociate contexts from the receiver */ - for (List_element *le; (le = _contexts.first()); ) - _unsynchronized_dissolve(le->object()); + while (_contexts.head()) + _unsynchronized_dissolve(_contexts.head()); _platform_destructor(); } @@ -225,7 +224,7 @@ void Signal_receiver::_unsynchronized_dissolve(Signal_context * const context) context->_cap = Signal_context_capability(); /* remove context from context list */ - _contexts.remove(&context->_receiver_le); + _contexts.remove(context); _platform_finish_dissolve(context); } @@ -250,9 +249,7 @@ bool Signal_receiver::pending() Lock::Guard list_lock_guard(_contexts_lock); /* look up the contexts for the pending signal */ - for (List_element *le = _contexts.first(); le; le = le->next()) { - - Signal_context *context = le->object(); + for (Signal_context *context = _contexts.head(); context; context = context->_next) { Lock::Guard lock_guard(context->_lock); @@ -261,3 +258,44 @@ bool Signal_receiver::pending() } return false; } + + +void Signal_receiver::Context_list::insert(Signal_context *re) +{ + if (_head) { + re->_prev = _head->_prev; + re->_next = nullptr; + _head->_prev->_next = re; + _head->_prev = re; + } else { + re->_prev = re; + re->_next = nullptr; + _head = re; + } +} + + +void Signal_receiver::Context_list::remove(Signal_context const *re) +{ + if (re->_prev == re) { + _head = nullptr; + } else { + if (_head == re) { + _head = re->_next; + } + re->_prev->_next = re->_next; + if (re->_next) { + re->_next->_prev = re->_prev; + } else { + _head->_prev = re->_prev; + } + } +} + + +void Signal_receiver::Context_list::head(Signal_context *re) +{ + _head->_prev->_next = _head; + _head = re; + _head->_prev->_next = nullptr; +}