refactor(library-config): reorganize Linux process context#2228
Conversation
cataphract
changed the title
BenchmarksComparisonCandidateCandidate benchmark detailsBaselineBaseline benchmark details |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 57cbcb8 | Docs | Datadog PR Page | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19438b34d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let chunk_len = (len - offset).min(self.chunk_size); | ||
| let chunk_addr = addr.wrapping_add(offset); | ||
|
|
||
| // SAFETY: write asks the kernel to copy from chunk_addr. Invalid user memory is |
There was a problem hiding this comment.
Reading the man page, it seems like error detection may occur but I don't see where it is guaranteed? POSIX does not appear to require EFAULT at all.
There was a problem hiding this comment.
We're only targeting Linux and macOS. Both Linux's and mac os man pages describe this return value.
| }) | ||
| } | ||
|
|
||
| fn copy(&self, addr: *const u8, len: usize) -> Result<Vec<u8>, PipeCopyError> { |
There was a problem hiding this comment.
Does this function have SAFETY requirements?
There was a problem hiding this comment.
The whole point of the function is that it should not generate a fault if attempting to read unmapped or otherwise inaccessible manner. So there is no safety contract that the caller must respect in order for the invocation of the function not to trigger UB.
There was a problem hiding this comment.
I think this contract (copy won't fail even for unmapped addresses or unitialized memory) should be specified at the trait level, in the documentation of copy. It's not obvious for anyone who hasn't sufficient context (as demonstrated by Daniel's question)
There was a problem hiding this comment.
I documented the contract on ProcessMemoryCopy::copy: callers do not need to establish that the range is valid, mapped, initialized, or remains mapped; inaccessible memory must return WouldBlock; success returns exactly len bytes; and the copy is not an atomic snapshot.
| let chunk_addr = addr.wrapping_add(offset); | ||
|
|
||
| // SAFETY: write asks the kernel to copy from chunk_addr. Invalid user memory is | ||
| // reported as EFAULT or a short write without being dereferenced by Rust. |
There was a problem hiding this comment.
What are the semantics in the case of a short write
There was a problem hiding this comment.
If there is a short write, then something interrupted the function after it's already written some data. It could have been interrupted, or it could have found an unreadable page.
But it turns out that, despite what the mac os man page says, write() always returns EFAULT upon encountering a fault, even if it's written something. This requires marking its error with pipe_dirty: true, which I'll do in a bit.
| } | ||
|
|
||
| // SAFETY: every byte was initialized by the pipe reads above. | ||
| unsafe { bytes.set_len(len) }; |
There was a problem hiding this comment.
would it be safer to vec![0,len], or std::io::BorrowedBuf?
There was a problem hiding this comment.
I answered a similar comment when this code was introduced (it was just moved in this PR): https://github.com/DataDog/libdatadog/pull/2176/changes/BASE..12ca390b7683b6efaeae6cf06078b26eaf41a3d9#diff-e3f807d69a162e59e2e82257164c7642090139e9d90f6be2fecde5205ead3636 :
What difference does it make if it's initialized with zeros or not? The read syscall writes directly into the buffer and it should fill it to capacity. And if it there was a bug and it didn't, zeros or uninitialized memory would be equally wrong (I'd probably even prefer uninitialized memory because it's more likely to be caught by MSan).
BorrowedBuf would indeed be a small improvement, but it doesn't appear to be stable.
There was a problem hiding this comment.
The difference is that you would get rid of this unsafe set_len I believe. You initialize it with the right length from the beginning and just use it as a fixed-sized array [u8; N]. No need to uphold any safety condition anymore.
There was a problem hiding this comment.
I changed the destination to vec![0; len], removed the unsafe set_len, and now construct a bounds-checked mutable slice before calling read. I also documented the slice-bounds proof.
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
|
f1169de to
f5eccae
Compare
|
This is very cool, and I definitely want to give it a pass ASAP. Having said that, I'd like to request a change in the way we refer to this. Specifically, at this time, we're not trying or proposing upstream any way of using the OTel process context and thread context specifications on macOS and Windows. Thus I humbly request:
My intent here is to give us fully the space to experiment and validate -- if this goes really well it will be great to fold it into the spec; but not confuse outside-Datadog folks, since the specs explicitly call out process context and thread context as being Linux-specific mechanisms for the time being. |
|
Furthermore (forgot to add to the above) -- not being yet tied to the spec gives us flexibility to go "Oh actually we want to do it another different way on Windows" (or macOS) while we're experimenting, whereas if we were folding this right away into the spec it'd be weird to flip-flop as we experiment. |
cataphract
changed the title
|
OK, I've renamed the PR and added a few comments. Note that this follows the specification to the extent that it can be followed. I's only the part that can only be implemented on Linux, the discovery mechanism, that differs. All the rest -- the data itself, how it's laid out, the update protocol, follows the spec. |
|
At this point, this PR is too big -- I humbly request that we break it down into smaller steps. E.g. first one with all changes for Linux, and then separate windows and macOS support. With so many changes it'll be a lot of work to figure out what's only moving and what's actually changing. |
9bd8dd1 to
011c154
Compare
|
@ivoanjo I've split it into 3 commits. The first is just moves code around and has no change in the linux functionality. The second contains the updates I did to the linux implementation. Finally, the third adds windows and mac os. Hope this helps. |
|
I'd really like separate PRs -- I feel like we're creating a huge PR that will take a bunch of time to deliver, whereas small PRs have small steps that we can agree on and move quickly on. |
|
To be clear, I am aware that split PRs are a pain for the submitter -- yet that's exactly what I'm asking for here, I'm asking you to help us ship this fast by making it easier on the reviewers to move fast on it. |
|
As a reviewer, I'd rather have the three commits in one PR, since the reorganization and abstractions make more sense in light in the mac os/windows implementations in the later commits. But to each his own |
011c154 to
1a04bb0
Compare
cataphract
changed the title
1a04bb0 to
e6af895
Compare
e6af895 to
89a636d
Compare
| @@ -0,0 +1,240 @@ | |||
| // Copyright 2026-Present Datadog, Inc. https://www.datadoghq.com/ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
There was a problem hiding this comment.
Nit: add a short module description
There was a problem hiding this comment.
Added module documentation explaining the fault-safe Unix pipe mechanism and how inaccessible source memory becomes WouldBlock.
| }) | ||
| } | ||
|
|
||
| fn copy(&self, addr: *const u8, len: usize) -> Result<Vec<u8>, PipeCopyError> { |
There was a problem hiding this comment.
I think this contract (copy won't fail even for unmapped addresses or unitialized memory) should be specified at the trait level, in the documentation of copy. It's not obvious for anyone who hasn't sufficient context (as demonstrated by Daniel's question)
| } | ||
|
|
||
| #[cfg(target_os = "linux")] | ||
| fn create_pipe() -> io::Result<(OwnedFd, OwnedFd, usize)> { |
There was a problem hiding this comment.
Should the return type be a proper struct instead of a triplet?
There was a problem hiding this comment.
Yes. create_pipe() now constructs and returns the existing CopyPipe type directly instead of returning a positional triplet.
|
|
||
| // Re-exported for backwards compatibility. | ||
| #[cfg(feature = "process-context-writer")] | ||
| #[deprecated(note = "use libdd_library_config::otel_process_ctx directly")] |
There was a problem hiding this comment.
I would be inclined to ignore backward compatibility and just get rid of those. The process context implementation is rather recent, there's little to no code that uses the ProcessContext API directly outside of libadatadog to my knowledge, and even then, we control that code. Any opinion @ivoanjo ?
There was a problem hiding this comment.
Yeap +1 on this; I believe most writers are going through libdd-library-config, except maybe node, and we can update that one as well, so I think it's fine to clean these up.
Double so for the reader -- since it's basically new I'm pretty sure nobody's using it yet
There was a problem hiding this comment.
I removed the deprecated Linux compatibility module and re-exports. MAPPING_NAME remains because the Linux implementation still uses it.
|
|
||
| pub(super) trait ProcessMemoryCopy: Sized { | ||
| fn new() -> io::Result<Self>; | ||
| fn copy(&self, addr: *const u8, len: usize) -> Result<Vec<u8>, PipeCopyError>; |
There was a problem hiding this comment.
Re copy: the contract should be clearly specified here.
There was a problem hiding this comment.
Done. The trait now documents the caller’s lack of pointer-validity obligations, the exact WouldBlock error for inaccessible memory, the all-or-error result, and the lack of snapshot atomicity.
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub(super) struct PipeCopyError { |
There was a problem hiding this comment.
Isn't this a bit Linux-specific (the pipe implementation)? Maybe it's just a minor naming issue, and this should be MemoryCopyError or sth. But it feels like Linux implementation is leaking a bit here.
There was a problem hiding this comment.
All three supported implementations—Linux, macOS, and Windows—use a pipe, so the pipe terminology is intentional rather than Linux-specific. I kept PipeCopyError.
| pub struct ProcessContextSelfReader { | ||
| pid: u32, | ||
| pub(in crate::otel_process_ctx) header_ptr: NonNull<u8>, | ||
| pipe: Cell<Option<PlatformCopyPipe>>, |
There was a problem hiding this comment.
Isn't this linux-specific as well? I think PlatformCopyPipe is only defined on Linux, right? In general I would expect this module to have a very simple copy interface -- basically ProcessMemoryCopy that guarantees fault-free copy, without talking about pipes or whatnot, which is an implementation detail.
Then either ProcessContextSelfReader should be parametrized by a Copier: ProcessMemoryCopy, or if that's too heavy, define a ProcessMemoryCopyImpl for each platform in a dedicated module, and import it unconditionally here.
There was a problem hiding this comment.
macOS and Windows also use pipes, so PlatformCopyPipe describes the common mechanism across all supported platforms. I kept the current non-generic reader structure.
There was a problem hiding this comment.
TBF, I still find that at this level of abstraction on the reader code, whether we use a pipe or process_read_vm or whatnot is an implementation detail at this level of abstraction, and that what matters is that it's a fault-less copy. It could be switched for a different mechanism in the future, for example. But it's really not important
|
|
||
| // SAFETY: the reader owns its copy pipe and only stores a process-global address. Cell keeps the | ||
| // type !Sync, so the cached pipe cannot be used concurrently through shared references. | ||
| unsafe impl Send for ProcessContextSelfReader {} |
There was a problem hiding this comment.
Same, for modularity purpose, we should probably require ProcessMemoryCopy to be Send, moving the burden to the platform-specific implementation. Maybe the implementation on Windows wouldn't use pipe but a totally different mechanism; it's hard for ProcessContextSelfReader to justify in advance that all implementations for ProcessMemoryCopy in the future will be Send.
There was a problem hiding this comment.
ProcessMemoryCopy now requires Send, making each platform pipe prove that property. I also updated the reader’s unsafe Send justification so it only needs to account for the process-global NonNull address.
|
|
||
| [features] | ||
| otel-thread-ctx = [] | ||
| default = ["process-context-reader", "process-context-writer"] |
There was a problem hiding this comment.
I would not make process-context-reader the default. For now it seems quite specific to the PHP profiler.
There was a problem hiding this comment.
The reader intentionally remains enabled by default in this first PR because this commit promises to preserve main’s behavior. It is removed from the defaults in the next stacked PR.
| @@ -0,0 +1,268 @@ | |||
| // Copyright 2026-Present Datadog, Inc. https://www.datadoghq.com/ | |||
There was a problem hiding this comment.
I got quite confused by the fact that there's a top-level reader.rs file, but also submodules in a different reader/xxx subdirectory. Same for writer. Would you have anything against moving reader.s and writer.rs to respective mod.rs files in the corresponding subdirectories? We would have everything writer-related and reader-related at the same place.
There was a problem hiding this comment.
I kept the current layout. reader.rs with children under reader/ is the modern idiomatic Rust layout; reader/mod.rs is the older, still-supported form.
There was a problem hiding this comment.
Do you have reference for this being the recommended way? I'm genuinely curious. Though maybe I'm just... old 😅 (
There was a problem hiding this comment.
Nevermind I found it in the doc. TIL.
There was a problem hiding this comment.
The Rust Book's "Alternate File Paths" section explicitly calls foo/mod.rs the older style and presents foo.rs with foo/bar.rs as the idiomatic layout. The Rust 2018 Edition Guide also explains the move away from mod.rs -- to avoid having many indistinguishable mod.rs tabs open.
yannham
left a comment
yannham
left a comment
There was a problem hiding this comment.
(Sorry, I meant to approve - none of the comments above are blocking)
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
👍 LGTM -- I +1 Yann's notes and I've left some tiny notes of my own but this is basically good to go!
| ); | ||
| } | ||
|
|
||
| /// The only end-to-end test with `ProcessContextSelfReader` |
There was a problem hiding this comment.
Minor: The "The only" part of this comment looks like it can be made out-of-date very easily the next time someone adds another test; maybe simply to only "End to end test"?
There was a problem hiding this comment.
Done. The comment now describes it simply as an end-to-end test using ProcessContextSelfReader, without making a uniqueness claim.
| fn publish_read_update_read_and_unpublish() { | ||
| fn context(service_name: &str) -> ProcessContext { | ||
| ProcessContext { | ||
| resource: None, |
There was a problem hiding this comment.
Minor: Maybe put a few things in the resource? An empty resource is actually something we don't expect will happen very often, and thus putting stuff here would make the end-to-end test a bit more realistic
There was a problem hiding this comment.
Done. The fixture now includes service.name and telemetry.sdk.language resource attributes, plus datadog.process_tags as an extra attribute.
| "mmap failed" | ||
| )?; | ||
|
|
||
| // We (implicitly) close the file descriptor right away, but this ok |
There was a problem hiding this comment.
Minor: I know this comment already existed before, yet on a second read I think we can make it stronger even -- the spec does say that the fd is to be closed, it's not a "happy accident" that we do so here.
| // We (implicitly) close the file descriptor right away, but this ok | |
| // We (implicitly) close the file descriptor right away on purpose |
There was a problem hiding this comment.
Updated. The comment now says that the descriptor is closed immediately on purpose and explicitly notes that this does not invalidate the mapping.
|
|
||
| // Re-exported for backwards compatibility. | ||
| #[cfg(feature = "process-context-writer")] | ||
| #[deprecated(note = "use libdd_library_config::otel_process_ctx directly")] |
There was a problem hiding this comment.
Yeap +1 on this; I believe most writers are going through libdd-library-config, except maybe node, and we can update that one as well, so I think it's fine to clean these up.
Double so for the reader -- since it's basically new I'm pretty sure nobody's using it yet
| //! By design, this reader emulates an out-of-process reader: it does not take the publisher's | ||
| //! mutex or hold any guard that prevents the mapping from being unpublished, the header from being | ||
| //! rewritten, or the payload allocation from being replaced while it copies memory. The | ||
| //! seqlock-style timestamp checks and fences only let it detect concurrent publication or update; | ||
| //! unpublish can make the memory disappear entirely. | ||
| //! | ||
| //! To handle this safely, instead of dereferencing process memory directly, we ask the kernel to | ||
| //! copy those bytes into a pipe and then read the pipe back. This gives us the same failure mode an | ||
| //! out-of-process reader has: if memory is unmapped or invalid, the copy fails instead of | ||
| //! segfaulting the process. |
There was a problem hiding this comment.
Minor: This comment was kinda nice -- it explained why the reader looks like it does (it's weird), and what constraints we were working with. I think it's worth finding it a new home.
There was a problem hiding this comment.
Restored at the top of reader.rs, adjusted to distinguish the shared mapping-lifetime constraints from the platform-specific same-process copy mechanism. The Unix pipe details are documented separately in copy_pipe_unix.rs.
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
4 participants
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:
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;Stack
BREAKING CHANGE: moves
ProcessContextSelfReaderfrom the Linux-specific module to the platform-agnostic process-context API, while retaining the deprecated Linux compatibility path.