On Wed, Jan 18, 2023 at 8:32 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Wed, Jan 18, 2023 at 11:48:59AM +0100, Daniel Borkmann wrote:
> >
> > My $0.02 is that I don't think we need to make a hard-cut ban as part of this.
>
> The hard-cut is easier to enforce otherwise every developer will be arguing that
> their new feature is special and it requires a new discussion.
> This thread has been going for too long. We need to finish it now and
> don't come back to it again every now and then.
I wish that we could grant exception at least to complete dynptr
basics (bpf_dynptr_is_null, bpf_dynptr_get_size,
bpf_dynptr_{clone,trim,advance}) so that it is consistently provided
as a unified set of helpers. Similarly, for open coded loop iterator
(3 helpers), I believe it would be better for BPF ecosystem overall to
work on any BPF-enabled architecture and configuration (no matter JIT
or not, BTF of not, etc), just due to generality and unassuming nature
of this functionality.
But it is what it is, let's move on.
>
> imo this is the summary of the thread:
>
> bpf folks fall into two categories: kernel maintainers and bpf developers/users.
> - developers add new bpf features. They obviously want to use them and want bpf users
> to know that the feature they added is not going to disappear in the next kernel.
> They want stability.
> - maintainers want to make sure that the kernel development doesn't suffer because
> developers keep adding new apis. They want freedom to innovate and change apis.
> Maintainers also know that developers make mistakes and might leave the community.
> The kernel is huge and core infra changes all the time.
> bpf apis must never be a reason not to change something in the kernel.
>
> Freedom to change and stability just don't overlap.
> These two camps can never agree on what is more important.
> But we can make them co-exist.
>
> The bpf developers adding new kfunc should assume that it's stable and proceed
> to use it in bpf progs and production applications.
It's unclear what this means for library developers (libbpf, libxdp,
and others), but I guess we'll find out with time.
> The bpf maintainers will keep this stability promise. They obviously will not
> reap it out of the kernel on the whim, but they will nuke it if this kfunc
> will be in the way of the kernel innovation.
> The longer the kfunc is present the harder it will be for maintainers to justify
> removing it. The developers have to stick around and demonstrate that their
> kfunc is actually being used. The better developers do it the bigger the effort
> maintainers will put into keeping the kfunc perfectly intact.
>
[...]
>
> > The 'All new BPF kernel helper-like functionality must initially start out as
> > kfuncs.' is pretty clear where things would need to start out with, and we could
> > leave the option on the table if really needed to go BPF helper route when
> > promoting kfunc to stable at the same time. I had that in the text suggestion
> > earlier, it's more corner case and maybe we'll never need it but we also don't
> > drive ourselves into a corner where we close the door on it. Lets let the infra
> > around kfuncs evolve further first.
>
> Going kfunc->helper for stability was discussed already. It probably got lost
> in the noise. The summary was that it's not an option for the following reason:
> kfuncs and helpers are done through different mechanisms on prog and kernel side.
> The prog either sees = (void *)1 hack or normal call to extern func.
> The generated code is different.
> Say, we convert a kfunc to helper. Immediately the existing bpf prog that uses
> that kfunc will fail to load. That's the opposite of stability.
> We're going to require the developer to demonstrate the real world use of kfunc
> before promoting to stable, but with such 'promotion' we will break bpf progs.
> Say, we keep kfunc and introduce a new helper that does exactly the same.
> But it won't help bpf prog. The prog was using kfunc in production,
> why would it do some kind of CO-RE thing to compile 'call foo' differently
> depending whether 'foo' is kfunc or helper in the given kernel. And so on.
Correct. If we'd anticipated promotion of kfunc to helper (but it
doesn't seem like we do), we'd need to have kfunc with a different
name from its corresponding helper's name to avoid massive pain for
users. So if we were to add bpf_dynptr_is_null() as kfunc with an eye
for it to be stabilized as helper, I'd vote to add it as something
like bpf_kf_dynptr_is_null() or something, and then eventually add
bpf_dynptr_is_null(). That will at least less painful abstraction in
user's BPF code (and libbpf's helpers, if any).
