feat(heap-profiling): add initial heap profiling primitives [PROF-15190]#2166
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: aeb870c | Docs | Datadog PR Page | Give us feedback! |
b6aab83 to
72d9f77
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. |
b80ec4a to
11cf066
Compare
11cf066 to
f40cfdf
Compare
12cd59a to
fb19cfa
Compare
yannham
left a comment
There was a problem hiding this comment.
Partial review of libdd-heap-gotter
bebaa38 to
743576b
Compare
0bc2edd to
37c4b52
Compare
r1viollet
left a comment
There was a problem hiding this comment.
LGTM
The doc is great, tests are great, overhead looks good, architecture has been discussed in advance. I can't possibly audit all lines, though AI reviews have already done a good job.
I know this is a big one, and I expect we will be iterating on this code in following weeks.
OK to merge this first version
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
danielsn
left a comment
There was a problem hiding this comment.
Was half way through a review, here is what I had so far
…90] (#2166) # What does this PR do? Adds initial support for eBPF heap profiling to libdatadog for the [ebpf heap profiling project](https://docs.google.com/document/d/1cy6OUisjW4_vAIaAu9G5l3RGuPPLT_MRZuhDPJHxXuw/edit?tab=t.mtwnx5ijdb84). These changes provide the ability for libdatadog users to introduce a USDT into consuming applications allocation and deallocation paths which is fired behind a poisson distributed sampling path every ~0.5 MiB of allocation. This will give us something to hook in user applications from the eBPF side to sample allocations for allocation tracking and sample allocations+frees for live heap. I expect we will make more changes to this as we go along, but as this is a standalone feature and is largely based on prior work in ddprof it would be nice to get things in now. A brief description of the change being made with this pull request. ## Interesting Details * I have vendored `usdt.h`; it is not consistently available on all our build environments (read: old alpine) and this seems to be the common pattern. The API is stable and I see no concerns with this and have updated the license stuff appropriately * I have gated the binding generation behind an env var and committed the bindings. This is because the generation imposes a fair bit of extra _stuff_ (llvm and supporting bits) on _every_ build job and developer. I've added a CI job to validate that the bindings generated reflect what's checked in to avoid the footgun. # Motivation An enthusiasm for heap profiling # Additional Notes Anything else we should know when reviewing? * [PoC-y branch for the full host profiler with heap profiler](https://github.com/DataDog/opentelemetry-ebpf-profiler/tree/sgg/heap-prof-poc) * The profiling backend + frontend are done; you can see heap profiles pushed through using this infrastructure in [staging](https://dd.datad0g.com/profiling/explorer?query=service%3A%2Ausdt%2A&agg_m=%40prof_ebpf_cpu_cores&agg_m_source=base&agg_t=sum&extra_search_fields=%7B%22filters_query%22%3A%22%22%2C%22sample_type%22%3A%22cpu-time%22%7D&my_code=enabled&profile_type=ebpf-alloc-size&profiling-flame-graph__expanded_groups=3269946435%2C4099416842&refresh_mode=paused&viz=flame_graph&zoom=2156769548&from_ts=1782885547816&to_ts=1782889147816&live=false) # How to test the change? Describe here in detail how the change can be validated. Co-authored-by: scott.gerring <scott.gerring@datadoghq.com> Signed-off-by: Taegyun Kim <taegyun.kim@datadoghq.com>
|
|
||
| void *header = (char *)user - DD_HEADER_BYTES; | ||
| uint64_t magic; | ||
| memcpy(&magic, header, sizeof(magic)); |
There was a problem hiding this comment.
why do we need the memcpy vs just looking directly at the bytes?
There was a problem hiding this comment.
It's the UB-free way to read a typed value out of raw bytes. Casting header to uint64_t * and dereferencing would assume header is correctly aligned, and I believe also runs afoul of C's strict aliasing rules. memcpy sidesteps both, and for a fixed 8-byte copy I think it's usually lowered to a plain load, so there shouldn't be any meaningful runtime cost.
| /* | ||
| * arm64: TBI (Top Byte Ignore) pointer tag. | ||
| * | ||
| * ARMv8 userspace ignores bits 56..63 of a virtual address, so we stash |
There was a problem hiding this comment.
what happens if someone else wanted to use the tags too?
There was a problem hiding this comment.
We turn ourselves off. The hope is that this doesn't happen much (memory tagging not being useful on amd64-linux because the kernel actively removed support for it hopefully helps folk to not want to use it).
Again here we might need some mechanism for logging so we test this hypothesis. We've talked about having a diagnostic uprobe we can fire - so you'd say otel_heap_diagnostic(0x123) with some magic number that lets us communicate particular states back to the profiler. This frees us of having to log within the user's apps, but still lets us aggregate signal on error cases.
WDYT?
| return false; | ||
| } | ||
|
|
||
| uint64_t offset; |
There was a problem hiding this comment.
have we explored the tradeoff between keeping an offset for every alloc, vs having a hashtable for when magic matches?
There was a problem hiding this comment.
We talked about it a bit - there's a writeup over here.
Reasons we like this one:
- It's reasonably efficient - we're burning something a bit more than 16 bytes (cuz of wild alignments) per sampled allocation (so on avg. every 512 KiB), which gets us to around 32 KiB per GiB allocated
- We have good locality
- We don't have to think about how and if to statically size the table and what happens when it is full
- We don't have the cost of indirection through the hash table (although this is less of a big deal as this is only on the cold path through free once we know we are sampled)
| When we get a realloc, we treat it as a separate free and a new allocation | ||
| to keep things simple. This means a resized allocation that was sampled | ||
| before the resize is not sampled after it: we report the free and just let | ||
| the new block go untracked, rather than trying to carry the sample |
There was a problem hiding this comment.
why not check the threshold as if it was a new allocation?
There was a problem hiding this comment.
We could, but two issues. First the mechanical one the doc mentions: sampling the new block needs a fresh header stamped with the original user-requested size, which we don't store. The bigger one is byte accounting, the sampler is Poisson-over-bytes and dd_allocation_requested(size) decrements the per-thread counter by size. A realloc's genuinely-new memory is only the growth delta; the carried-forward bytes were already counted at the original allocation. Running the full threshold on new_size would double-count them and bias the size estimator, which is why the unsampled realloc path stays a pure passthrough that never touches the counter. Given that we suspect that realloc-of-sampled-block is rare, we've deferred trying to handle this for now.
| is done through `prctl(PR_SET_TAGGED_ADDR_CTRL)` in | ||
| `dd_sample_flag_thread_init`, which must run once per thread before any | ||
| tagging happens on that thread. If the kernel refuses (older kernels, | ||
| seccomp policies, etc.) we disable sampling entirely on that thread. We might |
There was a problem hiding this comment.
In what case would it succeed for one thread and not another?
There was a problem hiding this comment.
In practice it usually wouldn't. All I can think of for realistic per-thread divergence is a thread that installed its own restrictive seccomp filter blocking prctl or something wildly inappropriate happening with eBPF. So the per-thread handling is really just conservative, we do it per thread because the ABI control is per-thread, not because we expect the result to differ.
| The header holds two 8-byte values: | ||
|
|
||
| * a magic constant (`DD_MAGIC`) | ||
| * the offset from the user pointer back to the raw pointer the allocator |
There was a problem hiding this comment.
why is this needed, isn't the header fixed sized?
There was a problem hiding this comment.
The header is fixed-size (16 bytes, always right before the user pointer, which is how we locate it), but the distance from the user pointer back to the raw allocator pointer isn't fixed. For alignment > 16, or when we bump the user pointer forward to avoid landing within 16 bytes of a page boundary (see the Page Alignment section below), raw ends up further back than 16 bytes. The offset records that variable distance so free can recover the real base to hand to the allocator. Without it we'd only be able to handle the alignment <= 16, no-page-bump case.
| (`x86_apply` in `sample_flag.h`), and when we work out how big the original | ||
| allocation was so we can free the right amount (`dd_allocation_freed_slow` | ||
| in `allocation_freed.c`). If any of these disagree we corrupt memory. Touch | ||
| one, check the other two. |
There was a problem hiding this comment.
can't it be a shared function in the header?
There was a problem hiding this comment.
makes sense; i'll see if i can refactor this nicely in the follow-up PR
| /* Always close the reentry guard, even on allocation failure (raw == NULL), | ||
| * so the thread isn't permanently locked out of sampling. */ | ||
| dd_tl_state_t *tl = dd_tl_state_get(); | ||
| if (tl) tl->reentry_guard = false; |
There was a problem hiding this comment.
under what conditions would that happen? Should we assert in debug cases? Should we assert the reentry_guard was true before?
There was a problem hiding this comment.
Good idea. To answer the "under what conditions": on a correctly-paired call, never, we only reach the slow path on a sampled request (weight > 0), which means dd_allocation_requested_slow already initialised the thread's TLS and set the guard. So tl non-null and reentry_guard == true both hold; a failure means created was called without a matching requested. Added debug asserts for both.
|
|
||
| #if defined(__x86_64__) | ||
| /* Recover the bumped size the allocator actually holds. This must | ||
| * exactly mirror allocation_requested.c's bumped_alloc_size(): |
There was a problem hiding this comment.
should this be a shared inline function then?
There was a problem hiding this comment.
Good point, same idea as the other thread, I'll pull the size formula into a shared inline in the follow-up PR.
| }; | ||
|
|
||
| if (old_user == NULL) { | ||
| dd_alloc_req_t req = dd_allocation_requested( |
There was a problem hiding this comment.
it would be clearer to just create and return the properly initilized out in each branch rather than making it mutable
| void *hdr = (char *)old_user - DD_HEADER_BYTES; | ||
| uint64_t m = DD_MAGIC; | ||
| uint64_t o = (uint64_t)old_offset; | ||
| memcpy(hdr, &m, sizeof(m)); |
There was a problem hiding this comment.
Would it be clearer to have a header struct, and copy the thing rather than two seperate fields?
…2226) # What does this PR do? Clarifies a bunch of stuff that was not obvious, and improves testing, addressing feedback from @danielsn on #2166 # 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. Co-authored-by: scott.gerring <scott.gerring@datadoghq.com>
What does this PR do?
Adds initial support for eBPF heap profiling to libdatadog for the ebpf heap profiling project. These changes provide the ability for libdatadog users to introduce a USDT into consuming applications allocation and deallocation paths which is fired behind a poisson distributed sampling path every ~0.5 MiB of allocation. This will give us something to hook in user applications from the eBPF side to sample allocations for allocation tracking and sample allocations+frees for live heap.
I expect we will make more changes to this as we go along, but as this is a standalone feature and is largely based on prior work in ddprof it would be nice to get things in now.
A brief description of the change being made with this pull request.
Interesting Details
usdt.h; it is not consistently available on all our build environments (read: old alpine) and this seems to be the common pattern. The API is stable and I see no concerns with this and have updated the license stuff appropriatelyMotivation
An enthusiasm for heap profiling
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.