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

add support for struct_ops maps and programs #1603

Closed
wants to merge 5 commits into from

Conversation

shun159
Copy link
Contributor

@shun159 shun159 commented Oct 27, 2024

This commit introduces the ability for users to define and use struct_ops maps and associated programs. With this addition, eBPF programs can implement and register kernel structures, enabling struct_ops functionalities like sched_ext.

Example Usage:

Users can now define struct_ops maps and associated programs like this:

SEC("struct_ops/dummy_test_1")
int dummy_test_1(void *arg) {
    return 0;
}

SEC("struct_ops.s/dummy_test_sleepable")
int dummy_test_sleepable(void *arg) {
    return 0;
}

SEC(".struct_ops.link")
struct bpf_dummy_ops dummy_ops = {
    .test_1         = dummy_test_1,
    .test_sleepable = dummy_test_sleepable,
};

In this example, the program defines a bpf_dummy_ops struct with function pointers and registers implementations of these functions. The .struct_ops.link section indicates that the struct should be linked with the kernel.

This commit introduces the ability for users to define and use struct_ops maps
and associated programs. With this addition, eBPF programs can implement and
register kernel structures, enabling struct_ops functionalities like `sched_ext`.

Example Usage:

Users can now define struct_ops maps and associated programs like this:
```c
SEC("struct_ops/dummy_test_1")
int dummy_test_1(void *arg) {
    return 0;
}

SEC("struct_ops.s/dummy_test_sleepable")
int dummy_test_sleepable(void *arg) {
    return 0;
}

SEC(".struct_ops.link")
struct bpf_dummy_ops dummy_ops = {
    .test_1         = dummy_test_1,
    .test_sleepable = dummy_test_sleepable,
};
```

In this example, the program defines a `bpf_dummy_ops` struct with function
pointers and registers implementations of these functions. The `.struct_ops.link`
section indicates that the struct should be linked with the kernel.

Signed-off-by: shun159 <[email protected]>
@ti-mo
Copy link
Collaborator

ti-mo commented Oct 27, 2024

Hi @shun159, this PR clearly represents a significant amount of work, so first of all, thank you for the time investment. Unfortunately, given the size of this change, we need to discuss architecture first. Maybe you intended to drive this conversation through this PR, but understand that it's a lot of code and quite a few adjustments will be needed to make things fit. In the future, it would be ideal if the initial conversation, like we had in the issue, goes a bit more in-depth. If this would be a contribution from a colleague, this is the kind of change I'd ask a code walk for.

Having skimmed the PR (on mobile 😢), a few observations:

  • MapSpec and btf.Spec are important pieces of exported API and the bar is high for extending them. Avoid using them for storing arbitrary state that isn't useful for the caller.
  • Likewise, avoid trivial, overly-specific helpers on btf.Spec. The ones added here are basically struct ops business logic. Conder making them free functions in a separate package, e.g. internal/structops.
  • No fixup commits, these are hard to review.
  • Consider splitting this into 5-ish logical commits, progressively adding the needed infrastructure. If a subset of the code provides value as-is, even a separate PR makes sense to reduce the context of a review.
  • Since this is a major feature, adding something to examples/ as well as a docs/ manual page with a high-level feature overview and code examples would be appropriate.

Note: I'll be off next week, so I may take some time to respond.

Thanks again!

@shun159
Copy link
Contributor Author

shun159 commented Oct 28, 2024

Hi @ti-mo,

Thank you for your feedback.
Based on your points, I am considering the following actions:

  • Review changes to MapSpec and btf.Spec to ensure there are no unnecessary changes to the public API.
  • Move certain helper functions to the internal/structops package.

Also, I'd like to discuss the architecture, so let's discuss it here.
Thank you for your time. I'll leave some comments there, but before the discussion, I'll share some explanatory materials about this PR later if you need.

Let me close this PR.

Have a good vacation!

@shun159 shun159 closed this Oct 28, 2024
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.

2 participants