Re: [PATCH bpf-next v2 09/25] bpf: Support bpf_list_head in map values

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Wed, Oct 19, 2022 at 09:27:57PM IST, Alexei Starovoitov wrote:
> On Tue, Oct 18, 2022 at 10:48 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Wed, Oct 19, 2022 at 07:29:16AM IST, Alexei Starovoitov wrote:
> > > On Thu, Oct 13, 2022 at 11:52:47AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > Add the basic support on the map side to parse, recognize, verify, and
> > > > build metadata table for a new special field of the type struct
> > > > bpf_list_head. To parameterize the bpf_list_head for a certain value
> > > > type and the list_node member it will accept in that value type, we use
> > > > BTF declaration tags.
> > > >
> > > > The definition of bpf_list_head in a map value will be done as follows:
> > > >
> > > > struct foo {
> > > >     struct bpf_list_node node;
> > > >     int data;
> > > > };
> > > >
> > > > struct map_value {
> > > >     struct bpf_list_head head __contains(foo, node);
> > > > };
> > > >
> > > > Then, the bpf_list_head only allows adding to the list 'head' using the
> > > > bpf_list_node 'node' for the type struct foo.
> > > >
> > > > The 'contains' annotation is a BTF declaration tag composed of four
> > > > parts, "contains:kind:name:node" where the kind and name is then used to
> > > > look up the type in the map BTF. The node defines name of the member in
> > > > this type that has the type struct bpf_list_node, which is actually used
> > > > for linking into the linked list. For now, 'kind' part is hardcoded as
> > > > struct.
> > >
> > > ...
> > >
> > > > +   value_type = btf_find_decl_tag_value(btf, pt, comp_idx, "contains:");
> > > > +   if (!value_type)
> > > > +           return -EINVAL;
> > > > +   if (strncmp(value_type, "struct:", sizeof("struct:") - 1))
> > > > +           return -EINVAL;
> > > > +   value_type += sizeof("struct:") - 1;
> > >
> > > I don't get it.
> > > The patch 24 does:
> > > +#define __contains(name, node) __attribute__((btf_decl_tag("contains:struct:" #name ":" #node)))
> > >
> > > The 'struct:' part is invisible to users. They won't make a mistake.
> > > Why bother adding it to BTF and then check for it?
> > > Backward compat concerns?
> > > But it's in bpf_experimental.h.
> > > That probably be the last thing to change and so easy to do.
> > > Please drop it?
> > >
> >
> > Fair, I just left it there anticipating atleast union with a discriminant might
> > be a possible candidate, but since this is all unstable it's not a big deal.
> >
> > > > diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
> > > > new file mode 100644
> > > > index 000000000000..4e31790e433d
> > > > --- /dev/null
> > > > +++ b/tools/testing/selftests/bpf/bpf_experimental.h
> > > > @@ -0,0 +1,23 @@
> > > > +#ifndef __KERNEL__
> > > > +
> > > > +#include <vmlinux.h>
> > > > +#include <bpf/bpf_tracing.h>
> > > > +#include <bpf/bpf_helpers.h>
> > > > +#include <bpf/bpf_core_read.h>
> > > > +
> > >
> > > Why bother with the above?
> > > The below should be enough ?
> > >
> >
> > Actually, I'm using this header inside the kernel, userspace, and BPF programs.
> > In the kernel to provide type definitions for bpf_list_head and bpf_list_node,
> > which are then emitted to vmlinux.h (and also used inside the kernel ofcourse).
> >
> > In userspace for these types as otherwise including skeleton fails to build, as
> > such types are global variables, but there I have to define __KERNEL__ around
> > include.
> >
> > In the BPF program, for the kfunc declarations.
> >
> > I guess I can split the header into two to avoid confusion. I agree it's a bit
> > ugly.
>
> I think we can add bpf_list_head and bpf_list_node to uapi/bpf.h
> The chances of them changing the size are pretty low.

Sounds good to me, the rest I'll keep in bpf_experimental.h.