[PATCH 1/1] bpftool: Allow referring to maps by its name

[PATCH 1/1] bpftool: Allow referring to maps by its name

[Arnaldo Carvalho de Melo]

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(-)

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);
-- 
2.20.1


----- End forwarded message -----

-- 

- Arnaldo

Re: [PATCH 1/1] bpftool: Allow referring to maps by its name

[Jakub Kicinski]

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.

Re: [PATCH 1/1] bpftool: Allow referring to maps by its name

[Quentin Monnet]

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

Re: [PATCH 1/1] bpftool: Allow referring to maps by its name

[Arnaldo Carvalho de Melo]

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

Re: [PATCH 1/1] bpftool: Allow referring to maps by its name

[Arnaldo Carvalho de Melo]

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.

-- 

- Arnaldo

Re: [PATCH 1/1] bpftool: Allow referring to maps by its name

[Jakub Kicinski]

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.

Re: [PATCH 1/1] bpftool: Allow referring to maps by its name

[Jakub Kicinski]

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.