-
Notifications
You must be signed in to change notification settings - Fork 770
add StructOpsMap support #1845
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
base: main
Are you sure you want to change the base?
add StructOpsMap support #1845
Conversation
7b8a348
to
f7a32d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is missing the part where program fds are inserted into the struct ops map. Without that it's hard to tell what is going on.
@lmb yeah, agreed. |
vimto tests on 6.1 and 6.6 has passed on my setup, so I feel this is a temporary error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a good look at the PR and there are some high level problems we need to solve:
- Too much metadata which is kept on the side and not in the right places. This makes the logic hard to follow and doesn't integrate well with the rest of the code. The solution is to duplicate some metadata by storing type name and member in ProgramSpec.AttachTo. We also need to store the mapping from MapSpec.Value.Member[].Name to program somehow. For now I think we can get away with setting ProgramSpec.Name to MapSpec.Value.Member[].Name.
- There is a lot of duplication in helper functions that do unconventional stuff with btf types. Most of these should go. Comparisons between types to find the equivalent should always be using the concrete type and name, not offset or index.
map.go
Outdated
} | ||
|
||
var b btf.Builder | ||
h, err := btf.NewHandle(&b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why create an empty BTF blob here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This question is still unanswered.
4126573
to
0e82c52
Compare
@shun159 please ping me explicitly when you need another review. |
Hi @lmb I believe I've addressed most of your feedbacks earlier. https://github.com/cilium/ebpf/actions/runs/17712680837/job/50333512472?pr=1845 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! There are a couple of comments which you didn't address. Please go through the conversations and resolve them if you've actioned them.
To fix the CI breakage: for now it's ok to set AttachTo = ""
if were dealing with a struct ops program in TestLibBPFCompat.
struct_ops_test.go
Outdated
} | ||
|
||
target := btf.Type((*btf.Struct)(nil)) | ||
_, module, err := findTargetInKernel(s, "bpf_struct_ops_bpf_testmod_ops", &target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Why is this necessary?
prog.go
Outdated
} | ||
defer module.Close() | ||
|
||
kType, ok := btf.As[*btf.Struct](target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just use a plain type assertion here since we initialize target just above.
return nil, fmt.Errorf("member %q not found in %s", targetMember, kType.Name) | ||
} | ||
|
||
attr.ExpectedAttachType = sys.AttachType(idx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment explaining this StructOps quirk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! There are a couple of comments which you didn't address. Please go through the conversations and resolve them if you've actioned them.
To fix the CI breakage: for now it's ok to set AttachTo = ""
if were dealing with a struct ops program in TestLibBPFCompat.
Please rebase on top of #1865. The end result will be that you shouldn't have to call |
…ation 1. Verify creating a StructOpsMap from a hand-crafted MapSpec with metadata. 2. Resolve attr.BtfVmlinuxValueTypeId from vmlinux BTF before MapCreate. 3. Value population is deferred to follow-up PRs. see: cilium#1502 Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
- Add a smoke test for LoadAndAssign with a StructOpsMap (skips on unsupported kernels) - Call stOps.preLoad / .onProgramLoaded / .postLoad from LoadAndAssign - postLoad: no-op for now; value population will follow Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
1. implemented the above functions so that build kern_vdata 2. support "mod_btf" handling for testing 3. fix newProgramWithOptions() so that load structOps programs 4. fix createMap() so that find struct types from mod_btf Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
`wID` should be resolved in createMap() Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Replace use of structOpsProgMeta metadata with ProgramSpec.AttachTo (e.g. "bpf_testmod_ops:test_1") to encode attach target, and drop structOpsProgMeta, adjust tests accordingly. Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
- add translateStructData() and populateFuncPtr() and use them in populateDeferredMaps - delete structOpsMeta and redundant helpers Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Set MapSpec.Value to the "value struct" (`bpf_struct_ops_<ops>`) instead of the "kern struct". This matches the actual BTF layout where the sturct_ops struct matches with the `data` member inside the value type. Also remove unused helper functions (`findStructByNameWithPrefix`, `doFindStructTypeByName`, etc.) to simplify type resolution. Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
b19ded9
to
f5f64b2
Compare
done |
pattern Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
@lmb Since the comment disappeared, I’ll respond by quoting below.
From my understanding, libbpf seems to handle it as follows:
It may be need to keep the parsed result of the ELF somewhere. |
Signed-off-by: shun159 <[email protected]>
At the moment, we only have a test that writes progFD into kern_vdata, but please let me also add some test to check whether scalar values can be written. |
Signed-off-by: shun159 <[email protected]>
This PR adds initial support for StructOpsMap and StructOps.
see: #1502