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

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Dave Marchevsky <davemarchevsky@meta.com>,
	Delyan Kratunov <delyank@meta.com>
Subject: Re: [PATCH bpf-next v2 09/25] bpf: Support bpf_list_head in map values
Date: Tue, 18 Oct 2022 18:59:16 -0700	[thread overview]
Message-ID: <20221019015916.cyrmxrqskztsn7gf@macbook-pro-4.dhcp.thefacebook.com> (raw)
In-Reply-To: <20221013062303.896469-10-memxor@gmail.com>

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?

> 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 ?

> +#else
> +
> +struct bpf_list_head {
> +	__u64 __a;
> +	__u64 __b;
> +} __attribute__((aligned(8)));
> +
> +struct bpf_list_node {
> +	__u64 __a;
> +	__u64 __b;
> +} __attribute__((aligned(8)));
> +
> +#endif

> +
> +#ifndef __KERNEL__
> +#endif

hmm.