* Re: [PATCH bpf-next] libbpf: try to add a name for bpftool self-created maps
2022-08-08 9:33 [PATCH bpf-next] libbpf: try to add a name for bpftool self-created maps Hangbin Liu
@ 2022-08-08 13:45 ` Quentin Monnet
2022-08-08 22:34 ` Andrii Nakryiko
2022-08-08 22:33 ` Andrii Nakryiko
1 sibling, 1 reply; 4+ messages in thread
From: Quentin Monnet @ 2022-08-08 13:45 UTC (permalink / raw)
To: Hangbin Liu, netdev
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf
On 08/08/2022 10:33, Hangbin Liu wrote:
> As discussed before[1], the bpftool self-created maps can appear in final
> map show output due to deferred removal in kernel. These maps don't have
> a name, which would make users confused about where it comes from.
>
> Adding names for these maps could make users know what these maps used for.
> It also could make some tests (like test_offload.py, which skip base maps
> without names as a workaround) filter them out.
>
> As Quentin suggested, add a small wrapper to fall back with no name
> if kernel is not supported.
>
> [1] https://lore.kernel.org/bpf/CAEf4BzY66WPKQbDe74AKZ6nFtZjq5e+G3Ji2egcVytB9R6_sGQ@mail.gmail.com/
>
> Suggested-by: Quentin Monnet <quentin@isovalent.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> tools/lib/bpf/libbpf.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 77e3797cf75a..db4f1a02b9e0 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4423,6 +4423,22 @@ static int probe_kern_prog_name(void)
> return probe_fd(ret);
> }
>
> +static int probe_kern_map_name(enum bpf_map_type map_type,
> + const char *map_name, __u32 key_size,
> + __u32 value_size, __u32 max_entries,
> + const struct bpf_map_create_opts *opts)
> +{
> + int map;
> +
> + map = bpf_map_create(map_type, map_name, key_size, value_size, max_entries, opts);
> + if (map < 0 && errno == EINVAL) {
> + /* Retry without name */
> + map = bpf_map_create(map_type, NULL, key_size, value_size, max_entries, opts);
> + }
> +
> + return map;
> +}
> +
> static int probe_kern_global_data(void)
> {
> char *cp, errmsg[STRERR_BUFSIZE];
> @@ -4434,7 +4450,7 @@ static int probe_kern_global_data(void)
> };
> int ret, map, insn_cnt = ARRAY_SIZE(insns);
>
> - map = bpf_map_create(BPF_MAP_TYPE_ARRAY, NULL, sizeof(int), 32, 1, NULL);
> + map = probe_kern_map_name(BPF_MAP_TYPE_ARRAY, "global_data", sizeof(int), 32, 1, NULL);
Thanks! Some comments on the naming: It reads strange here to "probe"
for the maps, given that we still need to compare the return value
below. Maybe use something else instead of "probe_kern_map_name()"?
Maybe "map_create_adjust_name()" or "map_create_compat()" (or something
else)?
Regarding "global_data": If the intent is to filter out these maps from
the output of bpftool for example, should we use a common prefix for the
three of them? "libbpf_" or "probe_"? Or even something shorter? I know
we're limited to 15 characters.
Quentin
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH bpf-next] libbpf: try to add a name for bpftool self-created maps
2022-08-08 9:33 [PATCH bpf-next] libbpf: try to add a name for bpftool self-created maps Hangbin Liu
2022-08-08 13:45 ` Quentin Monnet
@ 2022-08-08 22:33 ` Andrii Nakryiko
1 sibling, 0 replies; 4+ messages in thread
From: Andrii Nakryiko @ 2022-08-08 22:33 UTC (permalink / raw)
To: Hangbin Liu
Cc: netdev, Quentin Monnet, Andrii Nakryiko, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
bpf
On Mon, Aug 8, 2022 at 2:33 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> As discussed before[1], the bpftool self-created maps can appear in final
> map show output due to deferred removal in kernel. These maps don't have
> a name, which would make users confused about where it comes from.
>
> Adding names for these maps could make users know what these maps used for.
> It also could make some tests (like test_offload.py, which skip base maps
> without names as a workaround) filter them out.
>
> As Quentin suggested, add a small wrapper to fall back with no name
> if kernel is not supported.
>
> [1] https://lore.kernel.org/bpf/CAEf4BzY66WPKQbDe74AKZ6nFtZjq5e+G3Ji2egcVytB9R6_sGQ@mail.gmail.com/
>
> Suggested-by: Quentin Monnet <quentin@isovalent.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> tools/lib/bpf/libbpf.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
Hm... this is pretty ugly. And unfortunately I mostly ignored the
previous discussion, sorry about that.
To give you my view of this. I've considered making bpf_prog_load()
and bpf_map_create() smart enough to ignore name if kernel doesn't
support program or map name, respectively. I previously decided to
keep it simple and follow the approach that low-level APIs (which
bpf_prog_load and bpf_map_create are) should not do any such
adjustments to user arguments. But we are not 100% following that
anyways (e.g., bpf_prog_load does retries and is more clever about
log_level, as one example), and it does seem very unlikely that user
will explicitly want to cause failure by passing non-NULL name to
bpf_prog_load/bpf_map_create on old kernels that don't support names.
It's way more likely that a user doesn't want to care and always wants
to specify a name and use it if the kernel supports it.
So I propose we just teach bpf_map_create and bpf_prog_load to ignore
non-NULL names if the kernel doesn't support it. Please double check
if map name support was added at the same time as prog name support,
and if yes, just use kernel_supports(NULL, FEAT_PROG_NAME) in both
cases. If not, we can add a dedicated map name feature detector.
If a user really-really wants to force failure (for some reason),
they'd need to do their own bpf() syscall. Which I think is totally
fine, given the upside of not having to care about whether the kernel
supports names.
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 77e3797cf75a..db4f1a02b9e0 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4423,6 +4423,22 @@ static int probe_kern_prog_name(void)
> return probe_fd(ret);
> }
>
> +static int probe_kern_map_name(enum bpf_map_type map_type,
> + const char *map_name, __u32 key_size,
> + __u32 value_size, __u32 max_entries,
> + const struct bpf_map_create_opts *opts)
> +{
> + int map;
> +
> + map = bpf_map_create(map_type, map_name, key_size, value_size, max_entries, opts);
> + if (map < 0 && errno == EINVAL) {
> + /* Retry without name */
> + map = bpf_map_create(map_type, NULL, key_size, value_size, max_entries, opts);
> + }
> +
> + return map;
> +}
> +
> static int probe_kern_global_data(void)
> {
> char *cp, errmsg[STRERR_BUFSIZE];
> @@ -4434,7 +4450,7 @@ static int probe_kern_global_data(void)
> };
> int ret, map, insn_cnt = ARRAY_SIZE(insns);
>
> - map = bpf_map_create(BPF_MAP_TYPE_ARRAY, NULL, sizeof(int), 32, 1, NULL);
> + map = probe_kern_map_name(BPF_MAP_TYPE_ARRAY, "global_data", sizeof(int), 32, 1, NULL);
> if (map < 0) {
> ret = -errno;
> cp = libbpf_strerror_r(ret, errmsg, sizeof(errmsg));
> @@ -4567,7 +4583,7 @@ static int probe_kern_array_mmap(void)
> LIBBPF_OPTS(bpf_map_create_opts, opts, .map_flags = BPF_F_MMAPABLE);
> int fd;
>
> - fd = bpf_map_create(BPF_MAP_TYPE_ARRAY, NULL, sizeof(int), sizeof(int), 1, &opts);
> + fd = probe_kern_map_name(BPF_MAP_TYPE_ARRAY, "array_mmap", sizeof(int), sizeof(int), 1, &opts);
> return probe_fd(fd);
> }
>
> @@ -4614,7 +4630,7 @@ static int probe_prog_bind_map(void)
> };
> int ret, map, prog, insn_cnt = ARRAY_SIZE(insns);
>
> - map = bpf_map_create(BPF_MAP_TYPE_ARRAY, NULL, sizeof(int), 32, 1, NULL);
> + map = probe_kern_map_name(BPF_MAP_TYPE_ARRAY, "bind_map_detect", sizeof(int), 32, 1, NULL);
> if (map < 0) {
> ret = -errno;
> cp = libbpf_strerror_r(ret, errmsg, sizeof(errmsg));
> --
> 2.35.3
>
^ permalink raw reply [flat|nested] 4+ messages in thread