-
Notifications
You must be signed in to change notification settings - Fork 768
link: allow attaching with ProgramFd (needed for struct_ops) #1844
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?
Conversation
This PR extends RawLinkOptions with an optional ProgramFd so callers can attach link types whose target isn't a bpf_prog (e.g. struct_ops maps). See: cilium#1502 Signed-off-by: shun159 <[email protected]>
Signed-off-by: shun159 <[email protected]>
link/link_test.go
Outdated
_ = cg.Close() | ||
}) | ||
|
||
ln, err := AttachRawLink(RawLinkOptions{ |
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.
How would you use this with a struct ops map? Target
is the map and ProgramFd is what exactly?
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.
@lmb For struct_ops we follow libbpf’s semantics:
https://github.com/libbpf/libbpf/blob/master/src/libbpf.c#L13163
struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
{
struct bpf_link_struct_ops *link;
__u32 zero = 0;
int err, fd;
.
.
.
fd = bpf_link_create(map->fd, 0, BPF_STRUCT_OPS, NULL);
if (fd < 0) {
free(link);
return libbpf_err_ptr(fd);
}
}
bpf_link_create(/* prog_fd = */ map_fd, /* target_fd = */ 0, BPF_STRUCT_OPS, …)
So the intent is to pass the struct_ops map FD as ProgramFd and not use Target (i.e., target_fd = 0). Usage would look like:
m := objs.AStructOpsMap
ln, err := link.AttachRawLink(link.RawLinkOptions{
ProgramFd: int(m.FD()), // map FD goes into prog_fd
Attach: ebpf.AttachStructOps,
// Target is unused (0)
})
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.
In that case it seems better to just add a new, custom API specifically targeted at StructOps. Then the argument can simply be a *Map
. Taking an int like this is confusing, but also unsafe: int(m.Fd())
doesn't protect m from being closed by the GC.
func AttachStructOps(opts StructOpsOptions) (*RawLink, 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.
@lmb sounds good. let me try this in this PR. how do you see this shape?
type StructOpsOptions struct {
Map *ebpf.Map // required
Flags uint32 // optional link_create flags
}
func AttachStructOps(opts StructOpsOptions) (*RawLink, 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.
let me revert the changes to main
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'm guessing there must be some way to specify more parameters to the attachment though? What if there are multiple instances of the same struct ops? (I guess what I'm asking: given multiple sched_ext maps, which one ends up being active? Can you only create a single link?)
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.
What if there are multiple instances of the same struct ops?
let's say we try to load multiple sched_ext maps, in that case, it should be failed with EBUSY (in practical cases, for each struct_ops can have only one instance for struct_ops map). in other words, it should be selected by like "first load wins"
btw, but (technically) one ELF file can have 2 or more different struct_ops instances,
for example:
__section(".struct_ops.link") struct bpf_testmod_ops2 dummy_ops_1 = {
.test_1 = (void *)dummy_test_1,
};
__section(".struct_ops.link") struct bpf_testmod_ops3 dummy_ops_2 = {
.test_1 = (void *)dummy_test_2,
.test_2 = (void *)dummy_test_3,
};
even in that case, because one program can belong to only one struct_ops instance,
this case, should be failed
__section("struct_ops/dummy_test_1") int dummy_test_1(void) {
return 0;
}
__section("struct_ops/dummy_test_1") int dummy_test_2(void) {
return 0;
}
__section(".struct_ops.link") struct bpf_testmod_ops2 dummy_ops_1 = {
.test_1 = (void *)dummy_test_1,
};
__section(".struct_ops.link") struct bpf_testmod_ops3 dummy_ops_2 = {
.test_1 = (void *)dummy_test_1,
.test_2 = (void *)dummy_test_2,
};
Signed-off-by: shun159 <[email protected]>
depends on #1845. we can proceed with this once the MR get merged. |
This PR extends RawLinkOptions with an optional ProgramFd so callers can attach link types whose target isn't a bpf_prog (e.g. struct_ops maps).
See: #1502