Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

info: expose ksym info and func info by ProgramInfo #1576

Merged
merged 4 commits into from
Oct 9, 2024

Conversation

Asphaltt
Copy link
Contributor

@Asphaltt Asphaltt commented Oct 3, 2024

In pwru, I'm planning to improve the tracing info of bpf prog by mapping ksym addresses to bpf prog info, i.e. the function name of subprograms.

Therefore, it requires more prog info exposed from ProgramInfo, including ksym info and func info.

bpf_prog_info supports ksym info since commit torvalds/linux@dbecd7388476 ("bpf: get kernel symbol addresses via syscall"), and supports func info since commit torvalds/linux@838e96904ff3 ("bpf: Introduce bpf_func_info").

@Asphaltt Asphaltt requested a review from a team as a code owner October 3, 2024 14:08
@Asphaltt Asphaltt force-pushed the feature/prog_info branch 3 times, most recently from 03e5d62 to 8a66787 Compare October 4, 2024 13:09
Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch! Left a few comments.

info.go Outdated Show resolved Hide resolved
info.go Outdated Show resolved Hide resolved
info.go Outdated Show resolved Hide resolved
@ti-mo
Copy link
Collaborator

ti-mo commented Oct 8, 2024

(By the way, a common format for referring to a commit in Linux is dbecd7388476 ("bpf: get kernel symbol addresses via syscall") or torvalds/linux@dbecd7388476 ("bpf: get kernel symbol addresses via syscall") to make it clickable on GH.)

ti-mo and others added 4 commits October 9, 2024 11:46
This commit renames btf.FuncInfos to btf.FuncOffsets and makes its contents
(now a btf.FuncOffset) accessible to external packages. As a result,
LoadFuncInfos now returns FuncOffsets instead of FuncInfos.

LoadFuncInfos has always been for internal use only, since the contents of
btf.FuncInfos were not externally accessible. A follow-up commit will add
a ProgramInfo method that returns a btf.FuncOffsets, making this more widely
accessible.

Signed-off-by: Timo Beckers <timo@isovalent.com>
This removes the use of package unsafe in info.go. A follow-up commit will
use NewSlicePointer in a similar way.

Signed-off-by: Timo Beckers <timo@isovalent.com>
Exposes additional metadata fields from `bpf_prog_info` to `ProgramInfo`:
- nr_jited_ksyms
- jited_ksyms

Meanwhile, expose ksym addresses from ProgramInfo.

`bpf_prog_info` supports ksym info since commit
torvalds/linux@dbecd7388476 ("bpf: get kernel symbol addresses via syscall").

Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
`bpf_prog_info` supports func info since commit
torvalds/linux@838e96904ff3 ("bpf: Introduce bpf_func_info").

Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
Co-authored-by: Timo Beckers <timo@isovalent.com>
@ti-mo ti-mo force-pushed the feature/prog_info branch from 813cc7a to deb3885 Compare October 9, 2024 10:35
@ti-mo ti-mo requested a review from dylandreimerink as a code owner October 9, 2024 10:35
@github-actions github-actions bot added the breaking-change Changes exported API label Oct 9, 2024
@ti-mo
Copy link
Collaborator

ti-mo commented Oct 9, 2024

@Asphaltt I ended up making a few changes after I noticed we already had most of the code for parsing funcinfos in btf.LoadFuncInfos. I added two commits: one removing the use of unsafe in info.go, the other to call btf.LoadFuncInfos instead of reinventing the wheel. I also changed the tests to use quicktest.

@dylandreimerink Please have a look. LoadFuncInfos was always mostly for internal use. It could be called, but the resulting object wasn't useful for external callers. There's now a new type called FuncOffset(s) that pairs a raw insn offset with a btf.Func to denote its position. Previously this info was only embedded through ProgramInfo.Instructions(), but this is way too expensive (and complicated) if the caller only cares about subprog signatures and bytecode layout.

Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes all make sense to me

@ti-mo ti-mo merged commit c7441ce into cilium:main Oct 9, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes exported API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants