| Message ID | 20190315183211.GB24482@kernel.org |
|---|---|
| State | Deferred |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [1/1] bpftool: Allow referring to maps by its name | expand |
On Fri, 15 Mar 2019 15:41:41 -0300, Arnaldo Carvalho de Melo wrote: > While developing 'perf trace' and looking at BPF maps it puts in place I > noticed that one needs to first use 'bpftool map' to lookup a map id to > then use 'bpftool map dump map id <map-id-looked-up>'. > > This is needed because everytime we restart 'perf trace' the map IDs > gets changed so we need to do the ID lookup again. > > To speed up this sequence, allow specifying just the map name, look up > its ID and then use the existing routines as if the user had provided > the map id. I can see how it could be useful for quick debug. Names are not guaranteed to be unique, though, do you not care about potential duplicates? > This: > > # bpftool map > 13: lpm_trie flags 0x1 > key 8B value 8B max_entries 1 memlock 4096B > 14: lpm_trie flags 0x1 > key 20B value 8B max_entries 1 memlock 4096B > 15: lpm_trie flags 0x1 > key 8B value 8B max_entries 1 memlock 4096B > 16: lpm_trie flags 0x1 > key 20B value 8B max_entries 1 memlock 4096B > 17: lpm_trie flags 0x1 > key 8B value 8B max_entries 1 memlock 4096B > 18: lpm_trie flags 0x1 > key 20B value 8B max_entries 1 memlock 4096B > 21: lpm_trie flags 0x1 > key 8B value 8B max_entries 1 memlock 4096B > 22: lpm_trie flags 0x1 > key 20B value 8B max_entries 1 memlock 4096B > 28: perf_event_array name __augmented_sys flags 0x0 > key 4B value 4B max_entries 8 memlock 4096B > 29: array name syscalls flags 0x0 > key 4B value 1B max_entries 512 memlock 8192B > 30: hash name pids_filtered flags 0x0 > key 4B value 1B max_entries 64 memlock 8192B > # > # bpftool map dump id 30 > [{ > "key": 26554, > "value": true > },{ > "key": 2592, > "value": true > } > ] > # > > Now is equivalent to: > > # bpftool map dump pids_filtered Please do keep the current model of name val, IOW dump map *name* pids.. > [{ > "key": 26554, > "value": true > },{ > "key": 2592, > "value": true > } > ] > # > > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Daniel Borkmann <daniel@iogearbox.net> > Cc: Jakub Kicinski <jakub.kicinski@netronome.com> > cc: Jiri Olsa <jolsa@kernel.org> > Cc: Martin KaFai Lau <kafai@fb.com> > Cc: Namhyung Kim <namhyung@kernel.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Quentin Monnet <quentin.monnet@netronome.com> > Cc: Song Liu <songliubraving@fb.com> > Cc: Stanislav Fomichev <sdf@google.com> > Cc: Yonghong Song <yhs@fb.com> > Link: https://lkml.kernel.org/n/tip-rrnxuhvety3j3rf0r9zlbfro@git.kernel.org > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> Please also update the bash completions and all the help/mans/docs.
2019-03-15 12:16 UTC-0700 ~ Jakub Kicinski <jakub.kicinski@netronome.com> > On Fri, 15 Mar 2019 15:41:41 -0300, Arnaldo Carvalho de Melo wrote: >> While developing 'perf trace' and looking at BPF maps it puts in place I >> noticed that one needs to first use 'bpftool map' to lookup a map id to >> then use 'bpftool map dump map id <map-id-looked-up>'. >> >> This is needed because everytime we restart 'perf trace' the map IDs >> gets changed so we need to do the ID lookup again. >> >> To speed up this sequence, allow specifying just the map name, look up >> its ID and then use the existing routines as if the user had provided >> the map id. > > I can see how it could be useful for quick debug. Names are not > guaranteed to be unique, though, do you not care about potential > duplicates? > Jakub beat me to it by a few seconds :). I discussed possible workarounds on that point with him though, so here are my two cents. I see two options: first one would be to return the ids of, and then dump info about all maps having that name. The second one would be to simply return an error if duplicate names are found. We agreed that returning an error if there are duplicates is probably the best way to go, because dealing with several maps for anything else than "bpftool map show" would be problematic (clearly, we don't want to update all maps with a given name at once). Of course if you have other suggestions, feel free to propose :). Best, Quentin
Em Fri, Mar 15, 2019 at 07:41:35PM +0000, Quentin Monnet escreveu: > 2019-03-15 12:16 UTC-0700 ~ Jakub Kicinski <jakub.kicinski@netronome.com> > > On Fri, 15 Mar 2019 15:41:41 -0300, Arnaldo Carvalho de Melo wrote: > > > While developing 'perf trace' and looking at BPF maps it puts in place I > > > noticed that one needs to first use 'bpftool map' to lookup a map id to > > > then use 'bpftool map dump map id <map-id-looked-up>'. > > > > > > This is needed because everytime we restart 'perf trace' the map IDs > > > gets changed so we need to do the ID lookup again. > > > > > > To speed up this sequence, allow specifying just the map name, look up > > > its ID and then use the existing routines as if the user had provided > > > the map id. > > > > I can see how it could be useful for quick debug. Names are not > > guaranteed to be unique, though, do you not care about potential > > duplicates? > > > > Jakub beat me to it by a few seconds :). I discussed possible workarounds on > that point with him though, so here are my two cents. I see two options: > first one would be to return the ids of, and then dump info about all maps > having that name. The second one would be to simply return an error if > duplicate names are found. > > We agreed that returning an error if there are duplicates is probably the > best way to go, because dealing with several maps for anything else than > "bpftool map show" would be problematic (clearly, we don't want to update > all maps with a given name at once). Of course if you have other > suggestions, feel free to propose :). So, if there are dups, just tell that and print the ids, i.e. the following, for any 'bpf map' command that finds dups, except for this one: [root@quaco ~]# bpftool map list pids_filtered 45: hash name pids_filtered flags 0x0 key 4B value 1B max_entries 64 memlock 8192B 90: hash name pids_filtered flags 0x0 key 4B value 80B max_entries 512 memlock do-the-mathB [root@quaco ~]# I.e. if there are multiple 'pids_filtered' maps, the output for, say, 'dump' would be: [root@quaco ~]# bpftool map dump pids_filtered There are multiple maps with this name, use 'bpftool map dump id NR' to disambiguate: ------------------------------------------------------------------------------------- 45: hash name pids_filtered flags 0x0 key 4B value 1B max_entries 64 memlock 8192B 90: hash name pids_filtered flags 0x0 key 4B value 80B max_entries 512 memlock do-the-mathB [root@quaco ~]# - Arnaldo
Em Fri, Mar 15, 2019 at 12:16:32PM -0700, Jakub Kicinski escreveu: > Please do keep the current model of name val, IOW dump map *name* pids.. I don't have a problem with that, but what would be the problem with supporting both: bpftool map dump name pids_filtered and: bpftool map dump pids_filtered And for that matter, even: bpftool map dump id 30 And bpftool map dump 30 Ditto for 'pinned'? I.e. less typing, the tool can be smart enough to figure out what is that is being asked, i.e. is it a number? Try first it as an 'id', etc. - Arnaldo > > [{ > > "key": 26554, > > "value": true > > },{ > > "key": 2592, > > "value": true > > } > > ] > > # > > > > Cc: Alexei Starovoitov <ast@kernel.org> > > Cc: Daniel Borkmann <daniel@iogearbox.net> > > Cc: Jakub Kicinski <jakub.kicinski@netronome.com> > > cc: Jiri Olsa <jolsa@kernel.org> > > Cc: Martin KaFai Lau <kafai@fb.com> > > Cc: Namhyung Kim <namhyung@kernel.org> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: Quentin Monnet <quentin.monnet@netronome.com> > > Cc: Song Liu <songliubraving@fb.com> > > Cc: Stanislav Fomichev <sdf@google.com> > > Cc: Yonghong Song <yhs@fb.com> > > Link: https://lkml.kernel.org/n/tip-rrnxuhvety3j3rf0r9zlbfro@git.kernel.org > > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > Please also update the bash completions and all the help/mans/docs.
On Fri, 15 Mar 2019 16:51:42 -0300, Arnaldo Carvalho de Melo wrote: > Em Fri, Mar 15, 2019 at 12:16:32PM -0700, Jakub Kicinski escreveu: > > Please do keep the current model of name val, IOW dump map *name* pids.. > > I don't have a problem with that, but what would be the problem with > supporting both: > > bpftool map dump name pids_filtered > > and: > > bpftool map dump pids_filtered > > And for that matter, even: > > bpftool map dump id 30 > > And > > bpftool map dump 30 > > Ditto for 'pinned'? > > I.e. less typing, the tool can be smart enough to figure out what is > that is being asked, i.e. is it a number? Try first it as an 'id', etc. It's error prone, we have excellent bash completions and accept prefixing (bpftool m d n pids_filtered), so typing should not be a problem.
On Fri, 15 Mar 2019 16:48:00 -0300, Arnaldo Carvalho de Melo wrote: > Em Fri, Mar 15, 2019 at 07:41:35PM +0000, Quentin Monnet escreveu: > > 2019-03-15 12:16 UTC-0700 ~ Jakub Kicinski <jakub.kicinski@netronome.com> > > > On Fri, 15 Mar 2019 15:41:41 -0300, Arnaldo Carvalho de Melo wrote: > > > > While developing 'perf trace' and looking at BPF maps it puts in place I > > > > noticed that one needs to first use 'bpftool map' to lookup a map id to > > > > then use 'bpftool map dump map id <map-id-looked-up>'. > > > > > > > > This is needed because everytime we restart 'perf trace' the map IDs > > > > gets changed so we need to do the ID lookup again. > > > > > > > > To speed up this sequence, allow specifying just the map name, look up > > > > its ID and then use the existing routines as if the user had provided > > > > the map id. > > > > > > I can see how it could be useful for quick debug. Names are not > > > guaranteed to be unique, though, do you not care about potential > > > duplicates? > > > > > > > Jakub beat me to it by a few seconds :). I discussed possible workarounds on > > that point with him though, so here are my two cents. I see two options: > > first one would be to return the ids of, and then dump info about all maps > > having that name. The second one would be to simply return an error if > > duplicate names are found. > > > > We agreed that returning an error if there are duplicates is probably the > > best way to go, because dealing with several maps for anything else than > > "bpftool map show" would be problematic (clearly, we don't want to update > > all maps with a given name at once). Of course if you have other > > suggestions, feel free to propose :). > > So, if there are dups, just tell that and print the ids, i.e. the > following, for any 'bpf map' command that finds dups, except for this > one: > > [root@quaco ~]# bpftool map list pids_filtered > 45: hash name pids_filtered flags 0x0 > key 4B value 1B max_entries 64 memlock 8192B > 90: hash name pids_filtered flags 0x0 > key 4B value 80B max_entries 512 memlock do-the-mathB > [root@quaco ~]# > > I.e. if there are multiple 'pids_filtered' maps, the output for, say, > 'dump' would be: > > [root@quaco ~]# bpftool map dump pids_filtered > There are multiple maps with this name, use 'bpftool map dump id NR' to disambiguate: > ------------------------------------------------------------------------------------- > 45: hash name pids_filtered flags 0x0 > key 4B value 1B max_entries 64 memlock 8192B > 90: hash name pids_filtered flags 0x0 > key 4B value 80B max_entries 512 memlock do-the-mathB > [root@quaco ~]# Yes, this looks good (modulo JSON output, which should probably be just a single "error" value). For simplicity I'd personally go for just always printing an error if name is duplicated and not bother with listing the options.
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c index e0c650d91784..94789992b934 100644 --- a/tools/bpf/bpftool/map.c +++ b/tools/bpf/bpftool/map.c @@ -89,12 +89,55 @@ static void *alloc_value(struct bpf_map_info *info) return malloc(info->value_size); } +static int bpf_map_get_id_by_name(const char *name, __u32 *idp) +{ + struct bpf_map_info info = {}; + __u32 len = sizeof(info); + __u32 id = 0; + int err; + int fd; + + while (true) { + err = bpf_map_get_next_id(id, &id); + if (err) { + if (errno == ENOENT) + break; + p_err("can't get next map: %s%s", strerror(errno), + errno == EINVAL ? " -- kernel too old?" : ""); + break; + } + + fd = bpf_map_get_fd_by_id(id); + if (fd < 0) { + if (errno == ENOENT) + continue; + p_err("can't get map by id (%u): %s", + id, strerror(errno)); + break; + } + + err = bpf_obj_get_info_by_fd(fd, &info, &len); + if (err) { + p_err("can't get map info: %s", strerror(errno)); + close(fd); + break; + } + + if (strcmp(info.name, name) == 0) { + *idp = id; + return 0; + } + } + + return errno == ENOENT ? 0 : -1; +} + int map_parse_fd(int *argc, char ***argv) { + unsigned int id = 0; int fd; if (is_prefix(**argv, "id")) { - unsigned int id; char *endptr; NEXT_ARGP(); @@ -104,8 +147,8 @@ int map_parse_fd(int *argc, char ***argv) p_err("can't parse %s as ID", **argv); return -1; } +get_fd_by_id: NEXT_ARGP(); - fd = bpf_map_get_fd_by_id(id); if (fd < 0) p_err("get map by id (%u): %s", id, strerror(errno)); @@ -119,9 +162,12 @@ int map_parse_fd(int *argc, char ***argv) NEXT_ARGP(); return open_obj_pinned_any(path, BPF_OBJ_MAP); + } else { + if (bpf_map_get_id_by_name(**argv, &id) == 0) + goto get_fd_by_id; } - p_err("expected 'id' or 'pinned', got: '%s'?", **argv); + p_err("expected 'id' or 'pinned' or an existing map name, got: '%s'?", **argv); return -1; } @@ -625,7 +671,7 @@ static int do_show(int argc, char **argv) if (show_pinned) build_pinned_obj_table(&map_table, BPF_OBJ_MAP); - if (argc == 2) { + if (argc == 2 || argc == 1) { fd = map_parse_fd_and_info(&argc, &argv, &info, &len); if (fd < 0) return -1; @@ -741,7 +787,7 @@ static int do_dump(int argc, char **argv) int err; int fd; - if (argc != 2) + if (argc != 2 && argc != 1) usage(); fd = map_parse_fd_and_info(&argc, &argv, &info, &len);
While developing 'perf trace' and looking at BPF maps it puts in place I noticed that one needs to first use 'bpftool map' to lookup a map id to then use 'bpftool map dump map id <map-id-looked-up>'. This is needed because everytime we restart 'perf trace' the map IDs gets changed so we need to do the ID lookup again. To speed up this sequence, allow specifying just the map name, look up its ID and then use the existing routines as if the user had provided the map id. This: # bpftool map 13: lpm_trie flags 0x1 key 8B value 8B max_entries 1 memlock 4096B 14: lpm_trie flags 0x1 key 20B value 8B max_entries 1 memlock 4096B 15: lpm_trie flags 0x1 key 8B value 8B max_entries 1 memlock 4096B 16: lpm_trie flags 0x1 key 20B value 8B max_entries 1 memlock 4096B 17: lpm_trie flags 0x1 key 8B value 8B max_entries 1 memlock 4096B 18: lpm_trie flags 0x1 key 20B value 8B max_entries 1 memlock 4096B 21: lpm_trie flags 0x1 key 8B value 8B max_entries 1 memlock 4096B 22: lpm_trie flags 0x1 key 20B value 8B max_entries 1 memlock 4096B 28: perf_event_array name __augmented_sys flags 0x0 key 4B value 4B max_entries 8 memlock 4096B 29: array name syscalls flags 0x0 key 4B value 1B max_entries 512 memlock 8192B 30: hash name pids_filtered flags 0x0 key 4B value 1B max_entries 64 memlock 8192B # # bpftool map dump id 30 [{ "key": 26554, "value": true },{ "key": 2592, "value": true } ] # Now is equivalent to: # bpftool map dump pids_filtered [{ "key": 26554, "value": true },{ "key": 2592, "value": true } ] # Cc: Alexei Starovoitov <ast@kernel.org> Cc: Daniel Borkmann <daniel@iogearbox.net> Cc: Jakub Kicinski <jakub.kicinski@netronome.com> cc: Jiri Olsa <jolsa@kernel.org> Cc: Martin KaFai Lau <kafai@fb.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Quentin Monnet <quentin.monnet@netronome.com> Cc: Song Liu <songliubraving@fb.com> Cc: Stanislav Fomichev <sdf@google.com> Cc: Yonghong Song <yhs@fb.com> Link: https://lkml.kernel.org/n/tip-rrnxuhvety3j3rf0r9zlbfro@git.kernel.org Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> --- tools/bpf/bpftool/map.c | 56 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 5 deletions(-)