Refactor libpf.Frame mappings #749
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
florianl
left a comment
florianl
left a comment
There was a problem hiding this comment.
just a first impression. will have a look again.
| if i >= 0 { | ||
| entry := &maps[i] | ||
| fm := entry.FrameMapping.Value() | ||
| f := fm.File.Value() |
There was a problem hiding this comment.
What about the case, where fm.Valid() returns false?
There was a problem hiding this comment.
It is not possible. Only valid entries are inserted to this slice.
- use unique FrameMappingData to reduce the Frame size - construct FrameMappingData directly from the cached ELF info - remove the FileID LRU cache as unneeded - replace processmnanager.processInfo.mappings map with a sorted slice of the []Mappings - atomically do batch updates of the []Mappings - remove processmanager.processInfo.mappingsByFileID as redudant TODO: - eim should probably get libpf.FrameMappingFile instead of host.FileID - eventually move processmanager.Mapping to interpreters and pass it as the mapping type
fabled
force-pushed
the
tt-refactor-mappings-metadata
branch
from
7887bbf to
b5172f9
Compare
|
Rebased on top of master. There was a number of quite conflicting changes that made the merge difficult / not so useful. I ended up doing multiple rebases to resolve the conflicts. |
|
@florianl @christos68k Could you do a review on this when you get some time? Thanks! |
- use slices.SortFunc - add mapping.Length to processmanager Mapping - move sorting and publishing of changed mappings *after* the handling of the ebpf modifications -> the sort reordered things and mpAdd pointers point to wrong data - don't hold lock during interpreter synchronize mappings call
christos68k
left a comment
christos68k
left a comment
There was a problem hiding this comment.
Did one pass, will also run some local tests
|
I spent a few hours today running local tests and found a regression around main thread exit. You should be able to reproduce using main_thread_exit.c. Here's how The thread exit is detected and mappings are extracted, once, successfully from the new thread. This branch detects thread exit but keeps trying to extract mappings which doesn't seem to work: In devfiler (old/new): 
I ran out of time today but I'll find some time next week to look deeper. Also have a few more edge cases to test. @korniltsev: Since you previously reported the main thread exit edge case, if you also have interesting edge cases / workloads, feel free to test this too. |
Thanks. Seems
Thanks! |
stdout is line cached. its glibc extension that reading stdin flushes stdout, but its not standard. fix test case to explicitly flush stdout.
3 participants
processmanager/manager_test.go, the tests were about implementation, not functionality; and usually caused more trouble than benefit. coredump and integration tests cover the same area.Follow up todo: