btl/tcp: fix the multi-module wire-up race (replace the #5917 workaround)#14195
Draft
jsquyres wants to merge 1 commit into
Draft
btl/tcp: fix the multi-module wire-up race (replace the #5917 workaround)#14195jsquyres wants to merge 1 commit into
jsquyres wants to merge 1 commit into
Conversation
Since commit 2acc4b7 (PR open-mpi#5917), the TCP BTL has set MCA_BTL_FLAGS_SINGLE_ADD_PROCS whenever there are multiple modules and threads (user or progress) in use. That flag was an explicit workaround, not a fix, for a race in proc/endpoint setup: the BML calls add_procs() once per TCP module, and each call added only that module's endpoint to the shared mca_btl_tcp_proc_t. An incoming connection that looked up the proc between two of those calls found a partially populated endpoint list; if the connection's source address corresponded to a not-yet-created endpoint, the connection was dropped ("dropped inbound connection"). See the analysis in open-mpi#3035. Fix the race the way PR open-mpi#5917's author suggested: create the endpoints for *all* TCP modules inside mca_btl_tcp_proc_create(), before the proc is published in the component's proc table (all under tcp_lock). Publication is now the commit point: any thread that can find the proc sees a complete endpoint list. This is straightforward today because the bipartite interface matching (added after the workaround) already computes every module's remote address pairing at proc-creation time, in proc_create itself. With the race gone: * mca_btl_tcp_add_procs() reduces to looking up the calling module's endpoint. * mca_btl_tcp_proc_insert() is no longer needed; its logic moved into endpoint creation. * mca_btl_tcp_proc_lookup() no longer replays add_procs() for every module. * The SINGLE_ADD_PROCS workaround is removed, restoring lazy connection setup (and normal startup scalability) for multi-NIC / threaded jobs. Also release endpoints outside tcp_endpoints_mutex in mca_btl_tcp_del_procs(): the endpoint destructor takes the proc lock (and possibly the component lock), locks that proc creation now acquires before tcp_endpoints_mutex; releasing while holding the mutex could deadlock against a concurrent proc creation. Signed-off-by: Jeff Squyres <jeff@squyres.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While reviewing BTL framework code/comments recently, I came across the
MCA_BTL_FLAGS_SINGLE_ADD_PROCSworkaround that #5917 added to the TCP BTL back in 2018. That PR's description explicitly says it is a workaround, not a fix, and sketches what the real fix should be. This PR is purely a target of opportunity: it implements that real fix and removes the workaround.The race (recap of #5917 / issue #3035)
The full analysis is in #3035 (comment). Summary: with multiple TCP BTL modules (one per usable interface), the BML calls
add_procs()once per module, and each call added only that module's endpoint to the sharedmca_btl_tcp_proc_t. Meanwhilemca_btl_tcp_proc_accept()assumes that any proc it can find viamca_btl_tcp_proc_lookup()has its complete endpoint list, because it matches the incoming connection's source address against that list. With threads in the picture (user threads or the TCP progress thread), an incoming connection can arrive between two of the per-moduleadd_procs()calls, find a partially-wired proc, fail the address match, and get dropped ("dropped inbound connection"). #5917 avoided the race by settingMCA_BTL_FLAGS_SINGLE_ADD_PROCS(threads + >1 module), which forces the PML intoMCA_PML_BASE_FLAG_REQUIRE_WORLD— every proc in the job is wired up insideMPI_INIT— at the cost of lazy connection setup and startup scalability.The fix
#5917's description says: "The long term fix is to do all endpoint setup in the first call to add_procs for a given remote proc, removing the race." That is what this PR does, and it turns out to be quite natural in today's code: since the bipartite interface-matching rework (2019),
mca_btl_tcp_proc_create()already computes the module-to-remote-address pairing for all local modules (thebtl_index_to_endpointtable) at proc-creation time. So this PR moves endpoint creation there too:mca_btl_tcp_proc_create()now creates the endpoints for every matched module before publishing the proc in the component's proc table, all undertcp_lock. Publication becomes the commit point: any thread that can find the proc — the BML'sadd_procs()loop or the connection-accept path — sees a complete, immutable endpoint list. The race window is structurally gone, not just narrowed.Consequences:
mca_btl_tcp_add_procs()reduces to looking up the calling module's endpoint (and now propagatesOPAL_ERR_OUT_OF_RESOURCEfrom proc creation instead of silently treating allocation failure as "unreachable").mca_btl_tcp_proc_insert()is gone; its logic moved into endpoint creation insideproc_create().mca_btl_tcp_proc_lookup()no longer replaysadd_procs()against every module to wire up an unknown incoming peer.mca_btl_tcp_component_init()is removed, restoring lazy add_procs for multi-NIC/threaded jobs. (MCA_BTL_FLAGS_SINGLE_ADD_PROCSitself stays: portals4 and usnic still use it.)mca_btl_tcp_del_procs()now releases endpoints outsidetcp_endpoints_mutex: the endpoint destructor chain takes the proc lock (and possibly the component lock), which proc creation now acquires beforetcp_endpoints_mutex, so releasing inside the mutex could deadlock against a concurrent proc creation.Testing
--enable-mpi-threadsdefault config, internal hwloc/pmix/prrte/libevent.hello_c/ring_cover--mca btl tcp,selfon both platforms.collective/allgather_init) did —MPI_THREAD_MULTIPLE, 4 ranks x 4 threads each doing immediate simultaneous bidirectional sendrecv with every peer so both sides race to connect during lazy wire-up — over 4 TCP modules (2 interfaces xbtl_tcp_links=2), with and withoutbtl_tcp_progress_thread=1. 60+ fresh-wire-up runs across both platforms: zero dropped connections, zero hangs.Caveat: the original failure was observed on a real multi-node, multi-NIC cluster under MTT load, which I cannot replicate locally — single-host multi-interface stress is the closest approximation. Extra eyes (and any multi-NIC cluster testing) much appreciated.
@bwbarrett — you wrote #5917 and the race analysis, so you're the best judge of whether this matches the fix you had in mind. @hppritcha @bosilca — review appreciated as well.