On Tue, Oct 18, 2022 at 10:48 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> 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.
