* [PATCH bpf-next 0/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id
@ 2020-01-14 22:43 Martin KaFai Lau
2020-01-14 22:44 ` [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object Martin KaFai Lau
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Martin KaFai Lau @ 2020-01-14 22:43 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, David Miller, kernel-team, netdev
When a map is storing a kernel's struct, its
map_info->btf_vmlinux_value_type_id is set. The first map type
supporting it is BPF_MAP_TYPE_STRUCT_OPS.
This series adds support to dump this kind of map with BTF.
The first two patches are bug fixes which only applicable to
in bpf-next.
Please see individual patches for details.
Martin KaFai Lau (5):
bpftool: Fix a leak of btf object
bpftool: Fix missing BTF output for json during map dump
libbpf: Expose bpf_find_kernel_btf to libbpf_internal.h
bpftool: Add struct_ops map name
bpftool: Support dumping a map with btf_vmlinux_value_type_id
tools/bpf/bpftool/map.c | 84 ++++++++++++++++++---------------
tools/lib/bpf/libbpf.c | 3 +-
tools/lib/bpf/libbpf_internal.h | 1 +
3 files changed, 49 insertions(+), 39 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object
2020-01-14 22:43 [PATCH bpf-next 0/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id Martin KaFai Lau
@ 2020-01-14 22:44 ` Martin KaFai Lau
2020-01-15 1:10 ` Andrii Nakryiko
2020-01-14 22:44 ` [PATCH bpf-next 2/5] bpftool: Fix missing BTF output for json during map dump Martin KaFai Lau
` (3 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Martin KaFai Lau @ 2020-01-14 22:44 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, David Miller, kernel-team,
netdev, Paul Chaignon
When testing a map has btf or not, maps_have_btf() tests it by actually
getting a btf_fd from sys_bpf(BPF_BTF_GET_FD_BY_ID). However, it
forgot to btf__free() it.
In maps_have_btf() stage, there is no need to test it by really
calling sys_bpf(BPF_BTF_GET_FD_BY_ID). Testing non zero
info.btf_id is good enough.
Also, the err_close case is unnecessary, and also causes double
close() because the calling func do_dump() will close() all fds again.
Fixes: 99f9863a0c45 ("bpftool: Match maps by name")
Cc: Paul Chaignon <paul.chaignon@orange.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
tools/bpf/bpftool/map.c | 16 ++--------------
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index c01f76fa6876..e00e9e19d6b7 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -915,32 +915,20 @@ static int maps_have_btf(int *fds, int nb_fds)
{
struct bpf_map_info info = {};
__u32 len = sizeof(info);
- struct btf *btf = NULL;
int err, i;
for (i = 0; i < nb_fds; i++) {
err = bpf_obj_get_info_by_fd(fds[i], &info, &len);
if (err) {
p_err("can't get map info: %s", strerror(errno));
- goto err_close;
- }
-
- err = btf__get_from_id(info.btf_id, &btf);
- if (err) {
- p_err("failed to get btf");
- goto err_close;
+ return -1;
}
- if (!btf)
+ if (!info.btf_id)
return 0;
}
return 1;
-
-err_close:
- for (; i < nb_fds; i++)
- close(fds[i]);
- return -1;
}
static int
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH bpf-next 2/5] bpftool: Fix missing BTF output for json during map dump
2020-01-14 22:43 [PATCH bpf-next 0/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id Martin KaFai Lau
2020-01-14 22:44 ` [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object Martin KaFai Lau
@ 2020-01-14 22:44 ` Martin KaFai Lau
2020-01-15 1:34 ` Andrii Nakryiko
2020-01-14 22:44 ` [PATCH bpf-next 3/5] libbpf: Expose bpf_find_kernel_btf to libbpf_internal.h Martin KaFai Lau
` (2 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Martin KaFai Lau @ 2020-01-14 22:44 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, David Miller, kernel-team,
netdev, Paul Chaignon
The btf availability check is only done for plain text output.
It causes the whole BTF output went missing when json_output
is used.
This patch simplifies the logic a little by avoiding passing "int btf" to
map_dump().
For plain text output, the btf_wtr is only created when the map has
BTF (i.e. info->btf_id != 0). The nullness of "json_writer_t *wtr"
in map_dump() alone can decide if dumping BTF output is needed.
As long as wtr is not NULL, map_dump() will print out the BTF-described
data whenever a map has BTF available (i.e. info->btf_id != 0)
regardless of json or plain-text output.
In do_dump(), the "int btf" is also renamed to "int do_plain_btf".
Fixes: 99f9863a0c45 ("bpftool: Match maps by name")
Cc: Paul Chaignon <paul.chaignon@orange.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
tools/bpf/bpftool/map.c | 42 ++++++++++++++++++++---------------------
1 file changed, 20 insertions(+), 22 deletions(-)
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index e00e9e19d6b7..45c1eda6512c 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -933,7 +933,7 @@ static int maps_have_btf(int *fds, int nb_fds)
static int
map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
- bool enable_btf, bool show_header)
+ bool show_header)
{
void *key, *value, *prev_key;
unsigned int num_elems = 0;
@@ -950,18 +950,16 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
prev_key = NULL;
- if (enable_btf) {
- err = btf__get_from_id(info->btf_id, &btf);
- if (err || !btf) {
- /* enable_btf is true only if we've already checked
- * that all maps have BTF information.
- */
- p_err("failed to get btf");
- goto exit_free;
+ if (wtr) {
+ if (info->btf_id) {
+ err = btf__get_from_id(info->btf_id, &btf);
+ if (err || !btf) {
+ err = err ? : -ESRCH;
+ p_err("failed to get btf");
+ goto exit_free;
+ }
}
- }
- if (wtr) {
if (show_header) {
jsonw_start_object(wtr); /* map object */
show_map_header_json(info, wtr);
@@ -1009,7 +1007,7 @@ static int do_dump(int argc, char **argv)
{
json_writer_t *wtr = NULL, *btf_wtr = NULL;
struct bpf_map_info info = {};
- int nb_fds, i = 0, btf = 0;
+ int nb_fds, i = 0;
__u32 len = sizeof(info);
int *fds = NULL;
int err = -1;
@@ -1029,17 +1027,17 @@ static int do_dump(int argc, char **argv)
if (json_output) {
wtr = json_wtr;
} else {
- btf = maps_have_btf(fds, nb_fds);
- if (btf < 0)
+ int do_plain_btf;
+
+ do_plain_btf = maps_have_btf(fds, nb_fds);
+ if (do_plain_btf < 0)
goto exit_close;
- if (btf) {
+
+ if (do_plain_btf) {
btf_wtr = get_btf_writer();
- if (btf_wtr) {
- wtr = btf_wtr;
- } else {
+ wtr = btf_wtr;
+ if (!btf_wtr)
p_info("failed to create json writer for btf. falling back to plain output");
- btf = 0;
- }
}
}
@@ -1050,7 +1048,7 @@ static int do_dump(int argc, char **argv)
p_err("can't get map info: %s", strerror(errno));
break;
}
- err = map_dump(fds[i], &info, wtr, btf, nb_fds > 1);
+ err = map_dump(fds[i], &info, wtr, nb_fds > 1);
if (!wtr && i != nb_fds - 1)
printf("\n");
@@ -1061,7 +1059,7 @@ static int do_dump(int argc, char **argv)
if (wtr && nb_fds > 1)
jsonw_end_array(wtr); /* root array */
- if (btf)
+ if (btf_wtr)
jsonw_destroy(&btf_wtr);
exit_close:
for (; i < nb_fds; i++)
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH bpf-next 3/5] libbpf: Expose bpf_find_kernel_btf to libbpf_internal.h
2020-01-14 22:43 [PATCH bpf-next 0/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id Martin KaFai Lau
2020-01-14 22:44 ` [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object Martin KaFai Lau
2020-01-14 22:44 ` [PATCH bpf-next 2/5] bpftool: Fix missing BTF output for json during map dump Martin KaFai Lau
@ 2020-01-14 22:44 ` Martin KaFai Lau
2020-01-15 1:43 ` Andrii Nakryiko
2020-01-14 22:44 ` [PATCH bpf-next 4/5] bpftool: Add struct_ops map name Martin KaFai Lau
2020-01-14 22:44 ` [PATCH bpf-next 5/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id Martin KaFai Lau
4 siblings, 1 reply; 19+ messages in thread
From: Martin KaFai Lau @ 2020-01-14 22:44 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, David Miller, kernel-team, netdev
This patch exposes bpf_find_kernel_btf() to libbpf_internal.h.
It will be used in 'bpftool map dump' in a following patch
to dump a map with btf_vmlinux_value_type_id set.
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
tools/lib/bpf/libbpf.c | 3 +--
tools/lib/bpf/libbpf_internal.h | 1 +
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 0c229f00a67e..0666d6383285 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -73,7 +73,6 @@
#define __printf(a, b) __attribute__((format(printf, a, b)))
-static struct btf *bpf_find_kernel_btf(void);
static struct bpf_map *bpf_object__add_map(struct bpf_object *obj);
static struct bpf_program *bpf_object__find_prog_by_idx(struct bpf_object *obj,
int idx);
@@ -4339,7 +4338,7 @@ static struct btf *btf_load_raw(const char *path)
* Probe few well-known locations for vmlinux kernel image and try to load BTF
* data out of it to use for target BTF.
*/
-static struct btf *bpf_find_kernel_btf(void)
+struct btf *bpf_find_kernel_btf(void)
{
struct {
const char *path_fmt;
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 8c3afbd97747..8593954d8f81 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -95,6 +95,7 @@ static inline bool libbpf_validate_opts(const char *opts,
#define OPTS_GET(opts, field, fallback_value) \
(OPTS_HAS(opts, field) ? (opts)->field : fallback_value)
+struct btf *bpf_find_kernel_btf(void);
int parse_cpu_mask_str(const char *s, bool **mask, int *mask_sz);
int parse_cpu_mask_file(const char *fcpu, bool **mask, int *mask_sz);
int libbpf__load_raw_btf(const char *raw_types, size_t types_len,
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH bpf-next 4/5] bpftool: Add struct_ops map name
2020-01-14 22:43 [PATCH bpf-next 0/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id Martin KaFai Lau
` (2 preceding siblings ...)
2020-01-14 22:44 ` [PATCH bpf-next 3/5] libbpf: Expose bpf_find_kernel_btf to libbpf_internal.h Martin KaFai Lau
@ 2020-01-14 22:44 ` Martin KaFai Lau
2020-01-15 1:45 ` Andrii Nakryiko
2020-01-14 22:44 ` [PATCH bpf-next 5/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id Martin KaFai Lau
4 siblings, 1 reply; 19+ messages in thread
From: Martin KaFai Lau @ 2020-01-14 22:44 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, David Miller, kernel-team, netdev
This patch adds BPF_MAP_TYPE_STRUCT_OPS to "struct_ops" name mapping
so that "bpftool map show" can print the "struct_ops" map type
properly.
[root@arch-fb-vm1 bpf]# ~/devshare/fb-kernel/linux/tools/bpf/bpftool/bpftool map show id 8
8: struct_ops name dctcp flags 0x0
key 4B value 256B max_entries 1 memlock 4096B
btf_id 7
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
tools/bpf/bpftool/map.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 45c1eda6512c..4c5b15d736b6 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -48,6 +48,7 @@ const char * const map_type_name[] = {
[BPF_MAP_TYPE_QUEUE] = "queue",
[BPF_MAP_TYPE_STACK] = "stack",
[BPF_MAP_TYPE_SK_STORAGE] = "sk_storage",
+ [BPF_MAP_TYPE_STRUCT_OPS] = "struct_ops",
};
const size_t map_type_name_size = ARRAY_SIZE(map_type_name);
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH bpf-next 5/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id
2020-01-14 22:43 [PATCH bpf-next 0/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id Martin KaFai Lau
` (3 preceding siblings ...)
2020-01-14 22:44 ` [PATCH bpf-next 4/5] bpftool: Add struct_ops map name Martin KaFai Lau
@ 2020-01-14 22:44 ` Martin KaFai Lau
2020-01-15 1:49 ` Andrii Nakryiko
4 siblings, 1 reply; 19+ messages in thread
From: Martin KaFai Lau @ 2020-01-14 22:44 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, David Miller, kernel-team, netdev
This patch makes bpftool support dumping a map's value properly
when the map's value type is a type of the running kernel's btf.
(i.e. map_info.btf_vmlinux_value_type_id is set instead of
map_info.btf_value_type_id). The first usecase is for the
BPF_MAP_TYPE_STRUCT_OPS.
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
tools/bpf/bpftool/map.c | 43 +++++++++++++++++++++++++++++++----------
1 file changed, 33 insertions(+), 10 deletions(-)
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 4c5b15d736b6..d25f3b2355ad 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -20,6 +20,7 @@
#include "btf.h"
#include "json_writer.h"
#include "main.h"
+#include "libbpf_internal.h"
const char * const map_type_name[] = {
[BPF_MAP_TYPE_UNSPEC] = "unspec",
@@ -252,6 +253,7 @@ static int do_dump_btf(const struct btf_dumper *d,
struct bpf_map_info *map_info, void *key,
void *value)
{
+ __u32 value_id;
int ret;
/* start of key-value pair */
@@ -265,9 +267,12 @@ static int do_dump_btf(const struct btf_dumper *d,
goto err_end_obj;
}
+ value_id = map_info->btf_vmlinux_value_type_id ?
+ : map_info->btf_value_type_id;
+
if (!map_is_per_cpu(map_info->type)) {
jsonw_name(d->jw, "value");
- ret = btf_dumper_type(d, map_info->btf_value_type_id, value);
+ ret = btf_dumper_type(d, value_id, value);
} else {
unsigned int i, n, step;
@@ -279,8 +284,7 @@ static int do_dump_btf(const struct btf_dumper *d,
jsonw_start_object(d->jw);
jsonw_int_field(d->jw, "cpu", i);
jsonw_name(d->jw, "value");
- ret = btf_dumper_type(d, map_info->btf_value_type_id,
- value + i * step);
+ ret = btf_dumper_type(d, value_id, value + i * step);
jsonw_end_object(d->jw);
if (ret)
break;
@@ -932,6 +936,27 @@ static int maps_have_btf(int *fds, int nb_fds)
return 1;
}
+static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
+{
+ struct btf *btf = NULL;
+
+ if (info->btf_vmlinux_value_type_id) {
+ btf = bpf_find_kernel_btf();
+ if (IS_ERR(btf))
+ p_err("failed to get kernel btf");
+ } else if (info->btf_value_type_id) {
+ int err;
+
+ err = btf__get_from_id(info->btf_id, &btf);
+ if (err || !btf) {
+ p_err("failed to get btf");
+ btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
+ }
+ }
+
+ return btf;
+}
+
static int
map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
bool show_header)
@@ -952,13 +977,11 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
prev_key = NULL;
if (wtr) {
- if (info->btf_id) {
- err = btf__get_from_id(info->btf_id, &btf);
- if (err || !btf) {
- err = err ? : -ESRCH;
- p_err("failed to get btf");
- goto exit_free;
- }
+ btf = get_map_kv_btf(info);
+ if (IS_ERR(btf)) {
+ err = PTR_ERR(btf);
+ btf = NULL;
+ goto exit_free;
}
if (show_header) {
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object
2020-01-14 22:44 ` [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object Martin KaFai Lau
@ 2020-01-15 1:10 ` Andrii Nakryiko
2020-01-15 5:46 ` Martin Lau
0 siblings, 1 reply; 19+ messages in thread
From: Andrii Nakryiko @ 2020-01-15 1:10 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, David Miller,
Kernel Team, Networking, Paul Chaignon
On Tue, Jan 14, 2020 at 2:44 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> When testing a map has btf or not, maps_have_btf() tests it by actually
> getting a btf_fd from sys_bpf(BPF_BTF_GET_FD_BY_ID). However, it
> forgot to btf__free() it.
>
> In maps_have_btf() stage, there is no need to test it by really
> calling sys_bpf(BPF_BTF_GET_FD_BY_ID). Testing non zero
> info.btf_id is good enough.
>
> Also, the err_close case is unnecessary, and also causes double
> close() because the calling func do_dump() will close() all fds again.
>
> Fixes: 99f9863a0c45 ("bpftool: Match maps by name")
> Cc: Paul Chaignon <paul.chaignon@orange.com>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
this is clearly a simplification, but isn't do_dump still buggy? see below
> tools/bpf/bpftool/map.c | 16 ++--------------
> 1 file changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index c01f76fa6876..e00e9e19d6b7 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -915,32 +915,20 @@ static int maps_have_btf(int *fds, int nb_fds)
> {
> struct bpf_map_info info = {};
> __u32 len = sizeof(info);
> - struct btf *btf = NULL;
> int err, i;
>
> for (i = 0; i < nb_fds; i++) {
> err = bpf_obj_get_info_by_fd(fds[i], &info, &len);
> if (err) {
> p_err("can't get map info: %s", strerror(errno));
> - goto err_close;
> - }
> -
> - err = btf__get_from_id(info.btf_id, &btf);
> - if (err) {
> - p_err("failed to get btf");
> - goto err_close;
> + return -1;
> }
>
> - if (!btf)
> + if (!info.btf_id)
> return 0;
if info.btf_id is non-zero, shouldn't we immediately return 1 and be
done with it?
I'm also worried about do_dump logic. What's the behavior when some
maps do have BTF and some don't? Should we use btf_writer for all,
some or none maps for that case? I'd expect we'd use BTF info for
those maps that have BTF and fall back to raw output for those that
don't, but I'm not sure that how code behaves right now.
Maybe Paul can clarify...
> }
>
> return 1;
> -
> -err_close:
> - for (; i < nb_fds; i++)
> - close(fds[i]);
> - return -1;
> }
>
> static int
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH bpf-next 2/5] bpftool: Fix missing BTF output for json during map dump
2020-01-14 22:44 ` [PATCH bpf-next 2/5] bpftool: Fix missing BTF output for json during map dump Martin KaFai Lau
@ 2020-01-15 1:34 ` Andrii Nakryiko
2020-01-15 5:54 ` Martin Lau
0 siblings, 1 reply; 19+ messages in thread
From: Andrii Nakryiko @ 2020-01-15 1:34 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, David Miller,
Kernel Team, Networking, Paul Chaignon
On Tue, Jan 14, 2020 at 2:45 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> The btf availability check is only done for plain text output.
> It causes the whole BTF output went missing when json_output
> is used.
>
> This patch simplifies the logic a little by avoiding passing "int btf" to
> map_dump().
>
> For plain text output, the btf_wtr is only created when the map has
> BTF (i.e. info->btf_id != 0). The nullness of "json_writer_t *wtr"
> in map_dump() alone can decide if dumping BTF output is needed.
> As long as wtr is not NULL, map_dump() will print out the BTF-described
> data whenever a map has BTF available (i.e. info->btf_id != 0)
> regardless of json or plain-text output.
>
> In do_dump(), the "int btf" is also renamed to "int do_plain_btf".
>
> Fixes: 99f9863a0c45 ("bpftool: Match maps by name")
> Cc: Paul Chaignon <paul.chaignon@orange.com>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
just one nit below
Acked-by: Andrii Nakryiko <andriin@fb.com>
> tools/bpf/bpftool/map.c | 42 ++++++++++++++++++++---------------------
> 1 file changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index e00e9e19d6b7..45c1eda6512c 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -933,7 +933,7 @@ static int maps_have_btf(int *fds, int nb_fds)
>
> static int
> map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
> - bool enable_btf, bool show_header)
> + bool show_header)
> {
> void *key, *value, *prev_key;
> unsigned int num_elems = 0;
> @@ -950,18 +950,16 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
>
> prev_key = NULL;
>
> - if (enable_btf) {
> - err = btf__get_from_id(info->btf_id, &btf);
> - if (err || !btf) {
> - /* enable_btf is true only if we've already checked
> - * that all maps have BTF information.
> - */
> - p_err("failed to get btf");
> - goto exit_free;
> + if (wtr) {
> + if (info->btf_id) {
combine into if (wtr && info->btf_id) and reduce nestedness?
> + err = btf__get_from_id(info->btf_id, &btf);
> + if (err || !btf) {
> + err = err ? : -ESRCH;
> + p_err("failed to get btf");
> + goto exit_free;
> + }
[...]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH bpf-next 3/5] libbpf: Expose bpf_find_kernel_btf to libbpf_internal.h
2020-01-14 22:44 ` [PATCH bpf-next 3/5] libbpf: Expose bpf_find_kernel_btf to libbpf_internal.h Martin KaFai Lau
@ 2020-01-15 1:43 ` Andrii Nakryiko
2020-01-15 5:56 ` Martin Lau
0 siblings, 1 reply; 19+ messages in thread
From: Andrii Nakryiko @ 2020-01-15 1:43 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, David Miller,
Kernel Team, Networking
On Tue, Jan 14, 2020 at 2:44 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> This patch exposes bpf_find_kernel_btf() to libbpf_internal.h.
> It will be used in 'bpftool map dump' in a following patch
> to dump a map with btf_vmlinux_value_type_id set.
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
This function comes up not a first time, I guess it makes sense to
finally make it a public API. We should also try to minimize number of
internal APIs used by bpftool.
So if no one objects to expose it, should we call it a bit more
precisely and according to libbpf naming guidelines:
libbpf_load_kernel_btf? libbpf_find_kernel_btf is acceptable, but it
does more than just finding, thus "load". It should also probably live
in btf.c+btf.h? WDYT?
> tools/lib/bpf/libbpf.c | 3 +--
> tools/lib/bpf/libbpf_internal.h | 1 +
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
[...]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH bpf-next 4/5] bpftool: Add struct_ops map name
2020-01-14 22:44 ` [PATCH bpf-next 4/5] bpftool: Add struct_ops map name Martin KaFai Lau
@ 2020-01-15 1:45 ` Andrii Nakryiko
0 siblings, 0 replies; 19+ messages in thread
From: Andrii Nakryiko @ 2020-01-15 1:45 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, David Miller,
Kernel Team, Networking
On Tue, Jan 14, 2020 at 2:45 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> This patch adds BPF_MAP_TYPE_STRUCT_OPS to "struct_ops" name mapping
> so that "bpftool map show" can print the "struct_ops" map type
> properly.
>
> [root@arch-fb-vm1 bpf]# ~/devshare/fb-kernel/linux/tools/bpf/bpftool/bpftool map show id 8
> 8: struct_ops name dctcp flags 0x0
> key 4B value 256B max_entries 1 memlock 4096B
> btf_id 7
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
Acked-by: Andrii Nakryiko <andriin@fb.com>
> tools/bpf/bpftool/map.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 45c1eda6512c..4c5b15d736b6 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -48,6 +48,7 @@ const char * const map_type_name[] = {
> [BPF_MAP_TYPE_QUEUE] = "queue",
> [BPF_MAP_TYPE_STACK] = "stack",
> [BPF_MAP_TYPE_SK_STORAGE] = "sk_storage",
> + [BPF_MAP_TYPE_STRUCT_OPS] = "struct_ops",
> };
>
> const size_t map_type_name_size = ARRAY_SIZE(map_type_name);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH bpf-next 5/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id
2020-01-14 22:44 ` [PATCH bpf-next 5/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id Martin KaFai Lau
@ 2020-01-15 1:49 ` Andrii Nakryiko
2020-01-15 6:04 ` Martin Lau
0 siblings, 1 reply; 19+ messages in thread
From: Andrii Nakryiko @ 2020-01-15 1:49 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, David Miller,
Kernel Team, Networking
On Tue, Jan 14, 2020 at 2:46 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> This patch makes bpftool support dumping a map's value properly
> when the map's value type is a type of the running kernel's btf.
> (i.e. map_info.btf_vmlinux_value_type_id is set instead of
> map_info.btf_value_type_id). The first usecase is for the
> BPF_MAP_TYPE_STRUCT_OPS.
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
> tools/bpf/bpftool/map.c | 43 +++++++++++++++++++++++++++++++----------
> 1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 4c5b15d736b6..d25f3b2355ad 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -20,6 +20,7 @@
> #include "btf.h"
> #include "json_writer.h"
> #include "main.h"
> +#include "libbpf_internal.h"
>
> const char * const map_type_name[] = {
> [BPF_MAP_TYPE_UNSPEC] = "unspec",
> @@ -252,6 +253,7 @@ static int do_dump_btf(const struct btf_dumper *d,
> struct bpf_map_info *map_info, void *key,
> void *value)
> {
> + __u32 value_id;
> int ret;
>
> /* start of key-value pair */
> @@ -265,9 +267,12 @@ static int do_dump_btf(const struct btf_dumper *d,
> goto err_end_obj;
> }
>
> + value_id = map_info->btf_vmlinux_value_type_id ?
> + : map_info->btf_value_type_id;
> +
> if (!map_is_per_cpu(map_info->type)) {
> jsonw_name(d->jw, "value");
> - ret = btf_dumper_type(d, map_info->btf_value_type_id, value);
> + ret = btf_dumper_type(d, value_id, value);
> } else {
> unsigned int i, n, step;
>
> @@ -279,8 +284,7 @@ static int do_dump_btf(const struct btf_dumper *d,
> jsonw_start_object(d->jw);
> jsonw_int_field(d->jw, "cpu", i);
> jsonw_name(d->jw, "value");
> - ret = btf_dumper_type(d, map_info->btf_value_type_id,
> - value + i * step);
> + ret = btf_dumper_type(d, value_id, value + i * step);
> jsonw_end_object(d->jw);
> if (ret)
> break;
> @@ -932,6 +936,27 @@ static int maps_have_btf(int *fds, int nb_fds)
> return 1;
> }
>
> +static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
> +{
> + struct btf *btf = NULL;
> +
> + if (info->btf_vmlinux_value_type_id) {
> + btf = bpf_find_kernel_btf();
If there are multiple maps we are dumping, it might become quite
costly to re-read and re-parse kernel BTF all the time. Can we lazily
load it, when required, and cache instead?
> + if (IS_ERR(btf))
> + p_err("failed to get kernel btf");
> + } else if (info->btf_value_type_id) {
> + int err;
> +
> + err = btf__get_from_id(info->btf_id, &btf);
> + if (err || !btf) {
> + p_err("failed to get btf");
> + btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
> + }
> + }
> +
> + return btf;
> +}
> +
> static int
> map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
> bool show_header)
> @@ -952,13 +977,11 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
> prev_key = NULL;
>
> if (wtr) {
> - if (info->btf_id) {
> - err = btf__get_from_id(info->btf_id, &btf);
> - if (err || !btf) {
> - err = err ? : -ESRCH;
> - p_err("failed to get btf");
> - goto exit_free;
> - }
> + btf = get_map_kv_btf(info);
> + if (IS_ERR(btf)) {
> + err = PTR_ERR(btf);
> + btf = NULL;
> + goto exit_free;
> }
>
> if (show_header) {
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object
2020-01-15 1:10 ` Andrii Nakryiko
@ 2020-01-15 5:46 ` Martin Lau
2020-01-15 6:40 ` Andrii Nakryiko
2020-01-16 8:00 ` Paul Chaignon
0 siblings, 2 replies; 19+ messages in thread
From: Martin Lau @ 2020-01-15 5:46 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, David Miller,
Kernel Team, Networking, Paul Chaignon
On Tue, Jan 14, 2020 at 05:10:03PM -0800, Andrii Nakryiko wrote:
> On Tue, Jan 14, 2020 at 2:44 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > When testing a map has btf or not, maps_have_btf() tests it by actually
> > getting a btf_fd from sys_bpf(BPF_BTF_GET_FD_BY_ID). However, it
> > forgot to btf__free() it.
> >
> > In maps_have_btf() stage, there is no need to test it by really
> > calling sys_bpf(BPF_BTF_GET_FD_BY_ID). Testing non zero
> > info.btf_id is good enough.
> >
> > Also, the err_close case is unnecessary, and also causes double
> > close() because the calling func do_dump() will close() all fds again.
> >
> > Fixes: 99f9863a0c45 ("bpftool: Match maps by name")
> > Cc: Paul Chaignon <paul.chaignon@orange.com>
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > ---
>
> this is clearly a simplification, but isn't do_dump still buggy? see below
>
> > tools/bpf/bpftool/map.c | 16 ++--------------
> > 1 file changed, 2 insertions(+), 14 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > index c01f76fa6876..e00e9e19d6b7 100644
> > --- a/tools/bpf/bpftool/map.c
> > +++ b/tools/bpf/bpftool/map.c
> > @@ -915,32 +915,20 @@ static int maps_have_btf(int *fds, int nb_fds)
> > {
> > struct bpf_map_info info = {};
> > __u32 len = sizeof(info);
> > - struct btf *btf = NULL;
> > int err, i;
> >
> > for (i = 0; i < nb_fds; i++) {
> > err = bpf_obj_get_info_by_fd(fds[i], &info, &len);
> > if (err) {
> > p_err("can't get map info: %s", strerror(errno));
> > - goto err_close;
> > - }
> > -
> > - err = btf__get_from_id(info.btf_id, &btf);
> > - if (err) {
> > - p_err("failed to get btf");
> > - goto err_close;
> > + return -1;
> > }
> >
> > - if (!btf)
> > + if (!info.btf_id)
> > return 0;
>
> if info.btf_id is non-zero, shouldn't we immediately return 1 and be
> done with it?
No. maps_have_btf() returns 1 only if all the maps have btf.
>
> I'm also worried about do_dump logic. What's the behavior when some
> maps do have BTF and some don't? Should we use btf_writer for all,
> some or none maps for that case?
For plain_text, btf output is either for all or for none.
It is the intention of the "Fixes" patch if I read it correctly,
and it is kept as is in this bug fix.
It will become clear by doing a plain text dump on maps with and
without btf. They are very different.
Can the output format for with and without BTF somehow merged for
plain text? May be if it is still common to have no-BTF map
going forward but how this may look like will need another
discussion.
> I'd expect we'd use BTF info for
> those maps that have BTF and fall back to raw output for those that
> don't, but I'm not sure that how code behaves right now.
The json_output is doing what you described, print BTF info
whenever available.
>
> Maybe Paul can clarify...
>
>
> > }
> >
> > return 1;
> > -
> > -err_close:
> > - for (; i < nb_fds; i++)
> > - close(fds[i]);
> > - return -1;
> > }
> >
> > static int
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH bpf-next 2/5] bpftool: Fix missing BTF output for json during map dump
2020-01-15 1:34 ` Andrii Nakryiko
@ 2020-01-15 5:54 ` Martin Lau
2020-01-15 6:36 ` Andrii Nakryiko
0 siblings, 1 reply; 19+ messages in thread
From: Martin Lau @ 2020-01-15 5:54 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, David Miller,
Kernel Team, Networking, Paul Chaignon
On Tue, Jan 14, 2020 at 05:34:33PM -0800, Andrii Nakryiko wrote:
> On Tue, Jan 14, 2020 at 2:45 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > The btf availability check is only done for plain text output.
> > It causes the whole BTF output went missing when json_output
> > is used.
> >
> > This patch simplifies the logic a little by avoiding passing "int btf" to
> > map_dump().
> >
> > For plain text output, the btf_wtr is only created when the map has
> > BTF (i.e. info->btf_id != 0). The nullness of "json_writer_t *wtr"
> > in map_dump() alone can decide if dumping BTF output is needed.
> > As long as wtr is not NULL, map_dump() will print out the BTF-described
> > data whenever a map has BTF available (i.e. info->btf_id != 0)
> > regardless of json or plain-text output.
> >
> > In do_dump(), the "int btf" is also renamed to "int do_plain_btf".
> >
> > Fixes: 99f9863a0c45 ("bpftool: Match maps by name")
> > Cc: Paul Chaignon <paul.chaignon@orange.com>
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > ---
>
> just one nit below
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
> > tools/bpf/bpftool/map.c | 42 ++++++++++++++++++++---------------------
> > 1 file changed, 20 insertions(+), 22 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > index e00e9e19d6b7..45c1eda6512c 100644
> > --- a/tools/bpf/bpftool/map.c
> > +++ b/tools/bpf/bpftool/map.c
> > @@ -933,7 +933,7 @@ static int maps_have_btf(int *fds, int nb_fds)
> >
> > static int
> > map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
> > - bool enable_btf, bool show_header)
> > + bool show_header)
> > {
> > void *key, *value, *prev_key;
> > unsigned int num_elems = 0;
> > @@ -950,18 +950,16 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
> >
> > prev_key = NULL;
> >
> > - if (enable_btf) {
> > - err = btf__get_from_id(info->btf_id, &btf);
> > - if (err || !btf) {
> > - /* enable_btf is true only if we've already checked
> > - * that all maps have BTF information.
> > - */
> > - p_err("failed to get btf");
> > - goto exit_free;
> > + if (wtr) {
> > + if (info->btf_id) {
>
> combine into if (wtr && info->btf_id) and reduce nestedness?
There is other logic under the same "if (wtr)".
Thus, it is better to leave it as is.
and this indentation will be gone in patch 5.
>
>
> > + err = btf__get_from_id(info->btf_id, &btf);
> > + if (err || !btf) {
> > + err = err ? : -ESRCH;
> > + p_err("failed to get btf");
> > + goto exit_free;
> > + }
>
> [...]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH bpf-next 3/5] libbpf: Expose bpf_find_kernel_btf to libbpf_internal.h
2020-01-15 1:43 ` Andrii Nakryiko
@ 2020-01-15 5:56 ` Martin Lau
0 siblings, 0 replies; 19+ messages in thread
From: Martin Lau @ 2020-01-15 5:56 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, David Miller,
Kernel Team, Networking
On Tue, Jan 14, 2020 at 05:43:58PM -0800, Andrii Nakryiko wrote:
> On Tue, Jan 14, 2020 at 2:44 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > This patch exposes bpf_find_kernel_btf() to libbpf_internal.h.
> > It will be used in 'bpftool map dump' in a following patch
> > to dump a map with btf_vmlinux_value_type_id set.
> >
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > ---
>
> This function comes up not a first time, I guess it makes sense to
> finally make it a public API. We should also try to minimize number of
> internal APIs used by bpftool.
>
> So if no one objects to expose it, should we call it a bit more
> precisely and according to libbpf naming guidelines:
> libbpf_load_kernel_btf? libbpf_find_kernel_btf is acceptable, but it
> does more than just finding, thus "load". It should also probably live
> in btf.c+btf.h? WDYT?
Sure, I will add it as LIBBPF_API in btf.h
>
>
> > tools/lib/bpf/libbpf.c | 3 +--
> > tools/lib/bpf/libbpf_internal.h | 1 +
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
>
> [...]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH bpf-next 5/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id
2020-01-15 1:49 ` Andrii Nakryiko
@ 2020-01-15 6:04 ` Martin Lau
2020-01-15 6:39 ` Andrii Nakryiko
0 siblings, 1 reply; 19+ messages in thread
From: Martin Lau @ 2020-01-15 6:04 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, David Miller,
Kernel Team, Networking
On Tue, Jan 14, 2020 at 05:49:00PM -0800, Andrii Nakryiko wrote:
> On Tue, Jan 14, 2020 at 2:46 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > This patch makes bpftool support dumping a map's value properly
> > when the map's value type is a type of the running kernel's btf.
> > (i.e. map_info.btf_vmlinux_value_type_id is set instead of
> > map_info.btf_value_type_id). The first usecase is for the
> > BPF_MAP_TYPE_STRUCT_OPS.
> >
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > ---
> > tools/bpf/bpftool/map.c | 43 +++++++++++++++++++++++++++++++----------
> > 1 file changed, 33 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > index 4c5b15d736b6..d25f3b2355ad 100644
> > --- a/tools/bpf/bpftool/map.c
> > +++ b/tools/bpf/bpftool/map.c
> > @@ -20,6 +20,7 @@
> > #include "btf.h"
> > #include "json_writer.h"
> > #include "main.h"
> > +#include "libbpf_internal.h"
> >
> > const char * const map_type_name[] = {
> > [BPF_MAP_TYPE_UNSPEC] = "unspec",
> > @@ -252,6 +253,7 @@ static int do_dump_btf(const struct btf_dumper *d,
> > struct bpf_map_info *map_info, void *key,
> > void *value)
> > {
> > + __u32 value_id;
> > int ret;
> >
> > /* start of key-value pair */
> > @@ -265,9 +267,12 @@ static int do_dump_btf(const struct btf_dumper *d,
> > goto err_end_obj;
> > }
> >
> > + value_id = map_info->btf_vmlinux_value_type_id ?
> > + : map_info->btf_value_type_id;
> > +
> > if (!map_is_per_cpu(map_info->type)) {
> > jsonw_name(d->jw, "value");
> > - ret = btf_dumper_type(d, map_info->btf_value_type_id, value);
> > + ret = btf_dumper_type(d, value_id, value);
> > } else {
> > unsigned int i, n, step;
> >
> > @@ -279,8 +284,7 @@ static int do_dump_btf(const struct btf_dumper *d,
> > jsonw_start_object(d->jw);
> > jsonw_int_field(d->jw, "cpu", i);
> > jsonw_name(d->jw, "value");
> > - ret = btf_dumper_type(d, map_info->btf_value_type_id,
> > - value + i * step);
> > + ret = btf_dumper_type(d, value_id, value + i * step);
> > jsonw_end_object(d->jw);
> > if (ret)
> > break;
> > @@ -932,6 +936,27 @@ static int maps_have_btf(int *fds, int nb_fds)
> > return 1;
> > }
> >
> > +static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
> > +{
> > + struct btf *btf = NULL;
> > +
> > + if (info->btf_vmlinux_value_type_id) {
> > + btf = bpf_find_kernel_btf();
>
> If there are multiple maps we are dumping, it might become quite
> costly to re-read and re-parse kernel BTF all the time. Can we lazily
> load it, when required,
It is loaded lazily.
> and cache instead?
Cache it in bpftool/map.c? Sure.
>
> > + if (IS_ERR(btf))
> > + p_err("failed to get kernel btf");
> > + } else if (info->btf_value_type_id) {
> > + int err;
> > +
> > + err = btf__get_from_id(info->btf_id, &btf);
> > + if (err || !btf) {
> > + p_err("failed to get btf");
> > + btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
> > + }
> > + }
> > +
> > + return btf;
> > +}
> > +
> > static int
> > map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
> > bool show_header)
> > @@ -952,13 +977,11 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
> > prev_key = NULL;
> >
> > if (wtr) {
> > - if (info->btf_id) {
> > - err = btf__get_from_id(info->btf_id, &btf);
> > - if (err || !btf) {
> > - err = err ? : -ESRCH;
> > - p_err("failed to get btf");
> > - goto exit_free;
> > - }
> > + btf = get_map_kv_btf(info);
> > + if (IS_ERR(btf)) {
> > + err = PTR_ERR(btf);
> > + btf = NULL;
> > + goto exit_free;
> > }
> >
> > if (show_header) {
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH bpf-next 2/5] bpftool: Fix missing BTF output for json during map dump
2020-01-15 5:54 ` Martin Lau
@ 2020-01-15 6:36 ` Andrii Nakryiko
0 siblings, 0 replies; 19+ messages in thread
From: Andrii Nakryiko @ 2020-01-15 6:36 UTC (permalink / raw)
To: Martin Lau
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, David Miller,
Kernel Team, Networking, Paul Chaignon
On Tue, Jan 14, 2020 at 9:54 PM Martin Lau <kafai@fb.com> wrote:
>
> On Tue, Jan 14, 2020 at 05:34:33PM -0800, Andrii Nakryiko wrote:
> > On Tue, Jan 14, 2020 at 2:45 PM Martin KaFai Lau <kafai@fb.com> wrote:
> > >
> > > The btf availability check is only done for plain text output.
> > > It causes the whole BTF output went missing when json_output
> > > is used.
> > >
> > > This patch simplifies the logic a little by avoiding passing "int btf" to
> > > map_dump().
> > >
> > > For plain text output, the btf_wtr is only created when the map has
> > > BTF (i.e. info->btf_id != 0). The nullness of "json_writer_t *wtr"
> > > in map_dump() alone can decide if dumping BTF output is needed.
> > > As long as wtr is not NULL, map_dump() will print out the BTF-described
> > > data whenever a map has BTF available (i.e. info->btf_id != 0)
> > > regardless of json or plain-text output.
> > >
> > > In do_dump(), the "int btf" is also renamed to "int do_plain_btf".
> > >
> > > Fixes: 99f9863a0c45 ("bpftool: Match maps by name")
> > > Cc: Paul Chaignon <paul.chaignon@orange.com>
> > > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > > ---
> >
> > just one nit below
> >
> > Acked-by: Andrii Nakryiko <andriin@fb.com>
> >
> > > tools/bpf/bpftool/map.c | 42 ++++++++++++++++++++---------------------
> > > 1 file changed, 20 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > > index e00e9e19d6b7..45c1eda6512c 100644
> > > --- a/tools/bpf/bpftool/map.c
> > > +++ b/tools/bpf/bpftool/map.c
> > > @@ -933,7 +933,7 @@ static int maps_have_btf(int *fds, int nb_fds)
> > >
> > > static int
> > > map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
> > > - bool enable_btf, bool show_header)
> > > + bool show_header)
> > > {
> > > void *key, *value, *prev_key;
> > > unsigned int num_elems = 0;
> > > @@ -950,18 +950,16 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
> > >
> > > prev_key = NULL;
> > >
> > > - if (enable_btf) {
> > > - err = btf__get_from_id(info->btf_id, &btf);
> > > - if (err || !btf) {
> > > - /* enable_btf is true only if we've already checked
> > > - * that all maps have BTF information.
> > > - */
> > > - p_err("failed to get btf");
> > > - goto exit_free;
> > > + if (wtr) {
> > > + if (info->btf_id) {
> >
> > combine into if (wtr && info->btf_id) and reduce nestedness?
> There is other logic under the same "if (wtr)".
> Thus, it is better to leave it as is.
My bad, missed those tiny minuses in diff :) Of course that would be incorrect.
> and this indentation will be gone in patch 5.
>
> >
> >
> > > + err = btf__get_from_id(info->btf_id, &btf);
> > > + if (err || !btf) {
> > > + err = err ? : -ESRCH;
> > > + p_err("failed to get btf");
> > > + goto exit_free;
> > > + }
> >
> > [...]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH bpf-next 5/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id
2020-01-15 6:04 ` Martin Lau
@ 2020-01-15 6:39 ` Andrii Nakryiko
0 siblings, 0 replies; 19+ messages in thread
From: Andrii Nakryiko @ 2020-01-15 6:39 UTC (permalink / raw)
To: Martin Lau
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, David Miller,
Kernel Team, Networking
On Tue, Jan 14, 2020 at 10:04 PM Martin Lau <kafai@fb.com> wrote:
>
> On Tue, Jan 14, 2020 at 05:49:00PM -0800, Andrii Nakryiko wrote:
> > On Tue, Jan 14, 2020 at 2:46 PM Martin KaFai Lau <kafai@fb.com> wrote:
> > >
> > > This patch makes bpftool support dumping a map's value properly
> > > when the map's value type is a type of the running kernel's btf.
> > > (i.e. map_info.btf_vmlinux_value_type_id is set instead of
> > > map_info.btf_value_type_id). The first usecase is for the
> > > BPF_MAP_TYPE_STRUCT_OPS.
> > >
> > > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > > ---
> > > tools/bpf/bpftool/map.c | 43 +++++++++++++++++++++++++++++++----------
> > > 1 file changed, 33 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > > index 4c5b15d736b6..d25f3b2355ad 100644
> > > --- a/tools/bpf/bpftool/map.c
> > > +++ b/tools/bpf/bpftool/map.c
> > > @@ -20,6 +20,7 @@
> > > #include "btf.h"
> > > #include "json_writer.h"
> > > #include "main.h"
> > > +#include "libbpf_internal.h"
> > >
> > > const char * const map_type_name[] = {
> > > [BPF_MAP_TYPE_UNSPEC] = "unspec",
> > > @@ -252,6 +253,7 @@ static int do_dump_btf(const struct btf_dumper *d,
> > > struct bpf_map_info *map_info, void *key,
> > > void *value)
> > > {
> > > + __u32 value_id;
> > > int ret;
> > >
> > > /* start of key-value pair */
> > > @@ -265,9 +267,12 @@ static int do_dump_btf(const struct btf_dumper *d,
> > > goto err_end_obj;
> > > }
> > >
> > > + value_id = map_info->btf_vmlinux_value_type_id ?
> > > + : map_info->btf_value_type_id;
> > > +
> > > if (!map_is_per_cpu(map_info->type)) {
> > > jsonw_name(d->jw, "value");
> > > - ret = btf_dumper_type(d, map_info->btf_value_type_id, value);
> > > + ret = btf_dumper_type(d, value_id, value);
> > > } else {
> > > unsigned int i, n, step;
> > >
> > > @@ -279,8 +284,7 @@ static int do_dump_btf(const struct btf_dumper *d,
> > > jsonw_start_object(d->jw);
> > > jsonw_int_field(d->jw, "cpu", i);
> > > jsonw_name(d->jw, "value");
> > > - ret = btf_dumper_type(d, map_info->btf_value_type_id,
> > > - value + i * step);
> > > + ret = btf_dumper_type(d, value_id, value + i * step);
> > > jsonw_end_object(d->jw);
> > > if (ret)
> > > break;
> > > @@ -932,6 +936,27 @@ static int maps_have_btf(int *fds, int nb_fds)
> > > return 1;
> > > }
> > >
> > > +static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
> > > +{
> > > + struct btf *btf = NULL;
> > > +
> > > + if (info->btf_vmlinux_value_type_id) {
> > > + btf = bpf_find_kernel_btf();
> >
> > If there are multiple maps we are dumping, it might become quite
> > costly to re-read and re-parse kernel BTF all the time. Can we lazily
> > load it, when required,
> It is loaded lazily.
>
yeah, it was meant as "lazy load and cache" vs "pre-load always"
(which makes caching simpler).
> > and cache instead?
> Cache it in bpftool/map.c? Sure.
yeah, for the duration of dumping
>
> >
> > > + if (IS_ERR(btf))
> > > + p_err("failed to get kernel btf");
> > > + } else if (info->btf_value_type_id) {
> > > + int err;
> > > +
> > > + err = btf__get_from_id(info->btf_id, &btf);
> > > + if (err || !btf) {
> > > + p_err("failed to get btf");
> > > + btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
> > > + }
> > > + }
> > > +
> > > + return btf;
> > > +}
> > > +
> > > static int
> > > map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
> > > bool show_header)
> > > @@ -952,13 +977,11 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr,
> > > prev_key = NULL;
> > >
> > > if (wtr) {
> > > - if (info->btf_id) {
> > > - err = btf__get_from_id(info->btf_id, &btf);
> > > - if (err || !btf) {
> > > - err = err ? : -ESRCH;
> > > - p_err("failed to get btf");
> > > - goto exit_free;
> > > - }
> > > + btf = get_map_kv_btf(info);
> > > + if (IS_ERR(btf)) {
> > > + err = PTR_ERR(btf);
> > > + btf = NULL;
> > > + goto exit_free;
> > > }
> > >
> > > if (show_header) {
> > > --
> > > 2.17.1
> > >
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object
2020-01-15 5:46 ` Martin Lau
@ 2020-01-15 6:40 ` Andrii Nakryiko
2020-01-16 8:00 ` Paul Chaignon
1 sibling, 0 replies; 19+ messages in thread
From: Andrii Nakryiko @ 2020-01-15 6:40 UTC (permalink / raw)
To: Martin Lau
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, David Miller,
Kernel Team, Networking, Paul Chaignon
On Tue, Jan 14, 2020 at 9:46 PM Martin Lau <kafai@fb.com> wrote:
>
> On Tue, Jan 14, 2020 at 05:10:03PM -0800, Andrii Nakryiko wrote:
> > On Tue, Jan 14, 2020 at 2:44 PM Martin KaFai Lau <kafai@fb.com> wrote:
> > >
> > > When testing a map has btf or not, maps_have_btf() tests it by actually
> > > getting a btf_fd from sys_bpf(BPF_BTF_GET_FD_BY_ID). However, it
> > > forgot to btf__free() it.
> > >
> > > In maps_have_btf() stage, there is no need to test it by really
> > > calling sys_bpf(BPF_BTF_GET_FD_BY_ID). Testing non zero
> > > info.btf_id is good enough.
> > >
> > > Also, the err_close case is unnecessary, and also causes double
> > > close() because the calling func do_dump() will close() all fds again.
> > >
> > > Fixes: 99f9863a0c45 ("bpftool: Match maps by name")
> > > Cc: Paul Chaignon <paul.chaignon@orange.com>
> > > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > > ---
> >
> > this is clearly a simplification, but isn't do_dump still buggy? see below
> >
> > > tools/bpf/bpftool/map.c | 16 ++--------------
> > > 1 file changed, 2 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > > index c01f76fa6876..e00e9e19d6b7 100644
> > > --- a/tools/bpf/bpftool/map.c
> > > +++ b/tools/bpf/bpftool/map.c
> > > @@ -915,32 +915,20 @@ static int maps_have_btf(int *fds, int nb_fds)
> > > {
> > > struct bpf_map_info info = {};
> > > __u32 len = sizeof(info);
> > > - struct btf *btf = NULL;
> > > int err, i;
> > >
> > > for (i = 0; i < nb_fds; i++) {
> > > err = bpf_obj_get_info_by_fd(fds[i], &info, &len);
> > > if (err) {
> > > p_err("can't get map info: %s", strerror(errno));
> > > - goto err_close;
> > > - }
> > > -
> > > - err = btf__get_from_id(info.btf_id, &btf);
> > > - if (err) {
> > > - p_err("failed to get btf");
> > > - goto err_close;
> > > + return -1;
> > > }
> > >
> > > - if (!btf)
> > > + if (!info.btf_id)
> > > return 0;
> >
> > if info.btf_id is non-zero, shouldn't we immediately return 1 and be
> > done with it?
> No. maps_have_btf() returns 1 only if all the maps have btf.
>
> >
> > I'm also worried about do_dump logic. What's the behavior when some
> > maps do have BTF and some don't? Should we use btf_writer for all,
> > some or none maps for that case?
> For plain_text, btf output is either for all or for none.
> It is the intention of the "Fixes" patch if I read it correctly,
> and it is kept as is in this bug fix.
> It will become clear by doing a plain text dump on maps with and
> without btf. They are very different.
>
> Can the output format for with and without BTF somehow merged for
> plain text? May be if it is still common to have no-BTF map
> going forward but how this may look like will need another
> discussion.
I see, ok, seems like that behavior was intentional, I didn't mean to
start a new discussion about format :)
Acked-by: Andrii Nakryiko <andriin@fb.com>
>
> > I'd expect we'd use BTF info for
> > those maps that have BTF and fall back to raw output for those that
> > don't, but I'm not sure that how code behaves right now.
> The json_output is doing what you described, print BTF info
> whenever available.
>
> >
> > Maybe Paul can clarify...
> >
> >
> > > }
> > >
> > > return 1;
> > > -
> > > -err_close:
> > > - for (; i < nb_fds; i++)
> > > - close(fds[i]);
> > > - return -1;
> > > }
> > >
> > > static int
> > > --
> > > 2.17.1
> > >
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object
2020-01-15 5:46 ` Martin Lau
2020-01-15 6:40 ` Andrii Nakryiko
@ 2020-01-16 8:00 ` Paul Chaignon
1 sibling, 0 replies; 19+ messages in thread
From: Paul Chaignon @ 2020-01-16 8:00 UTC (permalink / raw)
To: Martin Lau
Cc: Paul Chaignon, bpf, Alexei Starovoitov, Daniel Borkmann,
David Miller, Kernel Team, Networking, Andrii Nakryiko
On Wed, Jan 15, 2020 at 6:46 AM Martin Lau <kafai@fb.com> wrote:
> On Tue, Jan 14, 2020 at 05:10:03PM -0800, Andrii Nakryiko wrote:
> > On Tue, Jan 14, 2020 at 2:44 PM Martin KaFai Lau <kafai@fb.com> wrote:
> > >
> > > When testing a map has btf or not, maps_have_btf() tests it by actually
> > > getting a btf_fd from sys_bpf(BPF_BTF_GET_FD_BY_ID). However, it
> > > forgot to btf__free() it.
> > >
> > > In maps_have_btf() stage, there is no need to test it by really
> > > calling sys_bpf(BPF_BTF_GET_FD_BY_ID). Testing non zero
> > > info.btf_id is good enough.
> > >
> > > Also, the err_close case is unnecessary, and also causes double
> > > close() because the calling func do_dump() will close() all fds again.
> > >
> > > Fixes: 99f9863a0c45 ("bpftool: Match maps by name")
> > > Cc: Paul Chaignon <paul.chaignon@orange.com>
> > > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > > ---
> >
> > this is clearly a simplification, but isn't do_dump still buggy? see
> below
> >
> > > tools/bpf/bpftool/map.c | 16 ++--------------
> > > 1 file changed, 2 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > > index c01f76fa6876..e00e9e19d6b7 100644
> > > --- a/tools/bpf/bpftool/map.c
> > > +++ b/tools/bpf/bpftool/map.c
> > > @@ -915,32 +915,20 @@ static int maps_have_btf(int *fds, int nb_fds)
> > > {
> > > struct bpf_map_info info = {};
> > > __u32 len = sizeof(info);
> > > - struct btf *btf = NULL;
> > > int err, i;
> > >
> > > for (i = 0; i < nb_fds; i++) {
> > > err = bpf_obj_get_info_by_fd(fds[i], &info, &len);
> > > if (err) {
> > > p_err("can't get map info: %s",
> strerror(errno));
> > > - goto err_close;
> > > - }
> > > -
> > > - err = btf__get_from_id(info.btf_id, &btf);
> > > - if (err) {
> > > - p_err("failed to get btf");
> > > - goto err_close;
> > > + return -1;
> > > }
> > >
> > > - if (!btf)
> > > + if (!info.btf_id)
> > > return 0;
> >
> > if info.btf_id is non-zero, shouldn't we immediately return 1 and be
> > done with it?
> No. maps_have_btf() returns 1 only if all the maps have btf.
>
> >
> > I'm also worried about do_dump logic. What's the behavior when some
> > maps do have BTF and some don't? Should we use btf_writer for all,
> > some or none maps for that case?
> For plain_text, btf output is either for all or for none.
> It is the intention of the "Fixes" patch if I read it correctly,
> and it is kept as is in this bug fix.
> It will become clear by doing a plain text dump on maps with and
> without btf. They are very different.
Yes, that was the intent of my patch. As you said, I wasn't sure mixes
of BTF/non-BTF maps were common, especially in a batch sharing the same
name.
Thanks for the fixes Martin!
>
> Can the output format for with and without BTF somehow merged for
> plain text? May be if it is still common to have no-BTF map
> going forward but how this may look like will need another
> discussion.
>
> > I'd expect we'd use BTF info for
> > those maps that have BTF and fall back to raw output for those that
> > don't, but I'm not sure that how code behaves right now.
> The json_output is doing what you described, print BTF info
> whenever available.
>
> >
> > Maybe Paul can clarify...
> >
> >
> > > }
> > >
> > > return 1;
> > > -
> > > -err_close:
> > > - for (; i < nb_fds; i++)
> > > - close(fds[i]);
> > > - return -1;
> > > }
> > >
> > > static int
> > > --
> > > 2.17.1
> > >
>
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-01-16 8:00 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-14 22:43 [PATCH bpf-next 0/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id Martin KaFai Lau
2020-01-14 22:44 ` [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object Martin KaFai Lau
2020-01-15 1:10 ` Andrii Nakryiko
2020-01-15 5:46 ` Martin Lau
2020-01-15 6:40 ` Andrii Nakryiko
2020-01-16 8:00 ` Paul Chaignon
2020-01-14 22:44 ` [PATCH bpf-next 2/5] bpftool: Fix missing BTF output for json during map dump Martin KaFai Lau
2020-01-15 1:34 ` Andrii Nakryiko
2020-01-15 5:54 ` Martin Lau
2020-01-15 6:36 ` Andrii Nakryiko
2020-01-14 22:44 ` [PATCH bpf-next 3/5] libbpf: Expose bpf_find_kernel_btf to libbpf_internal.h Martin KaFai Lau
2020-01-15 1:43 ` Andrii Nakryiko
2020-01-15 5:56 ` Martin Lau
2020-01-14 22:44 ` [PATCH bpf-next 4/5] bpftool: Add struct_ops map name Martin KaFai Lau
2020-01-15 1:45 ` Andrii Nakryiko
2020-01-14 22:44 ` [PATCH bpf-next 5/5] bpftool: Support dumping a map with btf_vmlinux_value_type_id Martin KaFai Lau
2020-01-15 1:49 ` Andrii Nakryiko
2020-01-15 6:04 ` Martin Lau
2020-01-15 6:39 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).