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.

Sign up for GitHub

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

Jump to bottom

bpf2go: generate assignment structs and Go types for Variables and VariableSpecs #1610

Merged
merged 4 commits into from
Dec 18, 2024
Merged

bpf2go: generate assignment structs and Go types for Variables and VariableSpecs #1610

merged 4 commits into from
Dec 18, 2024
+727 −130

Conversation

smagnani96
Copy link
Contributor

@smagnani96 smagnani96 commented Nov 13, 2024 •
edited
Loading

Commit messages should (hopefully) describe the introduced changes. In brief:

  1. add bpf2go basic support to emit Variables and VariableSpecs: as discussed in the RFC [RFC] - wrappers for type-safe ebpf.Variable #1543, this commit adds the minimal support to bpf2go for emitting Variables and VariableSpecs. Introduced in Collection: add Variables field to interact with global variables #1572, Variables are always emitted for every VariableSpec in the CollectionSpec, despite its usage being supported or not (kernel >=v5.5).
  2. fix typo in comment when bpf2go emits bpfProgramSpecs: fixed a typo in a bpf2go-generated comment (bpfSpecs rather than bpfProgramSpecs)
  3. regenerate all TARGETS in main: plain execution of make, to regenerate all the targets after (1).
  4. add bpf2go test emit-and-load specs+bpf objects: extended api_test.go to check also for Variables to be loaded, and all the {Map,Variable,Program}Specs to be loaded. Introduced new test in output_test.go to verify that the generated bpf2go output is as expected for every spec and bpf object.
  5. simplify and update tcx example to use Variable API: showcase the new Variable API in the TCX example. Back then when we introduced it, we wanted to use global variables, but there were no support for that, so we used bpf maps for counting packets.

EDIT: keeping new commits for btf.Var separated to facilitate the review process.

  1. btf: handle btf.Var in writeTypeLit: reuses the current logic to handle the btf.Var type while writing the output via GoFormatter.
  2. bpf2go: include btf.Var in CollectGlobalTypes: include btf.Var from spec.Variables while calling the CollectGlobalTypes function.
  3. regenerate all TARGETS in main after btf.Var: similarly as (2).

RFC with discussion #1543.

@smagnani96 smagnani96 force-pushed the pr/bpf2go-variables-basic branch from d68d6f0 to 96cd686 Compare November 13, 2024 11:08
@smagnani96 smagnani96 mentioned this pull request Nov 13, 2024
Draft
2 tasks
@smagnani96 smagnani96 changed the title land bpf2go basic support for Variable and VariableSpec basic bpf2go support for Variable and VariableSpec Dec 6, 2024
@smagnani96 smagnani96 force-pushed the pr/bpf2go-variables-basic branch 3 times, most recently from 93ccbd7 to e3598eb Compare December 6, 2024 12:51
@smagnani96
Copy link
Contributor Author

Had to repeat commit n.3, regenerating targets due to other changes.
In the meantime I mistakenly pushed local examples I was keeping.
Finally, I had to make a change in commit n.5.

Now should be good to review. Actual line changes should not be that much, the others are all auto-generated stuff 🤞

@smagnani96 smagnani96 marked this pull request as ready for review December 6, 2024 13:02
@smagnani96 smagnani96 requested review from ti-mo, mejedi and a team as code owners December 6, 2024 13:02
@mejedi
Copy link
Contributor

mejedi commented Dec 13, 2024 •
edited
Loading

Right now, maps key and value types are emitted. We should consider generating types exposed via variables by default. E.g.

struct struct_type variable;

Explicitly adding -type struct_type feels redundant.

@smagnani96 smagnani96 force-pushed the pr/bpf2go-variables-basic branch from 60a5ecc to e4bfeaf Compare December 13, 2024 18:37
@smagnani96
Copy link
Contributor Author

smagnani96 commented Dec 13, 2024 •
edited
Loading

Right now, maps key and value types are emitted. We should consider generating types exposed via variables by default. E.g.

struct struct_type variable;

Explicitly adding -type struct_type feels redundant.

For this, I just pushed 3 new commits for which I'd love to receive further feedback 🙏

The main doubt I had concerns a new function shouldEmit() I just introduced.
Let me explain why I needed this piece of code:

  1. taking btf.Types iterating through btf.Datasec would lead to a mismatch when checking an ebpf.VariableSpec type against the generated one.
  2. passing as it is a btf.Var would cause our GoFormatter to emit whatever type it receives, leading to potentially redundant generated code: I wouldn't expect us to generate a simple type t uint32, similarly as we do for maps.

For this reason, I'd like to prevent this behavior from bpf2go perspective, as I think it is mainly related to a bpf2go behavior rather than the btf logic itself.
Let me know if it sounds good or if there's a better way to address this.

@smagnani96 smagnani96 requested a review from mejedi December 13, 2024 18:52
ti-mo added a commit to ti-mo/ebpf that referenced this pull request Dec 16, 2024
@ti-mo
elf_reader: support value BTF on ringbuf and perf array maps
b41dc72 
While reviewing cilium#1610, I noticed we were generating VariableSpecs for variables
named 'unused' in our examples. There had to be a better way to get their BTF
info into the ELF.

I thoroughly checked all map types present in Linux 6.12 for any validation code
on attr->value_size, and only RingBuf and Arena maps require value_size to be
zero at all times. Special-case PerfEventArray so type annotations aren't limited
to 4 bytes.

