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.
> non-gpl and consistency don't even come close.
> We've been doing everything new as kfuncs and dynptr is not special.
>
> > > 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.
> >
> > 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.
>
> 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.
> It will evolve and has to evolve without uapi pain.
>
> kfuncs only. For everything. Please.
Thanks for your feedback, Alexei and Andrii. I share the same opinion
as Andrii about helpers for the APIs that are straightforward (eg
bpf_dynptr_get_offset), but I see your point as well about doing
everything new as kfuncs.
I'll change this to use kfuncs for v3.
