feat(library-config): otel process context reader#2176
Conversation
Add `read()`, `threadlocal_attribute_key_map()`, and `read_threadlocal_attribute_key_map()` to `otel_process_ctx::linux`, along with the `find_otel_mapping()` / `is_named_otel_mapping()` / `parse_mapping_start()`.
cataphract
force-pushed
the
levi/otel-thread-context
branch
from
2887d0b to
cfe279d
Compare
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results📦
|
![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: 2887d0b569
ℹ️ 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 payload = unsafe { std::slice::from_raw_parts(payload_ptr, payload_size as usize) }; | ||
| let context = ProcessContext::decode(payload)?; |
There was a problem hiding this comment.
When read() runs concurrently with another thread calling publish()/update(), the timestamp check does not keep the old payload allocation alive: after the first published_at load, ProcessContextHandle::update can assign self.payload = payload, dropping the Vec that payload_ptr still references, and this code then builds a slice and lets prost read from a dangling pointer. The final timestamp comparison happens only after the unsafe read, so it cannot prevent a crash/UB; the same-process reader needs to coordinate with the publisher or otherwise copy from memory that cannot be freed during decode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
yeah... This file seems to want to update some sort of seqlock, but there are several problems with it, one of these is this reclamation problem; a seqlock can protects only static region. Also a seqlock depends on a counter being odd to detect in-progress changes.
There was a problem hiding this comment.
Likely the solution involves in-process readers using the write lock.
There was a problem hiding this comment.
lso a seqlock depends on a counter being odd to detect in-progress changes.
I think the odd thing is because of multiple writers. If you have one writer only, and multiple reader, a monotonic value should be enough to detect torn reads (which I believe is the model for the process context).
one of these is this reclamation problem; a seqlock can protects only static region
I think I don't understand what you mean by that?
There was a problem hiding this comment.
I think the odd thing is because of multiple writers.
It's not, it's to detect that a modification is progress, so the read may be give torn/inconsistent data. Assume there is no reordering:
- update data
- ++seq
and the reader does
- read seq
- read data
- re-read seq to confirm it's the same
then the reader may read partially updated data and see the same seq.
But anyway, I was wrong about this, because the writer uses 0 to signal the update in progress.
one of these is this reclamation problem; a seqlock can protects only static region
What I mean is that the reader may fetch a pointer to the payload, and by the time it reads it, it's been freed already. This is a problem if the memory region has been unmapped (segfault).
There was a problem hiding this comment.
So I think the wrong publishing protocol was chosen for this. What the seqlock is protecting is a single pointer, so you could just update the pointer atomically with release-acquire (more or less, it also has a size, but that's not an obstacle because amd64/arm64 support 16-byte atomics, or the size could be moved to the beginning of the pointed to region). This doesn't eliminate the reclamation problem -- you'd still need something else to prevent in progress readers from segfaulting, but it does show that the seqlock buys nothing here.
There was a problem hiding this comment.
Ah, I see, thanks for the clarification for the odd counter part. I didn't remember that correctly.
I guess in the process context case the information is just split across two different fields.
What I mean is that the reader may fetch a pointer to the payload, and by the time it reads it, it's been freed already. This is a problem if the memory region has been unmapped (segfault).
@ivoanjo I don't remember all the details from the spec, but I think the model here is to map a page at init time and then never unmap it (beyond in-place update)? Of course you can't enforce that from the reader side, you have to rely on the writer to be compliant.
But it seems that process_vm_readv() returns an error, and doesn't cause SEGFAULT, for invalid/unmapped addresses (not clear from the doc, but so says my clanker). So I won't be worried about unmapping the context page.
However, if the payload is being modified, I think you're right. I don't see how you can avoid:
- reader see a consistent context and read it locally
- writer starts and completes an update by re-allocating the payload, causing the previous one to be dropped if heap-allocated, or overwritten if stored in the mapping.
- reader starts to dereference the payload ptr
This case might be even worse because you won't get EINVAL, since the memory will most likely still be mapped (even in the heap), just potential gibberish 🤔
There was a problem hiding this comment.
I guess one fix would be to restrict the reader to always use the same mapping for the payload, and include in the bracket as part of the "writing". The pointer would effectively be mostly useless, and the payload would be a static part (beside the length) of the whole context.
There was a problem hiding this comment.
I think the model here is to map a page at init time and then never unmap it (beyond in-place update)?
This is the case for the header. The problem is the payload.
But it seems that process_vm_readv() returns an error, and doesn't cause SEGFAULT, for invalid/unmapped addresses
Yes, out-of-process reads don't have this problem. A more difficult question, with process_vm_readv doing plain memcpy page-by-page and having the payload and header in different pages is whether they participate correctly in the protocol, which something that needs to be seen architecture-by-architecture, but probably it's not a big obstacle.
However, if the payload is being modified, I think you're right. I don't see how you can avoid:
The way this is usually avoided for in-process readers is to use specific reclamation strategies like RCU, hazard pointers, refcounting, etc... or use read-write locks.
I think that reading gibberish, even in-process, is not that bad, because it can be detected by the seqlock protocol (assuming you follow the pointer and read all your data before re-checking the sequence number). It's just the if the memory has been unmapped that it becomes a problem.
There was a problem hiding this comment.
We've already discussed in the channel, but looping back on this -- my suggestion is to either have an out-of-band mechanism for in-process reads synchronization OR use a process_vm_readv or equivalent reader.
| name.starts_with("/memfd:OTEL_CTX") | ||
| || name.starts_with("[anon_shmem:OTEL_CTX]") | ||
| || name.starts_with("[anon:OTEL_CTX]") |
There was a problem hiding this comment.
Because these checks use starts_with, a process that also has a mapping such as /memfd:OTEL_CTX_BACKUP or [anon:OTEL_CTX_old] can be selected before the real /memfd:OTEL_CTX entry, since /proc/self/maps is ordered by address rather than by mapping name. In that case read() will return an invalid-signature error (or read the wrong context) even though a valid OTEL context mapping exists later in the maps file; the name match should be exact, with only the kernel's separate (deleted) token handled as metadata.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Why would there be mappings with names like "/memfd:OTEL_CTX_BACKUP" or "[anon:OTEL_CTX_old]"? Yes, sure, it's technically true but... what are we supposed to do about this?
There was a problem hiding this comment.
Wasn't there a suggestion on adding several of these mappings for PHP to account for it processing several service/env? Ultimately we didn't do it, but I think that's indeed the idea of the spec, that you could have several.
This implements literally what the spec says: https://github.com/open-telemetry/opentelemetry-specification/blob/14137d0022f98bb59392f82255a70f3d9c2a7d4c/oteps/profiles/4719-process-ctx.md?plain=1#L164
Locate mapping: Parse
/proc/<pid>/mapsand search for entries with name starting with[anon_shmem:OTEL_CTX],[anon:OTEL_CTX]or/memfd:OTEL_CTX
though I think it should say [anon_shmem:OTEL_CTX and [anon:OTEL_CTX. Or maybe it just wanted to allow the (deleted) afterwards
There was a problem hiding this comment.
Yes, it's for the (deleted) stuff. It's not for multiple mappings to find.
There was a problem hiding this comment.
What @bwoebi said -- the process context is expected to be a singleton for a process, and anything that's not a singleton for a process should not go in there.
E.g. if a process is 1:1 to a service, then service name goes in there (for instance, this is the plan for dd-trace-rb); if a process is somehow multi-tenant and supports multiple services (e.g. dd-trace-php), then it shouldn't go in there.
It's not supported by the spec publishing several OTEL_CTX memory locations -- readers are only looking for one so they'll either error or ignore the others.
|
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
|
…:Result -> io::Result
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| fence(Ordering::SeqCst); | ||
| AtomicU64::from_ptr(addr_of_mut!((*header).monotonic_published_at_ns)) | ||
| (*header) | ||
| .monotonic_published_at_ns | ||
| .store(published_at_ns, Ordering::Relaxed); |
There was a problem hiding this comment.
I think the fence is unnecessary if I change the store to use Ordering::SeqCst, yeah? In the previous state, it did a SeqCst fence with a Relaxed store, I didn't change it, I just changed to use the AtomicU64 natively instead of through a ptr.
There was a problem hiding this comment.
I've read a bunch of material which helped me understand some other cases better but... not precisely this one. If anyone can educate me here, I'd appreciate understanding how they'd be different and which one is desired!
There was a problem hiding this comment.
Yes, and seqcst is even too strong, it only needs release. If you're interested, you can read this paper on synchronization for the seqlock algorithm.
But there is a problem that makes reasoning purely about the C++ memory model here insufficient, and that is that we need to support out-of-process readers with process_vm_readv. This requires understanding both a) what happens architecturally on the writer and b) how process_vm_readv (which I think does just memcpy page by page) and access_remote_vm work exactly.
In any case, we only need to support amd64 and arm64, so just a Release store should be enough.
There was a problem hiding this comment.
My suggestion is -- let's go with the strongest barrier we have. Why? Because the process context is not performance-sensitive, and the cost of getting this detail wrong is high.
So even though we need barriers here for correctness, I don't think we should be trying to think about them in terms of performance. If we had a lock that would use a much stronger atomic and nobody would be mad if we grab a log once an hour so that's the mental model to go for here ;)
-- my 0.02c :D
There was a problem hiding this comment.
I think the fence is unnecessary if I change the store to use Ordering::SeqCst, yeah? In the previous state, it did a SeqCst fence with a Relaxed store, I didn't change it, I just changed to use the AtomicU64 natively instead of through a ptr.
Yes, I believe SeqCst store is more constraining than a SeqCst fence + a relaxed store. The difference is rather technical, but it's that in the latter case, the store isn't part of the total sequential consistent order. For all other orderings, a fence with ordering O followed (resp. preceded by) a relaxed store (resp. a relaxed load) is strictly equivalent to a store (resp. a load) with ordering O. The main usage for fences is that you can only use one for several atomic operations at once, or that you can use them conditionally. Here there's no strong motivation other than following closely the spec.
Yes, and seqcst is even too strong, it only needs release. If you're interested, you can read this paper on synchronization for the seqlock algorithm.
Yup. We used SeqCst mostly because:
- the process context publication isn't performance-critical (happens once or not very often)
- since the whole seqlock-like thing is fishy in Rust, we upgraded to the strongest ordering.
But there is a problem that makes reasoning purely about the C++ memory model here insufficient, and that is that we need to support out-of-process readers with process_vm_readv. This requires understanding both a) what happens architecturally on the writer and b) how process_vm_readv (which I think does just memcpy page by page) and access_remote_vm work exactly.
Yes, I personally have no idea what is the memory semantics of process_vm_readv, and in any case it can't be considered by the C++ memory model, which doesn't model OS-level operations like these. So we're a bit in best-effort places.
There was a problem hiding this comment.
Though, process_vm_readv can't have stronger requirements than direct memory accesses, I believe. So if we're direct-memory-access-safe, I don't see how we wouldn't be syscall-read-memory-safe.
There was a problem hiding this comment.
Yeah I think the way to reason about this is about constraining what the compiler does in such a way that it can't be anything else than what we want (even though the spec may not technically promise it...)
morrisonlevi
force-pushed
the
levi/otel-thread-context
branch
from
19c2470 to
d363e03
Compare
In this case, we could just make the pointer and the size atomic and access them with relaxed memory order. This would remove the technicality of the race being UB. But even without it, I don't think it's much of a problem. I'm pretty sure the exact same code would be generated. |
|
My original question still stands (as it's not written in the PR description - to be clear I haven't look at the PR code yet in details): what is the motivation for having a reader in libdatadog? |
I could be implemented somewhere else. I guess the same reason the writer is implemented in libdatadog -- to do it uniformly across tracers. And with the writer already implemented in it, and the reader being sort of implemented already for the tests, everything stays more cohesive. That said, I have no great objection to this being moved to dd-trace-php |
|
More precisely: You need a reader in (PHP) profiling to support using the profiler with another otel compatible tracer (other than our tracer) running in the same process and being able to access that tracers context data. libdatadog is the right place for this - it's not necessarily a pure PHP concern. |
|
In practice we're allocating a whole OS page to put this header in there, and the header is like 32 bytes. That leaves roughly 4,064 bytes (because we're talking about Linux here, and nearly everything is running 4096 pages or more on Linux) to put a serialized payload into. Why use a separate allocation for the payload? Nobody should use more than ~4k bytes for this purpose, methinks? And this utilizes otherwise wasted space. If the spec is flexible at this stage, instead of storing a ptr you can store a length for the usable portion of the page, which can vary e.g. 16 KiB on macOS if we ever port there. I think someone may have hinted at doing this in the discussion already, and the spec hints at an optimization, but does anyone (@ivoanjo?) have a problem mandating that? Because otherwise you can't really do a safe read, if the ptr has a separate lifetime. I suppose it could point to some other safe place like a static section of memory, but that doesn't really seem too useful compared to just using the memory page. Torn reads can be handled, but a separate lifetime complicates making that torn read safe for both threads and out-of-process readers. |
|
@morrisonlevi Regarding the allocation living elsewhere and not in the header, it's a pure matter of doing the simplest thing for the first implementation. The plan has indeed been to put it in the header as a next step. No other technical reason than TODO. I don't know why this is not mandated in the spec, or why you'd want a separate allocation. I'll leave that part to Ivo 🙂 For the correctness part, we've casually discussed with @scottgerring and @nsavoire yesterday. My current understanding is that having a separate allocation is OK because:
But most importantly, in any of those cases, you don't get a crash. From there, y you'll also be able to know because you check the |
|
@yannham so the plan is to use |
I think the spec was mainly written with an out-of-process, privileged reader in mind, typically an eBPF profiler. If you really need to read from the same process, I would be inclined to go with |
I don't think that would help. Yes, you could tell whether the memory was mapped at the point of the |
yannham
left a comment
yannham
left a comment
There was a problem hiding this comment.
Mostly non-blocking comments
| .monotonic_published_at_ns | ||
| .store(UNPUBLISHED_OR_UPDATING, Ordering::Relaxed); | ||
| } | ||
| fence(Ordering::Release); |
There was a problem hiding this comment.
+1 I think this one could be strenghtened as well
| fence(Ordering::Release); | |
| fence(Ordering::SeqCst); |
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
Gave it a pass -- looks reasonable, although I still kinda think it's not worth optimizing the reads/writes and just use SeqCst everywhere -- we're way more likely to have a "oops actually this is not correct" with the weaker models, so I think here we should optimize for our own sanity.
Also -- confused about the use of process_vm_readv still (although I guess it's OK to keep it and then decide later if we want to replace it, but I think in that case we should call it out?)
| //! Implementation of the publisher part of the [OTEL process | ||
| //! context](https://github.com/open-telemetry/opentelemetry-specification/pull/4719) | ||
| //! Implementation of the publisher and same-process reader parts of the [OTEL process | ||
| //! context specification](https://github.com/open-telemetry/opentelemetry-specification/pull/4719). |
There was a problem hiding this comment.
Minor: This change is no longer accurate, since the reader is separate ;)
| //! Process context sharing also crosses Rust's memory model boundary. In-process header fields | ||
| //! that can change during publication are atomics, while payload bytes are copied with | ||
| //! `process_vm_readv`; that syscall turns accesses to reclaimed payload memory that has been | ||
| //! unmapped into a syscall error or short read instead of a segfault, but its ordering relative to | ||
| //! the publisher has to be reasoned about at the OS/architecture level rather than only in Rust. |
There was a problem hiding this comment.
Minor: This is no longer relevant to this file?
| (*header) | ||
| .monotonic_published_at_ns | ||
| .store(published_at_ns, Ordering::Release); |
There was a problem hiding this comment.
Minor: For simplicity here, I suggest using SeqCst. This is not going to be run that often, so I suggest making it the strongest semantics possible.
There was a problem hiding this comment.
I could get behind this in principle, especially for the writer which is not performance-critical, but we're not really breaking new ground here. The algorithm is well described in the literature, and is already implemented in libdatadog for publishing remote config data.
For the reader, which we are trying to optimize, esp. in the next PR, we would prefer to avoid the expensive full fences.
| let monotonic_published_at_ns = | ||
| monotonic_published_at_ns.max(previous_published_at_ns.saturating_add(1)); | ||
|
|
||
| fence(Ordering::Release); |
There was a problem hiding this comment.
I still kinda suggest always use SeqCst here -- I feel like we're micro-optimizing something that doesn't need to be, and it's so much easier to reason about SeqCst.
| // SAFETY: `header_ptr` is non-null and points to our own process memory at an address | ||
| // we found in /proc/self/maps for `self.pid`. The mapping must be readable if it is | ||
| // listed as the OTel context. | ||
| let header = unsafe { self.header_ptr.as_ref() }; |
There was a problem hiding this comment.
Can we use Self::read_process_memory here? For the same reasons as it's useful to have a safe read of the payload, we can extend the same read to the header and thus not have to assume anything about it.
There was a problem hiding this comment.
We could... and make unpublish() not be unsafe anymore. At that point, however, all the accesses are non-atomic and no behavior can be inferred from the memory model.
If we allow the header to be unpublished with readers active we also risk that random values start being read (e.g. huge sizes, seq always 0). Then there's the two extra syscalls. On the whole, I don't think it's worth it.
There was a problem hiding this comment.
We could... and make unpublish() not be unsafe anymore. At that point, however, all the accesses are non-atomic and no behavior can be inferred from the memory model.
If we allow the header to be unpublished with readers active we also risk that random values start being read (e.g. huge sizes, seq always 0). Then there's the two extra syscalls. On the whole, I don't think it's worth it.
That's true! Yet that's not particularly different than an out-of-process reader might observe ;)
In particular:
- Huge sizes => We can cap the size (and that's probably a good idea here anyway)
- Seq always 0 => This is something that even with the in-process writer might happen, even if it's unlikely
I'll definitely agree that we have implementation flexibility here on how much we want to do. I see this is a continuum:
[In-process reader and writer ] [In-process reader doesn't know anything about the ]
[cooperate fully and know each other] =========================== [writer and behaves exactly as an out-of-process reader ]
^
\--- this pR ;)
The "weird" thing to me is that we're currently half-way between both: we decided to not fully integrate the reader and writer YET we don't fully go "In practice I'm just behaving as an out-of-process reader that happened to be pointed at current pid".
My suggestion is -- let's go full on -- let's drop all assumptions about the writer and make a reader that's fully independent of whatever happens. (To be clear, we don't need to make it read other processes now, yet in this version I envision the actual only missing part to make it work with other processes is replacing the function to do memory reads with one that can read from a remote process)
There was a problem hiding this comment.
I don't think what you said is correct. Neither of these are allowed under the current model -- assuming that the unpublish unsafe contract is respected:
Huge sizes => We can cap the size (and that's probably a good idea here anyway)
We either see 0 (early publish via memfd) or a valid value that was at some point written, even if out-of-date.
Seq always 0 => This is something that even with the in-process writer might happen, even if it's unlikely
We can only see 0 if we're in the process of publishing (again, the early publish case), or we're mid-write.
Although, in practice, because of the signature and because anonymous memory is always provided zeroed (save very extraordinary circumstances), even the out-of-process reader is unlikely to rely on these bad values, as the signature check would fail before -- and even this is unlikely because it would depend on the memory being remapped and written to in the interim between reading the mapping from /proc/.../maps and accessing it.
My suggestion is -- let's go full on -- let's drop all assumptions about the writer and make a reader that's fully independent of whatever happens. (To be clear, we don't need to make it read other processes now, yet in this version I envision the actual only missing part to make it work with other processes is replacing the function to do memory reads with one that can read from a remote process)
Well, as you concede, the out-of-process reader and in-process reader need to be different, since, at the very least, the method of reading the memory is going to differ (we decided to avoid process_vm_readv), and the only valid reasons I see for going that path are:
- avoiding problems with unpublish()
- sharing implementation as much as possible between the in-process and out-of-process readers
Which, at least arguably, are significant advantages, despite the penalty of doing two extra syscalls. However, this is missing what we agreed to do after this PR. We agreed to place the payload next to header if possible. When this happens, we can significantly optimize the in-process reader -- no syscalls, not even a single fence or locked operation is needed on amd64 on the reader path (arm64 still needs a half-fence). So we're already planning to significantly optimize the read path for the in-process reader, and the performance would be an order of magnitude worse if we introduced two syscalls for reading the header.
There was a problem hiding this comment.
Wait, I'm confused again. So if we're going to place the payload after the header, why are we reading with sigpipe at all?
I think now that we have a safe reader, we don't need to do the change to place things after the header -- we're good to go!
I don't think we should be worrying about optimization here -- process context updates are expected to be rare, so I think we should focus on ease of maintenance AND safety, with performance only being a concern last.
This is why I also keep suggesting SeqCst -- I think we're optimizing code that does not need to be optimized.
There was a problem hiding this comment.
Ok, thanks for adding the extra context that I was missing. Let me go bit by bit:
I may be wrong, but I don't think that, in general, we can assume that reads will be rare. For instance, in PHP, the profiler reads the thread local context on every sample, and the keys for its attributes exist in the process context.
This is the key detail I was hoping to uncover and convey. The intention here is that fully the thread context is the thing on the hot path, and the process context is never on the hot path. That's the reason they were separated; if our spec always forced readers to read both all the time, this design would be awful.
The objective here is to fully have the thing that changes very rarely (the dictionary in the process context) stored separately from the thing that changes all the time (the payload in the thread context) so that the thread context read is even more compact -- otherwise we could've just made the thread context keep BOTH keys and values.
The current (unmerged) code in the profiler assumes attributes can only be changed in the direction of adding more of them at the tail, and so only rechecks the process context if it finds a larger number of attributes
You can rely on the "attributes can only be added, not changed". It's in the spec, exactly to allow this optimization.
Since the dictionary keys are capped to one byte, this means at most 255 reads per process, regardless of the lifetime of the process. I'm guessing even with a safe reader we probably are already at thousands, if not millions or more reads per second.
We could also easily imagine an optimization where, in a non-threaded PHP runtime (actually, the most popular choice), we place the env/service in the process context rather than in the thread-local context. Reminder that in PHP service/env can change every request.
Note that the process context is intended to convey things that are true about the whole process. Given that service/env is not true for the whole process for PHP, this information should never go on process context.
There was a problem hiding this comment.
Just a minor point beforehand:
Note that the process context is intended to convey things that are true about the whole process. Given that service/env is not true for the whole process for PHP, this information should never go on process context.
In non-threaded builds of PHP (e.g. as used in php-FPM), the service/env can only by true about the whole process, because the whole processes consists of one PHP thread.
But to the larger point: I was under the impression that the whole reason you, Bob, and Levi had discussed further optimizations like the payload being placed alongside the header was to optimize the read path. If the performance of the read path is irrelevant, then why even bother? Yes, in that case I agree we might as well have only one implementation
There was a problem hiding this comment.
In non-threaded builds of PHP (e.g. as used in php-FPM), the service/env can only by true about the whole process, because the whole processes consists of one PHP thread.
Ack! If we are able to detect this one, I'd say for this one it would be really nice to have if we'd still put this info in the process context.
But to the larger point: I was under the impression that the whole reason you, Bob, and Levi had discussed further optimizations like the payload being placed alongside the header was to optimize the read path.
Apologies for the confusion. We did discuss some of those things, but only insofar as possible simplifications, not "let's do all of them, all the time".
In the scope of this PR my suggestion to move forward is -- let's do the smallest, safest and simplest thing possible and then later we can pile more stuff on if needed. In practice I would suggest we keep the safe reader + also do safe read on the header and then call it a day.
On the writer side, I still advocate for using SeqCst as a simplification and not trying to micro-optimize that code -- it's not that we can't get it correct, it's that micro-optimizations here are very brittle: we make it fully correct today and reason about it from first principles and then tomorrow someone opens an innocent-looking PR that then breaks our assumptions in some subtle way.
Thus, for our own sanity, I'd keep the writer and reader as simple and no-sharp-edges as possible.
There was a problem hiding this comment.
OK, I've pushed the suggested pessimization. The situation is a bit worse than I initially let on; in fact, we need to read the header (or parts thereof) at three separate times, which means 6 syscalls (plus the pipe creation for the first time).
I can't say really say that I agree with these changes: 6/7 extra syscalls is a lot, plus two unnecessary mfence for no compelling reason. I don't really find the brittleness argument very persuasive -- any lockless algorithm needs careful verification, preferably formal checks with something like dat3m. In the new code, which tracks the spec, all 4 fences are necessary and need to be in the correct place. Seems about as easy to screw up a change to this code.
There was a problem hiding this comment.
I really like the current version, thanks for humoring me 😅.
| let published_at = header.monotonic_published_at_ns.load(Ordering::Acquire); | ||
| if published_at == UNPUBLISHED_OR_UPDATING { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::WouldBlock, | ||
| "process context is currently being updated", | ||
| )); | ||
| } | ||
|
|
||
| // `signature` and `version` are initialized before the release store that publishes | ||
| // a non-zero timestamp. If the acquire load above observed that timestamp, those | ||
| // writes are visible; if it observed `UNPUBLISHED_OR_UPDATING`, we returned before | ||
| // reading them. Updates never mutate these fields, so their accesses are race-free. | ||
| // The seqlock-controlled fields must be loaded atomically because they can change | ||
| // during an update. | ||
| let signature = header.signature; | ||
| let version = header.version; | ||
|
|
||
| if signature != *SIGNATURE { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| "invalid signature in process context mapping", | ||
| )); | ||
| } | ||
| if version != PROCESS_CTX_VERSION { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| format!("unsupported process context version {version}"), | ||
| )); | ||
| } | ||
|
|
||
| let payload_size = header.payload_size.load(Ordering::Relaxed); | ||
| let payload_ptr = header.payload_ptr.load(Ordering::Relaxed).cast_const(); | ||
|
|
||
| if payload_ptr.is_null() { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| "process context payload pointer is null", | ||
| )); | ||
| } | ||
|
|
||
| let payload_bytes = | ||
| Self::read_process_memory(self.pid, payload_ptr, payload_size as usize)?; | ||
|
|
||
| // pairs with the first release fence on update() to ensure that, if we read data | ||
| // updated after the initial published time, we at least see the published | ||
| // time being set to 0 in the next load of the published time (or we could | ||
| // see a later time rather than 0) | ||
| fence(Ordering::Acquire); |
There was a problem hiding this comment.
My usual note about SeqCst applies -- I think we should use the strongest level, rather than trying to micro-optimize with Acquire as this is not going to be called often
cataphract
force-pushed
the
levi/otel-thread-context
branch
from
404a1eb to
216c87b
Compare
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
This looks really good! I've left some very small comments, but TL;DR it's only the "reading thread attributes" and the incorrect test about the capacity that held me back from pressing approve.
Since we've been through a few changes I'll ping @yannham for a final rusty pass, since my rust-fu isn't amazing.
| /// Header fields intentionally use the plain C layout types specified by OTel. Same-process | ||
| /// readers copy the header through [`ProcessContextSelfReader`]'s pipe-based reader instead of | ||
| /// dereferencing this mapping directly. |
There was a problem hiding this comment.
Minor: I'm not sure this comment is very useful -- in particular, the writer doesn't really need to care how the reader does its safety magic (e.g. "pipe").
At most we could mention here that any reader "needs to take care of its own safety and that we have no mechanisms to synchronize with it beyond those in the spec" (or something along those lines), but no more.
| /// Header fields intentionally use the plain C layout types specified by OTel. Same-process | |
| /// readers copy the header through [`ProcessContextSelfReader`]'s pipe-based reader instead of | |
| /// dereferencing this mapping directly. |
| .or_else(|_| try_memfd(MAPPING_NAME, libc::MFD_CLOEXEC | libc::MFD_ALLOW_SEALING)) | ||
| .and_then(|fd| { | ||
| // Safety: fd is a valid open file descriptor. | ||
| // SAFETY: fd is a valid open file descriptor. |
There was a problem hiding this comment.
I don't think so? We keep the fd open -- I think the "TOCTTOU" issues are exactly caused by situations where the "something checked" isn't directly handed to the next consumer, and instead the next consumer repeats some logic (such as opening a file again).
| /// | ||
| /// # Safety | ||
| /// | ||
| /// This function may only be called if it can be guaranteed that there are no in-process | ||
| /// readers, or at least that they will not be used after the call. |
There was a problem hiding this comment.
Minor: I guess with the safe reader this comment is no longer needed?
| /// | |
| /// # Safety | |
| /// | |
| /// This function may only be called if it can be guaranteed that there are no in-process | |
| /// readers, or at least that they will not be used after the call. |
| .monotonic_published_at_ns | ||
| .store(UNPUBLISHED_OR_UPDATING, Ordering::Relaxed); | ||
| } | ||
| fence(Ordering::Release); |
There was a problem hiding this comment.
+1 I think this one could be strenghtened as well
| fence(Ordering::Release); | |
| fence(Ordering::SeqCst); |
| assert!(offset_of!(MappingHeaderSnapshot, signature) == 0); | ||
| assert!(offset_of!(MappingHeaderSnapshot, version) == 8); | ||
| assert!(offset_of!(MappingHeaderSnapshot, payload_size) == 12); | ||
| assert!(offset_of!(MappingHeaderSnapshot, monotonic_published_at_ns) == 16); | ||
| assert!(offset_of!(MappingHeaderSnapshot, payload_ptr) == 24); | ||
| assert!(size_of::<MappingHeaderSnapshot>() == size_of::<super::MappingHeader>()); | ||
| assert!(align_of::<MappingHeaderSnapshot>() == align_of::<super::MappingHeader>()); | ||
| }; |
There was a problem hiding this comment.
Minor: I think these checks are redundant? In particular:
- We already check the offsets on
MappingHeader - If the offsets between
MappingHeaderSnapshotandMappingHeaderdeviate, the tests below will end up failing anyway
So I think we could simplify a bit and get rid of these
| /// forked. After a `fork()`, reads fail and a new reader must be constructed. | ||
| pub struct ProcessContextSelfReader { | ||
| pid: libc::pid_t, | ||
| header_ptr: NonNull<MappingHeader>, |
There was a problem hiding this comment.
Minor: More of as a meta design note, should we avoid using the type MappingHeader here?
E.g. this type slightly makes it look like you can read it directly, whereas in practice this should be treated as a magic cookie that can somehow be passed into read_process_memory but otherwise has no meaning outside of that call.
Only really read_process_memory needs to know what a pointer is and that this is a pointer, everything else could be opaque to this implementation detail.
There was a problem hiding this comment.
It's not very important, but I disagree. It makes for useless conversions: if we always use it as a MappingHeader pointer, why not encode it in the type system? Note that the header_ptr field is not public, even if the struct is (you'd need pub header_ptr: ... for that), so nobody outside of this module can see/access it.
There was a problem hiding this comment.
I don't think it makes much difference, but I think the point is that it makes it harder to directly dereference the pointer (which we don't want) -- you need to cast it.
There was a problem hiding this comment.
Yup, that's what I meant -- MappingHeader invites direct usage, whereas my suggestion is that the type should be an opaque one (but it can still be named)
| // SAFETY: the loop exits with `offset == len`, and every byte of `buf[..offset]` was | ||
| // initialized by the pipe reads above. | ||
| unsafe { buf.set_len(len) }; |
There was a problem hiding this comment.
Minor: I think this could be regular safe code? It's kinda weird to see unsafe code around "reading bytes from an fd into a data structure"
There was a problem hiding this comment.
I'd rather leave it as is in order not to create an asymmetry with the write code. You'd need to store the read fd as a File instead.
There was a problem hiding this comment.
Lol I'll categorize this under "rust is weird and I'm not sure I'll ever understand it" ;D
There was a problem hiding this comment.
We are already doing a bunch of syscalls to read memory, so it's not like we're micro-optimizing yet. Why not just initialize the buffer with zeros and avoid messing with Vec internals? It's probably almost free to get a zeroed buffer anyway these days
There was a problem hiding this comment.
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).
| // SAFETY: `header_ptr` is non-null and points to our own process memory at an address | ||
| // we found in /proc/self/maps for `self.pid`. The mapping must be readable if it is | ||
| // listed as the OTel context. | ||
| let header = unsafe { self.header_ptr.as_ref() }; |
There was a problem hiding this comment.
I really like the current version, thanks for humoring me 😅.
|
@ivoanjo I believe I've addresses all your comments, plus I added a bit of a defensive commit on the |
cataphract
force-pushed
the
levi/otel-thread-context
branch
from
31057ba to
bc04dd8
Compare
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
👍 LGTM, looks great, no concerns!
I ping'd Yann for a second pass, but I wouldn't consider it blocking (e.g. if there's any suggestions, we can address them as a follow-up rather than blocking this one)
cataphract
force-pushed
the
levi/otel-thread-context
branch
from
bc04dd8 to
941a036
Compare
cataphract
force-pushed
the
levi/otel-thread-context
branch
from
941a036 to
62dd744
Compare
There was a problem hiding this comment.
We can't get rid of all atomics and expect fences to work. There's a very specific warning in the Rust documentation, which mirrors one in the C++20 memory model: https://doc.rust-lang.org/std/sync/atomic/fn.fence.html#mandatory-atomic. Whether a compiler is sufficiently smart to know if there are no atomics around ever is another question, but I won't take that risk. On paper, if the compiler decides the fence is not paired with any atomic, it could theoretically get rid of it entirely. Fences can't enforce synchronization with only normal or volatile memory accesses.
| payload_ptr: ptr::with_exposed_provenance(payload_addr), | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: add a short documentation to explain what this function does.
| let pipe = match self.pipe.take() { | ||
| Some(pipe) => pipe, | ||
| None => CopyPipe::new()?, | ||
| }; |
There was a problem hiding this comment.
| let pipe = match self.pipe.take() { | |
| Some(pipe) => pipe, | |
| None => CopyPipe::new()?, | |
| }; | |
| let pipe = self.pipe.take().unwrap_or_else(CopyPipe::new); |
There was a problem hiding this comment.
Doesn't exactly work because CopyPipe::new returns a Result
| // Draining exactly `nbytes` every iteration also keeps the pipe empty before each | ||
| // write, so `chunk_len <= capacity` should leave every write ready immediately. | ||
| let mut drained = 0; | ||
| while drained < nbytes { |
There was a problem hiding this comment.
Do you expect the signal situation to happen in practice? I would have just considered drained < nbytes an error , and trigger a new read from scratch, instead of having a codepath for this.
There was a problem hiding this comment.
How likely it is depends mostly on the application where the reader is inserted, so better be safe. Looping on EINTR is fairly standard.
There was a problem hiding this comment.
Actually multiple of our profilers cause EINTR so handling it is very very important.
| libc::PR_SET_VMA, | ||
| libc::PR_SET_VMA_ANON_NAME as libc::c_ulong, | ||
| self.start_addr as libc::c_ulong, | ||
| self.start_addr.as_ptr() as libc::c_ulong, |
There was a problem hiding this comment.
Another possibility would be to use try_from and error out/panic/whatever. For platform when everything is fine it should be optimized to a no-op
| // | ||
| // To do so, we implement synchronization during publication _as if the reader were | ||
| // another thread of this program_, using atomics and fences. | ||
| ptr::addr_of_mut!((*header).signature).write(*SIGNATURE); |
There was a problem hiding this comment.
Out of curiosity, why depart from the original "write-everything-at-once" approach and use addr_of_mut instead? FWIW. Is it just to avoid the initial write of zero to monotonic_published_at?
There was a problem hiding this comment.
It's generally much safer to use ptr::read and ptr::write than to convert into a reference. If you do the latter, you need to uphold many more invariants: for instance, if there is a mut (exclusive reference) already, there can be no other, no references to uninitialized memory, provenance is only OK if it's the pointer returned by mmap, not some offset therein, etc. No need to reason about any of this with ptr::write.
There was a problem hiding this comment.
I don't think we materialized any reference in the previous code, did we? I'm really talking about a single ptr::write(MappingHeader { ...}) of the whole structure at once vs multiple ptr::write.
I think the only theoretical difference is that ptr::write of the whole structure requires the base address to be aligned, while ptr::write on each fields only require the written field address to be aligned, but...in practice I can't see a situation where the whole structure would be unaligned but by miracle all the fields would be 🤷
Another difference could be that if rustc is really stupid it would materialize the structure on the stack first, but I would be very upset if that was the case in 2026 for a direct ptr::write call 😛
There was a problem hiding this comment.
ah sorry, I thought it wrote through a dereference of a &mut header. Yes, it's the same (in an optimized build) but for the redundant write of the monotonic_published_at_n
|
(Once again I'm not sure why the publisher has changed fundamentally since the last review - ideally the reader could be implemented without touching the existing publisher at all, especially since the publisher changes are internal here) |
👀 Let me sync with Yann! |
|
Yup, let's sync yeah! In the meantime, maybe it wasn't clear enough, sorry for the confusion if that's the case: I'm talking only about the publisher side (the reader is fine), and the fact that unless I'm mistaken, there are no atomic accesses remaining there. There used to be (before this PR) at least |
|
@yannham I see. For what it's worth, I don't think the atomic accesses that existed before solve anything. Seqcst writes/reads on But anyway, yes, I overcorrected after Ivo's requested changes. I brought back the atomics, except now they're all relaxed given the fences do all the work instead. |
cataphract
force-pushed
the
levi/otel-thread-context
branch
from
4b33043 to
246559e
Compare
6 participants
Add
read(),threadlocal_attribute_key_map(), andread_threadlocal_attribute_key_map()tootel_process_ctx::linux, along with thefind_otel_mapping()/is_named_otel_mapping()/parse_mapping_start().What does this PR do?
A brief description of the change being made with this pull request.
Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.
BREAKING CHANGE: unpublish becomes unsafe.