On Wed, Jan 4, 2023 at 12:03 PM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Wed, Jan 04, 2023 at 10:59:15AM -0800, Andrii Nakryiko wrote: > > > > > >> to actual BPF helpers by then where we go and say, that kfunc has proven itself in production > > > >> and from an API PoV that it is ready to be a proper BPF helper, and until this point > > > > > > > > "Proper BPF helper" model is broken. > > > > static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1; > > > > > > > > is a hack that works only when compiler optimizes the code. > > > > See gcc's attr(kernel_helper) workaround. > > > > This 'proper helper' hack is the reason we cannot compile bpf programs with -O0. > > > > And because it's uapi we cannot even fix this > > > > With kfuncs we will be able to compile with -O0 and debug bpf programs with better tools. > > > > These tools don't exist yet, but we have a way forward whereas with helpers > > > > we are stuck with -O2. > > > > > > > But specifically about how the BPF helper model is broken, that's at > > least an exaggeration. BPF helper call is defined at BPF ISA level, it > > has to be a `call <some constant>;`, and as long as compiler generates > > such code, it's all good. From C standpoint UAPI is just a function > > call: > > > > bpf_map_lookup_elem(&map, ...); > > > > As long as this compiles and generates proper `call 1;` assembly > > instruction, we are good. If/when both Clang and GCC support an > > alternative way to define helper and not as a static func pointer, -O0 > > builds (at least in the aspect of calling BPF helpers, I suspect other > > stuff will break still) will just work. And what's better, > > bpf_helper_defs.h would be able to pick the best option based on > > compiler's support with end users not having to care or notice the > > difference. > > Right and that's what gcc did with attribute((kernel_helper(1)), > but we didn't like it because gcc and clang would diverge. > Now you're arguing it's just a bpf_helper_defs.h change and we should > have allowed it? No, I'm saying if you feel so strongly that the current situation is bad and attribute-based approach is preferable (presumably to allow -O0 to work), then we can do that (both on GCC and Clang sides) and everything will work with no UAPI changes. And I did suggest a relatively clean approach with BPF_HELPER_DEF() ([0]) which would combine both old and new ways. But I personally have no problem with the current approach. You are bringing it up as an UAPI problem, which I'm claiming it is not. [0] https://lore.kernel.org/bpf/CAEf4BzYwRyXG1zE5BK1ZXmxLh+ZPU0=yQhNhpqr0JmfNA30tdQ@xxxxxxxxxxxxxx/ > > Also consider that 'call <some constant>' or more precise 'call absolute_address' > as an instruction exist in only one CPU architecture. It's BPF ISA. > It's a mistake that I made 8 years ago and inability to fix it bothers me. > Now we have 100 times more developers than we had 8 years ago. > I expect 100 time more UAPI and ABI mistakes. > Minimizing unfixable mistakes is what I'm after.
