linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v1] tools/bpftool: create map of maps
@ 2019-03-05 16:38 Alban Crequy
  2019-03-05 17:32 ` Jakub Kicinski
  2019-03-05 18:27 ` Quentin Monnet
  0 siblings, 2 replies; 4+ messages in thread
From: Alban Crequy @ 2019-03-05 16:38 UTC (permalink / raw)
  To: ast, daniel; +Cc: netdev, linux-kernel, alban, iago

From: Alban Crequy <alban@kinvolk.io>

Before this patch, there was no way to fill attr.inner_map_fd, necessary
for array_of_maps or hash_of_maps.

This patch adds keyword 'innermap' to pass the innermap, either as an id
or as a pinned map.

Example of commands:

$ sudo bpftool map create /sys/fs/bpf/innermap type hash \
        key 8 value 8 entries 64 name innermap flags 1
$ sudo bpftool map create /sys/fs/bpf/outermap type hash_of_maps \
        innermap pinned /sys/fs/bpf/innermap key 64 value 4 \
        entries 64 name myoutermap flags 1
$ sudo bpftool map show pinned /sys/fs/bpf/outermap
47: hash_of_maps  name myoutermap  flags 0x1
	key 64B  value 4B  max_entries 64  memlock 12288B

Documentation and bash completion updated as well.

Signed-off-by: Alban Crequy <alban@kinvolk.io>
---
 tools/bpf/bpftool/Documentation/bpftool-prog.rst |  2 +-
 tools/bpf/bpftool/bash-completion/bpftool        | 13 +++++++++++++
 tools/bpf/bpftool/map.c                          |  5 ++++-
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 9386bd6e0396..35902ab95cc5 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -25,7 +25,7 @@ PROG COMMANDS
 |	**bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes** | **visual** | **linum**}]
 |	**bpftool** **prog dump jited**  *PROG* [{**file** *FILE* | **opcodes** | **linum**}]
 |	**bpftool** **prog pin** *PROG* *FILE*
-|	**bpftool** **prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
+|	**bpftool** **prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] [**innermap** MAP]
 |	**bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
 |	**bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
 |	**bpftool** **prog tracelog**
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index b803827d01e8..dcdc39510dc3 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -461,6 +461,18 @@ _bpftool()
                             _sysfs_get_netdevs
                             return 0
                             ;;
+                        innermap)
+                            COMPREPLY+=( $( compgen -W "id pinned" -- "$cur" ) )
+                            return 0
+                            ;;
+                        id)
+                            _bpftool_get_map_ids
+                            return 0
+                            ;;
+                        pinned)
+                            _filedir
+                            return 0
+                            ;;
                         *)
                             _bpftool_once_attr 'type'
                             _bpftool_once_attr 'key'
@@ -469,6 +481,7 @@ _bpftool()
                             _bpftool_once_attr 'name'
                             _bpftool_once_attr 'flags'
                             _bpftool_once_attr 'dev'
+                            _bpftool_once_attr 'innermap'
                             return 0
                             ;;
                     esac
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index e0c650d91784..7d8ce903a471 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -1151,6 +1151,9 @@ static int do_create(int argc, char **argv)
 				return -1;
 			}
 			NEXT_ARG();
+		} else if (is_prefix(*argv, "innermap")) {
+			NEXT_ARG();
+			attr.inner_map_fd = map_parse_fd(&argc, &argv);
 		}
 	}
 
@@ -1231,7 +1234,7 @@ static int do_help(int argc, char **argv)
 		"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"
+		"                              [dev NAME] [innermap MAP]\n"
 		"       %s %s dump       MAP\n"
 		"       %s %s update     MAP [key DATA] [value VALUE] [UPDATE_FLAGS]\n"
 		"       %s %s lookup     MAP [key DATA]\n"
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next v1] tools/bpftool: create map of maps
  2019-03-05 16:38 [PATCH bpf-next v1] tools/bpftool: create map of maps Alban Crequy
