[PATCH bpf-next] tools: bpftool: add map create command

Subsystems: bpf [general] (safe dynamic programs and tools), bpf [tooling] (bpftool), the rest

STALE2880d REVIEWED: 1 (0M)

1 review trailer.

5 messages, 2 authors, 2018-10-16 · open the first message on its own page

[PATCH bpf-next] tools: bpftool: add map create command

From: Jakub Kicinski <hidden>
Date: 2018-10-13 01:40:08

Add a way of creating maps from user space.  The command takes
as parameters most of the attributes of the map creation system
call command.  After map is created its pinned to bpffs.  This makes
it possible to easily and dynamically (without rebuilding programs)
test various corner cases related to map creation.

Map type names are taken from bpftool's array used for printing.
In general these days we try to make use of libbpf type names, but
there are no map type names in libbpf as of today.

As with most features I add the motivation is testing (offloads) :)

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Quentin Monnet <redacted>
---
 .../bpf/bpftool/Documentation/bpftool-map.rst |  15 ++-
 tools/bpf/bpftool/bash-completion/bpftool     |  38 ++++++-
 tools/bpf/bpftool/common.c                    |  21 ++++
 tools/bpf/bpftool/main.h                      |   1 +
 tools/bpf/bpftool/map.c                       | 105 +++++++++++++++++-
 5 files changed, 176 insertions(+), 4 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index a6258bc8ec4f..5c1baa714fbf 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -15,13 +15,15 @@ SYNOPSIS
 	*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
 
 	*COMMANDS* :=
-	{ **show** | **list** | **dump** | **update** | **lookup** | **getnext** | **delete**
-	| **pin** | **help** }
+	{ **show** | **list** | **create** | **dump** | **update** | **lookup** | **getnext**
+	| **delete** | **pin** | **help** }
 
 MAP COMMANDS
 =============
 
 |	**bpftool** **map { show | list }**   [*MAP*]
+|	**bpftool** **map create**     *FILE* **type** *TYPE* **key** *KEY_SIZE* **value** *VALUE_SIZE* \
+|		**entries** *MAX_ENTRIES* [**name** *NAME*] [**flags** *FLAGS*] [**dev** *NAME*]
 |	**bpftool** **map dump**       *MAP*
 |	**bpftool** **map update**     *MAP*  **key** *DATA*   **value** *VALUE* [*UPDATE_FLAGS*]
 |	**bpftool** **map lookup**     *MAP*  **key** *DATA*
@@ -36,6 +38,11 @@ MAP COMMANDS
 |	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
 |	*VALUE* := { *DATA* | *MAP* | *PROG* }
 |	*UPDATE_FLAGS* := { **any** | **exist** | **noexist** }
+|	*TYPE* := { **hash** | **array** | **prog_array** | **perf_event_array** | **percpu_hash**
+|		| **percpu_array** | **stack_trace** | **cgroup_array** | **lru_hash**
+|		| **lru_percpu_hash** | **lpm_trie** | **array_of_maps** | **hash_of_maps**
+|		| **devmap** | **sockmap** | **cpumap** | **xskmap** | **sockhash**
+|		| **cgroup_storage** | **reuseport_sockarray** | **percpu_cgroup_storage** }
 
 DESCRIPTION
 ===========
@@ -47,6 +54,10 @@ DESCRIPTION
 		  Output will start with map ID followed by map type and
 		  zero or more named attributes (depending on kernel version).
 
+	**bpftool map create** *FILE* **type** *TYPE* **key** *KEY_SIZE* **value** *VALUE_SIZE*  **entries** *MAX_ENTRIES* [**name** *NAME*] [**flags** *FLAGS*] [**dev** *NAME*]
+		  Create a new map with given parameters and pin it to *bpffs*
+		  as *FILE*.
+
 	**bpftool map dump**    *MAP*
 		  Dump all entries in a given *MAP*.
 
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index df1060b852c1..277bab46cd05 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -370,6 +370,42 @@ _bpftool()
                             ;;
                     esac
                     ;;
