-
Notifications
You must be signed in to change notification settings - Fork 10
Update experimental C/C++ implementation for process context OTEP with memfd + in-place modification #34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
felixge
merged 9 commits into
open-telemetry:main
from
ivoanjo:ivoanjo/memfd-process-ctx
Jan 7, 2026
Merged
Update experimental C/C++ implementation for process context OTEP with memfd + in-place modification #34
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
0ed3ac0
Minor: Introduce macro with OTEL_CTX literal
ivoanjo f85609e
Minor: Tweak error message
ivoanjo 6f8b21b
Create mmap from memfd as a first option
ivoanjo 5302427
Drop "probing mappings" as a fallback
ivoanjo 62cb445
Refactor: Extract `ctx_is_published` out from `otel_process_ctx_drop_…
ivoanjo c7ddce2
Refactor: Move check for NULL data and getting timestamp to beginning…
ivoanjo 7914d48
Perform in-place updates instead of dropping/recreating mapping
ivoanjo 4fcbc1b
Minor: Add comment for update step
ivoanjo 03501eb
Apply suggestions from code review
ivoanjo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is it possible to free this memory? How are you sure that the readers of the previous payload have finished reading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because readers are expected to be reading from outside the process, they will either read:
As per the "Reading protocol" in open-telemetry/opentelemetry-specification#4719, after reading the payload bytes, they readers need to re-check the
published_at_ns.That is, let's say the reader spotted
published_at_ns == 1and the oldpayload. When they go to re-read thepublished_at_ns, they MUST observepublished_at_ns > 1because the new timestamp gets installed before we free the oldpayload. So they'll be able to detect "oops whatever I got is not correct, there was a concurrent update, try again".For readers inside the process (as is this test code) this is documented in the header:
In practice, to avoid this happening in-process a user of this library could wrap a lock around every operation, thus making sure there's no concurrency between updates and other operations (e.g. reading). (We could provide such a feature of the box too -- feedback welcome?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm not familiar enough with what readers outside the process can do, or what is harmful to them. I'm only thinking that since
freeis called, there is in theory nothing stopping the allocator from unmapping the page that the memory was on, and how will an external reader react to the page disappearing while it is reading from it?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a few different APIs for reading remote process memory --
process_vm_readv,ptrace, or reading /proc/<pid>/mem.When using these APIs, trying to read pages that disappear/are not there anymore will get you an error as the return value for that function. In particular you won't get a
SEGFAULT.(
process_vm_readvis actually quite neat since you can use it from inside the process too -- for instance we could update the reader inotel_process_ctx.cif we wanted to make sure it could never segfault even in the face of concurrency in the update; the downside is that it can be disabled/blocked on some container setups so a regular user process can't always rely on it being there)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Then I'm all good with the approach.