fix(library-config): update Linux process context#2237
Conversation
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results📦
|
BenchmarksComparisonCandidateCandidate benchmark detailsBaselineBaseline benchmark details |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: a57627f | Docs | Datadog PR Page | Give us feedback! |
1a04bb0 to
e6af895
Compare
e5a142d to
a9e0f0e
Compare
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
e6af895 to
89a636d
Compare
a9e0f0e to
97138bc
Compare
yannham
left a comment
yannham
left a comment
There was a problem hiding this comment.
Thanks for stress-testing and fixing the fork case! Mostly non-blocking comments. If I understand correctly, if MADV_DONTFORK isn't supported and a fork happens, we don't have to do anything special because update, publish or unmap would touch the copy-on-write copy of the child. The only issue being that for some small time interval the child will appear as having the same context as its parent, instead of its own (which is why we are using MADV_DONTFORK in the first place). Is that more or less correct?
| // should anything have been written already we would get a short write | ||
| // However, this is not the case for macOS, despite what its manual | ||
| // says: See https://github.com/apple-oss-distributions/xnu/blob/5c306bec31e314fa4d8bbdafb2f6f5a6b7e7b291/bsd/man/man2/write.2#L168-L186 | ||
| pipe_dirty: true, |
There was a problem hiding this comment.
Should it then be cfg-gated, instead of using true for both platforms? Or maybe we don't really care, because putting true for Linux just causes the pipe to be reinitialized which is fine?
There was a problem hiding this comment.
Yeah I'd go for one simpler approach that works for both, this code is "weird enough" as-is ;)
There was a problem hiding this comment.
Yes, the optimized version would be to mark it non-dirty on linux to avoid an extra pipe2 syscall. But an EFAULT should be rare case anyway, as it requires racing with the payload reclamation.
| @@ -63,7 +60,7 @@ impl super::HeaderMemoryHolder for MemMapping { | |||
| } | |||
|
|
|||
| fn after_fork(self) { | |||
There was a problem hiding this comment.
What's the use-case for after_fork? If it's the same as drop, and drop is called automatically anyway when we exit a scope, it looks like it's a no-op
There was a problem hiding this comment.
Right, it's not needed anymore since the pid was moved to the writer::linux::MemMapping and the drop implementation became fork-safe.
| (*header) | ||
| .payload_size | ||
| .store(payload_size, Ordering::Relaxed); | ||
| (*header) |
There was a problem hiding this comment.
Any reason for the re-ordering here? Not that it's important, since both accesses are Relaxed, this shouldn't change anything from the point of view of the reader.
There was a problem hiding this comment.
I'll restore the previous order to avoid unnecessary churn
There was a problem hiding this comment.
I wonder if all the comment and the local_handler dance is relevant anymore. Now that you've pushed the check to the unmap implementation, I think we can just use swap and call it a day.
There was a problem hiding this comment.
Yes, you're right (see also: after_fork)
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
I'd like to know more about the MADV_DONTFORK change before we apply it 👀
| // should anything have been written already we would get a short write | ||
| // However, this is not the case for macOS, despite what its manual | ||
| // says: See https://github.com/apple-oss-distributions/xnu/blob/5c306bec31e314fa4d8bbdafb2f6f5a6b7e7b291/bsd/man/man2/write.2#L168-L186 | ||
| pipe_dirty: true, |
There was a problem hiding this comment.
Yeah I'd go for one simpler approach that works for both, this code is "weird enough" as-is ;)
| /// `Some(pid)` when `MADV_DONTFORK` succeeded, otherwise `None`. | ||
| only_for_pid: Option<u32>, |
There was a problem hiding this comment.
Wait, when can MADV_DONTFORK fail on Linux? It's not clear from the comments/PR description why we're treating it as might-fail now.
In particular, the spec and other implementations are requiring MADV_DONTFORK; this is not to say that the spec is correct -- perhaps there's a good reason to change this, which is why I'm asking.
There was a problem hiding this comment.
There are a few situations. Letting codex inspect the kernel source tree I got:
-
EINVAL: addr is not page-aligned, the rounded length overflows, or the requested subrange requires splitting a VMA that cannot be split—such as some
special mappings, or a HugeTLB/dev-DAX mapping at an invalid boundary. -
ENOMEM: some part of the range is unmapped or outside the process address space. Mapped portions are still processed.
-
EAGAIN: VMA splitting/merging could not allocate metadata, or the process reached vm.max_map_count. Internally these failures are ENOMEM, translated to
EAGAIN (mm/vma.c:590, mm/madvise.c:1439). -
EPERM: on current kernels, the range reaches a sealed, anonymous mapping that the caller cannot write—typically a read-only mseal()ed mapping
(mm/madvise.c:1272, kernel documentation (https://docs.kernel.org/userspace-api/mseal.html)). -
EINTR: interrupted while waiting for the mmap write lock (mm/madvise.c:1774).
-
ENOSYS: unusual kernel built without CONFIG_ADVISE_SYSCALLS.
We should prob retry on EINTR by the way
There was a problem hiding this comment.
Wait but most of those reasons would mean we can't use our context for other things anyway?
My recommendation is to keep treating it as required, and fail the context publishing if we can't do this syscall, similar to how we fail publishing the context if some of the other syscalls fail.
Having a process fail to MADV_DONTFORK and then fork means keeping stale process for readers to find, and will be a really hard to debug situation.
There was a problem hiding this comment.
Fair enough, I'll change it to fail for anything other than eintr
…n' into glopes/otel-process-ctx-linux-update
## What does this PR do? This is **1/3** in the stacked process-context series. It reorganizes the existing Linux process-context implementation without changing its behavior: - separates shared reader and writer logic into dedicated modules; - moves Linux-specific allocation, discovery, clock, and copy behavior behind platform modules; - introduces independent reader and writer Cargo features while preserving the existing Linux defaults at this layer; - adds platform-agnostic exports and retains the Linux compatibility path; - preserves the existing functionality exactly, including the edge cases and bugs addressed by the next PR. ## Why? This creates the module structure needed for additional operating systems without mixing a large code move with functional changes. Reviewers can evaluate the organization independently before the Linux implementation is updated and the new platforms are added. ## Impact There is no intended runtime behavior change. Linux remains the only supported implementation in this PR, and existing Linux callers retain the same behavior. ## Validation Validated independently at this commit with: - `cargo check -p libdd-library-config --all-features`; - `cargo check -p libdd-library-config-ffi`; - all-feature process-context tests; - reader-only process-context tests. ## Stack 1. **#2228 — reorganize the Linux process context** 2. #2237 — update the Linux implementation 3. #2238 — add macOS and Windows support BREAKING CHANGE: moves `ProcessContextSelfReader` from the Linux-specific module to the platform-agnostic process-context API, while retaining the deprecated Linux compatibility path. Co-authored-by: gustavo.lopes <gustavo.lopes@datadoghq.com>
a53e8ef to
91fb62b
Compare
If it fails:
If it doesn't fail (the normal case), the mapping will be gone so the new reader will report NotFound.
Yes. If it succeeds, a new reader will not find the mapping before the child publishes. |
| // SAFETY: MemMapping owns a live mapping of mapping_size() bytes. Failure is harmless; | ||
| // the mapping then follows the default inheritance behavior. | ||
| retry_on_eintr(|| { | ||
| check_syscall_retval( | ||
| unsafe { | ||
| libc::madvise( | ||
| mapping.start_addr.as_ptr(), | ||
| super::mapping_size(), | ||
| libc::MADV_DONTFORK, | ||
| ) | ||
| }, | ||
| "madvise MADV_DONTFORK failed", | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
Wait, the code seems correct but the comment is outdated I think?
| let mut local_handler = ProcessContextHandleGen::publish(payload)?; | ||
| // If we've been forked, we need to prevent the mapping from being dropped | ||
| // normally, as it would try to unmap a region that isn't mapped anymore in the | ||
| // child process, or worse, could have been remapped to something else in the | ||
| // meantime. | ||
| // | ||
| // To do so, we get the old handler back in `local_handler` and prevent `mapping` | ||
| // from being dropped specifically. | ||
| swap(&mut local_handler, handler); | ||
| local_handler.mapping.after_fork(); | ||
| let new_handler = ProcessContextHandleGen::publish(payload)?; | ||
| let _old_handler = replace(handler, new_handler); |
There was a problem hiding this comment.
Minor: I don't think we need a 7 line comment, but now the logic is a bit cryptic e.g. you need to understand what as_ptr().is_some() does before understanding why we publish here, so a comment reminding readers would be useful.
3 participants
What does this PR do?
This is 2/3 in the stacked process-context series and depends on #2228.
It adapts the Linux implementation after the code-organization layer:
MADV_DONTFORKbest-effort instead of treating it as a publishing failure;Why?
#2228 intentionally preserves the old Linux behavior while introducing the shared reader/writer structure. This PR applies the Linux behavior changes separately so they can be reviewed without being mixed with either the file reorganization or the macOS/Windows implementation.
Impact
Linux keeps the same platform-agnostic process-context API, with corrected fork handling and the final feature defaults. This layer does not add macOS or Windows support.
Validation
Validated independently at this commit with:
cargo check -p libdd-library-config --all-features;cargo check -p libdd-library-config-ffi;cargo nextest run -p libdd-library-config --all-features --no-fail-fast;cargo nextest run -p libdd-library-config --no-default-features --features process-context-reader --no-fail-fast.Stack
BREAKING CHANGE:
process-context-readeris no longer enabled by default; downstream users that require it must enable the feature explicitly.