+                create)
+                    case $prev in
+                        $command)
+                            _filedir
+                            return 0
+                            ;;
+                        type)
+                            COMPREPLY=( $( compgen -W 'hash array prog_array \
+                                perf_event_array percpu_hash percpu_array \
+                                stack_trace cgroup_array lru_hash \
+                                lru_percpu_hash lpm_trie array_of_maps \
+                                hash_of_maps devmap sockmap cpumap xskmap \
+                                sockhash cgroup_storage reuseport_sockarray \
+                                percpu_cgroup_storage' -- \
+                                                   "$cur" ) )
+                            return 0
+                            ;;
+                        key|value|flags|name|entries)
+                            return 0
+                            ;;
+                        dev)
+                            _sysfs_get_netdevs
+                            return 0
+                            ;;
+                        *)
+                            _bpftool_once_attr 'type'
+                            _bpftool_once_attr 'key'
+                            _bpftool_once_attr 'value'
+                            _bpftool_once_attr 'entries'
+                            _bpftool_once_attr 'name'
+                            _bpftool_once_attr 'flags'
+                            _bpftool_once_attr 'dev'
+                            return 0
+                            ;;
+                    esac
+                    ;;
                 lookup|getnext|delete)
                     case $prev in
                         $command)
@@ -483,7 +519,7 @@ _bpftool()
                 *)
                     [[ $prev == $object ]] && \
                         COMPREPLY=( $( compgen -W 'delete dump getnext help \
-                            lookup pin event_pipe show list update' -- \
+                            lookup pin event_pipe show list update create' -- \
                             "$cur" ) )
                     ;;
             esac
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index b3a0709ea7ed..3318da8060bd 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -618,3 +618,24 @@ void print_dev_json(__u32 ifindex, __u64 ns_dev, __u64 ns_inode)
 		jsonw_string_field(json_wtr, "ifname", name);
 	jsonw_end_object(json_wtr);
 }
+
+int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what)
+{
+	char *endptr;
+
+	NEXT_ARGP();
+
+	if (*val) {
+		p_err("%s already specified", what);
+		return -1;
+	}
+
+	*val = strtoul(**argv, &endptr, 0);
+	if (*endptr) {
+		p_err("can't parse %s as %s", **argv, what);
+		return -1;
+	}
+	NEXT_ARGP();
+
+	return 0;
+}
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 40492cdc4e53..cf29c691ca35 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -138,6 +138,7 @@ int do_cgroup(int argc, char **arg);
 int do_perf(int argc, char **arg);
 int do_net(int argc, char **arg);
 
+int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what);
 int prog_parse_fd(int *argc, char ***argv);
 int map_parse_fd(int *argc, char ***argv);
 int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len);
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 6003e9598973..eb4d781901c8 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -36,6 +36,7 @@
 #include <fcntl.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
+#include <net/if.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -94,6 +95,17 @@ static bool map_is_map_of_progs(__u32 type)
 	return type == BPF_MAP_TYPE_PROG_ARRAY;
 }
 
+static int map_type_from_str(const char *type)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(map_type_name); i++)
+		/* Don't allow prefixing in case of possible future shadowing */
+		if (map_type_name[i] && !strcmp(map_type_name[i], type))
+			return i;
+	return -1;
+}
+
 static void *alloc_value(struct bpf_map_info *info)
 {
 	if (map_is_per_cpu(info->type))
@@ -1024,6 +1036,87 @@ static int do_pin(int argc, char **argv)
 	return err;
 }
 
