Improve static data load support

[joestringer]

The current approach for templatizing BPF programs [ #7095 ] uses these fun tricks to convince LLVM to tag global data symbols and reserve space in the .data section:

https://github.com/cilium/cilium/pull/7095/files#diff-fb0192c65751d297ed36049608a36c89

These can then be interpreted in the following iproute2 changes to load the data we want:

isovalent/iproute2@16ed3bd

A similar approach was proposed to upstream libbpf, but rejected:

https://www.spinics.net/lists/netdev/msg550707.html

Possible solutions:

1. Use inline-asm tricks:
   • https://github.com/newtools/ebpf/blob/master/testdata/rewrite.c#L21
   • I prototyped this locally, seems doable but requires reworking all 32-bit static data access in the datapath. Not too much work, just tedious and has some potential to increase complexity.
   • With minimal changes, still need custom iproute2 changes to communicate the template substitutions to iproute2
     • Alternatively could investigate switching to newtools/ebpf for ELF load/rewrite. This likely will provide some additional performance gains, but would need to be evaluated as to whether the library provides everything we need. More work.
     • EDIT 2021-04-09: This approach has been deprecated in favor of BTF in the upstream library, it doesn't make sense to use this approach any more.
2. Upstream proposes an approach here: https://www.spinics.net/lists/netdev/msg551328.html
   • Will also need reworking all 32-bit static data access in the datapath.
   • Would also require rebasing iproute2 on libbpf
   • Then rebasing cilium-packer-ci against this iproute2
3. NEW 2020-07-09: Encode the constants in BTF in the ELF. In the loader (should be github.com/cilium/ebpf), load these constants and either (a) substitute the values in the instructions prior to load in a manner similar to Option (1); or (b) make use of upstream in-kernel constant support from Option (2)

[joestringer]

A note: The docker GSG is currently locked to v1.4 due to this issue (see comment added in #7574)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This issue has not seen any activity since it was marked stale. Closing.

[vadorovsky]

/cc @borkmann @jrfastab

Pointers in gobpf for program templating:

• https://github.com/cilium/ebpf/blob/master/elf_reader.go#L44
• https://github.com/cilium/ebpf/blob/master/testdata/rewrite.c#L21

Although it's not clear if we can already use gobpf for Cilium BPF programs.

[joestringer]

My high level take is that Cilium would benefit from moving the entire loader logic into Golang, to avoid recent situations like #12070 (which was a regression due to introducing map-handling logic into the Go side where it was previously only handled by iproute2/tc in the shell scripting) and also to improve code structuring, unit testing, integration between Cilium core logic and the loader logic. I note that this is not strictly the same problem as addressing the way we handle static data substitution, but it's related. If we switch the encoding of static data, we necessarily need an implementation to handle that encoding. That leads us back to the decision of whether to keep the same approach with Go->bash->C programs (where we need to modify the C programs) or to more natively integrate a loader directly in Go. This is the back of my mind while I think through this problem.

I note also that this is related to the mechanism which invokes the compiler to build the datapath ELF files. For a minimal solution here, I don't believe we will need to make any major modifications to this logic; most likely just some compiler flag changes.

The latest option that I've added to the PR description is inspired by some discussions I had with @jrfastab around BTF.

• I believe that with a new enough compiler, we should be able to attach the static data information to the ELF using BTF. This in itself may require adjusting the way that the datapath declares the static data to ensure that the compiler picks it up and encodes it into the BTF in the ELF. This will also need some compile flags. IIRC we currently disable the BTF generation in the compiler simply because we use an iproute2 version that doesn't support BTF. That particular flag change should be trivial. At best, the switch in the datapath should be a minimal change to the static data macros; at worst it may require a bit of tedious re-organising of the way that the static data variables are declared throughout the codebase.
  • Ideally this could be done as a separate step, but I don't know if it's possible to adjust the code to be compatible with both the current approach and the BTF approach at the same time. If it is, that would be desirable just to break the overall change down into smaller pieces for easier review.
• From there, we need loader support.
  • cilium/ebpf library has BTF support for reading AFAIK, but I don't know about the logic for pushing the constants from the BTF into the instructions. The current cilium/ebpf library implementation has some Go-side code to handle the LOAD_CONSTANT approach, not sure if there's something we can hijack there or if we need substantial new logic to handle the static data injection (/cc @jrfastab ).
    • The beauty of the LOAD_CONSTANT approach is that it is ambivalent to the underlying kernel version. Incidentally this also applies to our current approach. The BPF instructions are tweaked prior to kernel load, so there is no need for specific kernel support for static data. If we start by using an approach similar to this, we can universally support all kernels. We can optimize this later (last point below)
    • I note that the actual implementation will not be binary-compatible with the current cilium/ebpf approach for LOAD_CONSTANT so it'll be good to understand the implications there and make a proposal & discuss on that library to make sure we're all on the same page as to what the proposal is and why we think the ebpf library should change its approach (assuming it does; that's my initial assumption.)
  • An alternative that I haven't thought too far about is to rework iproute2 against libbpf to add BTF support to the loader there. This would help other users of iproute2, and has potential to be less work / less invasive for Cilium Go code. My personal take is that this isn't the long-term direction we want to get to in Cilium though so we'll inevitably end up re-implementing the approach in cilium/ebpf and migrating over at some point anyway so that we can get deeper integration between Cilium and the loader; better unit-testing; etc. etc.
• As a subsequent step, we could take a look at Option (2) to further optimize the static data handling on kernels that support static data maps. I believe that this will be desirable but not necessary for a minimal solution.

I also had a thought about whether we need to touch bpf/init.sh. At this stage I suspect that we would need to move the load of the tunnel BPF program into the golang side as that is the last bpf/init.sh user of tc directly; all other users are run from the Go side. This should be a pretty independent change and I think could be achieved based on a similar approach to how @pchaigno moved the bpf/bpf_host logic into pkg/datapath/loader during the v1.8 dev cycle.

[ti-mo]

Given the kernel gained support for direct map accesses in https://lore.kernel.org/bpf/20190409212018.32423-2-daniel@iogearbox.net/, shipped in 5.2, and ebpf-go has supported it for years, we shouldn't spend too much time inventing another solution to this problem. We can wait until 4.19 goes EOL and remove this hack when we target 5.2 or later.

One small improvement that could be made to the current implementation of inlineGlobalData is that it currently requires all constants to be 32 bits wide, since that's what the equivalent iproute2 code did in our fork. Now we have access to .data's BTF Datasec through MapSpec.Value, we can determine the exact width of the value the instruction is referring to, and issue a copy of the correct size instead of a fixed 32 bits. This means we would dynamically support values of 8/16/32/64 bits wide, not just 32-bits. See https://github.com/cilium/cilium/blob/main/bpf/lib/static_data.h for the C side of things. DEFINE_U16 happens to work because other uses of DEFINE_U32 in the compilation unit push the alignment of .data to 32 bits, making the inliner read a u16 value + 16 bits of padding.

Supporting larger values (esp. 64 bits) could have an impact on instruction count in ipv6-heavy code paths since currently, with only 32-bit values supported, ipv6 addresses need to be split into 4 chunks. These are then put into 4 imm64 instructions, only utilizing half of the space available. Worse, an imm64 is actually encoded using 2 raw instructions in the insn stream, so this totals 8 raw instructions for an ipv6. Knowing how much impact will require us to do the work anyway.

Edit: My proposal for the above: #25195.