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

bpf(): improve map and program compatibility #3980

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

lmb
Copy link
Collaborator

@lmb lmb commented Nov 2, 2024

Allow passing a name for maps and programs and allow retrieving it via BPF_OBJ_GET_INFO_BY_FD. This requires a bunch of glue code because the bpf_prog_info, etc. structs are not compatible with Linux.

libs/api/bpf_syscall.cpp Outdated Show resolved Hide resolved
libs/api/bpf_syscall.cpp Outdated Show resolved Hide resolved
libs/api/bpf_syscall.cpp Outdated Show resolved Hide resolved
@lmb lmb force-pushed the bpf-names-info branch 2 times, most recently from 047d4c2 to 9140f0f Compare November 11, 2024 16:19
tests/unit/libbpf_test.cpp Outdated Show resolved Hide resolved
dthaler
dthaler previously approved these changes Nov 12, 2024
@shankarseal
Copy link
Collaborator

@lmb - the CICD tests are failing on this PR. Can you please take a look?

@lmb
Copy link
Collaborator Author

lmb commented Dec 2, 2024

Yes, I’m aware. My dev environment destroyed itself and setting it back up had to take lower priority. I’ll update when I can.

@lmb
Copy link
Collaborator Author

lmb commented Dec 8, 2024

Rebased this on main. Compilation fails again for some unfathomable reason so I can't test this locally...

@saxena-anurag
Copy link
Contributor

Rebased this on main. Compilation fails again for some unfathomable reason so I can't test this locally...

Hi, since the build is passing in CICD pipeline, you can probably use the artifacts from the pipeline to run the tests locally.

lmb and others added 6 commits December 16, 2024 11:15
Allow passing a name for maps and programs and allow retrieving it via
BPF_OBJ_GET_INFO_BY_FD. This requires a bunch of glue code because the
bpf_prog_info, etc. structs are not compatible with Linux.
Internally both zero and EBPF_ID_NONE are used to refer to non-existing ID.
Use zero when retrieving link info, since that aligns with Linux better.
@lmb
Copy link
Collaborator Author

lmb commented Dec 18, 2024

Tests are finally passing!

dthaler
dthaler previously approved these changes Dec 18, 2024
libs/api/bpf_syscall.cpp Outdated Show resolved Hide resolved
libs/api/bpf_syscall.cpp Outdated Show resolved Hide resolved
libs/api/bpf_syscall.cpp Outdated Show resolved Hide resolved
@@ -652,7 +652,7 @@ ebpf_link_get_info(

memset(info, 0, sizeof(*info));
info->id = link->object.id;
info->prog_id = (link->program) ? ((ebpf_core_object_t*)link->program)->id : EBPF_ID_NONE;
info->prog_id = (link->program) ? ((ebpf_core_object_t*)link->program)->id : 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: should EBPF_ID_NONE be defined to be 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

attr.info.info = (uintptr_t)&info;
attr.info.info_len = sizeof(info);
REQUIRE(bpf(BPF_OBJ_GET_INFO_BY_FD, &attr, sizeof(attr)) == 0);
// TODO: Should this be BPF_LINK_TYPE_PLAIN?
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I know, yes.

Suggested change
// TODO: Should this be BPF_LINK_TYPE_PLAIN?
// TODO: This should be BPF_LINK_TYPE_PLAIN.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #4096

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants