diff mbox series

[RFC,bpf-next,1/3] bpf: add mapper macro for bpf_cmd enum

Message ID 20231207222755.3920286-2-andrii@kernel.org (mailing list archive)
State Handled Elsewhere
Headers show
Series BPF FS mount options parsing follow ups | expand

Commit Message

Andrii Nakryiko Dec. 7, 2023, 10:27 p.m. UTC 
Use similar approach to enum bpf_func_id and generate enumerators using
a macro with macro callback. This approach allows to generate derivative
tables for string-based lookups and whatnot. In this particular case,
this mapper macro will be used for parsing BPF FS delegate_cmds mount
option and their human-readable output format in mount info.

Validated no regressions using before/after BTF through
`bpftool btf dump <file> format c` command.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/uapi/linux/bpf.h       | 81 ++++++++++++++++++----------------
 tools/include/uapi/linux/bpf.h | 81 ++++++++++++++++++----------------
 2 files changed, 86 insertions(+), 76 deletions(-)

Comments

Alexei Starovoitov Dec. 12, 2023, 2:40 a.m. UTC | #1 
On Thu, Dec 7, 2023 at 2:28 PM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> +#define __BPF_CMD_MAPPER(FN, ctx...)                                   \
> +       FN(BPF_MAP_CREATE, 0)                                           \
> +       FN(BPF_MAP_LOOKUP_ELEM, 1)                                      \
> +       FN(BPF_MAP_UPDATE_ELEM, 2)                                      \
> +       FN(BPF_MAP_DELETE_ELEM, 3)                                      \
> +       FN(BPF_MAP_GET_NEXT_KEY, 4)                                     \

So macro conversion across 4 main enums in uapi/bpf.h
is just to do:
+static const struct constant_table cmd_kvs[] = {
+       __BPF_CMD_MAPPER(__BPF_KV_FN)
+       {}
+};

on the kernel side,
right?

While in libbpf we already hard code name to value in arrays:
prog_type_name[], map_type_name[]

which probably will remain as-is, since libbpf needs to be
built independently from the kernel.
(unless we will say that tools/uapi/bpf.h is part of libbpf,
which probably not a good way).

There are more pros than cons in this enum uglification,
but cons are definitely staring in the face.

Have you considered other options?
Like using vmlinix BTF for parsing bpffs delegation?
[14083] ENUM 'bpf_cmd' encoding=UNSIGNED size=4 vlen=39
        'BPF_MAP_CREATE' val=0
        'BPF_MAP_LOOKUP_ELEM' val=1
        'BPF_MAP_UPDATE_ELEM' val=2
        'BPF_MAP_DELETE_ELEM' val=3
        'BPF_MAP_GET_NEXT_KEY' val=4
        'BPF_PROG_LOAD' val=5

Names and values are available.
btf_find_by_name_kind(vmlinux_btf, "bpd_cmd", BTF_KIND_ENUM);
is fast enough.

I suspect you'll argue that you don't want to tie in
bpffs delegation parsing with BTF ;)

While I can preemptively answer that in the case vmlinux BTF
is not available it's fine not to parse names and rely on hex.
Andrii Nakryiko Dec. 12, 2023, 4:01 a.m. UTC | #2 
On Mon, Dec 11, 2023 at 6:40 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Dec 7, 2023 at 2:28 PM Andrii Nakryiko <andrii@kernel.org> wrote:
> >
> > +#define __BPF_CMD_MAPPER(FN, ctx...)                                   \
> > +       FN(BPF_MAP_CREATE, 0)                                           \
> > +       FN(BPF_MAP_LOOKUP_ELEM, 1)                                      \
> > +       FN(BPF_MAP_UPDATE_ELEM, 2)                                      \
> > +       FN(BPF_MAP_DELETE_ELEM, 3)                                      \
> > +       FN(BPF_MAP_GET_NEXT_KEY, 4)                                     \
>
> So macro conversion across 4 main enums in uapi/bpf.h
> is just to do:
> +static const struct constant_table cmd_kvs[] = {
> +       __BPF_CMD_MAPPER(__BPF_KV_FN)
> +       {}
> +};
>
> on the kernel side,
> right?

Right.

>
> While in libbpf we already hard code name to value in arrays:
> prog_type_name[], map_type_name[]
>
> which probably will remain as-is, since libbpf needs to be
> built independently from the kernel.
> (unless we will say that tools/uapi/bpf.h is part of libbpf,
> which probably not a good way).

No, we can easily use this on the libbpf side as well. Libbpf syncs
the latest UAPI headers ([0]) and uses them during build time.

  [0] https://github.com/libbpf/libbpf/tree/master/include/uapi/linux