Signed-off-by: Timo Beckers <timo@isovalent.com>
ti-mo added a commit that referenced this pull request Dec 16, 2024
@ti-mo
elf_reader: support value BTF on ringbuf and perf array maps
e9bcebb 
While reviewing #1610, I noticed we were generating VariableSpecs for variables
named 'unused' in our examples. There had to be a better way to get their BTF
info into the ELF.

I thoroughly checked all map types present in Linux 6.12 for any validation code
on attr->value_size, and only RingBuf and Arena maps require value_size to be
zero at all times. Special-case PerfEventArray so type annotations aren't limited
to 4 bytes.

Signed-off-by: Timo Beckers <timo@isovalent.com>
@ti-mo
Copy link
Collaborator

ti-mo commented Dec 16, 2024

I've been polishing this a bit to get it into shape for the release. Please hold off on pushing any changes here.

@ti-mo ti-mo force-pushed the pr/bpf2go-variables-basic branch from e4bfeaf to c1e865d Compare December 17, 2024 14:27
@ti-mo
Copy link
Collaborator

ti-mo commented Dec 17, 2024

@smagnani96 I've done some polishing and squashed this down into a few commits. In the future, please regenerate testdata in the same commit as when codegen/testdata changes are made, otherwise commits can't be individually reverted and rebasing becomes painful. I've mostly rewritten the functionality related to CollectGlobalTypes and added some tests. I had to get hands-on to familiarize myself with the problem, only to realize it's all very finnicky and the existing code was sort of broken, so doing this through code review would've been too time-consuming.

@mejedi PTAL so this can go into this week's release, see individual commits. Upon closer inspection, I found several flaws in CollectGlobalTypes and decided to give it a rework. It's still a bit hard to follow, but I've tried separating concerns as much as I could.

@ti-mo ti-mo changed the title basic bpf2go support for Variable and VariableSpec bpf2go: generate assignment structs and Go types for Variables and VariableSpecs Dec 17, 2024
ti-mo and others added 2 commits December 18, 2024 12:45
@ti-mo
bpf2go: test: remove unused typesEqualComparer function
7673737 
Signed-off-by: Timo Beckers <timo@isovalent.com>
@smagnani96 @ti-mo
bpf2go: generate assignment structs for Variables and VariableSpecs
fabe147 
This commit teaches bpf2go to emit assignment structs for VariableSpecs and
Variables. Similar to maps and programs, the generated bpf_bpfe{b,l}.go have
bpfVariableSpecs embedded in bpfSpecs and bpfVariables in bpfSpecs.

Also fixed a typo in the bpfProgramSpecs comments in output.tpl that emitted
bpfSpecs rather than bpfProgramSpecs.

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@ti-mo ti-mo force-pushed the pr/bpf2go-variables-basic branch 2 times, most recently from baeed3d to 7b51a04 Compare December 18, 2024 11:59
ti-mo and others added 2 commits December 18, 2024 13:05
@ti-mo @smagnani96
bpf2go: generate Go types used in global variables
9a89e65 
This commit generates Go type declarations for global variables declared
in a C BPF program. Some improvements to CollectGlobalTypes were
due:

- types are now deduplicated by name
- types are now selected based on allow-list instead of explicitly
  rejecting Datasec and Int
- the output is sorted by type name
- arrays' inner types are now emitted to interact with Variable{Spec}

Also added a new btf.QualifiedType(), a subset of btf.UnderlyingType()
to remove all qualifiers up to a Typedef, since Typedefs are named, and
typically directly point to the anonymous type they alias.

Typedefs should only be stripped for evaluating the concrete underlying
type, and they should be included when yielding types to the Go renderer.

Signed-off-by: Timo Beckers <timo@isovalent.com>
Co-authored-by: Simone Magnani <simone.magnani@isovalent.com>
@smagnani96 @ti-mo
examples: tcx: use Variable API
00fcefc 
This commit updates the `example/tcx` program to use the new Variable API
rather than traditional bpf maps. The example already depends on `bpf_link`,
which requires kernel >= v6.6, therefore the Variable API is already
supported and working (since v5.5).

Signed-off-by: Simone Magnani <simone.magnani@isovalent.com>
@ti-mo ti-mo force-pushed the pr/bpf2go-variables-basic branch from 7b51a04 to 00fcefc Compare December 18, 2024 12:05
@ti-mo ti-mo merged commit 228bb4e into cilium:main Dec 18, 2024
15 of 17 checks passed
@mejedi
Copy link
Contributor

mejedi commented Dec 19, 2024

@ti-mo

@mejedi PTAL so this can go into this week's release, see individual commits. Upon closer inspection, I found several flaws in CollectGlobalTypes and decided to give it a rework. It's still a bit hard to follow, but I've tried separating concerns as much as I could.

Congrats on the release! I love the changes in CollectTypes.

@mejedi
Copy link
Contributor

mejedi commented Dec 20, 2024

@ti-mo
I ended up rewriting sortTypes in #1636

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ti-mo ti-mo Awaiting requested review from ti-mo ti-mo is a code owner

@dylandreimerink dylandreimerink Awaiting requested review from dylandreimerink dylandreimerink is a code owner

@mejedi mejedi Awaiting requested review from mejedi mejedi is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smagnani96 @mejedi @ti-mo