On Thu, Dec 8, 2022 at 5:30 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Dec 8, 2022 at 4:42 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Wed, Dec 7, 2022 at 5:54 PM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > >
> > > On Wed, Dec 07, 2022 at 12:55:31PM -0800, Joanne Koong wrote:
> > > > This patchset is the 3rd in the dynptr series. The 1st can be found here [0]
> > > > and the 2nd can be found here [1].
> > > >
> > > > In this patchset, the following convenience helpers are added for interacting
> > > > with bpf dynamic pointers:
> > > >
> > > > * bpf_dynptr_data_rdonly
> > > > * bpf_dynptr_trim
> > > > * bpf_dynptr_advance
> > > > * bpf_dynptr_is_null
> > > > * bpf_dynptr_is_rdonly
> > > > * bpf_dynptr_get_size
> > > > * bpf_dynptr_get_offset
> > > > * bpf_dynptr_clone
> > > > * bpf_dynptr_iterator
> > >
> > > This is great, but it really stretches uapi limits.
> >
> > Stretches in what sense? They are simple and straightforward getters
> > and trim/advance/clone are fundamental modifiers to be able to work
> > with a subset of dynptr's overall memory area.
> >
> > > Please convert the above and those in [1] to kfuncs.
> > > I know that there can be an argument made for consistency with existing dynptr uapi
> >
> > yeah, given we have bpf_dynptr_{read,write} and bpf_dynptr_data() as
> > BPF helpers, it makes sense to have such basic things like is_null and
> > trim/advance/clone as BPF helpers as well. Both for consistency and
> > because there is nothing unstable about them. We are not going to
> > remove dynptr as a concept, it's pretty well defined.
> >
> > Out of the above list perhaps only move bpf_dynptr_iterator() might be
> > a candidate for kfunc. Though, personally, it makes sense to me to
> > keep it as BPF helper without GPL restriction as well, given it is
> > meant for networking applications in the first place, and you don't
> > need to be GPL-compatible to write useful networking BPF program, from
> > what I understand. But all the other ones is something you'd need to
> > make actual use of dynptr concept in real-world BPF programs.
> >
> > Can we please have those as BPF helpers, and we can decide to move
> > slightly fancier bpf_dynptr_iterator() (and future dynptr-related
> > extras) into kfunc?
>
> Sorry, uapi concerns are more important here.
What about the overall user experience and adoption?
There is no clean way to ever move from unstable kfunc to a stable helper.
BPF helpers also have the advantage of working on all architectures,
whether that architecture supports kfuncs or not, whether it supports
JIT or not.
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).
> 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.
By requiring kfunc-based helpers we are significantly raising the
obstacles towards adopting dynptr across a wide range of BPF
applications.
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.
2. Manipulators to copy existing dynptr's view and narrow it down to a
subset (e.g., for when you have a large memory blog, but need to
calculate hashes over smaller subset, without destroying original
dynptr, because it will be used later for some other access). We can
debate whether clone should get offset or not, but it doesn't change
much (except usability in common cases). Again, nothing to add or
remove otherwise, and pretty fundamental for real use of full power of
dynptr.
long bpf_dynptr_clone(struct bpf_dynptr *ptr, struct bpf_dynptr
*clone, u32 offset)
long bpf_dynptr_trim(struct bpf_dynptr *ptr, u32 len)
long bpf_dynptr_advance(struct bpf_dynptr *ptr, u32 len)
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)
All of the above don't add or change any semantics to dynptr as a
concept. There is nothing that we'd need to change.
>
> > > helpers, but we got burned on them once and scrambled to add 'flags' argument.
> > > kfuncs are unstable and can be adjusted/removed at any time later.
It's unfair to block these helpers just because we recided to add
flags to one of the previous ones (before the final release). And even
if we didn't managed to do it in time, the worst things would probably
be another variant of BPF helper. Definitely something to avoid, but
not end of the world. But as I pointed out above, this set of helpers
won't be change, as they just complete already established dynptr
ecosystem of helpers.
> >
> > I don't see why we would remove any of the above list ever? They are
> > generic and fundamental to dynptr as a concept, they can't restrict
> > what dynptr can do in the future.
>
> It's not about removing them, but about changing them.
>
> Just for example the whole discussion of whether frags should
> be handled transparently and how write is handled didn't inspire
> confidence that there is a strong consensus on semantics
> of these new dynptr accessors.
So let's start with acknowledging that skb and xdp buffer abstractions
as logically contiguous memory area are inherently complex and
non-perfect due to the way that kernel handles them for performance
and flexibility reasons.
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
that for SKB/XDP there is no perfect solution. Dynptr doesn't change
anything here, rather it actually simplifies a bunch of stuff,
especially for common scenarios.
I'd argue that for wider SKB/XDP dynptr adoption in the networking
world, those dynptr constructor helpers should be helpers and not
kfuncs as well. But I'd wish someone with more networking tie-ins
would argue this instead of me.
>
> Scrambling to add flags to dynptr helpers was another red flag.
>
> All signs are pointing out that we're not ready do fix dynptr api.
I disagree, it's an overly harsh generalization.
> It will evolve and has to evolve without uapi pain.
>
> kfuncs only. For everything. Please.
This is yet another generalized blanket statement I disagree with.
Over the years I've got an impression that the BPF subsystem is
generally a proud proponent of pragmatic, flexible, and common sense
engineering approaches, so this hard-and-fast rule with no room for
nuance sounds weird.
There are things that belong in fundamental and core BPF concepts, and
it makes sense to keep them as stable abstractions and helpers. And
there are various things (like interfacing into kernel mechanics, its
types and systems) which totally make sense to keep unstable.