+static int do_create(int argc, char **argv)
+{
+	struct bpf_create_map_attr attr = { NULL, };
+	const char *pinfile;
+	int err, fd;
+
+	if (!REQ_ARGS(7))
+		return -1;
+	pinfile = GET_ARG();
+
+	while (argc) {
+		if (!REQ_ARGS(2))
+			return -1;
+
+		if (is_prefix(*argv, "type")) {
+			NEXT_ARG();
+
+			if (attr.map_type) {
+				p_err("map type already specified");
+				return -1;
+			}
+
+			attr.map_type = map_type_from_str(*argv);
+			if ((int)attr.map_type < 0) {
+				p_err("unrecognized map type: %s", *argv);
+				return -1;
+			}
+			NEXT_ARG();
+		} else if (is_prefix(*argv, "name")) {
+			NEXT_ARG();
+			attr.name = GET_ARG();
+		} else if (is_prefix(*argv, "key")) {
+			if (parse_u32_arg(&argc, &argv, &attr.key_size,
+					  "key size"))
+				return -1;
+		} else if (is_prefix(*argv, "value")) {
+			if (parse_u32_arg(&argc, &argv, &attr.value_size,
+					  "value size"))
+				return -1;
+		} else if (is_prefix(*argv, "entries")) {
+			if (parse_u32_arg(&argc, &argv, &attr.max_entries,
+					  "max entries"))
+				return -1;
+		} else if (is_prefix(*argv, "flags")) {
+			if (parse_u32_arg(&argc, &argv, &attr.map_flags,
+					  "flags"))
+				return -1;
+		} else if (is_prefix(*argv, "dev")) {
+			NEXT_ARG();
+
+			if (attr.map_ifindex) {
+				p_err("offload device already specified");
+				return -1;
+			}
+
+			attr.map_ifindex = if_nametoindex(*argv);
+			if (!attr.map_ifindex) {
+				p_err("unrecognized netdevice '%s': %s",
+				      *argv, strerror(errno));
+				return -1;
+			}
+			NEXT_ARG();
+		}
+	}
+
+	fd = bpf_create_map_xattr(&attr);
+	if (fd < 0) {
+		p_err("map create failed: %s", strerror(errno));
+		return -1;
+	}
+
+	err = do_pin_fd(fd, pinfile);
+	close(fd);
+	if (err)
+		return err;
+
+	if (json_output)
+		jsonw_null(json_wtr);
+	return 0;
+}
+
 static int do_help(int argc, char **argv)
 {
 	if (json_output) {
@@ -1033,6 +1126,9 @@ static int do_help(int argc, char **argv)
 
 	fprintf(stderr,
 		"Usage: %s %s { show | list }   [MAP]\n"
+		"       %s %s create     FILE type TYPE key KEY_SIZE value VALUE_SIZE \\\n"
+		"                              entries MAX_ENTRIES [name NAME] [flags FLAGS] \\\n"
+		"                              [dev NAME]\n"
 		"       %s %s dump       MAP\n"
 		"       %s %s update     MAP  key DATA value VALUE [UPDATE_FLAGS]\n"
 		"       %s %s lookup     MAP  key DATA\n"
@@ -1047,11 +1143,17 @@ static int do_help(int argc, char **argv)
 		"       " HELP_SPEC_PROGRAM "\n"
 		"       VALUE := { DATA | MAP | PROG }\n"
 		"       UPDATE_FLAGS := { any | exist | noexist }\n"
+		"       TYPE := { hash | array | prog_array | perf_event_array | percpu_hash |\n"
+		"                 percpu_array | stack_trace | cgroup_array | lru_hash |\n"
+		"                 lru_percpu_hash | lpm_trie | array_of_maps | hash_of_maps |\n"
+		"                 devmap | sockmap | cpumap | xskmap | sockhash |\n"
+		"                 cgroup_storage | reuseport_sockarray | percpu_cgroup_storage }\n"
 		"       " HELP_SPEC_OPTIONS "\n"
 		"",
 		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
 		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
-		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2]);
+		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
+		bin_name, argv[-2]);
 
 	return 0;
 }
@@ -1067,6 +1169,7 @@ static const struct cmd cmds[] = {
 	{ "delete",	do_delete },
 	{ "pin",	do_pin },
 	{ "event_pipe",	do_event_pipe },
+	{ "create",	do_create },
 	{ 0 }
 };
 
-- 
2.17.1

Re: [PATCH bpf-next] tools: bpftool: add map create command

From: Alexei Starovoitov <hidden>
Date: 2018-10-13 13:52:56

On Fri, Oct 12, 2018 at 11:06:14AM -0700, Jakub Kicinski wrote:
Add a way of creating maps from user space.  The command takes
as parameters most of the attributes of the map creation system
call command.  After map is created its pinned to bpffs.  This makes
it possible to easily and dynamically (without rebuilding programs)
test various corner cases related to map creation.

Map type names are taken from bpftool's array used for printing.
In general these days we try to make use of libbpf type names, but
there are no map type names in libbpf as of today.