@ 2019-03-05 17:32 ` Jakub Kicinski
  2019-03-07  9:14   ` Alban Crequy
  2019-03-05 18:27 ` Quentin Monnet
  1 sibling, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2019-03-05 17:32 UTC (permalink / raw)
  To: Alban Crequy; +Cc: ast, daniel, netdev, linux-kernel, alban, iago

On Tue,  5 Mar 2019 17:38:03 +0100, Alban Crequy wrote:
> From: Alban Crequy <alban@kinvolk.io>
> 
> Before this patch, there was no way to fill attr.inner_map_fd, necessary
> for array_of_maps or hash_of_maps.
> 
> This patch adds keyword 'innermap' to pass the innermap, either as an id
> or as a pinned map.
> 
> Example of commands:
> 
> $ sudo bpftool map create /sys/fs/bpf/innermap type hash \
>         key 8 value 8 entries 64 name innermap flags 1
> $ sudo bpftool map create /sys/fs/bpf/outermap type hash_of_maps \
>         innermap pinned /sys/fs/bpf/innermap key 64 value 4 \
>         entries 64 name myoutermap flags 1
> $ sudo bpftool map show pinned /sys/fs/bpf/outermap
> 47: hash_of_maps  name myoutermap  flags 0x1
> 	key 64B  value 4B  max_entries 64  memlock 12288B
> 
> Documentation and bash completion updated as well.
> 
> Signed-off-by: Alban Crequy <alban@kinvolk.io>

bpf-next is closed let's continue reviewing, but you'll probably have
to repost after the merge window :(

> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index e0c650d91784..7d8ce903a471 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -1151,6 +1151,9 @@ static int do_create(int argc, char **argv)
>  				return -1;
>  			}
>  			NEXT_ARG();
> +		} else if (is_prefix(*argv, "innermap")) {
> +			NEXT_ARG();
> +			attr.inner_map_fd = map_parse_fd(&argc, &argv);

You need to check if the return value is not -1, and also close this
file descriptor (a) when done, (b) when error happens.

>  		}
>  	}
>  

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next v1] tools/bpftool: create map of maps
  2019-03-05 16:38 [PATCH bpf-next v1] tools/bpftool: create map of maps Alban Crequy
  2019-03-05 17:32 ` Jakub Kicinski
@ 2019-03-05 18:27 ` Quentin Monnet
  1 sibling, 0 replies; 4+ messages in thread
From: Quentin Monnet @ 2019-03-05 18:27 UTC (permalink / raw)
  To: Alban Crequy, ast, daniel; +Cc: netdev, linux-kernel, alban, iago

2019-03-05 17:38 UTC+0100 ~ Alban Crequy <alban.crequy@gmail.com>
> From: Alban Crequy <alban@kinvolk.io>
> 
> Before this patch, there was no way to fill attr.inner_map_fd, necessary
> for array_of_maps or hash_of_maps.
> 
> This patch adds keyword 'innermap' to pass the innermap, either as an id
> or as a pinned map.
> 
> Example of commands:
> 
> $ sudo bpftool map create /sys/fs/bpf/innermap type hash \
>         key 8 value 8 entries 64 name innermap flags 1
> $ sudo bpftool map create /sys/fs/bpf/outermap type hash_of_maps \
>         innermap pinned /sys/fs/bpf/innermap key 64 value 4 \
>         entries 64 name myoutermap flags 1
> $ sudo bpftool map show pinned /sys/fs/bpf/outermap
> 47: hash_of_maps  name myoutermap  flags 0x1
> 	key 64B  value 4B  max_entries 64  memlock 12288B
> 
> Documentation and bash completion updated as well.
> 
> Signed-off-by: Alban Crequy <alban@kinvolk.io>

Thanks Alban!

> ---
>  tools/bpf/bpftool/Documentation/bpftool-prog.rst |  2 +-
>  tools/bpf/bpftool/bash-completion/bpftool        | 13 +++++++++++++
>  tools/bpf/bpftool/map.c                          |  5 ++++-
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> index 9386bd6e0396..35902ab95cc5 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> @@ -25,7 +25,7 @@ PROG COMMANDS
>  |	**bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes** | **visual** | **linum**}]
>  |	**bpftool** **prog dump jited**  *PROG* [{**file** *FILE* | **opcodes** | **linum**}]
>  |	**bpftool** **prog pin** *PROG* *FILE*
> -|	**bpftool** **prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> +|	**bpftool** **prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] [**innermap** MAP]

This page is the documentation for "bpftool prog" commands. You want
"bpftool map" (tools/bpf/bpftool/Documentation/bpftool-map.rst) instead.

You also need to update the description line for the "bpftool map
create" command further down in the bpftool-map.rst page. And could you
please add a sentence to that description to explain what the argument
is for?

>  |	**bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
>  |	**bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
>  |	**bpftool** **prog tracelog**
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index b803827d01e8..dcdc39510dc3 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -461,6 +461,18 @@ _bpftool()
>                              _sysfs_get_netdevs
>                              return 0
>                              ;;
> +                        innermap)
> +                            COMPREPLY+=( $( compgen -W "id pinned" -- "$cur" ) )
> +                            return 0
> +                            ;;
> +                        id)
> +                            _bpftool_get_map_ids
> +                            return 0
> +                            ;;
> +                        pinned)
> +                            _filedir
> +                            return 0
> +                            ;;

No need to add completion here for "pinned", it is handled already [0].

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/bpf/bpftool/bash-completion/bpftool?h=v5.0#n172

>                          *)
>                              _bpftool_once_attr 'type'
>                              _bpftool_once_attr 'key'
> @@ -469,6 +481,7 @@ _bpftool()
>                              _bpftool_once_attr 'name'
>                              _bpftool_once_attr 'flags'
>                              _bpftool_once_attr 'dev'
> +                            _bpftool_once_attr 'innermap'
>                              return 0
>                              ;;
>                      esac
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index e0c650d91784..7d8ce903a471 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -1151,6 +1151,9 @@ static int do_create(int argc, char **argv)
>  				return -1;
>  			}
>  			NEXT_ARG();
> +		} else if (is_prefix(*argv, "innermap")) {
> +			NEXT_ARG();
> +			attr.inner_map_fd = map_parse_fd(&argc, &argv);
>  		}
>  	}
>  
> @@ -1231,7 +1234,7 @@ static int do_help(int argc, char **argv)
>  		"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"
> +		"                              [dev NAME] [innermap MAP]\n"
>  		"       %s %s dump       MAP\n"
>  		"       %s %s update     MAP [key DATA] [value VALUE] [UPDATE_FLAGS]\n"
>  		"       %s %s lookup     MAP [key DATA]\n"
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next v1] tools/bpftool: create map of maps
  2019-03-05 17:32 ` Jakub Kicinski
