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: migrate hand-written bpf syscalls to ebpf-go APIs #25355

Merged
merged 7 commits into from May 31, 2023

Conversation

ti-mo
Copy link
Contributor

@ti-mo ti-mo commented May 10, 2023

This is based on #22693 and contains all of its commits. That one is close to being merged, so this can be rebased later.

See individual commits. Again a bit of churn, but relatively straightforward.

Fixes: #22513
Fixes: #23558

Replace legacy bpf syscalls with ebpf-go library APIs

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 10, 2023
@ti-mo ti-mo force-pushed the tb/more-ebpf-maps branch 2 times, most recently from 62af59c to a43ea7a Compare May 10, 2023 13:34
@ti-mo ti-mo added the release-note/misc This PR makes changes that have no direct user impact. label May 10, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 10, 2023
@ti-mo ti-mo mentioned this pull request May 10, 2023
@ti-mo
Copy link
Contributor Author

ti-mo commented May 11, 2023

/test

1 similar comment
@ti-mo
Copy link
Contributor Author

ti-mo commented May 12, 2023

/test

@ti-mo ti-mo force-pushed the tb/more-ebpf-maps branch 3 times, most recently from 519a9ea to 2591156 Compare May 26, 2023 13:49
@ti-mo ti-mo changed the title bpf: remove remaining hand-written bpf syscalls in favor of ebpf-go APIs bpf: migrate hand-written bpf syscalls to ebpf-go APIs May 26, 2023
@ti-mo ti-mo marked this pull request as ready for review May 26, 2023 13:52
@ti-mo ti-mo requested review from a team as code owners May 26, 2023 13:52
@ti-mo ti-mo requested review from lmb and nbusseneau May 26, 2023 13:52
@ti-mo
Copy link
Contributor Author

ti-mo commented May 26, 2023

/test

return false, fmt.Errorf("unable to delete element %s from map %s: %w", key, m.name, err)
}

return true, nil
}

// SilentDelete deletes the map entry corresponding to the given key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Drive by: this description doesn't tell me at all what makes the function so silent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this needs to go. As you can see, the Map API is full of this sort of snowflake and unhelpful/incomplete doc. To answer your question: it skips incrementing the map's delete metric.

IMO this highlights a common problem with the design of the data model in the upper layer(s). The data (e.g. ct record) and the underlying data store (e.g. the ct map) are conflated. The bpf layer shouldn't lie about the amount of deletes it issues to the kernel, as it could mask a potential perf problem.

pkg/bpf/map_linux.go Outdated Show resolved Hide resolved
Net [16]byte

// v4 LPM maps have 8-byte key sizes even though the 20-byte cidrKey is used
// for map ops. This hack relies on passing unsafe.Pointers to the cidrKey so
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh boy

pkg/bpf/map_linux.go Outdated Show resolved Hide resolved
pkg/bpf/bpf.go Outdated Show resolved Hide resolved
pkg/datapath/linux/probes/attach_cgroup.go Outdated Show resolved Hide resolved
ti-mo added 7 commits May 30, 2023 13:52
Replace bpf.DeleteElement() by calls into ebpf.Map.Delete(). Move
metrics handling into bpf.Map.delete().

Signed-off-by: Timo Beckers <timo@isovalent.com>
In order to move away from map operations implemented in pkg/bpf/bpf_linux.go,
migrate Map.DumpWithCallback and friends onto ebpf-go based primitives. No
changes are made to the algorithm used in DumpReliably..().

Signed-off-by: Timo Beckers <timo@isovalent.com>
Signed-off-by: Timo Beckers <timo@isovalent.com>
Signed-off-by: Timo Beckers <timo@isovalent.com>
…port

This was moved in from the verifier tests and given a test suite of its own.
Test cases based on what the agent does on startup.

Signed-off-by: Timo Beckers <timo@isovalent.com>
This will be used in a follow-up commit to be executed during socketlb setup.
It implements only the cgroup link/prog_attach part of TestDummyProg so it
can be run once instead of on each prog/attach type probe combination.

Includes a privileged test that is expected to succeed in all CI environments.

Signed-off-by: Timo Beckers <timo@isovalent.com>
Builds on the APIs added in prior commits. Probing for cgroup attach support
is now only done once at the start of the socketlb setup, and prog/attach type
probes are now executed separately, underlyingly using APIs from ebpf-go.

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

ti-mo commented May 30, 2023

/test

@ti-mo
Copy link
Contributor Author

ti-mo commented May 30, 2023

/test-1.26-net-next

@ti-mo ti-mo merged commit 29fe3dc into cilium:main May 31, 2023
62 checks passed
@ti-mo ti-mo deleted the tb/more-ebpf-maps branch May 31, 2023 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove TestDummyProg() Migrate pkg/bpf to use cilium/ebpf
3 participants