>
> There are more pros than cons in this enum uglification,
> but cons are definitely staring in the face.
>
> Have you considered other options?
> Like using vmlinix BTF for parsing bpffs delegation?
> [14083] ENUM 'bpf_cmd' encoding=UNSIGNED size=4 vlen=39
>         'BPF_MAP_CREATE' val=0
>         'BPF_MAP_LOOKUP_ELEM' val=1
>         'BPF_MAP_UPDATE_ELEM' val=2
>         'BPF_MAP_DELETE_ELEM' val=3
>         'BPF_MAP_GET_NEXT_KEY' val=4
>         'BPF_PROG_LOAD' val=5
>
> Names and values are available.
> btf_find_by_name_kind(vmlinux_btf, "bpd_cmd", BTF_KIND_ENUM);
> is fast enough.
>
> I suspect you'll argue that you don't want to tie in
> bpffs delegation parsing with BTF ;)
>

Yep, that's the reason I didn't go for it in the initial version, of
course. But it's fine.

> While I can preemptively answer that in the case vmlinux BTF
> is not available it's fine not to parse names and rely on hex.

It's fine, I can do optional BTF-based parsing, if that's what you prefer.
Alexei Starovoitov Dec. 12, 2023, 4:06 a.m. UTC | #3 
On Mon, Dec 11, 2023 at 8:01 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
>
> > While I can preemptively answer that in the case vmlinux BTF
> > is not available it's fine not to parse names and rely on hex.
>
> It's fine, I can do optional BTF-based parsing, if that's what you prefer.

I prefer to keep uapi/bpf.h as-is and use BTF.
But I'd like to hear what Daniel's and Martin's preferences are.
Martin KaFai Lau Dec. 13, 2023, 1:37 a.m. UTC | #4 
On 12/11/23 8:06 PM, Alexei Starovoitov wrote:
> On Mon, Dec 11, 2023 at 8:01 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>>
>>> While I can preemptively answer that in the case vmlinux BTF
>>> is not available it's fine not to parse names and rely on hex.
>>
>> It's fine, I can do optional BTF-based parsing, if that's what you prefer.
> 
> I prefer to keep uapi/bpf.h as-is and use BTF.
> But I'd like to hear what Daniel's and Martin's preferences are.

I think user will find it useful to have a more readable uapi header file. It 
would be nice to keep the current uapi/bpf.h form if there is another solution.
Andrii Nakryiko Dec. 13, 2023, 5:26 p.m. UTC | #5 
On Tue, Dec 12, 2023 at 5:37 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 12/11/23 8:06 PM, Alexei Starovoitov wrote:
> > On Mon, Dec 11, 2023 at 8:01 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >>
> >>
> >>> While I can preemptively answer that in the case vmlinux BTF
> >>> is not available it's fine not to parse names and rely on hex.
> >>
> >> It's fine, I can do optional BTF-based parsing, if that's what you prefer.
> >
> > I prefer to keep uapi/bpf.h as-is and use BTF.
> > But I'd like to hear what Daniel's and Martin's preferences are.
>
> I think user will find it useful to have a more readable uapi header file. It

I'd say having numeric values make it more readable, but that's a
separate discussion. I purposefully kept full BPF_-prefixed names
intact for readability, as opposed to what we do for enum bpf_func_id.

> would be nice to keep the current uapi/bpf.h form if there is another solution.

Ok, I'll use BTF, no problem.
diff mbox series

