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

btf: move LoadKernelSpec into separate package #1015

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

lmb
Copy link
Collaborator

@lmb lmb commented Apr 20, 2023

vmlinux BTF turns out to be a big headache, because it is quite large at roughly 150k types. The API we currently have in place forces a copy of the whole vmlinux btf.Spec on every call to LoadKernelSpec. We copy to prevent distinct callers of LoadKernelSpec from modifying types that may be referenced by someone else. As a result, some things that require access to kernel BTF are really slow: attaching fentry/fexit hooks, kfuncs, etc.

We've optimized the copying a little bit, but we can't get around the fact that there are a lot of objects to be copied so it's unlikely to get much faster. An attempt to copy btf.Spec lazily was abandoned due to it's complexity.

Which leaves us with this approach: the library shares a single kernel Spec and must take care to avoid mutating or exposing types contained therein. Users of the library always get a fresh copy and must themselves take care to cache the spec if necessary.

Due to import cycles this means we can't expose the kernel spec from package btf anymore and instead need a separate package. This will cause compilation errors, but they should be simple to fix since the semantics don't change otherwise.

@lmb lmb force-pushed the btf-internal-kernel-spec branch 2 times, most recently from 54fe563 to 2bae88d Compare April 25, 2023 13:27
@lmb lmb marked this pull request as ready for review April 25, 2023 13:28
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.

Few nits/comments.

internal/linuxint/types.go Outdated Show resolved Hide resolved
internal/linuxint/types.go Outdated Show resolved Hide resolved
btf/core.go Show resolved Hide resolved
@lmb lmb force-pushed the btf-internal-kernel-spec branch from 2bae88d to 270b41e Compare May 30, 2023 12:51
vmlinux BTF turns out to be a big headache, because it is quite large
at roughly 150k types. The API we currently have in place forces a copy
of the whole vmlinux btf.Spec on every call to LoadKernelSpec.
We copy to prevent distinct callers of LoadKernelSpec from modifying
types that may be referenced by someone else. As a result, some
things that require access to kernel BTF are really slow: attaching
fentry/fexit hooks, kfuncs, etc.

We've optimized the copying a little bit, but we can't get around the
fact that there are a lot of objects to be copied so it's unlikely to
get much faster. An attempt to copy btf.Spec lazily was abandoned due
to it's complexity.

Which leaves us with this approach: the library shares a single
kernel Spec and must take care to avoid mutating or exposing types
contained therein. Users of the library always get a fresh copy
and must themselves take care to cache the spec if necessary.

Due to import cycles this means we can't expose the kernel spec
from package btf anymore and instead need a separate package.
This will cause compilation errors, but they should be simple to
fix since the semantics don't change otherwise.

All library code must use internal/linux, while users have access
to package linux.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
@lmb lmb force-pushed the btf-internal-kernel-spec branch from 270b41e to d07784b Compare May 30, 2023 12:54
@lmb lmb changed the title btf: avoid copies of vmlinux BTF for internal callers btf: move LoadKernelSpec into separate package Jun 1, 2023
@lmb lmb merged commit c3a89fc into cilium:master Jun 5, 2023
@lmb lmb deleted the btf-internal-kernel-spec branch June 5, 2023 07:51
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

2 participants