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

cmd/bpf2go: enable building custom tools (wip) #1320

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mejedi
Copy link
Contributor

@mejedi mejedi commented Jan 29, 2024

lmb once said: My personal preference would be to make the parts of bpf2go that are useful available via Go API so that power users can build their own tools.

Turning bpf2go code into a library of reusable components is a massive effort. The resulting API surface would be quite large and it will be hard to commit to API stability.

Explore the next best option:

  • expose bpf2go Run for use by custom tools built on top;

  • provide hooks to make it possible to customise certain aspects of bpf2go.

Hook interface is purposedly minimal, based on demand, and gives no API stability guarantees.

It will enable the community to experiment with new powerful features and immediately benefit from these new features in their projects. Hopefully, users will still work towards getting their features accepted.

If it so happens that a feature is not in line with cilium/ebpf goals users could stick to their custom tool while benefitting from the upstream effort going into improving bpf2go.

lmb once said: My personal preference would be to make the parts of
bpf2go that are useful available via Go API so that power users can
build their own tools.

Turning bpf2go code into a library of reusable components is a massive
effort. The resulting API surface would be quite large and it will be
hard to commit to API stability.

Explore the next best option:

 - expose bpf2go Run for use by custom tools built on top;

 - provide hooks to make it possible to customise certain aspects of
   bpf2go.

Hook interface is purposedly minimal, based on demand, and gives no
API stability guarantees.

It will enable the community to experiment with new powerful features
and immediately benefit from these new features in their projects.
Hopefully, users will still work towards getting their features accepted.

If it so happens that a feature is not in line with cilium/ebpf goals
users could stick to their custom tool while benefitting from the
upstream effort going into improving bpf2go.
@mejedi mejedi requested a review from a team as a code owner January 29, 2024 13:25
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.

My initial impression: this is going to be a pain to support long-term. As this abstraction grows, it will grow tendrils throughout the codebase, as we're basically invoking caller-supplied logic everywhere. Any modifications will need to take into account that "oh, we always need to invoke the user's specified Compile() that's Docker-aware", which will end up severly limiting what this tool can become over time.

Also, this model implies all contributors are aware of the user's intentions at all times, lest we break something, which is not something we should sign ourselves up for IMO. This is where the term 'callback hell' originates from. You're no longer really in control over your program's execution flow, since the caller can inject behaviour everywhere, and you can no longer assume the state of things on the filesystem, etc.

Turning bpf2go into a library is probably the way forward. Composition over injection/inheritance.

}

// Customisations enables building custom tools on top of bpf2go
type Customisations interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Drive-by nit: isn't this basically a variation of *Options, but with an interface instead of a struct?

}

// TargetCustomisations enables building custom tools on top of bpf2go
type TargetCustomisations interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the difference with a Customizations? Why are they split?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are split to enable storing target-specific state in TargetCustomisations. E.g. read DWARF in .Compile, later use the data in .AugmentSpec.

@mejedi
Copy link
Contributor Author

mejedi commented Jan 29, 2024

@ti-mo

My initial impression: this is going to be a pain to support long-term. As this abstraction grows, it will grow tendrils throughout the codebase, as we're basically invoking caller-supplied logic everywhere. Any modifications will need to take into account that "oh, we always need to invoke the user's specified Compile() that's Docker-aware", which will end up severly limiting what this tool can become over time.

Thank you for the feedback! I'd like to understand the issues you see in .Compile. Is that roughly — "What if bpf2go starts invoking compiler in different contexts so that it is no longer a simple N targets — a compile per target?"

The model I had in mind is summarised in the commit description:

Hook interface is purposedly minimal, based on demand, and gives no API stability guarantees.

If the tool evolution necessitates breaking Customisations interface, so be it! From the consumer's side it is probably still going to be more convenient than a fork.

Turning bpf2go into a library is probably the way forward. Composition over injection/inheritance.

I am happy to explore that route as well. My concerns:

  • if bpf2go +customised compile is to be supported, then bpf2go split needs to be fine grained which could be a liability going forward as well;

  • unless some unergonomic coding patterns are adopted, customising compile will require duplicating a significant amount of bpf2go code if we reject injection/inheritance.

By unergonomic I refer to e.g. splitting planning and execution. E.g. bpf2go library produces a DAG of Actions (akin to make), and a consumer edits the DAG before invoking it. Unsure if we should even consider it.

@lmb
Copy link
Collaborator

lmb commented Feb 14, 2024

Sorry for the long radio silence. I agree with Timo that hook based approaches are hard to maintain.

I'm guessing that there are specific changes you'd like to make to bpf2go, could you describe them? That will make it easier to discuss solutions.

@mejedi
Copy link
Contributor Author

mejedi commented Feb 26, 2024

@lmb

I'm guessing that there are specific changes you'd like to make to bpf2go, could you describe them?

I'd like to make it more of a turn-key solution. In order of priority (some of the items are highly specific to our use-case):

  1. Ensure that everyone running go generate creates the same output (even when NOT on Linux).
  2. Fast rebuilds (caches).
  3. Golang package-aware includes in C.
  4. Use both BTF and DWARF when generating type definitions.

1. Ensure that everyone running go generate creates the same output (even when NOT on Linux)

Cilium does have a Makefile and Docker wrapper for this purpose. Every project choosing to commit bpf object files need this.

The way I imagine it could work is to scan source files for #include and build a file set. The file set is then shipped to a docker container that provides clang and system headers. The tool doesn't need to implement a full-fledged C preprocessor; it is ok to ship extra files (due to not comprehending #if ... #endif).

Side note: while the following is valid C and it won't be supported, no one sane does it:

#define F "myfile"
#include F

2. Fast rebuilds (caches)

Our bpf code changes very often (we build traffic processing applications), so having to remember to run go generate after mostly every change becomes a nuisance. It would be more convenient to have a target in a Makefile that does go generate followed by go build.

But sometimes a change is in golang code, and rebuilding bpf code is unnecessary. It would be great to have fast rebuilds when nothing changed for bpf2go, similar to go build.

3. Golang package-aware includes in C

We'd like to share common C code between projects and use go modules as a delivery vehicle.

4. Use both BTF and DWARF when generating type definitions

It is often necessary to declare bogus variables to get a type captured in BTF, shouldn't be necessary.

@lmb
Copy link
Collaborator

lmb commented Mar 15, 2024

Very interesting, thanks! Sorry for the long wait, this slipped through the cracks and then I went on PTO.

  1. Ensure that everyone running go generate creates the same output (even when NOT on Linux).

Okay, seems like there are two goals here:

  1. Make things reproducible on Linux
  2. Make bpf2go invokable on non-Linux host (macOS?)

What if we allowed passing an OCI container spec for -cc and mounted the git repo / current working directory as a volume?

2. Fast rebuilds (caches).

Do you build for many arches? We could speed up builds by compiling in parallel. Or do you just compile lots of different files? I'm not sure about doing caching, it's complicated with a lot of edge cases. Maybe there are other things we can do to speed up builds? So far that has never been much of a priority.

3. Golang package-aware includes in C

I've thought about something similar, except that my idea was to ship pre-compiled libraries in go modules. eBPF code would use extern func(); or similar and the library would resolve these at load time. Maybe doing it at the compile stage is better though? Definitely something I would be happy to have in bpf2go.

Use both BTF and DWARF when generating type definitions

I agree with the goal, but I'm worried about the complexity of parsing DWARF. Maybe this is something where we need to work with upstream clang / gcc developers (or just libbpf? not sure what generates BTF these days). Or invest time into finding a better work around.

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.

None yet

3 participants