@ 2019-03-07  9:14   ` Alban Crequy
  0 siblings, 0 replies; 4+ messages in thread
From: Alban Crequy @ 2019-03-07  9:14 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Alban Crequy, Alexei Starovoitov, Daniel Borkmann, netdev, LKML,
	Iago López Galeiras, Thilo Fromm

Thanks for the reviews, Jakub and Quentin! I will address it and
resend a new version when bpf-next opens again.
I'm also preparing some other patches on "bpftool map" about pinning
and passing file descriptors to help applications that don't support
map pinning directly.

On Tue, Mar 5, 2019 at 6:32 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Tue,  5 Mar 2019 17:38:03 +0100, Alban Crequy wrote:
> > From: Alban Crequy <alban@kinvolk.io>
> >
> > Before this patch, there was no way to fill attr.inner_map_fd, necessary
> > for array_of_maps or hash_of_maps.
> >
> > This patch adds keyword 'innermap' to pass the innermap, either as an id
> > or as a pinned map.
> >
> > Example of commands:
> >
> > $ sudo bpftool map create /sys/fs/bpf/innermap type hash \
> >         key 8 value 8 entries 64 name innermap flags 1
> > $ sudo bpftool map create /sys/fs/bpf/outermap type hash_of_maps \
> >         innermap pinned /sys/fs/bpf/innermap key 64 value 4 \
> >         entries 64 name myoutermap flags 1
> > $ sudo bpftool map show pinned /sys/fs/bpf/outermap
> > 47: hash_of_maps  name myoutermap  flags 0x1
> >       key 64B  value 4B  max_entries 64  memlock 12288B
> >
> > Documentation and bash completion updated as well.
> >
> > Signed-off-by: Alban Crequy <alban@kinvolk.io>
>
> bpf-next is closed let's continue reviewing, but you'll probably have
> to repost after the merge window :(
>
> > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > index e0c650d91784..7d8ce903a471 100644
> > --- a/tools/bpf/bpftool/map.c
> > +++ b/tools/bpf/bpftool/map.c
> > @@ -1151,6 +1151,9 @@ static int do_create(int argc, char **argv)
> >                               return -1;
> >                       }
> >                       NEXT_ARG();
> > +             } else if (is_prefix(*argv, "innermap")) {
> > +                     NEXT_ARG();
> > +                     attr.inner_map_fd = map_parse_fd(&argc, &argv);
>
> You need to check if the return value is not -1, and also close this
> file descriptor (a) when done, (b) when error happens.
>
> >               }
> >       }
> >

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-03-07  9:14 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-05 16:38 [PATCH bpf-next v1] tools/bpftool: create map of maps Alban Crequy
2019-03-05 17:32 ` Jakub Kicinski
2019-03-07  9:14   ` Alban Crequy
2019-03-05 18:27 ` Quentin Monnet

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