As with most features I add the motivation is testing (offloads) :)

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Quentin Monnet <redacted>
...
 	fprintf(stderr,
 		"Usage: %s %s { show | list }   [MAP]\n"
+		"       %s %s create     FILE type TYPE key KEY_SIZE value VALUE_SIZE \\\n"
+		"                              entries MAX_ENTRIES [name NAME] [flags FLAGS] \\\n"
+		"                              [dev NAME]\n"
I suspect as soon as bpftool has an ability to create standalone maps
some folks will start relying on such interface.
Therefore I'd like to request to make 'name' argument to be mandatory.
I think in the future we will require BTF to be mandatory too.
We need to move towards more transparent and debuggable infra.
Do you think requiring json description of key/value would be managable to implement?
Then bpftool could convert it to BTF and the map full be fully defined.
I certainly understand that bpf prog can disregard the key/value layout today,
but we will make verifier to enforce that in the future too.

Re: [PATCH bpf-next] tools: bpftool: add map create command

From: Jakub Kicinski <hidden>
Date: 2018-10-16 00:35:14

On Fri, 12 Oct 2018 23:16:59 -0700, Alexei Starovoitov wrote:
On Fri, Oct 12, 2018 at 11:06:14AM -0700, Jakub Kicinski wrote:
quoted
Add a way of creating maps from user space.  The command takes
as parameters most of the attributes of the map creation system
call command.  After map is created its pinned to bpffs.  This makes
it possible to easily and dynamically (without rebuilding programs)
test various corner cases related to map creation.

Map type names are taken from bpftool's array used for printing.
In general these days we try to make use of libbpf type names, but
there are no map type names in libbpf as of today.

As with most features I add the motivation is testing (offloads) :)

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Quentin Monnet <redacted>  
...
quoted
 	fprintf(stderr,
 		"Usage: %s %s { show | list }   [MAP]\n"
+		"       %s %s create     FILE type TYPE key KEY_SIZE value VALUE_SIZE \\\n"
+		"                              entries MAX_ENTRIES [name NAME] [flags FLAGS] \\\n"
+		"                              [dev NAME]\n"  
I suspect as soon as bpftool has an ability to create standalone maps
some folks will start relying on such interface.
That'd be cool, do you see any real life use cases where its useful
outside of corner case testing?
Therefore I'd like to request to make 'name' argument to be mandatory.
Will do in v2!
I think in the future we will require BTF to be mandatory too.
We need to move towards more transparent and debuggable infra.
Do you think requiring json description of key/value would be managable to implement?
Then bpftool could convert it to BTF and the map full be fully defined.
I certainly understand that bpf prog can disregard the key/value layout today,
but we will make verifier to enforce that in the future too.
I was hoping that we can leave BTF support as a future extension, and
then once we have the option for the verifier to enforce BTF (a sysctl?)
the bpftool map create without a BTF will get rejected as one would
expect.  IOW it's fine not to make BTF required at bpftool level and
leave it to system configuration.

I'd love to implement the BTF support right away, but I'm not sure I
can afford that right now time-wise.  The whole map create command is
pretty trivial, but for BTF we don't even have a way of dumping it
AFAICT.  We can pretty print values, but what is the format in which to
express the BTF itself?  We could do JSON, do we use an external
library?  Should we have a separate BTF command for that?

Re: [PATCH bpf-next] tools: bpftool: add map create command

From: Alexei Starovoitov <hidden>
Date: 2018-10-16 03:44:54

On Mon, Oct 15, 2018 at 09:49:08AM -0700, Jakub Kicinski wrote:
On Fri, 12 Oct 2018 23:16:59 -0700, Alexei Starovoitov wrote:
quoted
On Fri, Oct 12, 2018 at 11:06:14AM -0700, Jakub Kicinski wrote:
quoted
Add a way of creating maps from user space.  The command takes
as parameters most of the attributes of the map creation system
call command.  After map is created its pinned to bpffs.  This makes
it possible to easily and dynamically (without rebuilding programs)
test various corner cases related to map creation.

Map type names are taken from bpftool's array used for printing.
In general these days we try to make use of libbpf type names, but
there are no map type names in libbpf as of today.

As with most features I add the motivation is testing (offloads) :)

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Quentin Monnet <redacted>  
...
quoted
 	fprintf(stderr,
 		"Usage: %s %s { show | list }   [MAP]\n"
+		"       %s %s create     FILE type TYPE key KEY_SIZE value VALUE_SIZE \\\n"
+		"                              entries MAX_ENTRIES [name NAME] [flags FLAGS] \\\n"
+		"                              [dev NAME]\n"  
I suspect as soon as bpftool has an ability to create standalone maps
some folks will start relying on such interface.
That'd be cool, do you see any real life use cases where its useful
outside of corner case testing?
In our XDP use case we have an odd protocol for different apps to share
common prog_array that is pinned in bpffs.
If cmdline creation of it via bpftool was available that would have been
an option to consider. Not saying that it would have been a better option.
Just another option.
quoted
Therefore I'd like to request to make 'name' argument to be mandatory.
Will do in v2!
thx!
 
quoted
I think in the future we will require BTF to be mandatory too.
We need to move towards more transparent and debuggable infra.
Do you think requiring json description of key/value would be managable to implement?
Then bpftool could convert it to BTF and the map full be fully defined.
I certainly understand that bpf prog can disregard the key/value layout today,
but we will make verifier to enforce that in the future too.
I was hoping that we can leave BTF support as a future extension, and
then once we have the option for the verifier to enforce BTF (a sysctl?)
the bpftool map create without a BTF will get rejected as one would
expect.  
right. something like sysctl in the future.
IOW it's fine not to make BTF required at bpftool level and
leave it to system configuration.

I'd love to implement the BTF support right away, but I'm not sure I
can afford that right now time-wise.  The whole map create command is
pretty trivial, but for BTF we don't even have a way of dumping it
AFAICT.  We can pretty print values, but what is the format in which to
express the BTF itself?  We could do JSON, do we use an external
library?  Should we have a separate BTF command for that?
I prefer standard C type description for both input and output :)
Anyway that wasn't a request for you to do it now. More of the feature
request for somebody to put on todo list :)

