From 633f2189111be18cb03fe847707b0f598e453abe Mon Sep 17 00:00:00 2001 From: Robin Gareus Date: Thu, 28 Apr 2016 21:15:26 +0200 Subject: fix a deadlock with jack2 when inserting a plugin adds ports. When adding a processor, the processor may add ports leading to a call to jack_port_register(). while Ardour holds a WritertLock on the processor-list (this commit removes this WriterLock). with jack2 that results in a graph-reorder callback (WHY?) jack2 issues that graph-reorder in a separate thread BUT port-registration does not return until the graph-reorder is complete. On Ardour's side, graph_reordered() calls Session::resort_routes () which eventually checks Route::direct_feeds_according_to_reality() which needs a ReadLock on the processor-list to check I/O. Since jack_port_register() does not return, this constitutes a deadlock. THE ACTUAL PROBLEM IS JACK2's THREAD DESIGN! Why does jack_port_register() trigger a graph-order in jack2? No connections are made. ..and why does it block jack_port_register() from returning if that graph-order callback is in a different thread? http://pastebin.com/DZANXJLz --- libs/ardour/ardour/route.h | 2 +- libs/ardour/route.cc | 72 ++++++++++++++++++++++++++++++++-------------- 2 files changed, 51 insertions(+), 23 deletions(-) (limited to 'libs') diff --git a/libs/ardour/ardour/route.h b/libs/ardour/ardour/route.h index 66a83cf67d..bac33ae7a1 100644 --- a/libs/ardour/ardour/route.h +++ b/libs/ardour/ardour/route.h @@ -848,7 +848,7 @@ class LIBARDOUR_API Route : public SessionObject, public Automatable, public Rou bool _initial_io_setup; bool _in_sidechain_setup; - int configure_processors_unlocked (ProcessorStreams*); + int configure_processors_unlocked (ProcessorStreams*, Glib::Threads::RWLock::WriterLock*); bool set_meter_point_unlocked (); void apply_processor_order (const ProcessorList& new_order); diff --git a/libs/ardour/route.cc b/libs/ardour/route.cc index 5bbb9d3562..0e01134986 100644 --- a/libs/ardour/route.cc +++ b/libs/ardour/route.cc @@ -1310,9 +1310,9 @@ Route::add_processor (boost::shared_ptr processor, boost::shared_ptr< // configure redirect ports properly, etc. { - if (configure_processors_unlocked (err)) { + if (configure_processors_unlocked (err, &lm)) { pstate.restore (); - configure_processors_unlocked (0); // it worked before we tried to add it ... + configure_processors_unlocked (0, &lm); // it worked before we tried to add it ... return -1; } } @@ -1495,9 +1495,9 @@ Route::add_processors (const ProcessorList& others, boost::shared_ptr /* Think: does this really need to be called for every processor in the loop? */ { - if (configure_processors_unlocked (err)) { + if (configure_processors_unlocked (err, &lm)) { pstate.restore (); - configure_processors_unlocked (0); // it worked before we tried to add it ... + configure_processors_unlocked (0, &lm); // it worked before we tried to add it ... return -1; } } @@ -1715,7 +1715,7 @@ Route::clear_processors (Placement p) } _processors = new_list; - configure_processors_unlocked (&err); // this can't fail + configure_processors_unlocked (&err, &lm); // this can't fail } processor_max_streams.reset(); @@ -1815,10 +1815,10 @@ Route::remove_processor (boost::shared_ptr processor, ProcessorStream return 1; } - if (configure_processors_unlocked (err)) { + if (configure_processors_unlocked (err, &lm)) { pstate.restore (); /* we know this will work, because it worked before :) */ - configure_processors_unlocked (0); + configure_processors_unlocked (0, &lm); return -1; } @@ -1912,9 +1912,9 @@ Route::replace_processor (boost::shared_ptr old, boost::shared_ptrset_user_latency (0); - if (configure_processors_unlocked (err)) { + if (configure_processors_unlocked (err, &lm)) { pstate.restore (); /* we know this will work, because it worked before :) */ - configure_processors_unlocked (0); + configure_processors_unlocked (0, &lm); return -1; } //lx.unlock(); @@ -2063,7 +2063,7 @@ Route::configure_processors (ProcessorStreams* err) if (!_in_configure_processors) { Glib::Threads::RWLock::WriterLock lm (_processor_lock); - return configure_processors_unlocked (err); + return configure_processors_unlocked (err, &lm); } return 0; @@ -2179,7 +2179,7 @@ Route::try_configure_processors_unlocked (ChanCount in, ProcessorStreams* err) * Return 0 on success, otherwise configuration is impossible. */ int -Route::configure_processors_unlocked (ProcessorStreams* err) +Route::configure_processors_unlocked (ProcessorStreams* err, Glib::Threads::RWLock::WriterLock* lm) { #ifndef PLATFORM_WINDOWS assert (!AudioEngine::instance()->process_lock().trylock()); @@ -2206,12 +2206,37 @@ Route::configure_processors_unlocked (ProcessorStreams* err) processor_out_streams = _input->n_ports(); processor_max_streams.reset(); + /* processor configure_io() may result in adding ports + * e.g. Delivery::configure_io -> ARDOUR::IO::ensure_io () + * + * with jack2 adding ports results in a graph-order callback, + * which calls Session::resort_routes() and eventually + * Route::direct_feeds_according_to_reality() + * which takes a ReaderLock (_processor_lock). + * + * so we can't hold a WriterLock here until jack2 threading + * is fixed. + * + * NB. we still hold the process lock + * + * (ardour's own engines do call graph-order from the + * process-thread and hence do not have this issue; besides + * merely adding ports won't trigger a graph-order, only + * making connections does) + */ + lm->release (); + + // TODO check for a potential ReaderLock after ReaderLock ?? + Glib::Threads::RWLock::ReaderLock lr (_processor_lock); + list< pair >::iterator c = configuration.begin(); for (ProcessorList::iterator p = _processors.begin(); p != _processors.end(); ++p, ++c) { if (!(*p)->configure_io(c->first, c->second)) { DEBUG_TRACE (DEBUG::Processors, string_compose ("%1: configuration failed\n", _name)); _in_configure_processors = false; + lr.release (); + lm->acquire (); return -1; } processor_max_streams = ChanCount::max(processor_max_streams, c->first); @@ -2245,6 +2270,9 @@ Route::configure_processors_unlocked (ProcessorStreams* err) } } + lr.release (); + lm->acquire (); + if (_meter) { _meter->set_max_channels (processor_max_streams); @@ -2438,7 +2466,7 @@ Route::reorder_processors (const ProcessorList& new_order, ProcessorStreams* err apply_processor_order (new_order); - if (configure_processors_unlocked (err)) { + if (configure_processors_unlocked (err, &lm)) { pstate.restore (); return -1; } @@ -2511,7 +2539,7 @@ Route::add_remove_sidechain (boost::shared_ptr proc, bool add) return false; } lx.acquire (); - configure_processors_unlocked (0); + configure_processors_unlocked (0, &lm); } if (pi->has_sidechain ()) { @@ -2554,7 +2582,7 @@ Route::plugin_preset_output (boost::shared_ptr proc, ChanCount outs) pi->set_preset_out (old); return false; } - configure_processors_unlocked (0); + configure_processors_unlocked (0, &lm); } processors_changed (RouteProcessorChange ()); /* EMIT SIGNAL */ @@ -2614,7 +2642,7 @@ Route::customize_plugin_insert (boost::shared_ptr proc, uint32_t coun return false; } - configure_processors_unlocked (0); + configure_processors_unlocked (0, &lm); } processors_changed (RouteProcessorChange ()); /* EMIT SIGNAL */ @@ -3426,7 +3454,7 @@ Route::set_processor_state (const XMLNode& node) _processors = new_order; if (must_configure) { - configure_processors_unlocked (0); + configure_processors_unlocked (0, &lm); } for (ProcessorList::const_iterator i = _processors.begin(); i != _processors.end(); ++i) { @@ -3746,8 +3774,8 @@ Route::direct_feeds_according_to_reality (boost::shared_ptr other, bool* return true; } + Glib::Threads::RWLock::ReaderLock lm (_processor_lock); - Glib::Threads::RWLock::ReaderLock lm (_processor_lock); // XXX for (ProcessorList::iterator r = _processors.begin(); r != _processors.end(); ++r) { boost::shared_ptr iop = boost::dynamic_pointer_cast(*r); @@ -4271,10 +4299,10 @@ Route::listen_position_changed () Glib::Threads::RWLock::WriterLock lm (_processor_lock); ProcessorState pstate (this); - if (configure_processors_unlocked (0)) { + if (configure_processors_unlocked (0, &lm)) { DEBUG_TRACE (DEBUG::Processors, "---- CONFIGURATION FAILED.\n"); pstate.restore (); - configure_processors_unlocked (0); // it worked before we tried to add it ... + configure_processors_unlocked (0, &lm); // it worked before we tried to add it ... return; } } @@ -4295,7 +4323,7 @@ Route::add_export_point() _capturing_processor.reset (new CapturingProcessor (_session)); _capturing_processor->activate (); - configure_processors_unlocked (0); + configure_processors_unlocked (0, &lw); } -- cgit v1.2.3