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.
