Reduce BPF memory consumption #620
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #620 +/- ##
==========================================
+ Coverage 79.19% 79.88% +0.69%
==========================================
Files 70 69 -1
Lines 6027 5967 -60
==========================================
- Hits 4773 4767 -6
+ Misses 1023 970 -53
+ Partials 231 230 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -21,7 +21,7 @@ | |||
| // we can share them later if we find is worth not including code per duplicate | |||
| struct { | |||
| __uint(type, BPF_MAP_TYPE_RINGBUF); | |||
| __uint(max_entries, 1 << 24); | |||
| __uint(max_entries, 1 << 16); | |||
There was a problem hiding this comment.
As max_entries are allocated dynamically as long as more space is needed, this should not affect in terms of memory (unless we end up having more than 2^16 events in the queue).
There was a problem hiding this comment.
When I printed BPF metrics, I see the whole ring buffer pre-allocated in locked memory.
Before:
870: ringbuf name events flags 0x0
key 0B value 0B max_entries 16777216 memlock 16,855,384B
After:
1079: ringbuf name events flags 0x0
key 0B value 0B max_entries 65536 memlock 78,424B
There are more issues with what we do. One example is that we'll load two separate programs when we have gin. Once for the gin functions, but we'll load the go http too, because it's there in the binary. So in a sense, we'll duplicate the memory and have 2 ring buffers when we instrument with gin.
Another issue is grpc, golang.org/x/net/http2/hpack.(*Encoder).WriteField belongs to the gosdk, so we load both http and grpc for grpc only applications, again two ring buffers, all maps are duplicate :(.
bpf/watch_helper.c
Outdated
| @@ -18,7 +18,7 @@ const watch_info_t *unused_2 __attribute__((unused)); | |||
|
|
|||
| struct { | |||
| __uint(type, BPF_MAP_TYPE_RINGBUF); | |||
| __uint(max_entries, 1 << 24); | |||
| __uint(max_entries, 1 << 8); | |||
There was a problem hiding this comment.
The minimum size should be multiple of 4096, according to this: https://stackoverflow.com/questions/63415220/bpf-ring-buffer-invalid-argument-22
There was a problem hiding this comment.
Interesting. I will increase it, although even with 256 I see the events flowing...
There was a problem hiding this comment.
Ok! So maybe the reference link is wrong. The documentation doesn't say anything but "must be power of 2".
Yes you are right, we do have 2 separate issues. I have to check if the container metrics report the kernel locked memory per container in the container memory or it's only per node. |
grcevski
changed the title
| &nethttp.GinTracer{Tracer: *nethttp.New(&cfg.EBPF, metrics)}, | ||
| grpc.New(&cfg.EBPF, metrics), | ||
| goruntime.New(&cfg.EBPF, metrics), | ||
| gosql.New(&cfg.EBPF, metrics), |
There was a problem hiding this comment.
Did you forget to re-add gosql?
There was a problem hiding this comment.
I removed it temporarily, the whole separate file and EBPF program, because it required more headers refactor in the BPF side, so we don't include collections we don't need twice. I'm going to follow-up with a PR to split that off again after I clean up the headers more.
|
Here's some data about the memory size reductions, really demonstrated with our Go testserver app, where we bundle Go HTTP, Gin and gRPC in one: All of this is output from Before: Total: 109,602,592 B, or 104 MB After: Total: 6,769,216 B, or 6.4 MB |
This PR refactors how we manage the ringbuffer and other data structures. Upon review of our kernel memory usage, there was a lot of unnecessary allocation going on. Each bpf program had it's own ringbuffer of 16MB and we used to include headers which accidentally brought in extra unused maps.
I think there's still some technical debt left in how the tracing headers are structured on the bpf side, it's a relic from a lot of experimentation and I'll follow-up with a PR to do further clean-up.
I also combined the SQL into the core http. Seems to be loaded always because the symbols exist in the binary and it duplicates some additional maps. I'm going to separate it out again with a new refactor, such that we include only minimum headers and not duplicate data structures.