-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
bpftrace should have a strong dependency on libbpf #2322
Comments
Hi @BurntBrunch, thanks for opening this! libbpf is currently required for building bpftrace (since #2265) so you can freely use functions that it provides (although we currently support also older libbpf versions). On the other hand, many of the ideas above are great and we should consider implementing them eventually. I'd suggest to continue discussion in this thread and then open separate issues for each of the features. |
Yeah no objections to any of this. Moving to libbpf will a lost of things simpler and standardised :) |
Alright, I'm glad this is well-received! :) How do we go about formalizing the min version? Is that strictly in the cmake config or do we do it anywhere else? Also, does this mean we can clean up all the HAVE_LIBBPF_* checks and fallbacks? I really don't think we care about a libbpf version that doesn't have bpf_prog_load, for example (if one ever existed). |
While from a development perspective only bleeding edge would be great we do try to support a wide range of kernels and library versions. Stable distros as still quite popular, so I'd say as low as we can reasonably go. It's tricky, bpf moves way to fast for the distros (and us) to keep up |
I am open to this as well. I evaluated the same idea ~1 year ago but stopped short of implementing for a few reasons:
|
To expand, I think vendoring libbpf is really the only sane long term solution. It is more or less inevitable that we will have to carry some out of tree libbpf patches at some point, especially if we punt all loading operations to libbpf. And it is also a very practical way to keep up with the rapid pace of bpf development (esp. in the interest of end user experience). For example, rust vendors llvm for most likely similar reasons. |
I agree, vendoring seems like the best way to go forward, also considering our limited dev power. I do think the end goal should be to 'unvendor' it again when (if ever) everything stabilizes. Or atleast track stable releases at a pace that works for distros as well. Doing so should also prevent collecting patches that never get upstreamed |
That's kinda why I suggested 1.0 as the min version - it's supposed to be the last big API cleanup before "stability" is a thing. It's been baking for quite a while. Now, requiring 1.0 would also require that distros update all their stragglers but overall I don't think it's an unreasonable design decision at this point in time.
I'd need examples but the thing about libbpf is that you can always bypass it. You'd have to do some of its job yourself (as I found myself doing) but there's no vendor lock-in per se. It just lowers the bar for most new features while keeping the bar for "exotic" features as-is.
Maybe? Beyond bug fixes that haven't landed yet, I don't see why we'd want to carry out of tree libbpf patches. Besides, I can't predict the future - all I know is that libbpf can today simplify bpftrace in the particular ways I described above. Beyond small things like exposing more helpers (we can just keep our own header), whether it's vendored or min-versioned at something high enough to be considered stable, I don't particularly mind.
Are the library versions formalized anywhere? I tried looking but I'm not seeing it clearly stated in any docs. If we can say "last X Ubuntu LTS releases + last X centos etc," then we can get very precise about the libbpf API coverage we'd have and the bugs we'd have to avoid. That said, I also don't think it's particularly unreasonable to say "yes, we support latest bpftrace on Ubuntu 16.04 but you need to use an AppImage/static build/flatpak/snap/whatever." If these distros want latest bpftrace packaged in-repo, they'd have to go through the trouble of also updating its dependencies. I guess I don't understand how being able to build against LLVM 6 on a stable distro helps people in practice - after all, the distro is not going to push bpftrace updates anymore. Is this purely for local builds against system libraries? Or for copr/etc builds in user repos? Isn't that better addressed by the official AppImage/static builds/etc? Sorry if these questions are obvious, I am just trying to build a complete mental model here :) |
Yes, vendoring / bundling library dependencies is problematic for distributions and it should be avoided if possible. Fedora's policy is that it's only allowed as a matter of last resort (meaning, if it is at all possible to build a package against system libraries is should be build against system libraries). I'm not as familiar with Debian (cc @michel-slm who might know), but I believe they have a similar policy in place. Bundling is frowned upon because, among other things, it leads to a major increase in complexity when dealing with security updates for one of the bundled dependencies. It also makes auditing harder as it might not be obvious when a bundled dependency has been modified (or even where it came from in the first place in some cases).
IMO this is a perfectly reasonable stance to take. |
Having done the backport work for Ubuntu 16.04 and 20.04 I can tell you that even if you do it, the result isn't terribly functional. You wind up missing kernel headers or libc headers and things of that nature. You need minimum kernel versions defined alongside libbpf versions. Insofar as Debian's policy on bundling, it's generally forbidden rather than allowed as a last resort. |
Agreed.
One example I had in mind was how bpftrace used to encode map FDs directly into bytecode during codegen. I ended up fixing that to use ids instead of fds and fixing up the ids -> fds at load time. This helped make the codegen tests less hacky (no longer dependent on a specific fd allocation order). I suppose if we used libbpf we could instead codegen however clang does it (emit relocations?) and have libbpf do the relocation. I can't think of any other examples right now so I think I agree with you for now.
Yeah, bugs are one reason. I remember I fixed a bug in libbpf a while ago where libbpf was incorrectly closing fd 0 in some scenarios. Stuff like that is painful for users and hard to explain. Another reason would be to reduce amount of build time probing and ifdefs. IOW I think vendoring is a way to make maintenance less painful given our limited resources. Less stuff to think about when writing code. An AFAIK statically linking / vendoring libbpf is the recommended way to use libbpf. For example bpftool statically links libbpf and is shipped by all distros I know of. |
I'd strongly advocate against doing this. Once we start patching the vendored libbpf, rebasing to new versions will eventually become a pain and un-vendoring (which I agree would be the goal in such a case) will be quite difficult. Ofc we won't have the latest libbpf bug patches immediately (especially the packaged versions of bpftrace won't) but I think (or hope) that it's a rare situation that we'd need them. As for the features, I believe we'll be able to implement in bpftrace what is not provided by libbpf. Another thing that we should keep in mind is that libbpf is in a way connected to the kernel. So I'd expect that if we vendor a new version of libbpf for a system with a much older kernel, that could cause some trouble. Maybe I'm wrong and it'll be alright, but we should at least give a thought about this. That being said, I prefer setting a minimum required libbpf version rather than vendoring it. Unfortunately, libbpf is moving very fast, so at this point, 1.0 seems a too strong requirement to me (e.g. Fedora 35, which is less than a year old, has 0.6.1).
AFAIK no, but it'd be nice to have as deciding on the minimum supported libbpf (and other libs) version would be very easy.
The difference here is that bpftool is built as a part of the kernel, so you always end up with exactly the same version of libbpf bundled as is the system one. |
Sigh, github ate my first comment so I apologize if I'm a bit shorter in this iteration. First off, thanks for chiming in, everyone, it's much appreciated! I'll address some specific points first and discuss a possible formalization after.
@Conan-Kudo, can you expand a bit on what exactly you found broken w.r.t. headers? My hope is that going forward, with BTF being enabled in the last LTS Ubuntu release, headers will be less of an issue. As for minimal kernel versions, I actually disagree - feature probing should be enough and errors should be raised at runtime. We want the same binary to be portable across the widest possible range of kernel versions and libbpf also has a similar mindset.
@danobi, yeah, that's the idea. Emit the same global variables with relocations and all and let libbpf handle instantiating and managing the maps. It may even be simpler from a codegen and runtime perspective.
I actually disagree, I think the best way to reduce build time probing is a high enough minimum version but that's orthogonal to vendoring the library itself. I'll discuss this in more detail below.
Unfortunately, that seems to be defining difference - bpftool lives in linux/tools, so its packaging can follow different rules. I think vendoring libbpf for bpftrace will not fly with the distros but that doesn't mean that it's the only way forward. (See below)
I'll address this below but I don't think it's a problem per se but rather an artifact of the current "weak" dependency on libbpf. Feature requirements and packaged versionsFrom a brief skim at the libbpf history, I think these requirements stand out:
On a separate note, no non-rolling distro is packaging a recent enough libbpf at all. I'm trusting pkgs.org on this but the list is not encouraging. Last Ubuntu LTS is at 0.5, CentOS 9 Stream at 0.6 and Amazon Linux 2 at 0.3. At the same time, their bpftrace versions are much newer (0.14, 0.13 and 0.11). This tells me that there's strong interest in bpftrace but no reason to update libbpf on its own (though it looks like Ubuntu Kinetic will have 0.8.0). From a certain angle, bpftrace raising the minimum libbpf version may be the impetus for distros to update their libbpf. ProposalI propose the following formalization of the libbpf dependency:
With these requirements, we get this process for new BPF features:
I think it overall does a good job of 1) giving users better compatibility via static builds, 2) improving the developer experience so we don't have reinvent the wheel or be able to build against ancient system libraries, and 3) giving clear feedback to distros on what we need as minimum library versions. It's also in line with how other projects operate, so I don't expect it to be a surprise to package maintainers. This is only an MVP proposal, so please poke holes in it! :) |
Thanks for the detailed analysis @BurntBrunch . I've only skimmed the details (about to head off the grid for a few days) but I think this is a nice compromise. I think running bpftrace in lockstep with libbpf is a good solution. We might initially prevent users on older distros from building bpftrace, but hopefully there is enough downstream pressure on distros to update libbpf such that things work out in the long term. We also provide dockerized builds + static CI builds so IMHO it's not really essential to have users natively build from source. And hopefully in doing this we can lift up the broader bpf ecosystem by putting pressure on distros to frequently update libbpf. I also like the vendor libbpf w/ no patches approach. That should help simplify development + CI a bit. |
I really like the proposal @BurntBrunch. Setting up a minimal version and vendoring libbpf with no patches is a nice compromise that will make distro packager's life easier and give developers the latest libbpf features at the same time. On a side note, from a distro packager point of view, doing a libbpf version update is not as simple as for other userspace tools (e.g. bpftrace). Since libbpf is a part of kernel, you typically need to update the entire BPF subsystem and often the kernel is what differs the most from the upstream version, so the backport requires quite some work. So, this may be the reason why distros have a much older version of libbpf than of bpftrace. All in all, this seems like a good piece of work ahead of us. I might be able to find some free cycles for it if necessary, but it'd be nice to split the work into smaller tasks so that we can synchronize. @BurntBrunch I'm completely fine if you want to do this on your own, but if you needed help, could you perhaps open several issues for individual steps? Thanks for moving this forward! I think that this is an important step in bpftrace development. |
Clarifying in case I misunderstand:
IOW libbpf source tree is already decoupled from kernel sources. And libbpf is designed to work across as many kernels as possible. So I'm not sure there is a reason libbpf is so out of date other than no other package is forcing packagers to upgrade. |
Oh, but that's the thing - we wouldn't. Local builds will by default use the vendored libbpf, so most users would be fine. Package maintainers will have to do a different dance but that's fine, it's part of the deal.
Daniel responded to this but I just want to add that any such issues (newer libbpf not working on older kernels or vice versa) should be raised upstream as they are 100% bugs.
Right, that's my read too. If anything, libbpf not being used by kernel tools as a dynamic library has hurt the up-to-dateness - otherwise the distro kernel teams would package it with perf, bpftool et al. Or maybe it's just the fact that so far libbpf has been backwards compatible to the beginning, more or less, and no one is using the newer features in distro-packaged software. 1.0 is the first API break and it can be simulated with
I'll take any and all help I can get! I'll write up a reasonable set of refactoring steps in a new issue and people can claim whichever pieces they want either on that issue or on a new one? I think I have some reasons to do certain things before others but I'll document all that. As a point of clarification, are we happy with the current static builds requiring glibc 2.27? The glibc 2.27 requirement is being brought in by Alternatively, a fully static build (builtin libc) or just postponing this decision is also a path forward. |
While this is true, bpftrace is more tightly coupled because it uses both libbpf and bcc. The latter is what fails on older kernels. Both bcc and bpftrace have much tighter coupling with Linux APIs, and my attempts around backporting to older distributions has led to enough funky results that I would probably recommend declaring baseline kernel uapi versions too. |
I should also mention that there's an LLVM dependency here, and outside of RHEL and SLE, LTS distributions typically don't ship fully functioning up to date LLVM stacks to use. And even when they are available (e.g. in Ubuntu LTS as part of HWE stuff), it usually requires some serious contortions to get it to work properly at build time. For example, I carried this downstream patch for work when I backported the Fedora bpftrace package to Ubuntu 16.04 over two years ago (yes, you read that right... I gave a talk about it a while ago!): bpftrace-0.9.3-add-custom-clang-module-files.patch.txt There is a fair amount of complexity around this, and even if you manage to compile it, it behaves badly enough at runtime that it's basically not worth it. |
Right, a lot of the refactoring here will involve moving things to use libbpf's equivalent functionality instead.
Hey, one major dependency at a time! 😄 But I hear you, that's definitely the next big thing to clear up and figure out. I'm hoping that a lot of people would be okay with using the static binaries but I understand that that's not always allowed and I don't have a silver bullet for that case (I'm assuming that vendored LLVM was not an option at your work place?). |
Not really, no. That's usually creates way more trouble. |
Then it should be dropped from the kernel source tree. The current situation is way too confusing. |
@Conan-Kudo feel free to start a discussion at the libbpf repo here on GitHub or on the bpf mailing list. In this issue, we don't really control how libbpf is version controlled or distributed but we do control how bpftrace finds and uses it. The proposed changes will likely make your life easier, llvm versioning notwithstanding. |
I didn't know that this is the recommended way, thanks for pointing out.
Probably yes, but as has been pointed out, that's a question for another forum.
Yes, that sounds like a good plan. We should have one "overview" issue with all the steps (and dependencies between them) clarified and then we can create individual issues for each step where it's worth.
We use
Again, this is a discussion for another place, but there is this issue #1845 which would drop dependency on LLVM completely. Ofc it's a long way to go, I just wanted to point it out since we're at it. |
That would be great. AFAIK, there are currently 3 major parts where we depend on BCC: USDTs, probe attachement, and symbol resolution. I think that at least the first two will be replaced by libbpf as a part of this, so there's hope we could decouple from bcc eventually. |
Closing as resolved. Let's discuss the actual work/steps in #2334. |
Chiming in late here (thanks to the link from the bcc issue), but I can also add the nixos and alpine packager hats for not liking vendoring. for nixos it's ok as last resort, but I don't think I've ever seen any alpine package with one yet (explicitly looking right now I see libcamera has some, but I see more patches that remove them than packages using one) I think it's ok to only support libbpf 1.0+ (or 0.8.1) though -- distros that are stuck at older libbpf versions like fedora 35 will also never upgrade bpftrace. If you're just thinking of making it easy for developers then an optional submodule never hurts, but I'd hugely appreciate if it's not made mandatory. EDIT: re-reading the thread now and it looks like this is what's intended already, sorry for the noise -- and thanks for the work :) |
While working on #2321, I had to manually - and poorly - reimplement functionality that's already in libbpf in a more complete form. (namely, subprogram and func_info relocation)
I'm opening this issue to discuss making libbpf a required dependency, especially given its imminent 1.0 release. This dependency would allow us to
Obeying the libbpf layout requires some work (e.g. BTF-based map definitions and the very particular section naming conventions) but honestly that's probably more shallow a rabbit hole than what I endured to get #2321 to the finish line.
I'm opening this issue primarily because I don't know the project's policy on build dependencies and would love to discuss any objections to this change.
The text was updated successfully, but these errors were encountered: