On Mon, Jun 24, 2024 at 08:55:43PM GMT, Andrii Nakryiko wrote:
> On Mon, Jun 24, 2024 at 7:12 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > Sorry to resurrect the old thread to continue the discussion of APIs for
> > qp-trie.
> >
> > On 8/26/2023 2:33 AM, Andrii Nakryiko wrote:
> > > On Tue, Aug 22, 2023 at 6:12 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
> > >> Hi,
> > >>
> >
> > SNIP
> >
> > >> updated to allow using dynptr as map key for qp-trie.
> > >>> And that's the problem I just mentioned.
> > >>> PTR_TO_MAP_KEY is special. I don't think we should hack it to also
> > >>> mean ARG_PTR_TO_DYNPTR depending on the first argument (map type).
> > >> Sorry for misunderstanding your reply. But before switch to the kfuncl
> > >> way, could you please point me to some code or function which shows the
> > >> specialty of PTR_MAP_KEY ?
> > >>
> > >>
> > > Search in kernel/bpf/verifier.c how PTR_TO_MAP_KEY is handled. The
> > > logic assumes that there is associated struct bpf_map * pointer from
> > > which we know fixed-sized key length.
> > >
> > > But getting back to the topic at hand. I vaguely remember discussion
> > > we had, but it would be good if you could summarize it again here to
> > > avoid talking past each other. What is the bpf_map_ops changes you
> > > were thinking to do? How bpf_attr will look like? How BPF-side API for
> > > lookup/delete/update will look like? And then let's go from there?
> > > Thanks!
> > >
> > > .
> >
> > The APIs for qp-trie are composed of the followings 5 parts:
> >
> > (1) map definition for qp-trie
> >
> > The key is bpf_dynptr and map_extra specifies the max length of key.
> >
> > struct {
> > __uint(type, BPF_MAP_TYPE_QP_TRIE);
> > __type(key, struct bpf_dynptr);
>
> I'm not sure we need `struct bpf_dynptr` as the key type. We can just
> say that key_size has to be zero, and actual keys are variable-sized.
>
> Alternatively, we can treat key_size as "maximum key size", any
> attempt to use longer keys will be rejected.
>
> But in either case "struct bpf_dynptr" as key type seems wrong to me.
I have some half-baked thoughts here. FYI I haven't thought thru the
details yet.
bpftrace has some right-sizing issues with strings. Consider the case
where we read a string from userspace. We have to first bring the string
into kernel memory (via bpf_probe_read_user()) in order to do anything.
So we must size the buffer the string gets read into.
Size it too small and you get a truncated string. Size it too large and
we'll waste memory. Especially if we want to key a map with a string.
I wonder if dynptrs can be used to provide a native string type for bpf
programs.
Ptr+len implementation seems like a natural fit. It might not help with
bpftrace multi-keys [0] but for single string key it could be useful.
[0]: https://man.archlinux.org/man/extra/bpftrace/bpftrace.8.en#Associative_Arrays