Re: [PATCH bpf-next] tools: bpftool: add map create command

From: Jakub Kicinski <hidden>
Date: 2018-10-16 06:40:28

On Mon, 15 Oct 2018 12:58:07 -0700, Alexei Starovoitov wrote:
quoted
quoted
quoted
 	fprintf(stderr,
 		"Usage: %s %s { show | list }   [MAP]\n"
+		"       %s %s create     FILE type TYPE key KEY_SIZE value VALUE_SIZE \\\n"
+		"                              entries MAX_ENTRIES [name NAME] [flags FLAGS] \\\n"
+		"                              [dev NAME]\n"    
I suspect as soon as bpftool has an ability to create standalone maps
some folks will start relying on such interface.  
That'd be cool, do you see any real life use cases where its useful
outside of corner case testing?  
In our XDP use case we have an odd protocol for different apps to share
common prog_array that is pinned in bpffs.
If cmdline creation of it via bpftool was available that would have been
an option to consider. Not saying that it would have been a better option.
Just another option.
I see, I didn't think of prog arrays.
quoted
quoted
Therefore I'd like to request to make 'name' argument to be mandatory.  
Will do in v2!  
thx!
 
quoted
quoted
I think in the future we will require BTF to be mandatory too.
We need to move towards more transparent and debuggable infra.
Do you think requiring json description of key/value would be managable to implement?
Then bpftool could convert it to BTF and the map full be fully defined.
I certainly understand that bpf prog can disregard the key/value layout today,
but we will make verifier to enforce that in the future too.  
I was hoping that we can leave BTF support as a future extension, and
then once we have the option for the verifier to enforce BTF (a sysctl?)
the bpftool map create without a BTF will get rejected as one would
expect.    
right. something like sysctl in the future.
quoted
IOW it's fine not to make BTF required at bpftool level and
leave it to system configuration.

I'd love to implement the BTF support right away, but I'm not sure I
can afford that right now time-wise.  The whole map create command is
pretty trivial, but for BTF we don't even have a way of dumping it
AFAICT.  We can pretty print values, but what is the format in which to
express the BTF itself?  We could do JSON, do we use an external
library?  Should we have a separate BTF command for that?  
I prefer standard C type description for both input and output :)
Anyway that wasn't a request for you to do it now. More of the feature
request for somebody to put on todo list :)
Oh, okay :)  

I will wait for John's patches to get merged and post v2, otherwise
we'd conflict on the man page.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help