diff --git a/base/include/base/signal.h b/base/include/base/signal.h index 9ed05239f8..51a42f44e5 100644 --- a/base/include/base/signal.h +++ b/base/include/base/signal.h @@ -48,43 +48,54 @@ namespace Genode { { private: - friend class Signal_receiver; - friend class Signal_context; + struct Data + { + Signal_context *context; + unsigned num; - Signal_context *_context; - unsigned _num; + /** + * Constructor + * + * \param context signal context specific for the + * signal-receiver capability used for signal + * transmission + * \param num number of signals received from the same + * transmitter + */ + Data(Signal_context *context, unsigned num) + : context(context), num(num) { } + + /** + * Default constructor, representing an invalid signal + */ + Data() : context(0), num(0) { } + + } _data; /** * Constructor * - * \param context signal context specific for the signal-receiver - * capability used for signal transmission - * \param num number of signals received from the same transmitter - * - * Signal objects are constructed only by signal receivers. + * Signal objects are constructed by signal receivers only. */ - Signal(Signal_context *context, unsigned num) - : _context(context), _num(num) - { } + Signal(Data data); + + friend class Signal_receiver; + friend class Signal_context; + + void _dec_ref_and_unlock(); + void _inc_ref(); public: - /** - * Default constructor, creating an invalid signal - */ - Signal() : _context(0), _num(0) { } + Signal(Signal const &other); + Signal &operator=(Signal const &other); + ~Signal(); - /** - * Return signal context - */ - Signal_context *context() { return _context; } - - /** - * Return number of signals received from the same transmitter - */ - unsigned num() const { return _num; } + Signal_context *context() { return _data.context; } + unsigned num() const { return _data.num; } }; + /** * Signal context * @@ -115,9 +126,13 @@ namespace Genode { */ Signal_receiver *_receiver; - Lock _lock; /* protect '_curr_signal' */ - Signal _curr_signal; /* most-currently received signal */ - bool _pending; /* current signal is valid */ + Lock _lock; /* protect '_curr_signal' */ + Signal::Data _curr_signal; /* most-currently received signal */ + bool _pending; /* current signal is valid */ + unsigned int _ref_cnt; /* number of references to this context */ + Lock _destroy_lock; /* prevent destruction while the + context is in use */ + /** * Capability assigned to this context after being assocated with @@ -127,6 +142,7 @@ namespace Genode { */ Signal_context_capability _cap; + friend class Signal; friend class Signal_receiver; friend class Signal_context_registry; @@ -137,7 +153,7 @@ namespace Genode { */ Signal_context() : _receiver_le(this), _registry_le(this), - _receiver(0), _pending(0) { } + _receiver(0), _pending(0), _ref_cnt(0) { } /** * Destructor @@ -271,7 +287,7 @@ namespace Genode { /** * Locally submit signal to the receiver */ - void local_submit(Signal signal); + void local_submit(Signal::Data signal); /** * Framework-internal signal-dispatcher diff --git a/base/src/base/signal/signal.cc b/base/src/base/signal/signal.cc index 493d726b82..a7b3cea6e0 100644 --- a/base/src/base/signal/signal.cc +++ b/base/src/base/signal/signal.cc @@ -165,6 +165,68 @@ Genode::Signal_context_registry *signal_context_registry() } +/************ + ** Signal ** + ************/ + +void Signal::_dec_ref_and_unlock() +{ + if (_data.context) { + Lock::Guard lock_guard(_data.context->_lock); + _data.context->_ref_cnt--; + if (_data.context->_ref_cnt == 0) + _data.context->_destroy_lock.unlock(); + } +} + + +void Signal::_inc_ref() +{ + if (_data.context) { + Lock::Guard lock_guard(_data.context->_lock); + _data.context->_ref_cnt++; + } +} + + +Signal::Signal(Signal::Data data) : _data(data) +{ + if (_data.context) { + _data.context->_ref_cnt = 1; + _data.context->_destroy_lock.lock(); + } +} + + +Signal::Signal(Signal const &other) : _data(other._data) +{ + _inc_ref(); +} + + +Signal::~Signal() +{ + _dec_ref_and_unlock(); +} + + +Signal &Signal::operator=(Signal const &other) +{ + if ((_data.context == other._data.context) + && (_data.num == other._data.num)) + return *this; + + _dec_ref_and_unlock(); + + _data.context = other._data.context; + _data.num = other._data.num; + + _inc_ref(); + + return *this; +} + + /************************ ** Signal transmitter ** ************************/ @@ -270,6 +332,8 @@ void Signal_receiver::dissolve(Signal_context *context) Lock::Guard list_lock_guard(_contexts_lock); _unsynchronized_dissolve(context); + + Lock::Guard context_destroy_lock_guard(context->_destroy_lock); } @@ -312,12 +376,12 @@ Signal Signal_receiver::wait_for_signal() continue; context->_pending = false; - Signal result = context->_curr_signal; + Signal::Data result = context->_curr_signal; /* invalidate current signal in context */ - context->_curr_signal = Signal(0, 0); + context->_curr_signal = Signal::Data(0, 0); - if (result.num() == 0) + if (result.num == 0) PWRN("returning signal with num == 0"); /* return last received signal */ @@ -334,21 +398,21 @@ Signal Signal_receiver::wait_for_signal() * the signal-causing context is absent from the list. */ } - return Signal(0, 0); /* unreachable */ + return Signal::Data(0, 0); /* unreachable */ } -void Signal_receiver::local_submit(Signal ns) +void Signal_receiver::local_submit(Signal::Data ns) { - Signal_context *context = ns.context(); + Signal_context *context = ns.context; /* * Replace current signal of the context by signal with accumulated * counters. In the common case, the current signal is an invalid * signal with a counter value of zero. */ - unsigned num = context->_curr_signal.num() + ns.num(); - context->_curr_signal = Signal(context, num); + unsigned num = context->_curr_signal.num + ns.num; + context->_curr_signal = Signal::Data(context, num); /* wake up the receiver if the context becomes pending */ if (!context->_pending) { @@ -372,7 +436,7 @@ void Signal_receiver::dispatch_signals(Signal_source *signal_source) } /* construct and locally submit signal object */ - Signal signal(context, source_signal.num()); + Signal::Data signal(context, source_signal.num()); context->_receiver->local_submit(signal); /* free context lock that was taken by 'test_and_lock' */ diff --git a/os/src/test/signal/main.cc b/os/src/test/signal/main.cc index 5dcb52cc29..c4338e0f43 100644 --- a/os/src/test/signal/main.cc +++ b/os/src/test/signal/main.cc @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -456,21 +457,25 @@ static void lazy_receivers_test() transmitter_1.submit(); transmitter_2.submit(); - Signal signal = rec_1.wait_for_signal(); - printf("returned from wait_for_signal for receiver 1\n"); + { + Signal signal = rec_1.wait_for_signal(); + printf("returned from wait_for_signal for receiver 1\n"); - signal = rec_2.wait_for_signal(); - printf("returned from wait_for_signal for receiver 2\n"); + signal = rec_2.wait_for_signal(); + printf("returned from wait_for_signal for receiver 2\n"); + } printf("submit and receive signals with multiple receivers out of order\n"); transmitter_1.submit(); transmitter_2.submit(); - signal = rec_2.wait_for_signal(); - printf("returned from wait_for_signal for receiver 2\n"); + { + Signal signal = rec_2.wait_for_signal(); + printf("returned from wait_for_signal for receiver 2\n"); - signal = rec_1.wait_for_signal(); - printf("returned from wait_for_signal for receiver 1\n"); + signal = rec_1.wait_for_signal(); + printf("returned from wait_for_signal for receiver 1\n"); + } printf("TEST %d FINISHED\n", test_cnt); } @@ -499,8 +504,10 @@ static void check_context_management() sender->idle(); /* collect pending signals and dissolve context from receiver */ - Signal signal = rec->wait_for_signal(); - printf("got %d signal(s) from %p\n", signal.num(), signal.context()); + { + Signal signal = rec->wait_for_signal(); + printf("got %d signal(s) from %p\n", signal.num(), signal.context()); + } rec->dissolve(context); /* let sender spin for some time */ @@ -515,6 +522,71 @@ static void check_context_management() } +/** + * Test if 'Signal_receiver::dissolve()' blocks as long as the signal context + * is still referenced by one or more 'Signal' objects + */ + +static Lock signal_context_destroyer_lock(Lock::LOCKED); +static bool signal_context_destroyed = false; + +class Signal_context_destroyer : public Thread<4096> +{ + private: + + Signal_receiver *_receiver; + Signal_context *_context; + + public: + + Signal_context_destroyer(Signal_receiver *receiver, Signal_context *context) + : _receiver(receiver), _context(context) { } + + void entry() + { + signal_context_destroyer_lock.lock(); + _receiver->dissolve(_context); + signal_context_destroyed = true; + destroy(env()->heap(), _context); + } +}; + +static void synchronized_context_destruction_test() +{ + Signal_receiver receiver; + + Signal_context *context = new (env()->heap()) Signal_context; + + Signal_transmitter transmitter(receiver.manage(context)); + transmitter.submit(); + + Signal_context_destroyer signal_context_destroyer(&receiver, context); + signal_context_destroyer.start(); + + /* The signal context destroyer thread should not be able to destroy the + * signal context during the 'Signal' objects life time. */ + { + Signal signal = receiver.wait_for_signal(); + + /* let the signal context destroyer thread try to destroy the signal context */ + signal_context_destroyer_lock.unlock(); + timer.msleep(1000); + + Signal signal_copy = signal; + Signal signal_copy2 = signal; + + signal_copy = signal_copy2; + + if (signal_context_destroyed) { + PERR("signal context destroyed too early"); + sleep_forever(); + } + } + + signal_context_destroyer.join(); +} + + /** * Main program */ @@ -527,6 +599,7 @@ int main(int, char **) stress_test(); lazy_receivers_test(); check_context_management(); + synchronized_context_destruction_test(); printf("--- signalling test finished ---\n"); return 0; diff --git a/ports/src/app/gdb_monitor/signal_handler_thread.cc b/ports/src/app/gdb_monitor/signal_handler_thread.cc index e723bb3a23..0c15fa5eff 100644 --- a/ports/src/app/gdb_monitor/signal_handler_thread.cc +++ b/ports/src/app/gdb_monitor/signal_handler_thread.cc @@ -39,10 +39,8 @@ Signal_handler_thread::Signal_handler_thread(Signal_receiver *receiver) void Signal_handler_thread::entry() { - Signal s; - while(1) { - s = _signal_receiver->wait_for_signal(); + Signal s = _signal_receiver->wait_for_signal(); if (verbose) PDBG("received exception signal"); diff --git a/ports/src/noux/main.cc b/ports/src/noux/main.cc index 7b149990fb..8ba1abec53 100644 --- a/ports/src/noux/main.cc +++ b/ports/src/noux/main.cc @@ -903,13 +903,19 @@ int main(int argc, char **argv) /* handle asynchronous events */ while (init_child) { - Genode::Signal signal = sig_rec.wait_for_signal(); + /* + * limit the scope of the 'Signal' object, so the signal context may + * get freed by the destruct queue + */ + { + Genode::Signal signal = sig_rec.wait_for_signal(); - Signal_dispatcher_base *dispatcher = - static_cast(signal.context()); + Signal_dispatcher_base *dispatcher = + static_cast(signal.context()); - for (unsigned i = 0; i < signal.num(); i++) - dispatcher->dispatch(1); + for (unsigned i = 0; i < signal.num(); i++) + dispatcher->dispatch(1); + } destruct_queue.flush();