libbpf-cargo: Support on-the-fly vmlinux.h extraction

[ueno]

This adds the extract_vmlinux_header method to SkelBuilder, which lets libbpf-cargo auto-generate the "vmlinux.h" header from the running kernel using bpftool. This would be particularly useful when packaging a crate with "cargo package", as it doesn't need the header file to be added to the repository.

[danielocfb]

Thanks for the pull request! Will take a closer look this week. One preliminary question: why do we need to introduce a feature? Can't this functionality be provided unconditionally? If the issue is the test then please make it ignored by default or something like that. Can you please also resolve the conflicts?

[danielocfb]

Left a few comments for improvements, on top of what I already mentioned.

Regarding the larger picture: it seems to be common practice to just carry around vmlinux.h files from everywhere, so I guess that conceptually this functionality is fine to add. However, the question is why would we want to have a dependency on and shell out to bpftool: from what I understand libbpf provides the means for dumping BTF as C code. bpftool's logic just seems to be a thin wrapper around that:

https://github.com/libbpf/bpftool/blob/784216403e89e9767833be3705bb8be0873eb52a/src/btf.c#L463-L505

In my opinion we should interface with libbpf here directly. Well, indirectly through libbpf-rs or potentially libbpf-sys. Let me know what you think.

[ueno]

In my opinion we should interface with libbpf here directly. Well, indirectly through libbpf-rs or potentially libbpf-sys. Let me know what you think.

Thank you for the suggestion; I will try that in this week. So far, the review comments should be addressed.

One preliminary question: why do we need to introduce a feature? Can't this functionality be provided unconditionally?

Let me explain the idea behind this: I thought that it is a good practice to "pin" a certain version of the kernel header when distributing a package alongside its binary (.deb, .rpm, etc.), while it is generally not when packaging a source crate, including auto-generated files. So the expected workflow is: enable the bpftool feature when building a crate, while making it still possible to swap the vmlinux.h when building a binary package. This is not fully supported by this PR yet, but the idea is to make extract_vmlinux_header a no-op for the latter case.

[danielocfb]

One preliminary question: why do we need to introduce a feature? Can't this functionality be provided unconditionally?

Let me explain the idea behind this: I thought that it is a good practice to "pin" a certain version of the kernel header when distributing a package alongside its binary (.deb, .rpm, etc.), while it is generally not when packaging a source crate, including auto-generated files. So the expected workflow is: enable the bpftool feature when building a crate, while making it still possible to swap the vmlinux.h when building a binary package.

Thanks for the background. This sounds like a lot of policy that I'd say does not belong in this crate. There is no concept of binary package from libbpf-rs's point of view, is there? I would expect that if clients need this kind of logic, they can build it themselves inside the build script. We only provide the building blocks/mechanisms.

Building this in the client has the added benefit that libbpf-cargo does not need to be rebuilt with a different feature.

[hzhuang1]

@ueno By default, vmlinux_515.h is only for x86. And it results in building error on aarch64 (#615). The workaround is using bpftool to generate a new vmlinux header instead.

I want to know how to generate a new vmlinux header with your patch. While I run "cargo build", I only find that "examples/capable/src/bpf/vmlinux.h" still links to "vmlinux_515.h". So I still have the build error. Do I miss any step to build libbpf-rs?

[ueno]

@hzhuang1 I'm still thinking about what UI would be appropriate, but with the current code, the following change might help:

diff --git a/examples/capable/build.rs b/examples/capable/build.rs
index 3758e12..580acc1 100644
--- a/examples/capable/build.rs
+++ b/examples/capable/build.rs
@@ -10,6 +10,7 @@ fn main() {
     out.push("capable.skel.rs");
     SkeletonBuilder::new()
         .source(SRC)
+        .extract_vmlinux_header(true)
         .build_and_generate(&out)
         .unwrap();
     println!("cargo:rerun-if-changed={SRC}");

[hzhuang1]

@hzhuang1 I'm still thinking about what UI would be appropriate, but with the current code, the following change might help:

diff --git a/examples/capable/build.rs b/examples/capable/build.rs
index 3758e12..580acc1 100644
--- a/examples/capable/build.rs
+++ b/examples/capable/build.rs
@@ -10,6 +10,7 @@ fn main() {
     out.push("capable.skel.rs");
     SkeletonBuilder::new()
         .source(SRC)
+        .extract_vmlinux_header(true)
         .build_and_generate(&out)
         .unwrap();
     println!("cargo:rerun-if-changed={SRC}");

s1.log

This patch doesn't help. Log is attached.

[github-actions]

This pull request is considered stale because it has been open 30 days with no activity. Remove stale label or comment or it will be closed in 5 days.

[ueno]

Sorry for the inactivity; I'll try to get back to it soon.

[github-actions]

This pull request is considered stale because it has been open 30 days with no activity. Remove stale label or comment or it will be closed in 5 days.

[github-actions]

Closing pull request as it is stale.