Switch from bpf_get_stackid() to bpf_get_stack()#1264
Switch from bpf_get_stackid() to bpf_get_stack()#1264fabled merged 2 commits intoopen-telemetry:mainfrom
Conversation
f8cfa7e to
a55ceb2
Compare
bobrik
left a comment
There was a problem hiding this comment.
I saw two approaches to this:
- Save the kernel stack as an array of u64 addresses and do specialized parsing in userspace. It's kind of similar to how stack id was turned into actual frames.
- Produce proper kernel frames and do no specialized parsing in userspace. Kernel is just another kind of an interpreter.
I think the second approach is nicer, so I opted for that. I can be persuaded otherwise.
This doesn't work on v5.4. I'm not sure what the status of dropping support for that.
I have this running in production and it looks right compared to #1257:
fabled
left a comment
There was a problem hiding this comment.
Thanks for working on this! Added some comments on the frame data format.
When one kernel fails, it cancels all the other tests, making it harder to see where the fault line lies. Let's run them all to completion instead.
abcaca7 to
dcf9a97
Compare
|
I updated to push kernel frames as a list of u64 at the beginning of I'll deploy this internally to make sure everything looks right. |
fabled
left a comment
There was a problem hiding this comment.
Thanks so much! Looks very neat and clean. Few minor clean up comments still.
To use `bpf_get_stack()` we need a newer kernel.
Getting rid of `bpf_get_stackid()` also lets us get rid of the 16MiB map:
```
$ sudo bpftool map show name kernel_stackmap
31557211: stack_trace name kernel_stackmap flags 0x0
key 4B value 1016B max_entries 16384 memlock 17039648B
pids ebpf-profiler(3973147)
```
Plus the costs of reading the map for every sample with kernel compoments.
Instead of saving the stack id into the map, we now save the actual
kernel frames into the same list of frames where all other frames go.
The format is different though: it's just addresses. That way it's
easier on the bpf verifier.
dcf9a97 to
2d22cfa
Compare
|
I incorporated all 3 suggestions. |
fabled
left a comment
There was a problem hiding this comment.
Thanks! lgtm! @florianl @christos68k can either of you take a second review?
florianl
left a comment
There was a problem hiding this comment.
Works fine and thanks for the work!
To use
bpf_get_stack()we need a newer kernel.Getting rid of
bpf_get_stackid()also lets us get rid of the 16MiB map:Plus the costs of reading the map for every sample with kernel compoments.
Instead of saving the stack id into the map, we now save the actual kernel frames into the same list of frames where all other frames go.
See: #1257.