You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
PROF-16213: Account CTimer table in NativeMem and gate thread registration - #912
Accounts the CTimer timer table. The tid-indexed table (pid_max × sizeof(int)) is recorded in NativeMem under a new NM_CTIMER (ctimer) category.
Makes CTimer table allocation failure safe.CTimer::start() returns an error and leaves no NULL table reachable through a non-zero bound.
Gates thread registration around engine start and stop. The pthread_create hook and the JVMTI ThreadStart/ThreadEnd callbacks now reach the engines only through Profiler::registerThread()/unregisterThread(). Those calls are gated, counted in flight, and run with thread cancellation disabled. This removes a race in which a hook call indexed the timer table while CTimer::start() freed and replaced it.
Most PerfEvents table defects found on the way are out of scope and tracked in PROF-16359. This PR changes PerfEvents only where the new gates or the CPU engine lifecycle need it.
Motivation:
PROF-16213: the CTimer table was allocated without a NativeMem::record call. It is about 16 MiB of address space at a 4M pid_max, and none of it appeared in the native_mem_* counters.
Accounting the table meant routing its allocation through one function. Doing that exposed two older defects in the same code:
a failed allocation left a NULL table behind a non-zero bound;
the thread hook used the table while start() replaced it, with no lock and no ordering.
Additional Notes:
Accounting
New category instead of NM_PERF: perf names perf_events in counters and docs. Every chunk now carries native_mem_*_bytes.ctimer. Consumers with a fixed category list need the new name.
The full table size is recorded, as NM_PERF does for the PerfEvent array. Only pages of live tids are resident, so the gauge overstates RSS for this category.
Allocation failure
The table stays empty (_max_timers = 0), so every tid bound check rejects. start() returns Failed to allocate CPU timer table.
Profiler::start() enables the CPU engine only if its own start() succeeded. Before, it enabled the engine whenever any mode activated. After a failed CPU engine start, timers or fds left armed by the previous recording could then emit CPU samples into the new one.
PerfEvents: a failed _events allocation resets _max_events to -1, logs an error, and fails start() before the signal handler, the hook and the registration gate are set up, so the engine stays off for the recording. walkKernel() and resetBuffer() skip the missing table, because fds armed by an earlier recording keep delivering SIGPROF and the handler calls resetBuffer() on every exit.
Thread registration gates
Why gates are needed: Profiler::start() installs the pthread_create hook before the CPU engine starts, and the hook runs without _state_lock. Disabling the JVMTI events in stop() does not wait for callbacks already running.
registerThread() needs _engines_ready. CTimer::start() and PerfEvents::start() open it through Profiler::openThreadRegistration(), after their table is sized and before they list existing threads. A thread that starts or exits while start() registers the existing ones is then handled by the hook. The per-slot CAS in both engines resolves the hook and start() registering the same tid. stop() closes the gate, and start() closes it again if the CPU engine fails.
The gate tracks the CPU engine, so it admits only that engine: registerThread() never calls the wall engine. No wall engine overrides Engine::registerThread(). One that ever does needs a gate of its own, because opening this one would also admit a CPU engine that failed.
PerfEvents::start() opens the gate the same way. Without that call, the gate stays closed until PerfEvents::start() returns, and a thread created while it lists and sweeps threads is never registered for the recording.
unregisterThread() needs _thread_tables_stable. ThreadTablesFreeze clears it only while CTimer::sizeTimerTable() replaces the timer table (first start, or pid_max changed) or while Profiler::replaceCpuEngine() lets the previous CPU engine release its threads. It stays set while idle: PerfEvents::stop() keeps per-thread fds open, so a thread that exits after stop() relies on this call to close its fd.
Why the freeze is that narrow: an exit that arrives while unregistration is closed is dropped. If the thread is still listed when the engine lists threads, its timer stays in the slot after it dies, and a thread that later reuses the tid cannot claim it. start()/stop() cycles repeat the window. A restart with an unchanged pid_max replaces no table, so it never closes unregistration.
Each call counts itself in flight (HookCallScope), on its gate's own counter. closeThreadRegistration() waits only for registration calls, so no registerThread() call is inside an engine when stop() tears it down. It does not wait for unregistration calls: they stay allowed during stop(), and under thread churn their count may not reach zero within the budget although none is stuck. ThreadTablesFreeze closes both gates and waits for both counts. The flags and the counters are seq_cst on both sides: either the hook sees the gate closed, or the closer sees the call and waits.
The wait is bounded (1 s) and logged on timeout. A freeze timeout fails the operation and replaces nothing. A closeThreadRegistration() timeout means a registration call is stuck inside an engine and may still use its table, so the profiler goes to ERROR, which rejects every later start(), the only place a table is replaced. stop() still completes, because no step frees a per-thread table. start() rolls back its setup (rollBackStart(), which also restores the library patches). The cost: a stuck CTimer::registerThread() that finishes after CTimer::stop() leaves one armed timer whose signals are dropped.
CPU engine change: when start() selects another CPU engine, Profiler::replaceCpuEngine() has the previous one release its per-thread resources (Engine::releaseThreads()) under a ThreadTablesFreeze before the slot changes. Otherwise PerfEvents' fds, kept open across stop(), would never be closed once thread exits go to the new engine. PerfEvents also releases its threads before start() frees its table on a pid_max change.
start() replaces _cpu_engine/_wall_engine while unregistration is open. It stores the pointers with release, and the hook paths load them with acquire. An unregistration that reaches the previous engine releases only that thread's own slot.
check() selects engines into locals and never writes the engine slots. It may run while a recording is RUNNING (checkState() rejects only NEW and ERROR). Writing the slots then would let the open gate admit an engine that never started, make the wall signal handler cast wallEngine() to the wrong type, and make stop() stop the wrong engines.
Per-call cost: every thread exit takes one seq_cst and one release RMW on the unregistration counter, on the counters' own cache line. Registration skips its counter while idle; unregistration cannot, because its gate stays open while idle. Accepted for now.
Thread cancellation is disabled for the call, because the engine calls reach cancellation points (close(), Log::warn()). Acting on a cancellation there would abort while unwinding through ~HookCallScope() with the statically linked libgcc, or, on musl, skip it and leave the count raised. The restore is a manual call made after every destructor-bearing scope, not a guard. With asynchronous cancellation, current glibc acts on a pending request as soon as cancellation is re-enabled, and doing that from an implicitly noexcept destructor terminates the process.
Side effects: a Java thread starting during stop() can no longer arm a timer after CTimer::stop(). A failed CPU engine start no longer logs tid > pid_max[0] for every Java thread.
Documentation. AGENTS.md gets a "Thread registration" critical invariant: reach the engines only through the gated calls, size a per-thread table before opening the gate, replace a table only while the gate is closed (with PerfEvents::start() listed as a known exception), close unregistration only around an actual table replacement, and restore cancellation outside destructors. ProfilingEngines.md#thread-registration holds the details, and the doc/README.md index entry mentions them.
Rejected alternatives
Gating on state() == RUNNING: RUNNING is set about 110 lines after the engines start and cleared only after they stop. Threads that start in that gap would never be registered, and threads that exit in it would leave a slot that blocks a reused tid.
Atomic publication inside CTimer alone: a thread that already passed the bound check could still write into the freed table.
Closing unregistration for the whole of Profiler::start(): it drops thread exits on every restart, including the common one that replaces no table.
What this does not fix. These are PerfEvents defects that predate this PR, tracked in PROF-16359:
PerfEvents::start() frees and replaces _events on a pid_max change without excluding thread unregistration or SIGPROF handlers that still read it. It now closes the old fds and rings first, but a handler that already holds a slot pointer can still use the freed table.
PerfEvents register/unregister race on _page.
PerfEvents::start()'s stale-fd sweep closes the fd of a thread the hook registered after the thread list was taken. That thread is not sampled for the recording.
PerfEvents has no test seam for its allocation-failure path in start().
Also for CTimer: a native thread that exits while the timer table is replaced, but is still listed when CTimer::start() lists threads, leaves its timer behind. This needs a pid_max change.
How to test the change?:
ddprof-lib/src/test/cpp/ctimer_ut.cpp (Linux):
TimerTableIsAccounted, SameSizeDoesNotReaccount: the gauge follows resizes, and a same-size restart keeps the table (a sentinel value survives).
FailedAllocationLeavesEmptyTable, StartFailsWhenTableAllocationFails: a forced calloc overflow leaves an empty table. start() returns the error before it installs the SIGPROF handler. Both skip under ASan/TSan, because the CI sanitizer jobs run gtest binaries without allocator_may_return_null=1.
StartOpensThreadRegistration: a successful CTimer start opens the gate.
RegisterThreadSkipsEnginesUntilReady, UnregisterThreadSkipsEnginesWhileTablesFrozen: both calls stay off the engines while their gate is closed.
CloseThreadRegistrationWaitsForHookCalls: a real registerThread() call is held inside a blocking test engine. Closing the gate does not return until it leaves.
CloseThreadRegistrationTimesOutOnStuckHookCall, ThreadTablesFreezeClosesGatesAndWaitsForHookCalls, ThreadTablesFreezeReportsUndrainedOnStuckHookCall: the wait is bounded, and a real freeze closes both gates, waits, keeps unregisterThread() off the engines, and reports a timeout.
FailedCpuEngineStartClosesThreadRegistration, StartedCpuEngineOpensThreadRegistration, CpuEngineEnabledOnlyIfItStarted, RegisterThreadFastPathDoesNotSkipOpenGate: the gate follows whether the CPU engine started, and a failed CPU engine is not enabled.
SameSizeRestartDoesNotFreezeUnregistration, ResizeKeepsTableWhenHookCallsDoNotDrain, ResizeReplacesTableWhenHookCallsDrain: with a hook call held in flight, a same-size restart succeeds (no freeze ran), and a resize fails retryably and keeps the old table.
NegativeTidIsRejected, TidAtBoundIsRejected: register/unregister with a negative tid, or a tid equal to the bound, never index outside the table (the page before or after it is PROT_NONE). The bound test uses the calling thread's own tid, so a wrongly accepted tid gets past timer_create and faults.
CloseThreadRegistrationIgnoresInFlightUnregistration, ThreadTablesFreezeWaitsForInFlightUnregistration: closing the registration gate does not wait for an unregistration in flight; a freeze does.
StuckGateAfterCpuEngineFailurePutsProfilerInError: if the gate cannot be closed after a CPU engine failure, the profiler goes to ERROR; FailedCpuEngineStartClosesThreadRegistration checks that a gate close within the budget leaves the state unchanged.
CpuEngineChangeReleasesPreviousEngineThreads, SameCpuEngineKeepsItsThreads, CpuEngineChangeKeepsPreviousEngineWhenHookCallsDoNotDrain: an engine change releases the previous engine's threads with both gates closed, a same-engine restart keeps them, and an undrained change is refused.
ResizeFreesOldTable: a resize frees the old table, which the NM_CTIMER gauge alone cannot show. It reads glibc's mallinfo2(), so it skips on musl and under ASan/TSan.
RegistrationGateAdmitsOnlyCpuEngine: with the gate open, a counting wall engine sees no registerThread() call, and the result is the CPU engine's.
CancellationDuringHookCallActsAfterTheCall: a thread with asynchronous cancellation is cancelled inside a hook call. The engine sees cancellation disabled, the process survives, and the count returns to zero. With the restore moved into ~HookCallScope(), the process aborts on glibc 2.39. glibc 2.35 keeps the request pending until the next cancellation point, so it cannot catch that regression.
ddprof-lib/src/test/cpp/perfEvents_ut.cpp (Linux): MissingTableIsNotIndexed: with no table (the state a failed allocation leaves), resetBuffer() and releaseThreads() do not index it.
Not covered by unit tests, because they need a real start()/stop(): rollBackStart(), the stop() timeout path, and PerfEvents' allocation failure and release inside start().
Java coverage for the start/stop windows: StartWithChurningThreadsTest, StopWithChurningThreadsTest, IdleThreadLifecycleTest, CTimerSamplerTest, NativeThreadTest.
Agentic review: two Sphinx reviews. The first (17 findings) is answered in the review threads. The second ran on the CTimer-only scope (26 findings: 4 MEDIUM, 22 LOW, no Critical/High). Its generalist and performance reviewers failed on API errors, so coverage came from the correctness, architecture and security specialists plus the mutation pass.
Fixed: the shared registration gate (now CPU engine only), check() writing the engine slots during a recording, stale line references (replaced by symbol references), stale comments and counter-name lists, and tests for the CTimer tid bound and the table free.
Deferred to PROF-16359: the PerfEvents NULL table after a failed allocation, and NM_PERF mixing the tid table with ring pages.
Not addressed: tests of the gate call sites inside Profiler::start()/stop()/check(), which need a real profiler start, and the remaining LOW design notes.
A later /code-review pass (10 findings) led to: per-gate in-flight counters, so a stop() under thread churn cannot time out falsely; the CPU engine change releasing the previous engine's threads; PerfEvents allocation-failure handling; ERROR on a gate-close timeout with a shared start rollback that restores library patches; accurate timeout messages; and one place for the gate contract in code. Declined: an idle pre-check for unregistration (no effect while its gate is open), the check() LBR behavior, and the releaseThreads() scan length.
For Datadog employees:
If this PR touches code that signs or publishes builds or packages, or handles
credentials of any kind, I've requested a security review (run the dd:platform-security-review
skill, or file a request via the PSEC review form). bewaire also runs automatically on every PR.
CTimer::start() allocated its tid-indexed timer table (pid_max ints)
without recording it, so it was missing from the native_mem_* counters.
- Move the table sizing into CTimer::resizeTimers() and record the
table under a new NM_CTIMER ("ctimer") category, mirroring how
PerfEvents accounts its per-thread array under NM_PERF.
- On calloc failure, leave the table empty (_max_timers = 0) instead of
a NULL table with a non-zero bound, and fail start() with
"Failed to allocate CPU timer table". The tid bound checks then reject
every tid, and the next start() retries the allocation.
- Add ctimer_ut covering accounting across resizes, same-size restart,
and the allocation-failure path.
- Document NM_CTIMER in ProfilingEngines.md.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Skip the calloc-overflow tests under ASan/TSan. The CI sanitizer jobs
run gtest binaries without *SAN_OPTIONS, so allocator_may_return_null
defaults to 0 and the overflow is reported as a fatal error.
- Route CTimer::start() through startWithTableSize() so a test can drive
the table-allocation failure and check that start() returns an error
before installing the handler or arming timers.
- Check a sentinel survives a same-size resize, proving the table is
not reallocated (calloc may return the same block after free).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Profiler::start() installs the pthread_create hook before it starts the
engines, and the hook calls Profiler::registerThread()/unregisterThread()
without _state_lock. A thread created in that window reached
CTimer::registerThread() while resizeTimers() freed and replaced the
timer table, with no lock or ordering between them.
Add Profiler::_engines_ready. start() sets it (release) once the engines
have started; stop() clears it after the signal drain and before any
engine stops. registerThread()/unregisterThread() return early unless it
is set (acquire). Engine start() registers existing threads and engine
stop() unregisters all of them, so skipped calls neither miss nor leak
timers outside the documented start window.
Add a gtest showing registerThread() does not touch the (null) engines
while the flag is clear, and update ProfilingEngines.md for the gate and
shifted profiler.cpp line references.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The _engines_ready check in Profiler::registerThread()/unregisterThread()
was a single load. A hook call that passed it just before stop() cleared
the flag could still arm a CTimer timer after CTimer::stop() unregistered
all slots, or touch the timer table while the next start() resized it.
- Count each hook call in flight with an RAII HookCallScope. stop()
clears the flag and waits for the count to reach zero before any engine
stops. Flag and counter use seq_cst on both sides, so either the hook
sees the flag cleared or stop() sees the call and waits.
- Set _engines_ready as soon as the CPU engine has started instead of
after all engines. The wall engines do not implement registerThread(),
and the earlier set narrows the start window in which a native thread
that starts or exits is missed to the rest of CTimer::start().
- Check the in-flight count returns to zero in the gate test, and update
ProfilingEngines.md for the drain, the new set point, and shifted
profiler.cpp line references.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The hook gate opened only after the CPU engine's start() returned, so
native-thread hook calls made while start() registered the existing
threads were dropped: a thread starting after the thread list was taken
was never registered, and one exiting after being registered left a
timer slot that blocked a reused tid for the whole recording.
- Add Profiler::openThreadRegistration()/closeThreadRegistration().
CTimer::start() and PerfEvents::start() open the gate once their
per-thread table is sized and before they list threads; concurrent
registration of one tid by the hook and by start() is resolved by the
existing per-slot CAS. Profiler::start() still opens it after a
successful CPU engine start, for engines that do not, and closes it
(with the in-flight wait) when the CPU engine fails. stop() uses
closeThreadRegistration().
- Add a gtest that a successful CTimer start opens the gate, and check
that a failed one leaves it closed.
- Update ProfilingEngines.md for the new open point, and fix
profiler.cpp, ctimer_linux.cpp and perfEvents_linux.cpp line
references, including ctimer_linux.cpp ones left stale earlier in
this branch.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…gate
Gating Profiler::unregisterThread() on _engines_ready skipped it after
stop(). PerfEvents::stop() keeps per-thread fds open, so a thread started
during a recording that exited after stop() leaked its perf fd and ring
pages, and a thread reusing that tid at the next start was never sampled
because registerThread() saw _fd > 0.
- Give unregisterThread() its own gate, _thread_tables_stable. It is
cleared only from CPU engine selection until the table is sized (RAII
ThreadTablesFreeze over start()) and stays set while idle. Exits skipped
in that window leave nothing behind: CTimer::stop() deleted every timer
and PerfEvents::start() closes the fd of every tid missing from its
thread list.
- Drop the redundant openThreadRegistration() after a successful CPU
engine start (only CTimer and PerfEvents implement registerThread(), and
both open the gate in start()); make closeThreadRegistration() and
waitForHookCalls() private; keep _hook_calls_inflight off _state's cache
line; simplify resizeTimers() using the _timers == NULL iff
_max_timers == 0 invariant; keep the gate rationale in one comment.
- Test that closeThreadRegistration() waits for in-flight hook calls and
that the unregistration gate is open while idle; add a bytes() helper.
- Shorten the ProfilingEngines.md thread-registration text, drop new
line-number references, and recompute the remaining ones.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eal hook drain
- PerfEvents::start() opened the hook gate even when the _events calloc
failed, leaving a NULL table with a non-zero _max_events for the hook's
registerThread() and the idle-open unregisterThread() to index. On
failure it now returns to the not-started state (_max_events == -1) and
returns an error before installing its hook or opening the gate, as
CTimer already does.
- CloseThreadRegistrationWaitsForHookCalls now holds a real
Profiler::registerThread() call inside a blocking test engine instead
of bumping the in-flight counter, so removing HookCallScope fails it.
- Profiler::registerThread() returns -1 when the gate is closed, matching
Engine::registerThread().
- StartOpensThreadRegistration restores the SIGPROF disposition;
StartFailsWhenTableAllocationFails checks the handler is unchanged.
- Note in resizeTimers() why the JVMTI path cannot race the resize, fix
the ThreadTablesFreeze line in the lifecycle diagram, and recompute
shifted ctimer_linux.cpp/perfEvents_linux.cpp references.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…st unregister gate
- HookCallScope disables thread cancellation for the hook call. The
engine calls reach cancellation points (close(), Log::warn()); acting on
a cancellation there would abort while unwinding through the destructor
with the statically linked libgcc, or skip it on musl and leave the
in-flight count raised so the next stop() waits forever.
- PerfEvents::resetBuffer(), called from the SIGPROF handler for fds left
armed by an earlier recording, checks the table and the bound before
indexing, so a failed _events allocation in start() cannot be indexed.
- Move the UNIT_TEST early return in Profiler::unregisterThread() below
the gate and add UnregisterThreadSkipsEnginesWhileTablesFrozen.
- Say in the stop() comment, the openThreadRegistration() doc and
ProfilingEngines.md that JVMTI ThreadStart can still arm a timer after
CTimer::stop(), and that PerfEvents::start() closes the fd of a thread
the hook registered after the thread list was taken. Reflow the
resizeTimers() comment and drop its NULL-iff-zero claim.
- Tests: ignore SIGPROF before CTimer::stop(), bound the blocking-engine
waits with a deadline, restore engine pointers with an RAII guard.
- Recompute shifted line references in ProfilingEngines.md.
The remaining PerfEvents issues are tracked in PROF-16359.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…opes
HookCallScope restored the cancellation state from its destructor. With
asynchronous cancellation, current glibc acts on a pending request as
soon as pthread_setcancelstate() re-enables it, so a cancellation
requested during a hook call unwound out of the implicitly noexcept
destructor and terminated the process.
- Keep HookCallScope for the in-flight count only. Profiler::
registerThread()/unregisterThread() disable cancellation, call new
registerThreadInEngines()/unregisterThreadInEngines() helpers that hold
every destructor-bearing scope, and restore it only after they return.
The manual pair is documented as required by the RAII rule.
- Add CancellationDuringHookCallActsAfterTheCall: a thread with
asynchronous cancellation is cancelled inside a hook call; the process
must survive, the thread must not run past its next cancellation point,
and the in-flight count must return to zero. With the restore back in
the destructor the process aborts on glibc 2.39; glibc 2.35 keeps the
request pending until the next cancellation point, so it cannot catch
that regression.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…fix doc refs
- Profiler::onThreadStart()/onThreadEnd() call Profiler::registerThread()/
unregisterThread() instead of the engines directly, so the JVMTI path
shares the gates, the in-flight count and the cancellation handling.
Disabling the JVMTI events in stop() does not wait for callbacks already
running; with the gate, a Java thread starting during stop() can no
longer arm a timer after CTimer::stop(), and a failed CPU engine start
no longer logs "tid > pid_max[0]" for every Java thread.
- The cancellation test now asserts the engine runs with cancellation
disabled; the hook-drain test waits until the gate is closed before its
timed check.
- Correct the ThreadTablesFreeze comment (an exiting thread still listed
by the engine leaves its slot behind), the PerfEvents allocation-failure
comment, and the CTimerTestAccessor comment; reference PROF-16359 for
the known PerfEvents gaps.
- Recompute profiler.cpp/ctimer_linux.cpp/perfEvents_linux.cpp line
references in ProfilingEngines.md, and the profiler.cpp/profiler.h
references in JfrIntegration.md and JvmSupportLayer.md that this branch
had shifted.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zhengyu123
changed the title
Ctimer memory is not counted
PROF-16213: Account CTimer table in NativeMem and gate thread registration
Oct 28, 2026
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
An unchecked _events[tid] lookup in walkKernel() can dereference a null table and crash the JVM after a restart allocation failure; a null/range guard and regression test are still needed.
This PR adds CTimer memory accounting to the profiler’s native-memory counters and coordinates thread registration with CPU-engine startup and shutdown.
Changes:
Adds the ctimer memory category and accounts for timer-table allocation and resizing.
Handles CPU-engine table allocation failures and guards PerfEvents buffer resets.
Adds registration gates, in-flight tracking, cancellation protection, and focused unit tests.
File
Description
doc/architecture/ProfilingEngines.md
Documents registration gates and CTimer memory accounting.
doc/architecture/JvmSupportLayer.md
Updates source-line references.
doc/architecture/JfrIntegration.md
Updates source-line references.
ddprof-lib/src/test/cpp/ctimer_ut.cpp
Tests accounting, allocation failure, registration gates, and cancellation.
ddprof-lib/src/main/cpp/profiler.h
Declares registration gates and lifecycle helpers.
ddprof-lib/src/main/cpp/profiler.cpp
Gates thread callbacks and drains in-flight engine calls.
ddprof-lib/src/main/cpp/perfEvents_linux.cpp
Handles allocation failure, opens registration, and guards buffer resets.
ddprof-lib/src/main/cpp/nativeMem.h
Adds the ctimer accounting category.
ddprof-lib/src/main/cpp/ctimer.h
Declares table-sizing helpers and test access.
ddprof-lib/src/main/cpp/ctimer_linux.cpp
Accounts for the timer table and opens registration after allocation.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The new freeze/open/drain lifecycle contract is missing required updates to AGENTS.md and the ProfilingEngines entry in doc/README.md.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Document the updated engine lifecycle and thread-registration contract
doc/architecture/ProfilingEngines.md:71
The freeze/open/drain protocol changes the engine lifecycle contract, but this change set does not update AGENTS.md or doc/README.md as required by the architecture-update rule in AGENTS.md. Summarize the new thread-registration contract in the relevant AGENTS.md section and update the ProfilingEngines entry in doc/README.md alongside this detail page.
The gated, in-flight-counted registration path introduced by this branch
is a load-bearing invariant for any code that touches per-thread engine
tables, but it was documented only in ProfilingEngines.md.
- Add a "Thread registration" critical invariant to AGENTS.md: reach the
engines only through Profiler::registerThread()/unregisterThread(); size
a per-thread table, then openThreadRegistration(), then list threads;
free or replace a table only while its gate is closed; restore the
cancellation state outside destructor-bearing scopes.
- Mention the gates in the AGENTS.md subsystem table and the doc/README.md
index entry.
- Describe the in-flight count and the cancellation rule in
ProfilingEngines.md#thread-registration.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Profiler::start() called _cpu_engine->enableEvents(true) whenever any
mode activated. If the CPU engine's start() failed (for example a
PerfEvents table allocation failure on a restart after pid_max changed)
and another mode started, the engine was enabled anyway, and perf fds
left armed by the previous recording resumed emitting CPU samples into a
recording whose CPU engine had failed.
Enable the CPU engine only when EM_CPU activated. Retiring the stale
PerfEvents fds themselves remains tracked in PROF-16359.
Update the ProfilingEngines.md lifecycle notes and recompute the shifted
profiler.cpp line references in ProfilingEngines.md and
JfrIntegration.md.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An agentic review of this PR (sphinx multi-stream, base 9d46570c…, head 08375d32…) produced 17 findings: 1 HIGH, 7 MEDIUM, 7 LOW, 2 INFO; 2 were rejected during verification.
All findings are posted as inline review threads below (anchored at the reviewed head 08375d32… so line numbers match the original anchors).
14 findings are addressed in commit a24bc50cc (pushed to this branch), verified against the full gtest suite — 11 fully, 3 partially. Highlights:
🔴 HIGH — eventFor() torn snapshot (perfEvents_linux.cpp): _events/_max_events were published as two plain stores with no release fence; on arm64 a SIGPROF reader could validate tid against the new bound while indexing the old, already-freed table. Now published atomically.
🟡 MEDIUM — unbounded waitForHookCalls() spin: the in-flight drain under _state_lock had no timeout or logging; now bounded with periodic warning on the stuck path.
🟡 MEDIUM — PerfEvents::start() failure path destroyed the tid→fd mapping (freed the old table before a failing calloc); orphaned fds + ring mmaps leaked permanently. Now allocated before the free, with the stale bound reset on failure.
🟡 MEDIUM — bound checks missed negative tids in CTimer::registerThread/unregisterThread and PerfEvents::unregisterThread; comments claiming the checks "reject every tid" were true only for tid >= 0.
🟡 MEDIUM — stop() comment overpromised: unregisterThreadInEngines() gates on _thread_tables_stable, which closeThreadRegistration() does not clear; the comment/contract now states what actually happens for late ThreadEnd callbacks during teardown.
🟡 MEDIUM — NM_CTIMER counter exposure documented, plus test-coverage gaps closed (allocation-failure paths, engine-failure gate close, ThreadTablesFreeze construction) and 2 related issues fixed in the same pass.
3 findings are only partially fixed and marked "Author decision needed" in their threads: downstream consumers of the new ctimer counter series; the shared _hook_calls_inflight RMW on every thread create/exit; the registration gate coupled to the CPU engine choice.
3 findings were dismissed: the CTimer stop-sweep cost claim (adjudicated invalid against code — the sweep touches only non-empty slots), a comment-accuracy INFO on seq_cst-vs-release decrement wording, and a mutation-analysis-skipped INFO.
The reason will be displayed to describe this comment to others. Learn more.
Sphinx review — threaded findings
An agentic review of this PR produced 17 findings: 1 HIGH, 7 MEDIUM, 7 LOW, 2 INFO; 2 rejected during verification. They are posted as inline threads below (one per finding).
14 are addressed in commit a24bc50cc (pushed to this branch), verified against the full gtest suite — 11 fully, 3 partially (threads marked "Author decision needed"). The 3 open items are: (1) downstream consumers of the new ctimer counter series, (2) the shared _hook_calls_inflight RMW on every thread lifecycle event, (3) the registration gate being coupled to the CPU engine choice.
3 further findings were dismissed: the CTimer stop-sweep cost claim (adjudicated invalid against code), a comment-accuracy INFO on seq_cst-vs-release wording, and a mutation-analysis-skipped INFO.
Anchored at the reviewed head 08375d32… so line numbers match the original findings; threads may show as outdated on files the fix commit already touched.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Sphinx review — MEDIUM · ddprof-lib/src/main/cpp/profiler.cpp:223 · Fixed in a24bc50cc — verified against the full gtest suite
Finding — waitForHookCalls() spins unboundedly with sched_yield() and has no timeout or failure handling, unlike the neighbouring SignalInflight::drain() which returns an error after a timeout (profiler.cpp:1935-1942). Every gate close in the new lifecycle depends on it: stop() calls closeThreadRegistration() at profiler.cpp:1949 while holding _state_lock (MutexLocker at profiler.cpp:1923), and ThreadTablesFreeze's constructor does the same inside start(). If _hook_calls_inflight ever stays non-zero, stop()/start() hang forever while holding _state_lock, permanently wedging the profiler lifecycle with no log and no recovery path. The code's own comment at profiler.cpp:232-233 identifies exactly such a leak on musl ('skip it on musl and leave the in-flight count raised, so the next stop() would wait forever'), and the PR description explicitly lists the missing timeout as 'Not addressed'.
Evidence
while (_hook_calls_inflight.load(std::memory_order_seq_cst) != 0) {
sched_yield();
}
Suggestion — Bound the wait: give waitForHookCalls() a deadline (e.g. the same budget SignalInflight::drain() uses), and on expiry log a warning and return a failure so stop() can return a retryable error (mirroring the 'signal handlers did not drain; teardown skipped, retry stop()' path at profiler.cpp:1941) instead of spinning forever under _state_lock. At minimum, emit a periodic Log::warn while spinning so a stuck in-flight count is observable in production.
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a24bc50. waitForHookCalls() waits at most _hook_drain_timeout_ns (1 s) and logs on timeout. stop() then returns a retryable error and tears nothing down; a table replacement fails without replacing anything.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Sphinx review — MEDIUM · ddprof-lib/src/main/cpp/perfEvents_linux.cpp:965 · Fixed in a24bc50cc — verified against the full gtest suite
Finding — On the failed-restart allocation path, start() frees the old _events table (line 965) before the calloc that then fails. That free destroys the tid->fd mapping for every thread the previous recording had registered (each PerfEvent slot held its fd and ring mmap). The in-code acknowledgment only covers this failure path not closing fds ("Fds left open by an earlier recording are not closed here (PROF-16359)"), but the consequence is worse than "not closed here": because the slots are zeroed and the mapping is gone, a later successful start() cannot close them either — its stale-fd sweep only sees the fresh (empty) table, and registerThread() 'skips threads that already have an fd' can no longer see the existing fd, so it opens a duplicate fd per still-live thread while the orphaned fd and its 2-page ring mmap leak for the lifetime of the process. With a pid_max-sized thread population, one failed restart permanently leaks one fd plus two mapped pages per live thread.
Evidence
free(_events);
Suggestion — On the allocation-failure path, close the per-thread fds recorded in the table being freed before free(_events) (mirroring what the stale-fd sweep does for missing tids), or retain the old table and bound and skip the resize on failure so the mapping survives for the next start(). If deferred to PROF-16359, note there that the mapping destruction makes the leak unrecoverable by a later start(), not merely deferred.
Anchor note: the freed-table line (965) sits outside the diff context of the reviewed head.
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a24bc50. resizeEvents() releases every slot of the old table (unregisterThread() closes the fd and unmaps the ring) before it frees the table, so no fd or ring is orphaned.
The reason will be displayed to describe this comment to others. Learn more.
🔴 Sphinx review — HIGH · ddprof-lib/src/main/cpp/perfEvents_linux.cpp:1167 · Fixed in a24bc50cc — verified against the full gtest suite
Finding — eventFor() builds its (table pointer, bound) snapshot from two independent atomic loads, while PerfEvents::start() publishes the pair with two plain stores (perfEvents_linux.cpp:969 free/calloc + _events store, then :980 _max_events store) with no release fence between them. A SIGPROF reader (walkKernel()/resetBuffer(), not gated by the hook gates) can therefore read a torn snapshot: on arm64 (weakly ordered, per this repo's own sharp-edges guidance) the new _max_events can become visible before the matching _events store, so the reader validates tid against the NEW bound while indexing the OLD table — and the old table has already been freed() at line 965, giving an out-of-bounds read into freed memory with a larger-than-table tid. The reverse tear (new _events, stale bound) only shrinks the check. The inline comment concedes that a freed-and-replaced table is not safe to read (PROF-16359), but the split snapshot undermines the NULL-table/out-of-range guarantee eventFor() is meant to provide.
Evidence
PerfEvent *PerfEvents::eventFor(int tid) {
PerfEvent *events = __atomic_load_n(&_events, __ATOMIC_ACQUIRE);
if (events == NULL || tid < 0 ||
tid >= __atomic_load_n(&_max_events, __ATOMIC_ACQUIRE)) {
returnNULL;
}
return &events[tid];
}
Suggestion — Make the snapshot atomic as a unit: either (a) read _events and _max_events into a single struct published with __atomic_store/release on the writer and __atomic_load/acquire (or seq_cst) on the reader, or (b) minimally, keep the pointer-bound invariant by storing the bound with a release store AFTER the pointer store (pointer bound to new bound only together), i.e. in start() write _events then a release fence then _max_events, and in eventFor() load _max_events FIRST (acquire), then _events — so the bound seen is never newer than the table it validates against. Until PROF-16359 removes the freed-table window entirely, at least close the torn-pairing hole so the bounds check cannot validate against a table it did not read.
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a24bc50. eventFor() validates the (table, bound) pair against _events_gen: resizeEvents() makes it odd for the replacement, and a reader that sees an odd or changed generation returns NULL. The bound can no longer belong to a different table than the pointer.
The reason will be displayed to describe this comment to others. Learn more.
Update: reverted in 688e368. The generation check and eventFor() are back out of PerfEvents, so walkKernel()/resetBuffer() index _events as on main. Deferred to PROF-16359.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Sphinx review — MEDIUM · ddprof-lib/src/main/cpp/ctimer_linux.cpp:123 · Fixed in a24bc50cc — verified against the full gtest suite
Finding — The bound checks added as the safety net for a failed/empty table reject only tids at or above the bound; a negative tid passes tid >= _max_timers and tid >= _max_events and indexes before the table. CTimer::registerThread (ctimer_linux.cpp:58) would CAS-write _timers[-k], CTimer::unregisterThread (line 129) would atomically load and CAS before the array, and PerfEvents::unregisterThread (perfEvents_linux.cpp:715/719) dereferences _events[negative]. The new comments explicitly claim these checks 'reject every tid' (ctimer_linux.cpp:140-142, perfEvents_linux.cpp:971-974), which is only true for tid >= 0. The new eventFor() does check tid < 0, so the PR itself treats negative tids as a real input class, but the register/unregister paths were not given the same half of the check.
Evidence
if (tid >= _max_timers) {
return;
}
Suggestion — Add the missing lower-bound half to all four one-sided checks (CTimer::registerThread ctimer_linux.cpp:58, CTimer::unregisterThread ctimer_linux.cpp:123, PerfEvents::registerThread, PerfEvents::unregisterThread perfEvents_linux.cpp:715), matching eventFor()'s tid < 0 guard.
Anchor note: the bound check (123) sits outside the diff context of the reviewed head.
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a24bc50. CTimer::registerThread(), CTimer::unregisterThread(), PerfEvents::registerThread() and PerfEvents::unregisterThread() all check tid < 0 || tid >= bound. Covered by NegativeTidIsRejected in ctimer_ut.cpp and perfEvents_ut.cpp.
The reason will be displayed to describe this comment to others. Learn more.
Update: the PerfEvents half is reverted in 688e368 and deferred to PROF-16359. The CTimer::registerThread()/unregisterThread() checks stay, covered by NegativeTidIsRejected in ctimer_ut.cpp.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Sphinx review — MEDIUM · ddprof-lib/src/main/cpp/profiler.cpp:1949 · Fixed in a24bc50cc — verified against the full gtest suite
Finding — stop()'s comment (lines 1944-1948) claims "This covers the JVMTI ThreadStart/ThreadEnd callbacks too: they go through the same gate, and their events are disabled only after the engines stop." This is only true for ThreadStart. registerThreadInEngines() gates on _engines_ready (cleared by closeThreadRegistration), but unregisterThreadInEngines() gates on _thread_tables_stable, which closeThreadRegistration() does NOT clear. JVMTI ThreadEnd events are disabled only later in stop() (switchThreadEvents after the engines stop), so a ThreadEnd callback arriving after line 1949 passes the still-open _thread_tables_stable gate, increments _hook_calls_inflight, and calls _cpu_engine->unregisterThread(tid) / _wall_engine->unregisterThread(tid) while stop() proceeds to _alloc_engine->stop(), ReferenceChainTracker::stopThread(), _cpu_engine->stop() (CTimer::stop iterating and deleting all timers) and _wall_engine->stop() below.
Evidence
closeThreadRegistration();
Suggestion — Either make stop()'s teardown genuinely safe for late ThreadEnd callbacks — e.g. clear _thread_tables_stable (via a ThreadTablesFreeze-like scope, or by having closeThreadRegistration also clear it and re-open only in openThreadRegistration) before stopping the engines, so late unregisters skip the engines like they do during start() — or correct the comment and the AGENTS.md/ProfilingEngines.md invariant to state that ThreadEnd is intentionally allowed to reach engines during stop() teardown and document why that is safe per engine (CTimer per-slot CAS; PerfEvents deferred under PROF-16359). As written, the comment guarantees a drain that does not happen.
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a24bc50. The stop() comment, ProfilingEngines.md and AGENTS.md now state that only registration closes in stop(), that unregistration stays open while the engines stop, and why each engine tolerates it (per-slot CAS in CTimer; fd CAS and slot lock in PerfEvents; no table is freed).
The reason will be displayed to describe this comment to others. Learn more.
🟢 Sphinx review — LOW · ddprof-lib/src/main/cpp/perfEvents_linux.cpp:977 · Fixed in a24bc50cc — verified against the full gtest suite
Finding — No test in this PR would detect: removing the _max_events = -1; assignment on the allocation-failure path in PerfEvents::start(). The new perfEvents_ut.cpp tests install (nullptr, -1, RING_KERNEL) directly via TableSwap and never drive start() with a failing calloc, so a mutation that drops this line (leaving the stale bound) still passes every test in the diff.
Evidence
_max_events = -1;
Suggestion — Add a gtest that swaps calloc to fail (or drives start via a test seam analogous to CTimer::startWithTableSize) and asserts _events == NULL and _max_events == -1 after a failed PerfEvents::start().
The reason will be displayed to describe this comment to others. Learn more.
🟢 Sphinx review — LOW · ddprof-lib/src/main/cpp/profiler.cpp:1788 · Fixed in a24bc50cc — verified against the full gtest suite
Finding — No test in this PR would detect: deleting this closeThreadRegistration() call after a CPU engine start() failure in Profiler::start(). The CTimer test asserts the gate is closed only because resizeTimers fails before openThreadRegistration() runs; no test covers an engine that opens the gate and then fails, which is the case this line exists for.
Evidence
closeThreadRegistration();
Suggestion — Add a test with a fake CPU engine that calls Profiler::openThreadRegistration() and then returns an error from start(), asserting the gate is closed afterwards.
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a24bc50. ProfilerEnginesGateTest.FailedCpuEngineStartClosesThreadRegistration uses a fake CPU engine that opens the gate and then fails, and asserts the gate is closed afterwards.
The reason will be displayed to describe this comment to others. Learn more.
🟢 Sphinx review — LOW · ddprof-lib/src/main/cpp/profiler.cpp:198 · Fixed in a24bc50cc — verified against the full gtest suite
Finding — No test in this PR would detect: mutating ThreadTablesFreeze's constructor to store true instead of false (or skipping waitForHookCalls()), because no test constructs a ThreadTablesFreeze; UnregisterThreadSkipsEnginesWhileTablesFrozen sets _thread_tables_stable directly via a test accessor instead.
Suggestion — Add a test that instantiates ThreadTablesFreeze (via a friend accessor or by invoking the code path in Profiler::start()) and asserts _thread_tables_stable is false and hook calls are drained while it is alive.
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a24bc50. ThreadTablesFreezeClosesGatesAndWaitsForHookCalls and ThreadTablesFreezeReportsUndrainedOnStuckHookCall construct a real ThreadTablesFreeze and assert both gates, the wait and the timeout report.
NegativeTidIsRejected called CTimer::registerThread() and
unregisterThread() directly, but both are protected. Route the calls
through CTimerTestAccessor, which CTimer befriends.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
Reconcile thread exits skipped during the table freeze
If a thread exits while ThreadTablesFreeze is active, this return can skip its engine cleanup; if the subsequent thread snapshot still contains that TID, CTimer::start() or PerfEvents::start() can leave its slot occupied. When the kernel later reuses the TID, registration sees that occupied slot and the new thread can miss CPU samples until cleanup. Defer or reconcile exits skipped during the freeze.
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 2de3fa9. The freeze no longer spans Profiler::start(). CTimer::sizeTimerTable() and PerfEvents::sizeEventTable() take it only when the table size changes (first start, or pid_max changed), so a start()/stop()/start() cycle with an unchanged pid_max never drops a thread exit. This matters most for PerfEvents, whose fds survive stop(). The remaining case, an exit during an actual table replacement, is tracked in PROF-16359 item 5.
The reason will be displayed to describe this comment to others. Learn more.
Correction: the PerfEvents part is reverted in 688e368. The narrowed freeze now applies to CTimer only (CTimer::sizeTimerTable()). PerfEvents::start() replaces its table without a freeze, as on main, which is deferred to PROF-16359.
musl's malloc maps the new table with mmap, and the kernel can place it
at the address the unmapped ring just released, so msync() on the old
ring address succeeds. Check the ring through a sentinel read back via
a pipe instead: a fresh mapping does not carry the sentinel, and the
pipe read fails with EFAULT rather than faulting on an unmapped or
PROT_NONE page.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SKIP_IF_CALLOC_OVERFLOW_ABORTS() tests ASAN_ENABLED/TSAN_ENABLED, which
common.h defines, but perfEvents_ut.cpp did not include it. The macro
expanded to nothing, so the allocation-failure tests called calloc(-1)
under the ASan/TSan allocators, which abort the whole gtest binary.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Profiler::start() held ThreadTablesFreeze from CPU engine selection
until the engine sized its table, a window that includes symbol loading
and JFR start. A thread exit in that window was dropped. If the thread
was still listed when the engine listed threads, its timer or fd stayed
in the slot after it died, and a thread that later reused the tid could
not claim it. start()/stop() cycles repeat the window. PerfEvents keeps
its fds across stop(), so such a slot can outlive every later restart.
On a restart with an unchanged pid_max no table is replaced, so the
freeze protected nothing. Move it into new CTimer::sizeTimerTable() and
PerfEvents::sizeEventTable(), which freeze only when the table size
changes. On a drain timeout the engine's start() fails retryably and
keeps its table. start() itself now only closes the registration gate.
start() and check() replace _cpu_engine/_wall_engine while unregistration
is open, so store them with release and load them with acquire in the
hook paths.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Restore perfEvents.h and perfEvents_linux.cpp to main and drop
perfEvents_ut.cpp, so this change covers CTimer only. PerfEvents keeps
one line: PerfEvents::start() opens thread registration before it lists
threads. Without it, the registration gate in Profiler stays closed
until start() returns, and a thread created while PerfEvents lists and
sweeps threads is never registered for the recording.
Comments and docs now describe PerfEvents as on main and point to
PROF-16359. AGENTS.md lists PerfEvents::start() as a known exception to
replacing a per-thread table only while its gate is closed.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The previous commit deleted perfEvents_ut.cpp instead of restoring it to
main. The file exists on main with the PerfEventsCompareTidsTest tests,
so the deletion would have removed them. This change no longer touches
the file.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The registration gate is open iff the CPU engine started, but
registerThreadInEngines() also called the wall engine through it, and
the docs told a future wall engine that needs registerThread() to open
the gate itself. Doing that would admit a CPU engine whose start()
failed, and a started CPU engine already opened the gate for a wall
engine that did not start.
No wall engine overrides Engine::registerThread(), so the wall call did
nothing. Registration now reaches only the CPU engine and returns its
result; no caller reads the value. The docs and AGENTS.md now say a
wall engine that needs registerThread() needs its own gate.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Timer table
Profiler::check() can run while a recording is RUNNING (checkState()
rejects only NEW and ERROR), but it stored its selection into
_cpu_engine, _wall_engine and _alloc_engine. The open registration gate
then admitted an engine that never started, the wall signal handler
cast wallEngine() to the wrong type, and stop() stopped the wrong
engines. check() now selects into locals and never writes the slots.
Docs and comments:
- Replace profiler.cpp/ctimer_linux.cpp line references in
ProfilingEngines.md, JfrIntegration.md and JvmSupportLayer.md with
symbol references; this change had already shifted most of them.
- The _wall_engine row no longer claims it receives registerThread.
- List all six per-category native_mem_*_bytes families and drop the
time-relative "ctimer is the newest".
- A leaked in-flight count makes later gate closes time out, not wait
forever.
Tests:
- TidAtBoundIsRejected: a tid equal to the bound never indexes past the
timer table. The table is sized to the caller's own tid, so a wrongly
accepted tid gets past timer_create() and faults on a guard page.
- ResizeFreesOldTable: a resize frees the old table, checked through
glibc mallinfo2() (skipped on musl and under ASan/TSan).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… loss
Engine change: stop() leaves PerfEvents' per-thread fds open so that
unregisterThread() closes them as threads exit, but once start() selected
another CPU engine those exits went to the new engine and the fds leaked.
Add Engine::releaseThreads() (PerfEvents closes every fd and ring) and
Profiler::replaceCpuEngine(), which has the previous engine release its
threads under a ThreadTablesFreeze before the slot changes. PerfEvents
also releases its threads before start() frees the table on a pid_max
change.
PerfEvents allocation failure: reset _max_events to -1, log an error and
fail start() before the signal handler, the hook and the registration
gate are set up. walkKernel() and resetBuffer() skip a missing table:
fds armed by an earlier recording keep delivering SIGPROF.
Gate timeouts: count registration and unregistration calls separately.
closeThreadRegistration() waits only for registration calls, because
unregistration stays open during stop() and its count may not reach
zero under thread churn. ThreadTablesFreeze waits for both. If
closeThreadRegistration() times out, the profiler goes to ERROR, which
rejects every later start(), the only place a table is replaced: stop()
still completes, since it frees no table, and start() rolls back its
setup. The rollback is one helper, rollBackStart(), which now also
restores the library patches.
Also: the gate contract is stated once, on the gate fields in
profiler.h, with pointers elsewhere; StartOpensThreadRegistration
installs its CTimer as the CPU engine while the gate is open.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
Don't skip registration on a stale closed-gate read
If a native pthread is created after CTimer::start() or PerfEvents::start() finishes its thread snapshot, this acquire load has no happens-before edge from the thread that opened the gate and may still observe the old false. This branch then skips both the in-flight count and the later sequentially consistent check, so the thread is never registered and receives no CPU samples for its lifetime.
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
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.
zhengyu123
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
![datadog-datadog-us1-prod[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
jbachorik
left a comment
What does this PR do?:
pid_max × sizeof(int)) is recorded inNativeMemunder a newNM_CTIMER(ctimer) category.CTimer::start()returns an error and leaves no NULL table reachable through a non-zero bound.pthread_createhook and the JVMTIThreadStart/ThreadEndcallbacks now reach the engines only throughProfiler::registerThread()/unregisterThread(). Those calls are gated, counted in flight, and run with thread cancellation disabled. This removes a race in which a hook call indexed the timer table whileCTimer::start()freed and replaced it.Most PerfEvents table defects found on the way are out of scope and tracked in PROF-16359. This PR changes PerfEvents only where the new gates or the CPU engine lifecycle need it.
Motivation:
PROF-16213: the CTimer table was allocated without a
NativeMem::recordcall. It is about 16 MiB of address space at a 4Mpid_max, and none of it appeared in thenative_mem_*counters.Accounting the table meant routing its allocation through one function. Doing that exposed two older defects in the same code:
start()replaced it, with no lock and no ordering.Additional Notes:
NM_PERF:perfnames perf_events in counters and docs. Every chunk now carriesnative_mem_*_bytes.ctimer. Consumers with a fixed category list need the new name.NM_PERFdoes for thePerfEventarray. Only pages of live tids are resident, so the gauge overstates RSS for this category._max_timers = 0), so every tid bound check rejects.start()returnsFailed to allocate CPU timer table.Profiler::start()enables the CPU engine only if its ownstart()succeeded. Before, it enabled the engine whenever any mode activated. After a failed CPU engine start, timers or fds left armed by the previous recording could then emit CPU samples into the new one._eventsallocation resets_max_eventsto -1, logs an error, and failsstart()before the signal handler, the hook and the registration gate are set up, so the engine stays off for the recording.walkKernel()andresetBuffer()skip the missing table, because fds armed by an earlier recording keep delivering SIGPROF and the handler callsresetBuffer()on every exit.Profiler::start()installs thepthread_createhook before the CPU engine starts, and the hook runs without_state_lock. Disabling the JVMTI events instop()does not wait for callbacks already running.registerThread()needs_engines_ready.CTimer::start()andPerfEvents::start()open it throughProfiler::openThreadRegistration(), after their table is sized and before they list existing threads. A thread that starts or exits whilestart()registers the existing ones is then handled by the hook. The per-slot CAS in both engines resolves the hook andstart()registering the same tid.stop()closes the gate, andstart()closes it again if the CPU engine fails.registerThread()never calls the wall engine. No wall engine overridesEngine::registerThread(). One that ever does needs a gate of its own, because opening this one would also admit a CPU engine that failed.PerfEvents::start()opens the gate the same way. Without that call, the gate stays closed untilPerfEvents::start()returns, and a thread created while it lists and sweeps threads is never registered for the recording.unregisterThread()needs_thread_tables_stable.ThreadTablesFreezeclears it only whileCTimer::sizeTimerTable()replaces the timer table (first start, orpid_maxchanged) or whileProfiler::replaceCpuEngine()lets the previous CPU engine release its threads. It stays set while idle:PerfEvents::stop()keeps per-thread fds open, so a thread that exits afterstop()relies on this call to close its fd.start()/stop()cycles repeat the window. A restart with an unchangedpid_maxreplaces no table, so it never closes unregistration.HookCallScope), on its gate's own counter.closeThreadRegistration()waits only for registration calls, so noregisterThread()call is inside an engine whenstop()tears it down. It does not wait for unregistration calls: they stay allowed duringstop(), and under thread churn their count may not reach zero within the budget although none is stuck.ThreadTablesFreezecloses both gates and waits for both counts. The flags and the counters areseq_cston both sides: either the hook sees the gate closed, or the closer sees the call and waits.closeThreadRegistration()timeout means a registration call is stuck inside an engine and may still use its table, so the profiler goes toERROR, which rejects every laterstart(), the only place a table is replaced.stop()still completes, because no step frees a per-thread table.start()rolls back its setup (rollBackStart(), which also restores the library patches). The cost: a stuckCTimer::registerThread()that finishes afterCTimer::stop()leaves one armed timer whose signals are dropped.start()selects another CPU engine,Profiler::replaceCpuEngine()has the previous one release its per-thread resources (Engine::releaseThreads()) under aThreadTablesFreezebefore the slot changes. Otherwise PerfEvents' fds, kept open acrossstop(), would never be closed once thread exits go to the new engine. PerfEvents also releases its threads beforestart()frees its table on apid_maxchange.start()replaces_cpu_engine/_wall_enginewhile unregistration is open. It stores the pointers with release, and the hook paths load them with acquire. An unregistration that reaches the previous engine releases only that thread's own slot.check()selects engines into locals and never writes the engine slots. It may run while a recording isRUNNING(checkState()rejects onlyNEWandERROR). Writing the slots then would let the open gate admit an engine that never started, make the wall signal handler castwallEngine()to the wrong type, and makestop()stop the wrong engines.seq_cstand one release RMW on the unregistration counter, on the counters' own cache line. Registration skips its counter while idle; unregistration cannot, because its gate stays open while idle. Accepted for now.close(),Log::warn()). Acting on a cancellation there would abort while unwinding through~HookCallScope()with the statically linked libgcc, or, on musl, skip it and leave the count raised. The restore is a manual call made after every destructor-bearing scope, not a guard. With asynchronous cancellation, current glibc acts on a pending request as soon as cancellation is re-enabled, and doing that from an implicitlynoexceptdestructor terminates the process.stop()can no longer arm a timer afterCTimer::stop(). A failed CPU engine start no longer logstid > pid_max[0]for every Java thread.PerfEvents::start()listed as a known exception), close unregistration only around an actual table replacement, and restore cancellation outside destructors.ProfilingEngines.md#thread-registrationholds the details, and thedoc/README.mdindex entry mentions them.state() == RUNNING:RUNNINGis set about 110 lines after the engines start and cleared only after they stop. Threads that start in that gap would never be registered, and threads that exit in it would leave a slot that blocks a reused tid.CTimeralone: a thread that already passed the bound check could still write into the freed table.Profiler::start(): it drops thread exits on every restart, including the common one that replaces no table.PerfEvents::start()frees and replaces_eventson apid_maxchange without excluding thread unregistration or SIGPROF handlers that still read it. It now closes the old fds and rings first, but a handler that already holds a slot pointer can still use the freed table._page.PerfEvents::start()'s stale-fd sweep closes the fd of a thread the hook registered after the thread list was taken. That thread is not sampled for the recording.start().CTimer::start()lists threads, leaves its timer behind. This needs apid_maxchange.How to test the change?:
ddprof-lib/src/test/cpp/ctimer_ut.cpp(Linux):TimerTableIsAccounted,SameSizeDoesNotReaccount: the gauge follows resizes, and a same-size restart keeps the table (a sentinel value survives).FailedAllocationLeavesEmptyTable,StartFailsWhenTableAllocationFails: a forcedcallocoverflow leaves an empty table.start()returns the error before it installs the SIGPROF handler. Both skip under ASan/TSan, because the CI sanitizer jobs run gtest binaries withoutallocator_may_return_null=1.StartOpensThreadRegistration: a successfulCTimerstart opens the gate.RegisterThreadSkipsEnginesUntilReady,UnregisterThreadSkipsEnginesWhileTablesFrozen: both calls stay off the engines while their gate is closed.CloseThreadRegistrationWaitsForHookCalls: a realregisterThread()call is held inside a blocking test engine. Closing the gate does not return until it leaves.CloseThreadRegistrationTimesOutOnStuckHookCall,ThreadTablesFreezeClosesGatesAndWaitsForHookCalls,ThreadTablesFreezeReportsUndrainedOnStuckHookCall: the wait is bounded, and a real freeze closes both gates, waits, keepsunregisterThread()off the engines, and reports a timeout.FailedCpuEngineStartClosesThreadRegistration,StartedCpuEngineOpensThreadRegistration,CpuEngineEnabledOnlyIfItStarted,RegisterThreadFastPathDoesNotSkipOpenGate: the gate follows whether the CPU engine started, and a failed CPU engine is not enabled.SameSizeRestartDoesNotFreezeUnregistration,ResizeKeepsTableWhenHookCallsDoNotDrain,ResizeReplacesTableWhenHookCallsDrain: with a hook call held in flight, a same-size restart succeeds (no freeze ran), and a resize fails retryably and keeps the old table.NegativeTidIsRejected,TidAtBoundIsRejected: register/unregister with a negative tid, or a tid equal to the bound, never index outside the table (the page before or after it isPROT_NONE). The bound test uses the calling thread's own tid, so a wrongly accepted tid gets pasttimer_createand faults.CloseThreadRegistrationIgnoresInFlightUnregistration,ThreadTablesFreezeWaitsForInFlightUnregistration: closing the registration gate does not wait for an unregistration in flight; a freeze does.StuckGateAfterCpuEngineFailurePutsProfilerInError: if the gate cannot be closed after a CPU engine failure, the profiler goes toERROR;FailedCpuEngineStartClosesThreadRegistrationchecks that a gate close within the budget leaves the state unchanged.CpuEngineChangeReleasesPreviousEngineThreads,SameCpuEngineKeepsItsThreads,CpuEngineChangeKeepsPreviousEngineWhenHookCallsDoNotDrain: an engine change releases the previous engine's threads with both gates closed, a same-engine restart keeps them, and an undrained change is refused.ResizeFreesOldTable: a resize frees the old table, which theNM_CTIMERgauge alone cannot show. It reads glibc'smallinfo2(), so it skips on musl and under ASan/TSan.RegistrationGateAdmitsOnlyCpuEngine: with the gate open, a counting wall engine sees noregisterThread()call, and the result is the CPU engine's.CancellationDuringHookCallActsAfterTheCall: a thread with asynchronous cancellation is cancelled inside a hook call. The engine sees cancellation disabled, the process survives, and the count returns to zero. With the restore moved into~HookCallScope(), the process aborts on glibc 2.39. glibc 2.35 keeps the request pending until the next cancellation point, so it cannot catch that regression../.claude/commands/build-and-summarize :ddprof-lib:gtestDebug_ctimer_utddprof-lib/src/test/cpp/perfEvents_ut.cpp(Linux):MissingTableIsNotIndexed: with no table (the state a failed allocation leaves),resetBuffer()andreleaseThreads()do not index it.start()/stop():rollBackStart(), thestop()timeout path, and PerfEvents' allocation failure and release insidestart().StartWithChurningThreadsTest,StopWithChurningThreadsTest,IdleThreadLifecycleTest,CTimerSamplerTest,NativeThreadTest.check()writing the engine slots during a recording, stale line references (replaced by symbol references), stale comments and counter-name lists, and tests for the CTimer tid bound and the table free.NM_PERFmixing the tid table with ring pages.Profiler::start()/stop()/check(), which need a real profiler start, and the remaining LOW design notes./code-reviewpass (10 findings) led to: per-gate in-flight counters, so astop()under thread churn cannot time out falsely; the CPU engine change releasing the previous engine's threads; PerfEvents allocation-failure handling;ERRORon a gate-close timeout with a shared start rollback that restores library patches; accurate timeout messages; and one place for the gate contract in code. Declined: an idle pre-check for unregistration (no effect while its gate is open), thecheck()LBR behavior, and thereleaseThreads()scan length.For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.Unsure? Have a question? Request a review!
🤖 Generated with Claude Code