* [PATCH bpf-next v2 0/2] tools: bpftool: support creating outer maps @ 2020-09-07 16:36 Quentin Monnet 2020-09-07 16:36 ` [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry Quentin Monnet 2020-09-07 16:36 ` [PATCH bpf-next v2 2/2] tools: bpftool: add "inner_map" to "bpftool map create" outer maps Quentin Monnet 0 siblings, 2 replies; 10+ messages in thread From: Quentin Monnet @ 2020-09-07 16:36 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann Cc: bpf, netdev, Andrii Nakryiko, Quentin Monnet This series makes bpftool able to create outer maps (maps of types array-of-maps and hash-of-maps). This is done by passing the relevant inner_map_fd, which we do through a new command-line keyword. A first patch also cleans up the function related to dumping map elements. v2: - v1 was wrongly expected to allow bpftool to dump the content of outer maps (already supported). v2 skipped that patch, and instead replaced it with a clean-up for the dump_map_elem() function. Quentin Monnet (2): tools: bpftool: clean up function to dump map entry tools: bpftool: add "inner_map" to "bpftool map create" outer maps .../bpf/bpftool/Documentation/bpftool-map.rst | 10 +- tools/bpf/bpftool/bash-completion/bpftool | 22 ++- tools/bpf/bpftool/map.c | 149 ++++++++++-------- 3 files changed, 114 insertions(+), 67 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry 2020-09-07 16:36 [PATCH bpf-next v2 0/2] tools: bpftool: support creating outer maps Quentin Monnet @ 2020-09-07 16:36 ` Quentin Monnet 2020-09-09 3:25 ` Andrii Nakryiko 2020-09-07 16:36 ` [PATCH bpf-next v2 2/2] tools: bpftool: add "inner_map" to "bpftool map create" outer maps Quentin Monnet 1 sibling, 1 reply; 10+ messages in thread From: Quentin Monnet @ 2020-09-07 16:36 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann Cc: bpf, netdev, Andrii Nakryiko, Quentin Monnet The function used to dump a map entry in bpftool is a bit difficult to follow, as a consequence to earlier refactorings. There is a variable ("num_elems") which does not appear to be necessary, and the error handling would look cleaner if moved to its own function. Let's clean it up. No functional change. v2: - v1 was erroneously removing the check on fd maps in an attempt to get support for outer map dumps. This is already working. Instead, v2 focuses on cleaning up the dump_map_elem() function, to avoid similar confusion in the future. Signed-off-by: Quentin Monnet <quentin@isovalent.com> --- tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++------------------- 1 file changed, 52 insertions(+), 49 deletions(-) diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c index bc0071228f88..c8159cb4fb1e 100644 --- a/tools/bpf/bpftool/map.c +++ b/tools/bpf/bpftool/map.c @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key, jsonw_end_object(json_wtr); } -static void print_entry_error(struct bpf_map_info *info, unsigned char *key, - const char *error_msg) +static void +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key, + const char *error_msg) { int msg_size = strlen(error_msg); bool single_line, break_names; @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key, printf("\n"); } +static void +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno) +{ + /* For prog_array maps or arrays of maps, failure to lookup the value + * means there is no entry for that key. Do not print an error message + * in that case. + */ + if (map_is_map_of_maps(map_info->type) || + map_is_map_of_progs(map_info->type)) + return; + + if (json_output) { + jsonw_start_object(json_wtr); /* entry */ + jsonw_name(json_wtr, "key"); + print_hex_data_json(key, map_info->key_size); + jsonw_name(json_wtr, "value"); + jsonw_start_object(json_wtr); /* error */ + jsonw_string_field(json_wtr, "error", strerror(lookup_errno)); + jsonw_end_object(json_wtr); /* error */ + jsonw_end_object(json_wtr); /* entry */ + } else { + const char *msg = NULL; + + if (lookup_errno == ENOENT) + msg = "<no entry>"; + else if (lookup_errno == ENOSPC && + map_info->type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY) + msg = "<cannot read>"; + + print_entry_error_msg(map_info, key, + msg ? : strerror(lookup_errno)); + } +} + static void print_entry_plain(struct bpf_map_info *info, unsigned char *key, unsigned char *value) { @@ -713,56 +748,23 @@ static int dump_map_elem(int fd, void *key, void *value, struct bpf_map_info *map_info, struct btf *btf, json_writer_t *btf_wtr) { - int num_elems = 0; - int lookup_errno; - - if (!bpf_map_lookup_elem(fd, key, value)) { - if (json_output) { - print_entry_json(map_info, key, value, btf); - } else { - if (btf) { - struct btf_dumper d = { - .btf = btf, - .jw = btf_wtr, - .is_plain_text = true, - }; - - do_dump_btf(&d, map_info, key, value); - } else { - print_entry_plain(map_info, key, value); - } - num_elems++; - } - return num_elems; + if (bpf_map_lookup_elem(fd, key, value)) { + print_entry_error(map_info, key, errno); + return -1; } - /* lookup error handling */ - lookup_errno = errno; - - if (map_is_map_of_maps(map_info->type) || - map_is_map_of_progs(map_info->type)) - return 0; - if (json_output) { - jsonw_start_object(json_wtr); - jsonw_name(json_wtr, "key"); - print_hex_data_json(key, map_info->key_size); - jsonw_name(json_wtr, "value"); - jsonw_start_object(json_wtr); - jsonw_string_field(json_wtr, "error", strerror(lookup_errno)); - jsonw_end_object(json_wtr); - jsonw_end_object(json_wtr); - } else { - const char *msg = NULL; - - if (lookup_errno == ENOENT) - msg = "<no entry>"; - else if (lookup_errno == ENOSPC && - map_info->type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY) - msg = "<cannot read>"; + print_entry_json(map_info, key, value, btf); + } else if (btf) { + struct btf_dumper d = { + .btf = btf, + .jw = btf_wtr, + .is_plain_text = true, + }; - print_entry_error(map_info, key, - msg ? : strerror(lookup_errno)); + do_dump_btf(&d, map_info, key, value); + } else { + print_entry_plain(map_info, key, value); } return 0; @@ -873,7 +875,8 @@ map_dump(int fd, struct bpf_map_info *info, json_writer_t *wtr, err = 0; break; } - num_elems += dump_map_elem(fd, key, value, info, btf, wtr); + if (!dump_map_elem(fd, key, value, info, btf, wtr)) + num_elems++; prev_key = key; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry 2020-09-07 16:36 ` [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry Quentin Monnet @ 2020-09-09 3:25 ` Andrii Nakryiko 2020-09-09 8:19 ` Quentin Monnet 0 siblings, 1 reply; 10+ messages in thread From: Andrii Nakryiko @ 2020-09-09 3:25 UTC (permalink / raw) To: Quentin Monnet; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote: > > The function used to dump a map entry in bpftool is a bit difficult to > follow, as a consequence to earlier refactorings. There is a variable > ("num_elems") which does not appear to be necessary, and the error > handling would look cleaner if moved to its own function. Let's clean it > up. No functional change. > > v2: > - v1 was erroneously removing the check on fd maps in an attempt to get > support for outer map dumps. This is already working. Instead, v2 > focuses on cleaning up the dump_map_elem() function, to avoid > similar confusion in the future. > > Signed-off-by: Quentin Monnet <quentin@isovalent.com> > --- > tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++------------------- > 1 file changed, 52 insertions(+), 49 deletions(-) > > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c > index bc0071228f88..c8159cb4fb1e 100644 > --- a/tools/bpf/bpftool/map.c > +++ b/tools/bpf/bpftool/map.c > @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key, > jsonw_end_object(json_wtr); > } > > -static void print_entry_error(struct bpf_map_info *info, unsigned char *key, > - const char *error_msg) > +static void > +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key, > + const char *error_msg) > { > int msg_size = strlen(error_msg); > bool single_line, break_names; > @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key, > printf("\n"); > } > > +static void > +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno) > +{ > + /* For prog_array maps or arrays of maps, failure to lookup the value > + * means there is no entry for that key. Do not print an error message > + * in that case. > + */ this is the case when error is ENOENT, all the other ones should be treated the same, no? > + if (map_is_map_of_maps(map_info->type) || > + map_is_map_of_progs(map_info->type)) > + return; > + > + if (json_output) { > + jsonw_start_object(json_wtr); /* entry */ > + jsonw_name(json_wtr, "key"); > + print_hex_data_json(key, map_info->key_size); > + jsonw_name(json_wtr, "value"); > + jsonw_start_object(json_wtr); /* error */ > + jsonw_string_field(json_wtr, "error", strerror(lookup_errno)); > + jsonw_end_object(json_wtr); /* error */ > + jsonw_end_object(json_wtr); /* entry */ > + } else { > + const char *msg = NULL; > + > + if (lookup_errno == ENOENT) > + msg = "<no entry>"; > + else if (lookup_errno == ENOSPC && > + map_info->type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY) > + msg = "<cannot read>"; > + > + print_entry_error_msg(map_info, key, > + msg ? : strerror(lookup_errno)); > + } > +} > + [...] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry 2020-09-09 3:25 ` Andrii Nakryiko @ 2020-09-09 8:19 ` Quentin Monnet 2020-09-09 16:30 ` Andrii Nakryiko 0 siblings, 1 reply; 10+ messages in thread From: Quentin Monnet @ 2020-09-09 8:19 UTC (permalink / raw) To: Andrii Nakryiko; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking On 09/09/2020 04:25, Andrii Nakryiko wrote: > On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote: >> >> The function used to dump a map entry in bpftool is a bit difficult to >> follow, as a consequence to earlier refactorings. There is a variable >> ("num_elems") which does not appear to be necessary, and the error >> handling would look cleaner if moved to its own function. Let's clean it >> up. No functional change. >> >> v2: >> - v1 was erroneously removing the check on fd maps in an attempt to get >> support for outer map dumps. This is already working. Instead, v2 >> focuses on cleaning up the dump_map_elem() function, to avoid >> similar confusion in the future. >> >> Signed-off-by: Quentin Monnet <quentin@isovalent.com> >> --- >> tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++------------------- >> 1 file changed, 52 insertions(+), 49 deletions(-) >> >> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c >> index bc0071228f88..c8159cb4fb1e 100644 >> --- a/tools/bpf/bpftool/map.c >> +++ b/tools/bpf/bpftool/map.c >> @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key, >> jsonw_end_object(json_wtr); >> } >> >> -static void print_entry_error(struct bpf_map_info *info, unsigned char *key, >> - const char *error_msg) >> +static void >> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key, >> + const char *error_msg) >> { >> int msg_size = strlen(error_msg); >> bool single_line, break_names; >> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key, >> printf("\n"); >> } >> >> +static void >> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno) >> +{ >> + /* For prog_array maps or arrays of maps, failure to lookup the value >> + * means there is no entry for that key. Do not print an error message >> + * in that case. >> + */ > > this is the case when error is ENOENT, all the other ones should be > treated the same, no? Do you mean all map types should be treated the same? If so, I can remove the check below, as in v1. Or do you mean there is a missing check on the error value? In which case I can extend this check to verify we have ENOENT. >> + if (map_is_map_of_maps(map_info->type) || >> + map_is_map_of_progs(map_info->type)) >> + return; >> + >> + if (json_output) { >> + jsonw_start_object(json_wtr); /* entry */ >> + jsonw_name(json_wtr, "key"); >> + print_hex_data_json(key, map_info->key_size); >> + jsonw_name(json_wtr, "value"); >> + jsonw_start_object(json_wtr); /* error */ >> + jsonw_string_field(json_wtr, "error", strerror(lookup_errno)); >> + jsonw_end_object(json_wtr); /* error */ >> + jsonw_end_object(json_wtr); /* entry */ >> + } else { >> + const char *msg = NULL; >> + >> + if (lookup_errno == ENOENT) >> + msg = "<no entry>"; >> + else if (lookup_errno == ENOSPC && >> + map_info->type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY) >> + msg = "<cannot read>"; >> + >> + print_entry_error_msg(map_info, key, >> + msg ? : strerror(lookup_errno)); >> + } >> +} >> + > > [...] > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry 2020-09-09 8:19 ` Quentin Monnet @ 2020-09-09 16:30 ` Andrii Nakryiko 2020-09-09 16:38 ` Quentin Monnet 0 siblings, 1 reply; 10+ messages in thread From: Andrii Nakryiko @ 2020-09-09 16:30 UTC (permalink / raw) To: Quentin Monnet; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking On Wed, Sep 9, 2020 at 1:19 AM Quentin Monnet <quentin@isovalent.com> wrote: > > On 09/09/2020 04:25, Andrii Nakryiko wrote: > > On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote: > >> > >> The function used to dump a map entry in bpftool is a bit difficult to > >> follow, as a consequence to earlier refactorings. There is a variable > >> ("num_elems") which does not appear to be necessary, and the error > >> handling would look cleaner if moved to its own function. Let's clean it > >> up. No functional change. > >> > >> v2: > >> - v1 was erroneously removing the check on fd maps in an attempt to get > >> support for outer map dumps. This is already working. Instead, v2 > >> focuses on cleaning up the dump_map_elem() function, to avoid > >> similar confusion in the future. > >> > >> Signed-off-by: Quentin Monnet <quentin@isovalent.com> > >> --- > >> tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++------------------- > >> 1 file changed, 52 insertions(+), 49 deletions(-) > >> > >> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c > >> index bc0071228f88..c8159cb4fb1e 100644 > >> --- a/tools/bpf/bpftool/map.c > >> +++ b/tools/bpf/bpftool/map.c > >> @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key, > >> jsonw_end_object(json_wtr); > >> } > >> > >> -static void print_entry_error(struct bpf_map_info *info, unsigned char *key, > >> - const char *error_msg) > >> +static void > >> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key, > >> + const char *error_msg) > >> { > >> int msg_size = strlen(error_msg); > >> bool single_line, break_names; > >> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key, > >> printf("\n"); > >> } > >> > >> +static void > >> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno) > >> +{ > >> + /* For prog_array maps or arrays of maps, failure to lookup the value > >> + * means there is no entry for that key. Do not print an error message > >> + * in that case. > >> + */ > > > > this is the case when error is ENOENT, all the other ones should be > > treated the same, no? > > Do you mean all map types should be treated the same? If so, I can > remove the check below, as in v1. Or do you mean there is a missing > check on the error value? In which case I can extend this check to > verify we have ENOENT. The former, probably. I don't see how map-in-map is different for lookups and why it needs special handling. > > >> + if (map_is_map_of_maps(map_info->type) || > >> + map_is_map_of_progs(map_info->type)) > >> + return; > >> + > >> + if (json_output) { > >> + jsonw_start_object(json_wtr); /* entry */ > >> + jsonw_name(json_wtr, "key"); > >> + print_hex_data_json(key, map_info->key_size); > >> + jsonw_name(json_wtr, "value"); > >> + jsonw_start_object(json_wtr); /* error */ > >> + jsonw_string_field(json_wtr, "error", strerror(lookup_errno)); > >> + jsonw_end_object(json_wtr); /* error */ > >> + jsonw_end_object(json_wtr); /* entry */ > >> + } else { > >> + const char *msg = NULL; > >> + > >> + if (lookup_errno == ENOENT) > >> + msg = "<no entry>"; > >> + else if (lookup_errno == ENOSPC && > >> + map_info->type == BPF_MAP_TYPE_REUSEPORT_SOCKARRAY) > >> + msg = "<cannot read>"; > >> + > >> + print_entry_error_msg(map_info, key, > >> + msg ? : strerror(lookup_errno)); > >> + } > >> +} > >> + > > > > [...] > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry 2020-09-09 16:30 ` Andrii Nakryiko @ 2020-09-09 16:38 ` Quentin Monnet 2020-09-09 16:46 ` Andrii Nakryiko 0 siblings, 1 reply; 10+ messages in thread From: Quentin Monnet @ 2020-09-09 16:38 UTC (permalink / raw) To: Andrii Nakryiko; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking On 09/09/2020 17:30, Andrii Nakryiko wrote: > On Wed, Sep 9, 2020 at 1:19 AM Quentin Monnet <quentin@isovalent.com> wrote: >> >> On 09/09/2020 04:25, Andrii Nakryiko wrote: >>> On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote: >>>> >>>> The function used to dump a map entry in bpftool is a bit difficult to >>>> follow, as a consequence to earlier refactorings. There is a variable >>>> ("num_elems") which does not appear to be necessary, and the error >>>> handling would look cleaner if moved to its own function. Let's clean it >>>> up. No functional change. >>>> >>>> v2: >>>> - v1 was erroneously removing the check on fd maps in an attempt to get >>>> support for outer map dumps. This is already working. Instead, v2 >>>> focuses on cleaning up the dump_map_elem() function, to avoid >>>> similar confusion in the future. >>>> >>>> Signed-off-by: Quentin Monnet <quentin@isovalent.com> >>>> --- >>>> tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++------------------- >>>> 1 file changed, 52 insertions(+), 49 deletions(-) >>>> >>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c >>>> index bc0071228f88..c8159cb4fb1e 100644 >>>> --- a/tools/bpf/bpftool/map.c >>>> +++ b/tools/bpf/bpftool/map.c >>>> @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key, >>>> jsonw_end_object(json_wtr); >>>> } >>>> >>>> -static void print_entry_error(struct bpf_map_info *info, unsigned char *key, >>>> - const char *error_msg) >>>> +static void >>>> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key, >>>> + const char *error_msg) >>>> { >>>> int msg_size = strlen(error_msg); >>>> bool single_line, break_names; >>>> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key, >>>> printf("\n"); >>>> } >>>> >>>> +static void >>>> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno) >>>> +{ >>>> + /* For prog_array maps or arrays of maps, failure to lookup the value >>>> + * means there is no entry for that key. Do not print an error message >>>> + * in that case. >>>> + */ >>> >>> this is the case when error is ENOENT, all the other ones should be >>> treated the same, no? >> >> Do you mean all map types should be treated the same? If so, I can >> remove the check below, as in v1. Or do you mean there is a missing >> check on the error value? In which case I can extend this check to >> verify we have ENOENT. > > The former, probably. I don't see how map-in-map is different for > lookups and why it needs special handling. I didn't find a particular reason in the logs. My guess is that they may be more likely to have "empty" entries than other types, and that it might be more difficult to spot the existing entries in the middle of a list of "<no entry>" messages. But I agree, let's get rid of this special case and have the same output for all types. I'll respin. Thanks again, Quentin ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry 2020-09-09 16:38 ` Quentin Monnet @ 2020-09-09 16:46 ` Andrii Nakryiko 2020-09-09 16:59 ` Quentin Monnet 0 siblings, 1 reply; 10+ messages in thread From: Andrii Nakryiko @ 2020-09-09 16:46 UTC (permalink / raw) To: Quentin Monnet; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking On Wed, Sep 9, 2020 at 9:38 AM Quentin Monnet <quentin@isovalent.com> wrote: > > On 09/09/2020 17:30, Andrii Nakryiko wrote: > > On Wed, Sep 9, 2020 at 1:19 AM Quentin Monnet <quentin@isovalent.com> wrote: > >> > >> On 09/09/2020 04:25, Andrii Nakryiko wrote: > >>> On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote: > >>>> > >>>> The function used to dump a map entry in bpftool is a bit difficult to > >>>> follow, as a consequence to earlier refactorings. There is a variable > >>>> ("num_elems") which does not appear to be necessary, and the error > >>>> handling would look cleaner if moved to its own function. Let's clean it > >>>> up. No functional change. > >>>> > >>>> v2: > >>>> - v1 was erroneously removing the check on fd maps in an attempt to get > >>>> support for outer map dumps. This is already working. Instead, v2 > >>>> focuses on cleaning up the dump_map_elem() function, to avoid > >>>> similar confusion in the future. > >>>> > >>>> Signed-off-by: Quentin Monnet <quentin@isovalent.com> > >>>> --- > >>>> tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++------------------- > >>>> 1 file changed, 52 insertions(+), 49 deletions(-) > >>>> > >>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c > >>>> index bc0071228f88..c8159cb4fb1e 100644 > >>>> --- a/tools/bpf/bpftool/map.c > >>>> +++ b/tools/bpf/bpftool/map.c > >>>> @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key, > >>>> jsonw_end_object(json_wtr); > >>>> } > >>>> > >>>> -static void print_entry_error(struct bpf_map_info *info, unsigned char *key, > >>>> - const char *error_msg) > >>>> +static void > >>>> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key, > >>>> + const char *error_msg) > >>>> { > >>>> int msg_size = strlen(error_msg); > >>>> bool single_line, break_names; > >>>> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key, > >>>> printf("\n"); > >>>> } > >>>> > >>>> +static void > >>>> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno) > >>>> +{ > >>>> + /* For prog_array maps or arrays of maps, failure to lookup the value > >>>> + * means there is no entry for that key. Do not print an error message > >>>> + * in that case. > >>>> + */ > >>> > >>> this is the case when error is ENOENT, all the other ones should be > >>> treated the same, no? > >> > >> Do you mean all map types should be treated the same? If so, I can > >> remove the check below, as in v1. Or do you mean there is a missing > >> check on the error value? In which case I can extend this check to > >> verify we have ENOENT. > > > > The former, probably. I don't see how map-in-map is different for > > lookups and why it needs special handling. > > I didn't find a particular reason in the logs. My guess is that they may > be more likely to have "empty" entries than other types, and that it > might be more difficult to spot the existing entries in the middle of a > list of "<no entry>" messages. > > But I agree, let's get rid of this special case and have the same output > for all types. I'll respin. Oh, wait, I think what I had in mind is to special case ENOENT for map-in-map and just skip those. So yeah, sorry, there is still a bit of a special handling, but **only** for -ENOENT. When I was replying I forgot bpftool emits "<no entry>" for each -ENOENT by default. > > Thanks again, > Quentin ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry 2020-09-09 16:46 ` Andrii Nakryiko @ 2020-09-09 16:59 ` Quentin Monnet 2020-09-09 19:25 ` Andrii Nakryiko 0 siblings, 1 reply; 10+ messages in thread From: Quentin Monnet @ 2020-09-09 16:59 UTC (permalink / raw) To: Andrii Nakryiko; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking On 09/09/2020 17:46, Andrii Nakryiko wrote: > On Wed, Sep 9, 2020 at 9:38 AM Quentin Monnet <quentin@isovalent.com> wrote: >> >> On 09/09/2020 17:30, Andrii Nakryiko wrote: >>> On Wed, Sep 9, 2020 at 1:19 AM Quentin Monnet <quentin@isovalent.com> wrote: >>>> >>>> On 09/09/2020 04:25, Andrii Nakryiko wrote: >>>>> On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote: >>>>>> >>>>>> The function used to dump a map entry in bpftool is a bit difficult to >>>>>> follow, as a consequence to earlier refactorings. There is a variable >>>>>> ("num_elems") which does not appear to be necessary, and the error >>>>>> handling would look cleaner if moved to its own function. Let's clean it >>>>>> up. No functional change. >>>>>> >>>>>> v2: >>>>>> - v1 was erroneously removing the check on fd maps in an attempt to get >>>>>> support for outer map dumps. This is already working. Instead, v2 >>>>>> focuses on cleaning up the dump_map_elem() function, to avoid >>>>>> similar confusion in the future. >>>>>> >>>>>> Signed-off-by: Quentin Monnet <quentin@isovalent.com> >>>>>> --- >>>>>> tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++------------------- >>>>>> 1 file changed, 52 insertions(+), 49 deletions(-) >>>>>> >>>>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c >>>>>> index bc0071228f88..c8159cb4fb1e 100644 >>>>>> --- a/tools/bpf/bpftool/map.c >>>>>> +++ b/tools/bpf/bpftool/map.c >>>>>> @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key, >>>>>> jsonw_end_object(json_wtr); >>>>>> } >>>>>> >>>>>> -static void print_entry_error(struct bpf_map_info *info, unsigned char *key, >>>>>> - const char *error_msg) >>>>>> +static void >>>>>> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key, >>>>>> + const char *error_msg) >>>>>> { >>>>>> int msg_size = strlen(error_msg); >>>>>> bool single_line, break_names; >>>>>> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key, >>>>>> printf("\n"); >>>>>> } >>>>>> >>>>>> +static void >>>>>> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno) >>>>>> +{ >>>>>> + /* For prog_array maps or arrays of maps, failure to lookup the value >>>>>> + * means there is no entry for that key. Do not print an error message >>>>>> + * in that case. >>>>>> + */ >>>>> >>>>> this is the case when error is ENOENT, all the other ones should be >>>>> treated the same, no? >>>> >>>> Do you mean all map types should be treated the same? If so, I can >>>> remove the check below, as in v1. Or do you mean there is a missing >>>> check on the error value? In which case I can extend this check to >>>> verify we have ENOENT. >>> >>> The former, probably. I don't see how map-in-map is different for >>> lookups and why it needs special handling. >> >> I didn't find a particular reason in the logs. My guess is that they may >> be more likely to have "empty" entries than other types, and that it >> might be more difficult to spot the existing entries in the middle of a >> list of "<no entry>" messages. >> >> But I agree, let's get rid of this special case and have the same output >> for all types. I'll respin. > > Oh, wait, I think what I had in mind is to special case ENOENT for > map-in-map and just skip those. So yeah, sorry, there is still a bit > of a special handling, but **only** for -ENOENT. When I was replying I > forgot bpftool emits "<no entry>" for each -ENOENT by default. So do you prefer me to extend the check with errno == -ENOENT? Or shall I remove it entirely and just have the "<no entry>" messages like for the other map types? Quentin ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry 2020-09-09 16:59 ` Quentin Monnet @ 2020-09-09 19:25 ` Andrii Nakryiko 0 siblings, 0 replies; 10+ messages in thread From: Andrii Nakryiko @ 2020-09-09 19:25 UTC (permalink / raw) To: Quentin Monnet; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Networking On Wed, Sep 9, 2020 at 9:59 AM Quentin Monnet <quentin@isovalent.com> wrote: > > On 09/09/2020 17:46, Andrii Nakryiko wrote: > > On Wed, Sep 9, 2020 at 9:38 AM Quentin Monnet <quentin@isovalent.com> wrote: > >> > >> On 09/09/2020 17:30, Andrii Nakryiko wrote: > >>> On Wed, Sep 9, 2020 at 1:19 AM Quentin Monnet <quentin@isovalent.com> wrote: > >>>> > >>>> On 09/09/2020 04:25, Andrii Nakryiko wrote: > >>>>> On Mon, Sep 7, 2020 at 9:36 AM Quentin Monnet <quentin@isovalent.com> wrote: > >>>>>> > >>>>>> The function used to dump a map entry in bpftool is a bit difficult to > >>>>>> follow, as a consequence to earlier refactorings. There is a variable > >>>>>> ("num_elems") which does not appear to be necessary, and the error > >>>>>> handling would look cleaner if moved to its own function. Let's clean it > >>>>>> up. No functional change. > >>>>>> > >>>>>> v2: > >>>>>> - v1 was erroneously removing the check on fd maps in an attempt to get > >>>>>> support for outer map dumps. This is already working. Instead, v2 > >>>>>> focuses on cleaning up the dump_map_elem() function, to avoid > >>>>>> similar confusion in the future. > >>>>>> > >>>>>> Signed-off-by: Quentin Monnet <quentin@isovalent.com> > >>>>>> --- > >>>>>> tools/bpf/bpftool/map.c | 101 +++++++++++++++++++++------------------- > >>>>>> 1 file changed, 52 insertions(+), 49 deletions(-) > >>>>>> > >>>>>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c > >>>>>> index bc0071228f88..c8159cb4fb1e 100644 > >>>>>> --- a/tools/bpf/bpftool/map.c > >>>>>> +++ b/tools/bpf/bpftool/map.c > >>>>>> @@ -213,8 +213,9 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key, > >>>>>> jsonw_end_object(json_wtr); > >>>>>> } > >>>>>> > >>>>>> -static void print_entry_error(struct bpf_map_info *info, unsigned char *key, > >>>>>> - const char *error_msg) > >>>>>> +static void > >>>>>> +print_entry_error_msg(struct bpf_map_info *info, unsigned char *key, > >>>>>> + const char *error_msg) > >>>>>> { > >>>>>> int msg_size = strlen(error_msg); > >>>>>> bool single_line, break_names; > >>>>>> @@ -232,6 +233,40 @@ static void print_entry_error(struct bpf_map_info *info, unsigned char *key, > >>>>>> printf("\n"); > >>>>>> } > >>>>>> > >>>>>> +static void > >>>>>> +print_entry_error(struct bpf_map_info *map_info, void *key, int lookup_errno) > >>>>>> +{ > >>>>>> + /* For prog_array maps or arrays of maps, failure to lookup the value > >>>>>> + * means there is no entry for that key. Do not print an error message > >>>>>> + * in that case. > >>>>>> + */ > >>>>> > >>>>> this is the case when error is ENOENT, all the other ones should be > >>>>> treated the same, no? > >>>> > >>>> Do you mean all map types should be treated the same? If so, I can > >>>> remove the check below, as in v1. Or do you mean there is a missing > >>>> check on the error value? In which case I can extend this check to > >>>> verify we have ENOENT. > >>> > >>> The former, probably. I don't see how map-in-map is different for > >>> lookups and why it needs special handling. > >> > >> I didn't find a particular reason in the logs. My guess is that they may > >> be more likely to have "empty" entries than other types, and that it > >> might be more difficult to spot the existing entries in the middle of a > >> list of "<no entry>" messages. > >> > >> But I agree, let's get rid of this special case and have the same output > >> for all types. I'll respin. > > > > Oh, wait, I think what I had in mind is to special case ENOENT for > > map-in-map and just skip those. So yeah, sorry, there is still a bit > > of a special handling, but **only** for -ENOENT. When I was replying I > > forgot bpftool emits "<no entry>" for each -ENOENT by default. > > So do you prefer me to extend the check with errno == -ENOENT? Or shall > I remove it entirely and just have the "<no entry>" messages like for > the other map types? Extend with -ENOENT check. > > Quentin ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf-next v2 2/2] tools: bpftool: add "inner_map" to "bpftool map create" outer maps 2020-09-07 16:36 [PATCH bpf-next v2 0/2] tools: bpftool: support creating outer maps Quentin Monnet 2020-09-07 16:36 ` [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry Quentin Monnet @ 2020-09-07 16:36 ` Quentin Monnet 1 sibling, 0 replies; 10+ messages in thread From: Quentin Monnet @ 2020-09-07 16:36 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann Cc: bpf, netdev, Andrii Nakryiko, Quentin Monnet, Andrii Nakryiko There is no support for creating maps of types array-of-map or hash-of-map in bpftool. This is because the kernel needs an inner_map_fd to collect metadata on the inner maps to be supported by the new map, but bpftool does not provide a way to pass this file descriptor. Add a new optional "inner_map" keyword that can be used to pass a reference to a map, retrieve a fd to that map, and pass it as the inner_map_fd. Add related documentation and bash completion. Note that we can reference the inner map by its name, meaning we can have several times the keyword "name" with different meanings (mandatory outer map name, and possibly a name to use to find the inner_map_fd). The bash completion will offer it just once, and will not suggest "name" on the following command: # bpftool map create /sys/fs/bpf/my_outer_map type hash_of_maps \ inner_map name my_inner_map [TAB] Fixing that specific case seems too convoluted. Completion will work as expected, however, if the outer map name comes first and the "inner_map name ..." is passed second. Signed-off-by: Quentin Monnet <quentin@isovalent.com> Acked-by: Andrii Nakryiko <andriin@fb.com> --- .../bpf/bpftool/Documentation/bpftool-map.rst | 10 +++- tools/bpf/bpftool/bash-completion/bpftool | 22 ++++++++- tools/bpf/bpftool/map.c | 48 +++++++++++++------ 3 files changed, 62 insertions(+), 18 deletions(-) diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst index 083db6c2fc67..ca9d62d7e0bd 100644 --- a/tools/bpf/bpftool/Documentation/bpftool-map.rst +++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst @@ -23,7 +23,8 @@ 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*] +| **entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**inner_map** *MAP*] \ +| [**dev** *NAME*] | **bpftool** **map dump** *MAP* | **bpftool** **map update** *MAP* [**key** *DATA*] [**value** *VALUE*] [*UPDATE_FLAGS*] | **bpftool** **map lookup** *MAP* [**key** *DATA*] @@ -67,7 +68,7 @@ DESCRIPTION maps. On such kernels bpftool will automatically emit this information as well. - **bpftool map create** *FILE* **type** *TYPE* **key** *KEY_SIZE* **value** *VALUE_SIZE* **entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**dev** *NAME*] + **bpftool map create** *FILE* **type** *TYPE* **key** *KEY_SIZE* **value** *VALUE_SIZE* **entries** *MAX_ENTRIES* **name** *NAME* [**flags** *FLAGS*] [**inner_map** *MAP*] [**dev** *NAME*] Create a new map with given parameters and pin it to *bpffs* as *FILE*. @@ -75,6 +76,11 @@ DESCRIPTION desired flags, e.g. 1024 for **BPF_F_MMAPABLE** (see bpf.h UAPI header for existing flags). + To create maps of type array-of-maps or hash-of-maps, the + **inner_map** keyword must be used to pass an inner map. The + kernel needs it to collect metadata related to the inner maps + that the new map will work with. + Keyword **dev** expects a network interface name, and is used to request hardware offload for the map. diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool index 7b68e3c0a5fb..3f1da30c4da6 100644 --- a/tools/bpf/bpftool/bash-completion/bpftool +++ b/tools/bpf/bpftool/bash-completion/bpftool @@ -709,9 +709,26 @@ _bpftool() "$cur" ) ) return 0 ;; - key|value|flags|name|entries) + key|value|flags|entries) return 0 ;; + inner_map) + COMPREPLY=( $( compgen -W "$MAP_TYPE" -- "$cur" ) ) + return 0 + ;; + id) + _bpftool_get_map_ids + ;; + name) + case $pprev in + inner_map) + _bpftool_get_map_names + ;; + *) + return 0 + ;; + esac + ;; *) _bpftool_once_attr 'type' _bpftool_once_attr 'key' @@ -719,6 +736,9 @@ _bpftool() _bpftool_once_attr 'entries' _bpftool_once_attr 'name' _bpftool_once_attr 'flags' + if _bpftool_search_list 'array_of_maps' 'hash_of_maps'; then + _bpftool_once_attr 'inner_map' + fi _bpftool_once_attr 'dev' return 0 ;; diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c index c8159cb4fb1e..7370fdd57de3 100644 --- a/tools/bpf/bpftool/map.c +++ b/tools/bpf/bpftool/map.c @@ -1250,7 +1250,7 @@ static int do_create(int argc, char **argv) { struct bpf_create_map_attr attr = { NULL, }; const char *pinfile; - int err, fd; + int err = -1, fd; if (!REQ_ARGS(7)) return -1; @@ -1265,13 +1265,13 @@ static int do_create(int argc, char **argv) if (attr.map_type) { p_err("map type already specified"); - return -1; + goto exit; } attr.map_type = map_type_from_str(*argv); if ((int)attr.map_type < 0) { p_err("unrecognized map type: %s", *argv); - return -1; + goto exit; } NEXT_ARG(); } else if (is_prefix(*argv, "name")) { @@ -1280,43 +1280,56 @@ static int do_create(int argc, char **argv) } else if (is_prefix(*argv, "key")) { if (parse_u32_arg(&argc, &argv, &attr.key_size, "key size")) - return -1; + goto exit; } else if (is_prefix(*argv, "value")) { if (parse_u32_arg(&argc, &argv, &attr.value_size, "value size")) - return -1; + goto exit; } else if (is_prefix(*argv, "entries")) { if (parse_u32_arg(&argc, &argv, &attr.max_entries, "max entries")) - return -1; + goto exit; } else if (is_prefix(*argv, "flags")) { if (parse_u32_arg(&argc, &argv, &attr.map_flags, "flags")) - return -1; + goto exit; } else if (is_prefix(*argv, "dev")) { NEXT_ARG(); if (attr.map_ifindex) { p_err("offload device already specified"); - return -1; + goto exit; } attr.map_ifindex = if_nametoindex(*argv); if (!attr.map_ifindex) { p_err("unrecognized netdevice '%s': %s", *argv, strerror(errno)); - return -1; + goto exit; } NEXT_ARG(); + } else if (is_prefix(*argv, "inner_map")) { + struct bpf_map_info info = {}; + __u32 len = sizeof(info); + int inner_map_fd; + + NEXT_ARG(); + if (!REQ_ARGS(2)) + usage(); + inner_map_fd = map_parse_fd_and_info(&argc, &argv, + &info, &len); + if (inner_map_fd < 0) + return -1; + attr.inner_map_fd = inner_map_fd; } else { p_err("unknown arg %s", *argv); - return -1; + goto exit; } } if (!attr.name) { p_err("map name not specified"); - return -1; + goto exit; } set_max_rlimit(); @@ -1324,17 +1337,22 @@ static int do_create(int argc, char **argv) fd = bpf_create_map_xattr(&attr); if (fd < 0) { p_err("map create failed: %s", strerror(errno)); - return -1; + goto exit; } err = do_pin_fd(fd, pinfile); close(fd); if (err) - return err; + goto exit; if (json_output) jsonw_null(json_wtr); - return 0; + +exit: + if (attr.inner_map_fd > 0) + close(attr.inner_map_fd); + + return err; } static int do_pop_dequeue(int argc, char **argv) @@ -1420,7 +1438,7 @@ static int do_help(int argc, char **argv) "Usage: %1$s %2$s { show | list } [MAP]\n" " %1$s %2$s create FILE type TYPE key KEY_SIZE value VALUE_SIZE \\\n" " entries MAX_ENTRIES name NAME [flags FLAGS] \\\n" - " [dev NAME]\n" + " [inner_map MAP] [dev NAME]\n" " %1$s %2$s dump MAP\n" " %1$s %2$s update MAP [key DATA] [value VALUE] [UPDATE_FLAGS]\n" " %1$s %2$s lookup MAP [key DATA]\n" -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-09-09 19:25 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-07 16:36 [PATCH bpf-next v2 0/2] tools: bpftool: support creating outer maps Quentin Monnet 2020-09-07 16:36 ` [PATCH bpf-next v2 1/2] tools: bpftool: clean up function to dump map entry Quentin Monnet 2020-09-09 3:25 ` Andrii Nakryiko 2020-09-09 8:19 ` Quentin Monnet 2020-09-09 16:30 ` Andrii Nakryiko 2020-09-09 16:38 ` Quentin Monnet 2020-09-09 16:46 ` Andrii Nakryiko 2020-09-09 16:59 ` Quentin Monnet 2020-09-09 19:25 ` Andrii Nakryiko 2020-09-07 16:36 ` [PATCH bpf-next v2 2/2] tools: bpftool: add "inner_map" to "bpftool map create" outer maps 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).