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.
