Fix a series of repeated startup/shutdown bugs, plus related thread-level reporting bugs#14164
Open
jsquyres wants to merge 9 commits into
Open
Fix a series of repeated startup/shutdown bugs, plus related thread-level reporting bugs#14164jsquyres wants to merge 9 commits into
jsquyres wants to merge 9 commits into
Conversation
jsquyres
force-pushed
the
pr/btl-tcp-event-base-double-free
branch
from
July 14, 2026 00:06
cf370fa to
1b06214
Compare
jsquyres
changed the title
Member
|
This PR does not fix anything, it breaks things in unexpected ways. The base event should definitely never be released while any communication infrastructure depends on it, or we are losing the ability to move data. Any correct fix to this issue addressed the MPI_T and does not touch the BTL. |
jsquyres
marked this pull request as draft
July 14, 2026 00:15
Member
Author
|
This got uglier than I wanted. Putting this back to draft while I re-evaluate. |
jsquyres
force-pushed
the
pr/btl-tcp-event-base-double-free
branch
from
July 18, 2026 19:35
1b06214 to
be31e8e
Compare
jsquyres
changed the title
jsquyres
force-pushed
the
pr/btl-tcp-event-base-double-free
branch
from
July 18, 2026 21:08
be31e8e to
2480c69
Compare
jsquyres
changed the title
jsquyres
force-pushed
the
pr/btl-tcp-event-base-double-free
branch
from
July 19, 2026 18:12
2480c69 to
c1e1400
Compare
jsquyres
changed the title
jsquyres
marked this pull request as ready for review
July 19, 2026 18:34
hppritcha
previously approved these changes
Jul 20, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens Open MPI’s repeated / interleaved lifecycle handling for the World Model, MPI Sessions, and MPI_T — including cross-thread interactions — and adds a targeted make check test matrix to prevent regressions in startup/shutdown ordering and thread-level reporting.
Changes:
- Reference-count OPAL’s shared libevent base so deferred framework/component closes from MPI_T cannot use-after-free the base after
opal_finalize(). - Fix teardown ordering and stale-base aliasing in the TCP BTL and reset stale BML/r2 state across session cycles held open by MPI_T.
- Serialize MPI_T init/finalize against instance init/finalize and add a suite of singleton lifecycle/concurrency tests wired into
make check.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| opal/util/event.c | Adds refcounting and “one-shot” latching around shared libevent base initialization/finalization across repeated cycles. |
| opal/mca/btl/tcp/btl_tcp_component.c | Fixes progress-thread teardown ordering (delete events before freeing private base) and clears stale base alias on close. |
| ompi/runtime/ompi_mpi_init.c | Clarifies world-model thread-level semantics and ratchets ompi_mpi_thread_multiple (never lowers). |
| ompi/mpi/tool/mpit-internal.h | Introduces a dedicated MPI_T thread-level variable pinned per MPI_T epoch. |
| ompi/mpi/tool/mpit_common.c | Defines MPI_T epoch thread-level storage. |
| ompi/mpi/tool/init_thread.c | Serializes MPI_T init with instance lock, takes/releases shared event base ref, fixes nested provided reporting, and prevents world-thread-level stomping. |
| ompi/mpi/tool/finalize.c | Serializes MPI_T finalize with instance lock, releases shared event-base reference, and resets MPI_T epoch thread level. |
| ompi/mca/bml/r2/bml_r2.c | Resets btls_added at finalize so repeated sessions under held MPI_T ref re-select BTLs correctly. |
| ompi/instance/instance.h | Exposes instance lock/unlock for MPI_T entry points to serialize against instance init/finalize. |
| ompi/instance/instance.c | Implements lock/unlock helpers and ratchets process-wide thread flags under the instance lock. |
| ompi/test/t/Makefile.am | Expands singleton MPI_T lifecycle test set and wires the new executables into make check. |
| ompi/test/t/mpi_t_lifecycle.h | Adds shared scaffolding (STEP macros + PML/BTL pinning helper) for lifecycle tests. |
| ompi/test/t/mpi_t_lifecycle.c | Adds tool-only MPI_T lifecycle ordering test (plain/nested/repeated). |
| ompi/test/t/mpi_t_outlives_mpi.c | Adds regression test for MPI_T outliving MPI (deferred component closes after MPI_Finalize()). |
| ompi/test/t/mpi_t_stress.c | Adds 10,000-cycle MPI_T init/finalize stress test. |
| ompi/test/t/mpi_t_sessions.c | Adds a single executable covering MPI_T × Sessions interleavings. |
| ompi/test/t/mpi_t_repeated_sessions.c | Adds regression test for repeated sessions under a held MPI_T reference. |
| ompi/test/t/mpi_t_world_close_first.c | Adds world-model ordering test where MPI_T opens/closes before MPI_Finalize(). |
| ompi/test/t/mpi_t_world_inner.c | Adds world-model ordering test where MPI_T is entirely inside MPI. |
| ompi/test/t/mpi_t_world_outlast.c | Adds world-model ordering test where MPI_T starts inside MPI and outlives it (nested refs). |
| ompi/test/t/mpi_t_world_sessions.c | Adds combined world + sessions + MPI_T coexistence test. |
| ompi/test/t/mpi_t_level_pinning.c | Adds test for pinned thread-level reporting across scopes (MPI_T vs world). |
| ompi/test/t/mpi_t_level_no_downgrade.c | Adds test for preventing MPI_T(SINGLE) from downgrading a MULTIPLE world model. |
| ompi/test/t/mpi_t_concurrent_init.c | Adds concurrent MPI_T and Sessions init/finalize stress test. |
| .gitignore | Ignores new ompi/test/t/* test binaries produced by make check. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jsquyres
force-pushed
the
pr/btl-tcp-event-base-double-free
branch
2 times, most recently
from
July 21, 2026 03:18
d44110c to
94de56b
Compare
MPI_T_init_thread() registers every MCA framework, bumping each framework's refcount. A framework only truly closes when its refcount drains, so with an MPI_T reference outstanding, MPI_Finalize()'s framework closes are mere decrements: the true closes -- component_close(), where components delete the events they registered on opal_sync_event_base -- are deferred into MPI_T_finalize(). MPI_T is reference counted independently of MPI, so that legally happens after MPI_Finalize() has already run opal_finalize() (MPI-5.0 sec. 15.3.1 permits MPI_T_init_thread() before MPI_Init() and MPI_T_finalize() after MPI_Finalize()). Since commit 21c8825, opal_finalize() frees the shared event base (fixing a real fd leak: one leaked libevent base per MPI session cycle). In the ordering above, the deferred component closes then run against a freed base. The visible symptom: the tcp btl, which decided whether the base it held was private by comparing it against opal_sync_event_base (NULL by then), concluded it owned the shared base and freed it a second time -- a segfault inside MPI_T_finalize(). Any component with events still registered would have dereferenced freed memory in opal_event_del() regardless. The accounting gap is in MPI_T, not in the components. MPI_T has always held references on the OPAL layers its deferred closes need -- it calls opal_init_util() precisely so that the MCA var and class systems outlive MPI -- but the event base lives in the full-OPAL layer that MPI_T never referenced. That did not matter while the base was immortal; 21c8825 made its lifetime finite and honest, and the missing reference became observable. Give opal_event_init()/opal_event_finalize() a reference count (the base is created on the first reference and freed on the last), have MPI_T_init_thread() take a reference, and release it in MPI_T_finalize() only after ompi_info_close_components() has run the deferred closes. The base is now alive whenever any component's close can run, for every legal init/finalize interleaving -- and for every component, current and future, with no per-component changes. opal_event_register_params() can now legally run twice (from MPI_T first, then from a later opal_init()), so it becomes idempotent. The fd leak fixed by 21c8825 stays fixed: with no MPI_T reference outstanding, each opal_init()/opal_finalize() cycle still creates and frees the base; with one outstanding, at most a single base stays alive, and it is freed at MPI_T_finalize(). Signed-off-by: Jeff Squyres <jeff@squyres.com>
When the tcp progress thread is in use, mca_btl_tcp_component_close() freed the component's private event base near the top of close -- and then went on to opal_event_del() the listening-socket events, the pending tcp_events, and (via the proc table teardown) the endpoint events, all of which are registered on that just-freed base. Every one of those deletions dereferenced freed memory. Defer the free to the end of close, after every event this component registered has been deleted. Also drop the base alias itself on every close path, owned base or not. OPAL may free and recreate the shared base between one open/close cycle of this component and the next (MPI Sessions closes and re-opens the btl framework), and mca_btl_tcp_component_create_listen() decides whether it still needs a progress-thread base by NULL-checking mca_btl_tcp_event_base -- which component_open() never resets. A stale alias from a previous cycle would make the next open skip base creation and register events on a base from a dead generation. Both problems are confined to teardown/reopen paths; nothing on the send/receive path changes. Signed-off-by: Jeff Squyres <jeff@squyres.com>
mca_bml_r2_finalize() frees the btl_modules array and zeroes
num_btl_modules, but left mca_bml_r2.btls_added set. That flag is the
guard that makes mca_bml_r2_add_btls() a no-op, and it was only ever
reset in mca_bml_r2_component_init() -- so it is stale-true whenever
the module is finalized and then used again without a full component
re-initialization in between.
That gap is reachable through MPI_T. MPI_T_init_thread() registers
every MCA framework, bumping each framework's refcount, and a framework
only truly closes when its refcount drains. With an MPI_T reference
outstanding, an MPI session's finalize runs the per-session pml
finalize (which calls bml_finalize(), emptying r2's arrays), but the
bml framework close is deferred -- so mca_bml_base_init() skips r2's
component init on the next session's init, nothing re-runs BTL
selection or resets the flag, and add_procs then attaches zero BTLs to
every proc. The second MPI_Session_init() in a
T_init -> (Session_init/Session_finalize) x N -> T_finalize
sequence failed with MPI_ERR_INTERN ("BTLs attempted: (null)") even
though the BTL modules themselves were still alive on
mca_btl_base_modules_initialized, exactly one rebuild away from
working.
Plain MPI session re-initialization -- the same cycles with no MPI_T
reference held -- does NOT hit this, which is why the bug survived so
long. Without an outstanding framework reference, the last session's
finalize drains the bml framework's refcount, mca_bml_base_close()
runs and resets mca_bml_component_init_called, and the next session
therefore re-runs mca_bml_r2_component_init(), which re-selects the
BTLs and resets btls_added. Verified empirically on unmodified main:
repeated session cycles pass without MPI_T and fail at the second
cycle with it. Any future mechanism that, like MPI_T, holds framework
refcounts across a last-instance finalize would have re-exposed the
same stale guard.
Reset the flag where the state it guards is torn down. The next
add_procs then rebuilds the array from mca_btl_base_modules_initialized
-- the btl modules that survive precisely because their framework is
being held open -- and reachability (via the self btl in the singleton
case) works again. Deliberately NOT re-running full BTL selection:
the components were never closed, so a second component_init would
re-initialize live components and duplicate their modules on the
selected-modules list.
Note an adjacent limitation this commit does not address: a surviving
tcp btl module's endpoint address was published into the PMIx namespace
of the session that created it, so across session cycles under MPI_T it
reports peers unreachable rather than re-publishing into the new
namespace. Harmless for singletons (self carries the traffic); a
multi-rank job spanning that ordering would need per-session address
re-publication, which is a larger component-lifecycle question.
Signed-off-by: Jeff Squyres <jeff@squyres.com>
MPI_T entry points serialize against each other (ompi_mpit_big_lock), and instance -- world and session -- initialization and teardown serializes against itself (instance_lock). Nothing serialized the two families against each other, yet a first MPI_T_init_thread() and an MPI_Init()/MPI_Session_init() on another thread execute the same unlocked under-layers concurrently: the opal_init_util()/opal_init() reference counters (plain ints), the shared event base reference count, MCA variable registration (unlocked tables), and every framework's refcnt (plain ints, bumped by MPI_T's register-everything pass on one side and by pml/btl/coll opens on the other). The matching teardown window is just as exposed: the last MPI_T_finalize() performs the deferred true component closes while a concurrent instance bring-up may be opening the same frameworks. This is not theoretical. A test running MPI_T init/finalize cycles on one thread against session init/finalize cycles on another dies on the unserialized code in nearly every run, in varied ways (SIGSEGV, SIGABRT, spurious MPI_T_ERR returns), depending on where the interleaving lands. Fix by hoisting rather than pushing locks down: MPI_T_init_thread() and MPI_T_finalize() now also take the instance lock (exposed through new ompi_mpi_instance_lock()/unlock() accessors) around their init and teardown bodies. One coarse serialization at the entry points covers every shared under-layer at once; per-counter atomics could not (the refcount 0->1 transitions perform creation work and need mutual exclusion, and the MCA registration machinery has no locking at all), and making the whole MCA base thread-safe is a far larger project than these two call sites. Lock ordering is documented at the accessor definition: MPI_T always takes ompi_mpit_big_lock first, then the instance lock; nothing on the instance side ever takes the MPI_T lock, so the order cannot invert. The instance lock is recursive, and both locks are confined to init/finalize entry points -- nothing on any communication path is affected. This is not a standards gray area. MPI 5.0 sec. 11.3.1 states that "MPI_SESSION_INIT is always thread safe; multiple threads within an application may invoke it concurrently" -- a blanket thread-safety guarantee, with no carve-out for what other threads may be doing at the time, including running MPI_T_init_thread(). The failure mode without this change is silent memory corruption in a conforming program. Signed-off-by: Jeff Squyres <jeff@squyres.com>
jsquyres
force-pushed
the
pr/btl-tcp-event-base-double-free
branch
from
July 21, 2026 12:55
94de56b to
bce3c43
Compare
jsquyres
force-pushed
the
pr/btl-tcp-event-base-double-free
branch
from
July 21, 2026 13:39
bce3c43 to
07483f1
Compare
Member
Author
|
The CI failures on the previous push ( Root cause: on a multi-interface host, the tcp btl sets Reproduced locally by forcing a second tcp interface (identical signal/address/backtrace to CI), verified gone on the fixed build under the same forcing, and the test harness now clears |
MPI_T_init_thread() implemented its thread-level handling by calling
ompi_mpi_thread_level() -- the helper that establishes the World
Model's thread level. That call unconditionally overwrote
ompi_mpi_thread_requested, ompi_mpi_thread_provided,
ompi_mpi_thread_multiple, and (when unset) ompi_mpi_main_thread with
MPI_T's arguments. The immediately preceding comment ("TODO -- this
might be wrong") was correct.
Per MPI 5.0 sec. 15.3.4, the MPI_T thread level is scoped to the tool
information interface routines alone -- it "initializes the thread
environment for all routines in the MPI tool information interface"
and the returned provided describes "the actual level of thread
support that will be provided by the MPI implementation for calls to
MPI tool information interface routines." And per sec. 11.6.2,
MPI_QUERY_THREAD must return "the level of thread support returned by
the original call to MPI_INIT_THREAD" for the life of the process.
The scopes are distinct, their reports are pinned, and the standard's
only sanctioned coupling ("may influence", secs. 11.6.2 and 15.3.4) is
of the level *granted to a later initialization* -- never mutation of
a level already reported.
The stomping had two concrete failure modes:
- MPI_T_init_thread(MPI_THREAD_MULTIPLE) before an MPI session flipped
the process onto THREAD_MULTIPLE code paths mid-flight (and broke
what MPI_QUERY_THREAD would later report); on macOS this crashed
with a SIGSEGV in ompi_comm_destruct() at the session's finalize,
serially, with no threads involved.
- Worse and silent: MPI_T_init_thread(MPI_THREAD_SINGLE) inside a
running MPI_THREAD_MULTIPLE application overwrote
ompi_mpi_thread_provided/thread_multiple downward, turning off
opal_using_threads()-gated locking and ob1's threaded request paths
in a genuinely multithreaded program.
Give MPI_T its own level (ompi_mpit_thread_level), pinned for the init
epoch: established by the first MPI_T_init_thread(), reported
unchanged by nested init calls (sec. 15.3.4: they have "no effect
beyond increasing the reference count" -- nested calls previously did
not write *provided at all), and reset when the last MPI_T_finalize()
ends the epoch. The granted level is exactly the requested level:
every MPI_T routine is serialized under ompi_mpit_big_lock, so any
level is supportable, and granting only what was asked keeps a
MPI_THREAD_SINGLE tool process free of atomics overhead.
Because a tool at any level above MPI_THREAD_SINGLE may legally drive
MPI_T from a different thread than the one driving MPI proper, a
first MPI_T_init_thread() with required > MPI_THREAD_SINGLE ratchets
OPAL's process-wide thread flags up (opal_set_using_threads(true),
opal_single_threaded = false) -- and only up, since the pinned
promises of other live scopes may already depend on them. This
happens under both the MPI_T and instance serialization locks, at a
point where the previous effective level's own contract guarantees no
other thread is concurrently inside OPAL, which is what makes the
mid-flight ratchet safe.
Also document the MPI_T-specific thread-level semantics -- the scope,
the per-epoch pinning, and the possible MPI_THREAD_SERIALIZED
downgrade -- in the MPI_T_init_thread(3) man page.
Signed-off-by: Jeff Squyres <jeff@squyres.com>
The process-wide thread flags were written by plain assignment from whichever scope initialized most recently, but MPI 5.0 gives every scope -- the World Model, each session, and MPI_T -- its own thread level, pinned at that scope's initialization (secs. 11.3.1, 11.6.2, 15.3.4). Multiple levels can therefore be live simultaneously, and the only sound process-wide state is the maximum over all active scopes: a monotonic ratchet. Two writers violated that: - ompi_mpi_instance_init() assigned opal_single_threaded = (ts_level == MPI_THREAD_SINGLE) on *every* instance creation, before taking the instance lock. Creating a session with thread_level=MPI_THREAD_SINGLE while a THREAD_MULTIPLE scope was live flipped the process back to "single threaded" -- last-writer-wins on a flag that consumers (e.g. osc/ucx, which copies it into opal_common_ucx_single_threaded to elide UCX locking) use to disable synchronization. - ompi_mpi_thread_level() assigned ompi_mpi_thread_multiple = (provided == MPI_THREAD_MULTIPLE), which lowers the flag when a THREAD_MULTIPLE session precedes a lower-level MPI_INIT_THREAD. That flag gates ob1's threaded request paths (pml_ob1_isend/irecv fast paths) dynamically, so lowering it mid-flight re-enables a single-threaded request cache under concurrent use. Both writes become raise-only, and the instance-side ratchet moves under the instance lock, where the previous effective level's own contract guarantees no other thread is concurrently inside OPAL -- which is what makes a mid-flight upward ratchet safe (dynamic consumers like the OPAL_THREAD_* macros and ob1's per-call checks pick the new value up immediately; state frozen into objects at creation is conformant per sec. 11.6.2, which scopes an object's thread support to the scope it was derived from). This also fixes a sessions-only gap: nothing ever set ompi_mpi_thread_multiple for a thread_level=MPI_THREAD_MULTIPLE session (ompi_mpi_thread_level() is only called from the World Model path), so such a session ran ob1's single-threaded request-cache fast path while genuinely multithreaded. The instance ratchet now engages it. The World Model's reporting globals (ompi_mpi_thread_provided et al.) remain written exactly once, from ompi_mpi_init(): sec. 11.6.2 pins MPI_QUERY_THREAD to "the level of thread support returned by the original call to MPI_INIT_THREAD" for the life of the process, regardless of what other scopes do later. Signed-off-by: Jeff Squyres <jeff@squyres.com>
This link flag dates to 42a05e9 (May 2004, the LAM-heritage test reorganization) and has worked on every supported system since. Note why it is hand-coded: these tests call pthread_* APIs directly, and configure's THREAD_* variables -- which describe whatever threading package the threads MCA framework selected, not necessarily pthreads -- are only applied to the wrapper compilers and are not AC_SUBSTed for Makefile use. Signed-off-by: Jeff Squyres <jeff@squyres.com>
ompi_comm_init_mpi3() marks the predefined world and self communicators OMPI_COMM_PML_ADDED, but the matching MCA_PML_CALL(add_comm()) calls exist only in the World Model path (ompi_mpi_init()). A sessions-only process whose pml or osc requires comm world -- e.g. ob1 over a multi-interface tcp btl at MPI_THREAD_MULTIPLE, where tcp sets MCA_BTL_FLAGS_SINGLE_ADD_PROCS on its modules -- runs ompi_comm_init_mpi3() from instance init, and then tears down world and self with the flag set but c_pml_comm still NULL: the communicator destructor calls the pml's del_comm() on a communicator the PML has never seen, and ob1 dereferences the NULL c_pml_comm (SIGSEGV at MPI_Session_finalize()). Actually add the predefined communicators to the PML in the instance init path, after add_procs() has run (matching the World Model's ordering). The c_pml_comm NULL guard keeps the World Model -- which performs its own add_comm() calls after instance init returns -- from adding twice. This bug is latent on main: it is reachable there with the tcp progress thread enabled on a multi-interface host. It became easy to hit once a quiescent session's MPI_THREAD_MULTIPLE request is granted in full -- CI's multi-interface runners crashed in the mpi_t_sessions test at the quiescent MULTIPLE session's finalize, while single-interface local runs (where tcp never sets MCA_BTL_FLAGS_SINGLE_ADD_PROCS) passed. Signed-off-by: Jeff Squyres <jeff@squyres.com>
MPI_T is reference counted independently of both the world model and
the sessions model, so a tool may legally interleave MPI_T
init/finalize with MPI_Init/MPI_Finalize and with MPI session cycles
in many orderings -- including bracketing MPI entirely (MPI-5.0
sec. 15.3.1) -- and, in a threaded program, may run its lifecycle
concurrently with theirs. Each scope also carries its own pinned
thread level (secs. 11.3.1, 11.6.2, 15.3.4). Nothing in the tree
exercised any of this, which is how the bugs fixed earlier in this
series went unnoticed.
Add a suite of tests covering the matrix, all singletons runnable
under 'make check' with no launcher:
- mpi_t_outlives_mpi: the ordering that crashed --
MPI_T_init_thread() -> MPI_Init() -> MPI_Finalize()
-> MPI_T_finalize()
MPI_T_init_thread() registers every MCA framework, bumping each
framework's refcount, so MPI_Finalize()'s framework closes are mere
decrements: the true closes (component_close(), where components
delete the events they registered on OPAL's shared event base) are
deferred into MPI_T_finalize(), after MPI_Finalize() has already run
opal_finalize(). This test fails (segfault) without the event-base
reference counting commit in this series and passes with it.
- mpi_t_lifecycle: tool-only MPI_T -- a plain cycle, nested inits
(refcounting), and repeated full cycles, which drive OPAL's util
layer and the MCA var system through complete teardown and
re-initialization.
- mpi_t_stress: 10,000 full MPI_T init/finalize cycles with no MPI
anywhere in the process, so growth bugs (fd leaks, heap growth,
stale pointers across var-system generations) surface long before
the loop ends. Runs in under 10 seconds.
- mpi_t_world_close_first, mpi_t_world_inner, mpi_t_world_outlast:
the remaining three orderings against MPI_Init/MPI_Finalize. Each
needs its own executable because MPI_Init() is legal only once per
process. mpi_t_world_outlast also nests two MPI_T references across
MPI_Finalize(), so the *second* MPI_T_finalize() is the one that
performs the deferred component closes.
- mpi_t_sessions: interleavings against the sessions model in one
program (sessions repeat): MPI_T bracketing a session, inside a
session, outliving a session (the sessions analog of the
mpi_t_outlives_mpi hazard), two overlapping sessions, nested MPI_T
around a session, interleaved full cycles of each flavor, and MPI_T
at MPI_THREAD_MULTIPLE bracketing a session -- the ordering that
crashed on macOS (SIGSEGV in ompi_comm_destruct()) when MPI_T's
thread level was still being written into the World Model's globals.
- mpi_t_repeated_sessions: repeated session cycles under one
continuously-held MPI_T reference. This ordering exposed the stale
btls_added guard fixed earlier in this series; the test would have
failed against every tree since sessions were introduced.
- mpi_t_world_sessions: all three lifecycles in one process, including
a session that outlives MPI_Finalize() and a final MPI_T_finalize()
that performs the deferred closes after both other models have wound
down.
- mpi_t_level_pinning: MPI_T at MPI_THREAD_MULTIPLE, then the world at
MPI_THREAD_SINGLE: the strong MPI_T level must not bleed into what
MPI_QUERY_THREAD reports (pinned to the original MPI_INIT_THREAD
return, sec. 11.6.2), and nested MPI_T inits must keep reporting the
epoch's pinned level regardless of what they request (sec. 15.3.4).
- mpi_t_level_no_downgrade: the downgrade direction of the same bug --
MPI_T_init_thread(MPI_THREAD_SINGLE) inside an MPI_THREAD_MULTIPLE
world must leave MPI_QUERY_THREAD and MPI_Is_thread_main()
unchanged. (The silent-downgrade variant also used to disable
locking process-wide; a singleton can only observe the reports.)
- mpi_t_concurrent_init: one thread loops full MPI_T init/finalize
cycles while another loops full session init/finalize cycles, with
no ordering between them, followed by a serial cycle of each. This
hammers the window the init-serialization commit closes; against
unserialized code it failed every run, each time differently
(SIGTRAP, SIGABRT, SIGSEGV, spurious MPI_T_ERR returns,
opal_init_util() failures); with the serialization it passes 30
consecutive runs in well under a second. A data race is
probabilistic by nature -- a failure proves the bug, a pass is only
evidence -- and the iteration counts trade coverage against 'make
check' runtime. MPI_T runs at MPI_THREAD_SERIALIZED (the process is
multithreaded and the MPI_T-driving thread is not promised to be the
main thread), which also engages MPI_T's thread-flag ratchet, so the
ratchet and the serialization are exercised together.
The repeated-cycle cases caught a real bug during development of the
event-base reference counting commit in this series:
opal_event_register_params()'s idempotence guard skipped
re-registration in a process's second OPAL cycle, even though the MCA
var teardown had deregistered the variables and NULL'ed the string
storage -- silently losing the opal_event_include variable and falling
back from epoll to select on Linux. That commit now re-registers per
var-system generation.
All tests pin OMPI_MCA_pml=ob1 and OMPI_MCA_btl=tcp,self so that a
component with events registered on the shared event base (the tcp
btl) is actually in play at every deferred close; without that, the
tests could pass vacuously on builds where another PML wins selection.
(Best effort: on a host with no usable TCP interface the tcp btl
yields no modules and registers no events.)
Signed-off-by: Jeff Squyres <jeff@squyres.com>
jsquyres
force-pushed
the
pr/btl-tcp-event-base-double-free
branch
from
July 21, 2026 17:50
07483f1 to
8b67567
Compare
Member
|
is this PR ready for review? |
Member
Author
Yes. Went through a bunch of rounds of adversarial review (+ fixes) overnight. |
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
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.
MPI has three separately reference-counted lifecycles — the World Model, sessions, and MPI_T — and programs may legally start and stop them repeatedly, in any interleaving, from different threads, each with its own thread level (MPI-5.0 secs. 11.3.1, 11.6.2, 15.3.1, 15.3.4). Exercising those orderings turned up a series of startup/shutdown bugs ranging from deterministic segfaults to silent memory corruption, plus a family of related thread-level reporting bugs. This PR fixes seven of them and adds a thirteen-program test matrix that walks the lifecycle orderings under
make check. (Reviewers who saw the first version of this PR: it was reworked from a per-component hardening approach to the root-cause fixes described here; see "History" below.)The startup/shutdown bugs
1. The shared event base was freed while deferred component closes still needed it (
opal/event: reference count the shared event base).MPI_T_init_thread()registers every MCA framework, bumping each framework's refcount, soMPI_Finalize()'s framework closes are mere decrements: the true closes —component_close(), where components delete the events they registered onopal_sync_event_base— are deferred intoMPI_T_finalize(), afteropal_finalize()has already freed the base (since #14154, which fixed a real fd leak per session cycle). The tcp btl double-freed the base (deterministic segfault inMPI_T_finalize()); any component'sopal_event_del()at deferred close was a silent use-after-free. Fix: reference countopal_event_init()/opal_event_finalize(); MPI_T takes a reference and releases it after its deferred closes run. References are held by the two lifetime scope owners (opal_init(), and MPI_T as the only deferrer of framework closes), so every component — current and future — is covered with no per-component changes. The commit also hardens the repeated-cycle path (per-var-system-generation parameter re-registration, latching libevent's one-shot process-global initializers, taking the reference only on successful init, honest error reporting when a partial MPI_T init cannot be unwound).2. The tcp btl freed its private event base before deleting the events on it (
btl/tcp: free the private event base only after deleting its events). Independent, pre-existing ordering bug on the (non-default) progress-thread path, plus a stale base alias that a framework re-open (MPI Sessions) could pick up across base generations.3. A second
MPI_Session_init()under a held MPI_T reference found zero BTLs (bml/r2: reset btls_added when the module finalizes). The per-session pml finalize empties r2's module array, but with MPI_T holding the bml framework's refcount, the component re-init that resets thebtls_addedguard never runs — so the guard was stale-true over an empty array and the next session'sadd_procsattached no BTLs (MPI_ERR_INTERN, "BTLs attempted: (null)"). Reproduced on unmodified main; the fix resets the guard where the state it guards is freed. Known adjacent limitation, documented in the commit: a surviving tcp module's address lives in the previous session's PMIx namespace, so multi-rank jobs spanning session cycles under MPI_T would additionally need per-session address re-publication (clean "unreachable" failure, not corruption).4. Nothing serialized a first MPI_T initialization against a concurrent world/session initialization (
ompi: serialize MPI_T init/finalize against instance init/finalize). The two families each serialize internally (ompi_mpit_big_lock;instance_lock) but not against each other, and they share unlocked under-layers: the OPAL init reference counters, the event-base refcount, MCA variable registration, and every framework's refcnt. MPI 5.0 sec. 11.3.1 is blunt that this must work: "MPI_SESSION_INIT is always thread safe; multiple threads within an application may invoke it concurrently." A threaded test failed every run on the unserialized code, five different ways (SIGTRAP, SIGABRT, SIGSEGV, spuriousMPI_T_ERRreturns,opal_init_util()failures). Fix:MPI_T_init_thread()/MPI_T_finalize()also take the instance lock around their init/teardown bodies — one coarse serialization at two entry points, covering every shared under-layer at once, with documented non-invertible lock ordering and nothing on any communication path. Follow-on hardening closed the adjacent windows: the world model now decides its thread level and performs instance initialization in a single critical section (a concurrentMPI_Session_initcould previously slip between the two lock acquisitions), and a concurrent lastMPI_T_finalize()can no longer releaseompi_mpi_main_threadout from under anMPI_Init()in progress.The thread-level reporting bugs
5. MPI_T's thread level was written into the World Model's (
mpi/tool: give MPI_T its own thread level; stop stomping the world's). Per sec. 15.3.4 the MPI_T level is scoped to the tools interface routines alone, and per sec. 11.6.2MPI_QUERY_THREADis pinned for the process lifetime to "the level of thread support returned by the original call to MPI_INIT_THREAD" — butMPI_T_init_thread()implemented its level by calling the world-model helper (the oldTODO -- this might be wrong), overwritingompi_mpi_thread_provided/thread_multiple/main_thread. Consequences:MPI_T_init_thread(MULTIPLE)before a session flipped the process onto THREAD_MULTIPLE code paths mid-flight — a deterministic SIGSEGV inompi_comm_destruct()on macOS — and, worse,MPI_T_init_thread(SINGLE)inside a THREAD_MULTIPLE application silently disabled locking process-wide and corrupted whatMPI_QUERY_THREADreports. Fix: MPI_T gets its own epoch-pinned level (granted = requested; every MPI_T call is serialized under the big lock, so any level is supportable), nested inits report the pinned value (previously they didn't writeprovidedat all), and a level above SINGLE ratchets OPAL's process-wide thread flags — upward only.6. The process-wide thread flags were last-writer-wins (
ompi: ratchet the process-wide thread flags; never lower them). Every scope's level is pinned, multiple levels can be live simultaneously, so the only sound process-wide state is the maximum over active scopes — a monotonic ratchet. Two writers violated that:ompi_mpi_instance_init()assignedopal_single_threadedon every session creation (a THREAD_SINGLE session flipped a THREAD_MULTIPLE process back to lock-elision), and the world-model helper could lowerompi_mpi_thread_multiple. Both become raise-only, written only at quiescent points (no active instance, and no MPI_T epoch whose threads could be reading them) and only on actual transitions, so the writes can never race an unlocked hot-path reader. This also fixed a sessions-only gap found during the audit: nothing ever setompi_mpi_thread_multiplefor athread_level=MPI_THREAD_MULTIPLEsession, so such a session ran ob1's single-threaded request-cache fast path while genuinely multithreaded.7. Sessions-path predefined communicators were marked PML-added but never actually added to the PML (
ompi/instance: PML-add world/self when sessions require the world). When the selected pml or osc requires comm world — ob1 sets this whenever a BTL carriesMCA_BTL_FLAGS_SINGLE_ADD_PROCS, which the tcp btl sets with two or more usable interfaces atMPI_THREAD_MULTIPLE— a sessions-only process runsompi_comm_init_mpi3()from instance init. That function marks world/selfOMPI_COMM_PML_ADDED, but the matching pmladd_comm()calls exist only in the World Model path (ompi_mpi_init()), so teardown handed the pml'sdel_comm()a communicator it had never seen and ob1 dereferenced the NULLc_pml_comm: deterministic SIGSEGV atMPI_Session_finalize(). Latent on main (reachable there with the tcp progress thread on any multi-NIC host); bug 6's fix made it much easier to hit, and CI's multi-interface runners caught it at the quiescent MULTIPLE session's finalize while single-interface local runs passed. The fix performs the pmladd_comm()for world/self in the instance init path afteradd_procs()(matching the World Model's ordering), guarded onc_pml_commso the World Model's own later adds cannot double-add. The test harness now also clearsbtl_tcp_if_excludeso loopback counts as a second tcp interface — the crash reproduces on the unfixed build on any host with a NIC, not just in CI.A user-visible consequence, sanctioned by MPI 5.0's "may influence" clauses (secs. 11.6.2, 15.3.4): when
MPI_THREAD_MULTIPLEis requested by a scope while another scope is already active at a lower level, the running process cannot be safely upgraded mid-flight (components were configured at the lower level, and the flags cannot change under unlocked readers), so the new scope is grantedMPI_THREAD_SERIALIZEDinstead — reported honestly through the session'sthread_levelinfo key or the world'sprovided. The common tool pattern is explicitly preserved and regression-tested:MPI_T_init_thread()from a PMPI wrapper before the application'sMPI_Init_thread(MPI_THREAD_MULTIPLE)still grants the application MULTIPLE. The MPI_T-specific semantics (scope, epoch pinning, possible downgrade) are documented in theMPI_T_init_thread(3)man page.Failure paths are now transactional.
mca_base_framework_register()fully unwinds itself on failure (components unloaded, variable group deregistered, output closed, reference dropped), the info-support project loops roll back their own successful registrations, and a failedMPI_T_init_thread()releases everything it acquired, leaving MPI_T honestly uninitialized. The one deliberate exception: onMPI_ERR_BAD_PARAM, registrations are left alive soompi_infocan still dump the offending parameter context as a diagnostic. A failed first instance initialization latches (its partial state cannot be safely re-entered), as does a failed MPI_T registration.The tests (last commit)
Thirteen programs, all singletons wired into
make checkwith no launcher: the original crash ordering (fails/segfaults without fix 1 — verified); tool-only lifecycles (plain, nested, repeated); a 10,000-cycle MPI_T stress test (~10 s); all four orderings against the world model; the sessions interleavings (bracketing / inside / outliving / overlapping / nested / interleaved, plus MPI_T-at-MULTIPLE bracketing a session — the macOS crash from bug 5); repeated sessions under a held MPI_T reference (bug 3); all three lifecycles in one process; thread-level pinning in both directions (bugs 5/6:MPI_QUERY_THREADand nested-MPI_Tprovidedmust not change); the threaded MPI_T-vs-sessions concurrency test (bug 4), plus grant-semantics assertions woven through the suite: mixed-level overlapping sessions must report the SERIALIZED downgrade, a MULTIPLE request at quiescence must be granted in full, and the PMPI tool pattern must not cost an application its MULTIPLE. All tests pinpml=ob1+btl=tcp,selfso a component with events on the shared base is genuinely in play at every deferred close, and clearbtl_tcp_if_excludeso the tcp btl yields multiple modules even on a single-NIC host (the bug 7 trigger).Validation
./autogen.pl+ fresh VPATH builds with fullmake checkon macOS (Apple Silicon, clang) and AlmaLinux 10 (arm64, gcc 14): zero failures (143/144 total PASS respectively), all thirteen MPI_T tests pass on both, no warnings from any file this PR touches.MPI_THREAD_MULTIPLEapplication running under an attached tool.History
The first version of this PR taught the tcp btl to tolerate a dead event base. Auditing for the same pattern kept widening the blast radius (usnic,
opal_hotel, the ULFM detector), which is the signature of treating symptoms; the reference count fixes the disease, and the regression test passes against the original unmodified tcp btl with the refcount fix alone. The pmix/base deadevbasefield removal that briefly rode along here is now #14176.