Patch

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index e0545201b55f..d05ea24ace3f 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -893,47 +893,52 @@  union bpf_iter_link_info {
  *	to the object have been closed and no references remain pinned to the
  *	filesystem or attached (for example, bound to a program or device).
  */
+#define __BPF_CMD_MAPPER(FN, ctx...)					\
+	FN(BPF_MAP_CREATE, 0)						\
+	FN(BPF_MAP_LOOKUP_ELEM, 1)					\
+	FN(BPF_MAP_UPDATE_ELEM, 2)					\
+	FN(BPF_MAP_DELETE_ELEM, 3)					\
+	FN(BPF_MAP_GET_NEXT_KEY, 4)					\
+	FN(BPF_PROG_LOAD, 5)						\
+	FN(BPF_OBJ_PIN, 6)						\
+	FN(BPF_OBJ_GET, 7)						\
+	FN(BPF_PROG_ATTACH, 8)						\
+	FN(BPF_PROG_DETACH, 9)						\
+	FN(BPF_PROG_TEST_RUN, 10)					\
+	FN(BPF_PROG_RUN, 10) /* alias for BPF_PROG_TEST_RUN */		\
+	FN(BPF_PROG_GET_NEXT_ID, 11)					\
+	FN(BPF_MAP_GET_NEXT_ID, 12)					\
+	FN(BPF_PROG_GET_FD_BY_ID, 13)					\
+	FN(BPF_MAP_GET_FD_BY_ID, 14)					\
+	FN(BPF_OBJ_GET_INFO_BY_FD, 15)					\
+	FN(BPF_PROG_QUERY, 16)						\
+	FN(BPF_RAW_TRACEPOINT_OPEN, 17)					\
+	FN(BPF_BTF_LOAD, 18)						\
+	FN(BPF_BTF_GET_FD_BY_ID, 19)					\
+	FN(BPF_TASK_FD_QUERY, 20)					\
+	FN(BPF_MAP_LOOKUP_AND_DELETE_ELEM, 21)				\
+	FN(BPF_MAP_FREEZE, 22)						\
+	FN(BPF_BTF_GET_NEXT_ID, 23)					\
+	FN(BPF_MAP_LOOKUP_BATCH, 24)					\
+	FN(BPF_MAP_LOOKUP_AND_DELETE_BATCH, 25)				\
+	FN(BPF_MAP_UPDATE_BATCH, 26)					\
+	FN(BPF_MAP_DELETE_BATCH, 27)					\
+	FN(BPF_LINK_CREATE, 28)						\
+	FN(BPF_LINK_UPDATE, 29)						\
+	FN(BPF_LINK_GET_FD_BY_ID, 30)					\
+	FN(BPF_LINK_GET_NEXT_ID, 31)					\
+	FN(BPF_ENABLE_STATS, 32)					\
+	FN(BPF_ITER_CREATE, 33)						\
+	FN(BPF_LINK_DETACH, 34)						\
+	FN(BPF_PROG_BIND_MAP, 35)					\
+	FN(BPF_TOKEN_CREATE, 36)					\
+	/* */
+#define __BPF_CMD_FN(x, y) x = y,
 enum bpf_cmd {
-	BPF_MAP_CREATE,
-	BPF_MAP_LOOKUP_ELEM,
-	BPF_MAP_UPDATE_ELEM,
-	BPF_MAP_DELETE_ELEM,
-	BPF_MAP_GET_NEXT_KEY,
-	BPF_PROG_LOAD,
-	BPF_OBJ_PIN,
-	BPF_OBJ_GET,
-	BPF_PROG_ATTACH,
-	BPF_PROG_DETACH,
-	BPF_PROG_TEST_RUN,
-	BPF_PROG_RUN = BPF_PROG_TEST_RUN,
-	BPF_PROG_GET_NEXT_ID,
-	BPF_MAP_GET_NEXT_ID,
-	BPF_PROG_GET_FD_BY_ID,
-	BPF_MAP_GET_FD_BY_ID,
-	BPF_OBJ_GET_INFO_BY_FD,
-	BPF_PROG_QUERY,
-	BPF_RAW_TRACEPOINT_OPEN,
-	BPF_BTF_LOAD,
-	BPF_BTF_GET_FD_BY_ID,
-	BPF_TASK_FD_QUERY,
-	BPF_MAP_LOOKUP_AND_DELETE_ELEM,
-	BPF_MAP_FREEZE,
-	BPF_BTF_GET_NEXT_ID,
-	BPF_MAP_LOOKUP_BATCH,
-	BPF_MAP_LOOKUP_AND_DELETE_BATCH,
-	BPF_MAP_UPDATE_BATCH,
-	BPF_MAP_DELETE_BATCH,
-	BPF_LINK_CREATE,
-	BPF_LINK_UPDATE,
-	BPF_LINK_GET_FD_BY_ID,
-	BPF_LINK_GET_NEXT_ID,
-	BPF_ENABLE_STATS,
-	BPF_ITER_CREATE,
-	BPF_LINK_DETACH,
-	BPF_PROG_BIND_MAP,
-	BPF_TOKEN_CREATE,
+	__BPF_CMD_MAPPER(__BPF_CMD_FN)
 	__MAX_BPF_CMD,
 };
+#undef __BPF_CMD_FN
 
 enum bpf_map_type {
 	BPF_MAP_TYPE_UNSPEC,
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index e0545201b55f..d05ea24ace3f 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -893,47 +893,52 @@  union bpf_iter_link_info {
  *	to the object have been closed and no references remain pinned to the
  *	filesystem or attached (for example, bound to a program or device).
  */
+#define __BPF_CMD_MAPPER(FN, ctx...)					\
+	FN(BPF_MAP_CREATE, 0)						\
+	FN(BPF_MAP_LOOKUP_ELEM, 1)					\
+	FN(BPF_MAP_UPDATE_ELEM, 2)					\
+	FN(BPF_MAP_DELETE_ELEM, 3)					\
+	FN(BPF_MAP_GET_NEXT_KEY, 4)					\
+	FN(BPF_PROG_LOAD, 5)						\
+	FN(BPF_OBJ_PIN, 6)						\
+	FN(BPF_OBJ_GET, 7)						\
+	FN(BPF_PROG_ATTACH, 8)						\
+	FN(BPF_PROG_DETACH, 9)						\
+	FN(BPF_PROG_TEST_RUN, 10)					\
+	FN(BPF_PROG_RUN, 10) /* alias for BPF_PROG_TEST_RUN */		\
+	FN(BPF_PROG_GET_NEXT_ID, 11)					\
+	FN(BPF_MAP_GET_NEXT_ID, 12)					\
+	FN(BPF_PROG_GET_FD_BY_ID, 13)					\
+	FN(BPF_MAP_GET_FD_BY_ID, 14)					\
+	FN(BPF_OBJ_GET_INFO_BY_FD, 15)					\
+	FN(BPF_PROG_QUERY, 16)						\
+	FN(BPF_RAW_TRACEPOINT_OPEN, 17)					\
+	FN(BPF_BTF_LOAD, 18)						\
+	FN(BPF_BTF_GET_FD_BY_ID, 19)					\
+	FN(BPF_TASK_FD_QUERY, 20)					\
+	FN(BPF_MAP_LOOKUP_AND_DELETE_ELEM, 21)				\
+	FN(BPF_MAP_FREEZE, 22)						\
+	FN(BPF_BTF_GET_NEXT_ID, 23)					\
+	FN(BPF_MAP_LOOKUP_BATCH, 24)					\
+	FN(BPF_MAP_LOOKUP_AND_DELETE_BATCH, 25)				\
+	FN(BPF_MAP_UPDATE_BATCH, 26)					\
+	FN(BPF_MAP_DELETE_BATCH, 27)					\
+	FN(BPF_LINK_CREATE, 28)						\
+	FN(BPF_LINK_UPDATE, 29)						\
+	FN(BPF_LINK_GET_FD_BY_ID, 30)					\
+	FN(BPF_LINK_GET_NEXT_ID, 31)					\
+	FN(BPF_ENABLE_STATS, 32)					\
+	FN(BPF_ITER_CREATE, 33)						\
+	FN(BPF_LINK_DETACH, 34)						\
+	FN(BPF_PROG_BIND_MAP, 35)					\
+	FN(BPF_TOKEN_CREATE, 36)					\
+	/* */
+#define __BPF_CMD_FN(x, y) x = y,
 enum bpf_cmd {
-	BPF_MAP_CREATE,
-	BPF_MAP_LOOKUP_ELEM,
-	BPF_MAP_UPDATE_ELEM,
-	BPF_MAP_DELETE_ELEM,
-	BPF_MAP_GET_NEXT_KEY,
-	BPF_PROG_LOAD,
-	BPF_OBJ_PIN,
-	BPF_OBJ_GET,
-	BPF_PROG_ATTACH,
-	BPF_PROG_DETACH,
-	BPF_PROG_TEST_RUN,
-	BPF_PROG_RUN = BPF_PROG_TEST_RUN,
-	BPF_PROG_GET_NEXT_ID,
-	BPF_MAP_GET_NEXT_ID,
-	BPF_PROG_GET_FD_BY_ID,
-	BPF_MAP_GET_FD_BY_ID,
-	BPF_OBJ_GET_INFO_BY_FD,
-	BPF_PROG_QUERY,
-	BPF_RAW_TRACEPOINT_OPEN,
-	BPF_BTF_LOAD,
-	BPF_BTF_GET_FD_BY_ID,
-	BPF_TASK_FD_QUERY,
-	BPF_MAP_LOOKUP_AND_DELETE_ELEM,
-	BPF_MAP_FREEZE,
-	BPF_BTF_GET_NEXT_ID,
-	BPF_MAP_LOOKUP_BATCH,
-	BPF_MAP_LOOKUP_AND_DELETE_BATCH,
-	BPF_MAP_UPDATE_BATCH,
-	BPF_MAP_DELETE_BATCH,
-	BPF_LINK_CREATE,
-	BPF_LINK_UPDATE,
-	BPF_LINK_GET_FD_BY_ID,
-	BPF_LINK_GET_NEXT_ID,
-	BPF_ENABLE_STATS,
-	BPF_ITER_CREATE,
-	BPF_LINK_DETACH,
-	BPF_PROG_BIND_MAP,
-	BPF_TOKEN_CREATE,
+	__BPF_CMD_MAPPER(__BPF_CMD_FN)
 	__MAX_BPF_CMD,
 };
+#undef __BPF_CMD_FN
 
 enum bpf_map_type {
 	BPF_MAP_TYPE_UNSPEC,