On Mon, Dec 12, 2022 at 12:12:09PM -0800, Andrii Nakryiko wrote:
>
> There is no clean way to ever move from unstable kfunc to a stable helper.
No clean way? Yet in the other email you proposed a way.
Not pretty, but workable.
I'm sure if ever there will be a need to stabilize the kfunc we will
find a clean way to do it.
Strongly arguing right now that this is an issue without doing the home work
is not productive.
> BPF helpers also have the advantage of working on all architectures,
> whether that architecture supports kfuncs or not, whether it supports
> JIT or not.
Correct, but applying the same argument we should argue that
all features must work in the interpreter as well, because
not all architectures support JIT.
This way struct-ops and bpf based TCP-CC would never be possible.
Some JITs don't support tail calls with subprogs.
freplace (bpf prog replacement) works when JITed only.
bpf trampoline works on x86-64 only.
while kfuncs work on more than one arch.
Now comapre the amount of .text that kernel has to contain
to support hundreds of helpers vs same amount of kfuncs.
In the former it's a whole bunch of code that is there in the kernel
in case bpf prog will call that helper. With 200+ helpers and half
of them already deprecated we have quite a bit of dead code in the kernel
that we cannot delete.
While with kfunc approach there is no extra code that deals with
conversion of the registers from bpf psABI to arch psABI.
With kfuncs we generate this code on demand.
> BPF helpers are also nicely self-discoverable and documented in
> include/uapi/linux/bpf.h, in one place where other BPF helpers are.
> This is a big deal, especially for non-expert BPF users (a vast
> majority of BPF users).
Good point. In general the kfuncs are not up to the level of
documentation of helpers and we should work on improving that,
but some of kfuncs are better documented than helpers.
So it's not black and white.
Discoverability we discussed in the past.
The task to automatically emit kfuncs into vmlinux.h is still not complete.
Time to prioritize it higher.
>
> > non-gpl and consistency don't even come close.
> > We've been doing everything new as kfuncs and dynptr is not special.
>
> I think dynptr is quite special. It's a very generic and fundamental
> concept, part of core BPF experience. It's a more dynamic counterpart
> to an inflexible statically sized `void * + size` pair of arguments
> sent to helpers for input or output memory regions. Dynptr has no
> inherent dependencies on BTF, kfuncs, trampolines, JIT, nothing.
imo dynptr and kptr are more or less equivalent in terms of being core
building blocks.
kptrs are done via kfuncs, so dynptr can do just as well.
> By requiring kfunc-based helpers we are significantly raising the
> obstacles towards adopting dynptr across a wide range of BPF
> applications.
Sorry, but I have to disagree. kptr and dynptr are left and right hand.
Both will work just fine as kfuncs.
> And the only advantage in return is that we get a hypothetical chance
> to change something in the future. But let's see if that will ever be
> necessary for the helpers Joanne is adding:
>
> 1. Generic accessors to check validity of *any* dynptr, and it's
> inherent properties like offset, available size, read-only property
> (just as useful somethings as bpf_ringbuf_query() is for ringbufs,
> both for debugging and for various heuristics in production).
>
> bpf_dynptr_is_null(struct bpf_dynptr *ptr)
> long bpf_dynptr_get_size(struct bpf_dynptr *ptr)
> long bpf_dynptr_get_offset(struct bpf_dynptr *ptr)
> bpf_dynptr_is_rdonly(struct bpf_dynptr *ptr)
>
> There is nothing to add or remove here. No flags, no change in semantics.
Disagree, since there is an obvious counter example.
See all of bpf_get_current_task*().
Some of them are still used, but
bpf_get_current_task vs bpf_get_current_task_btf is our acknowledgement
of the fact that we suck in inventing uapi.
It's the lesson that we've learned the hard way.
Not going to repeat that mistake again.
To be completely honest I expect that dynptr may get obsolete
as the whole concept several years from now.
We still don't have a single actual user of it.
Just like kptr. Could be deprecated eventually just as well.
> 3. This one is the only one I feel less strongly about, but mostly
> because I can implement the same (even though less ergonomically, of
> course) with bpf_loop() and bpf_dynptr_{clone,advance}.
>
> long bpf_dynptr_iterator(struct bpf_dynptr *ptr, void *callback_fn,
> void *callback_ctx, u64 flags)
Speaking of your upcoming inline iterators.
Please make sure that you're adding them as kfuncs.
We've made a mistake with bpf_loop. It's a stable helper,
but inline iterators will immediately deprecate most uses of bpf_loop.
If bpf_loop was a kfunc we would have deleted it.
> Let's also note that verifier knows specific flavor of dynptr and thus
> can enforce additional restrictions based on specifically SKB/XDP
> flavor vs LOCAL/RINGBUF. So just because there is no perfect way to
> handle all the SKB/XDP physical non-contiguity, doesn't mean that the
> dynptr concept itself is flawed or not well thought out. It's just
I think that's exactly what it means. dynptr concept is flawed.
It's ok to add this flawed feature to the kernel right now,
because we don't see a better way today, but that might change
in the future and we gotta be able to fix our mistakes.
