feat(memtrack): pause producers under ring pressure - #543
not-matthias wants to merge 11 commits into
Conversation
Merging this PR will not alter performance
|
15e7943 to
89556be
Compare
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
31b6da1 to
0431cd9
Compare
0431cd9 to
58f8c9b
Compare
Share one poll interval across the event, stack and attach pollers and lower it from 10ms to 1ms so bursts drain before the rings fill.
A full allocation-stack ring loses stack records the same way a full event ring loses events, so a run that overflowed it must fail the same incompleteness check.
The FNV lanes lived on the BPF stack. Large kprobe-family programs may spill that to per-CPU storage, which a nested uprobe on the same CPU can overwrite mid-capture, corrupting the hash. Accumulate the lanes in the not-yet-submitted ring record instead, which is private to this reservation.
58f8c9b to
cba9475
Compare
This comment has been minimized.
This comment has been minimized.
After every event or stack submission, BPF checks the ring's fill level. Once it is 75% full, the writing tracked process is recorded in `pressure_stopped` and gets SIGSTOP, so processes that don't write keep running. The event and stack pollers resume every recorded process once a poll leaves their ring empty, and resume everything still recorded on shutdown. A tracked process that writes to a nearly full ring after the pollers are gone stays stopped. A process can be stopped both for ring pressure and for an allocator attach request, and SIGSTOP is not counted. The exec-mapping watcher therefore records its stops in `attach_stopped`. Each side deletes its own entry before checking the other's, so the process resumes only once both are done with it. `RingBufferPoller::drain` no longer acknowledges a consume that stopped at an uncommitted reservation, and `wait_all_stopped` treats exited threads as stopped. The event and stack poll interval is configurable through the `poll_interval_ms` tracker option (env `CODSPEED_MEMTRACK_POLL_INTERVAL_MS`, default 1ms), which lets the event ring cross its watermark on demand. The attach poller keeps its fixed interval.
Add `alloc_storm` (threads) and `alloc_storm_procs` (forked processes) fixtures and pressure tests that run them with a 10s poll interval and assert that no events are dropped. The multi-process test checks that every writing process is stopped and resumed on its own.
On glibc >= 2.42 the per-thread tcache is initialized lazily. A thread's first small free() whose tcache is still inactive goes through tcache_free_init(), which tail-calls __libc_free() again, so the free uprobe fires twice for one call. Whether a thread reaches that path depends on arena assignment, i.e. scheduling, so the Free count of the same workload varies between runs. for_each_variant compared raw Free counts between the Legacy and Token runs, which made test_thread_dlopen flaky on ubuntu-26.04-arm (glibc 2.43). GLIBC_TUNABLES (tcache_count=0, tcache_max=0) does not avoid the re-entry. event_profile now replays events in timestamp order and counts a Free only when it releases an allocation still live in that run, which drops the duplicate hit as well as frees of memory allocated before tracking.
The stop maps kept a process's entry after it exited, so a later release could send SIGCONT to an unrelated process that reused the pid. The exit handler now deletes a process from `pressure_stopped` and `attach_stopped`, and a release resumes a process only if it removed its own entry. The exec-mapping watcher stops a process only once it is recorded, like the pressure check, so every stop has an entry to release. Both maps move to a shared header so the exit handler can reach them.
d57345a to
c023567
Compare
| if (bpf_map_update_elem(map, &tgid, &marker, BPF_ANY) != 0) { | ||
| /* Unrecorded, so nothing would resume it. */ | ||
| bpf_send_signal(MEMTRACK_SIGCONT); |
There was a problem hiding this comment.
If this stop-record map is full while the process is also held for attach work or ring pressure, the failed update sends SIGCONT without checking that other hold. The process can then run while it is supposed to remain stopped, allowing events during an attach or ring drain.
Knowledge Base Used: eBPF memory tracker
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/memtrack/src/ebpf/c/utils/stopped.h
Line: 29-31
Comment:
**Failed Update Releases Another Stop**
If this stop-record map is full while the process is also held for attach work or ring pressure, the failed update sends `SIGCONT` without checking that other hold. The process can then run while it is supposed to remain stopped, allowing events during an attach or ring drain.
**Knowledge Base Used:** [eBPF memory tracker](https://app.greptile.com/codspeed/-/custom-context/knowledge-base/codspeedhq/codspeed/-/docs/ebpf-memory-tracker.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
@not-matthias is this somethign we want to handle? At least have a big warning or so? 10000 pids is quite large but not irrealistic either
|
|
||
| /* Map helpers reject two arguments pointing into the same ring reservation, | ||
| * so the dedup value lives in .rodata while the key stays in the record. */ | ||
| static const __u8 seen_stack_marker = 1; |
There was a problem hiding this comment.
Should make this closer to the seen_stack hashmap, and focus the comment on the fact that we are using the hashmap as a hashset, then have a small sentence to explain what's explained above, else it's a bit hard to follow
| @@ -0,0 +1,35 @@ | |||
| #ifndef __STOPPED_H__ | |||
There was a problem hiding this comment.
this filename could be a bit better than stopped.h IMO
| pub(crate) fn release_pressure(&self) -> Result<()> { | ||
| // Deleting while iterating restarts hash iteration, so snapshot the keys first. | ||
| let keys: Vec<Vec<u8>> = self.pressure_stopped.keys().collect(); | ||
| if keys.is_empty() { | ||
| return Ok(()); | ||
| } | ||
| self.pressure_stops.fetch_add(keys.len() as u64, Relaxed); | ||
| for key in keys { | ||
| let pid = u32::from_le_bytes( | ||
| key.as_slice() | ||
| .try_into() | ||
| .context("Invalid pressure_stopped key size")?, | ||
| ); | ||
| debug!("Releasing pressure stop of pid {pid}"); | ||
| Self::release(pid, &self.pressure_stopped, &self.attach_stopped)?; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Does it make sense that we release the pressure for every single process at once? Do we have one queue per process and stop everything? Or cant we have one queue per process? Sorry if I dont make sense
| * so processes that do not write keep running. Userspace resumes the | ||
| * recorded producers once it has flushed the ring. */ | ||
|
|
||
| #define MEMTRACK_PRESSURE_HEADROOM_FRAC 4 /* stop at (FRAC-1)/FRAC = 75% used */ |
There was a problem hiding this comment.
Maybe I'm missing context, but this _FRAC suffix is extremely weird to me lol, maybe use full fraction word, or share ?
2 participants
Adds opt-in, best-effort backpressure for memtrack's BPF ring buffers.
When a ring crosses its watermark, BPF latches the episode and stops the winning tracked producer. A userspace coordinator then stops the owned process tree using pidfds, drains the event, stack, mapping, and attach pipelines, clears the latch, and resumes the tree. Finite timeouts terminate the tracked tree and fail the capture;
0remains the default and preserves drop detection, whileinfwaits until recovery. Detected ring loss still fails the capture.The change also:
This PR is stacked on #522.
Verification
cargo fmt --all -- --checkcargo clippy --release -p memtrack -p codspeed-runner -- -D warnings