* [PATCH bpf 1/2] bpftool: support queues and stacks in update command
@ 2019-01-03 18:33 Stanislav Fomichev
2019-01-03 18:33 ` [PATCH bpf 2/2] bpftool: support queues and stacks in lookup command Stanislav Fomichev
2019-01-03 19:43 ` [PATCH bpf 1/2] bpftool: support queues and stacks in update command Jakub Kicinski
0 siblings, 2 replies; 4+ messages in thread
From: Stanislav Fomichev @ 2019-01-03 18:33 UTC (permalink / raw)
To: netdev
Cc: davem, ast, daniel, jakub.kicinski, quentin.monnet, Stanislav Fomichev
Bpftool expects both key and value for 'update' operations. For some
map types, key should not be specified. Support updating those map types.
Before:
bpftool map create /sys/fs/bpf/q type queue value 4 entries 10 name q
bpftool map update pinned /sys/fs/bpf/q value 0 1 2 3
Error: did not find key
After:
bpftool map create /sys/fs/bpf/q type queue value 4 entries 10 name q
bpftool map update pinned /sys/fs/bpf/q value 0 1 2 3
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/bpf/bpftool/Documentation/bpftool-map.rst | 4 ++--
tools/bpf/bpftool/map.c | 15 +++++++++++----
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index 64b001b4f777..81842fdf483d 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -25,7 +25,7 @@ MAP COMMANDS
| **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 update** *MAP* [**key** *DATA*] **value** *VALUE* [*UPDATE_FLAGS*]
| **bpftool** **map lookup** *MAP* **key** *DATA*
| **bpftool** **map getnext** *MAP* [**key** *DATA*]
| **bpftool** **map delete** *MAP* **key** *DATA*
@@ -62,7 +62,7 @@ DESCRIPTION
**bpftool map dump** *MAP*
Dump all entries in a given *MAP*.
- **bpftool map update** *MAP* **key** *DATA* **value** *VALUE* [*UPDATE_FLAGS*]
+ **bpftool map update** *MAP* [**key** *DATA*] **value** *VALUE* [*UPDATE_FLAGS*]
Update map entry for a given *KEY*.
*UPDATE_FLAGS* can be one of: **any** update existing entry
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 2037e3dc864b..30b92715248d 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -781,11 +781,11 @@ static int do_dump(int argc, char **argv)
static int do_update(int argc, char **argv)
{
+ void *key = NULL, *value = NULL;
struct bpf_map_info info = {};
__u32 len = sizeof(info);
__u32 *value_fd = NULL;
__u32 flags = BPF_ANY;
- void *key, *value;
int fd, err;
if (argc < 2)
@@ -795,9 +795,16 @@ static int do_update(int argc, char **argv)
if (fd < 0)
return -1;
- key = malloc(info.key_size);
+ if (info.key_size) {
+ key = malloc(info.key_size);
+ if (!key) {
+ p_err("mem alloc failed");
+ err = -1;
+ goto exit_free;
+ }
+ }
value = alloc_value(&info);
- if (!key || !value) {
+ if (!value) {
p_err("mem alloc failed");
err = -1;
goto exit_free;
@@ -1135,7 +1142,7 @@ static int do_help(int argc, char **argv)
" 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 update MAP [key DATA] value VALUE [UPDATE_FLAGS]\n"
" %s %s lookup MAP key DATA\n"
" %s %s getnext MAP [key DATA]\n"
" %s %s delete MAP key DATA\n"
--
2.20.1.415.g653613c723-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH bpf 2/2] bpftool: support queues and stacks in lookup command
2019-01-03 18:33 [PATCH bpf 1/2] bpftool: support queues and stacks in update command Stanislav Fomichev
@ 2019-01-03 18:33 ` Stanislav Fomichev
2019-01-03 19:43 ` [PATCH bpf 1/2] bpftool: support queues and stacks in update command Jakub Kicinski
1 sibling, 0 replies; 4+ messages in thread
From: Stanislav Fomichev @ 2019-01-03 18:33 UTC (permalink / raw)
To: netdev
Cc: davem, ast, daniel, jakub.kicinski, quentin.monnet, Stanislav Fomichev
Bpftool expects key for 'lookup' operations. For some map types, key should
not be specified. Support looking up those map types.
Before:
bpftool map create /sys/fs/bpf/q type queue value 4 entries 10 name q
bpftool map update pinned /sys/fs/bpf/q value 0 1 2 3
bpftool map lookup pinned /sys/fs/bpf/q
Error: did not find key
After:
bpftool map create /sys/fs/bpf/q type queue value 4 entries 10 name q
bpftool map update pinned /sys/fs/bpf/q value 0 1 2 3
bpftool map lookup pinned /sys/fs/bpf/q
key: value: 00 01 02 03
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/bpf/bpftool/Documentation/bpftool-map.rst | 4 ++--
tools/bpf/bpftool/map.c | 15 +++++++++++----
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index 81842fdf483d..ea2353330c94 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -26,7 +26,7 @@ MAP COMMANDS
| **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*
+| **bpftool** **map lookup** *MAP* [**key** *DATA*]
| **bpftool** **map getnext** *MAP* [**key** *DATA*]
| **bpftool** **map delete** *MAP* **key** *DATA*
| **bpftool** **map pin** *MAP* *FILE*
@@ -75,7 +75,7 @@ DESCRIPTION
the bytes are parsed as decimal values, unless a "0x" prefix
(for hexadecimal) or a "0" prefix (for octal) is provided.
- **bpftool map lookup** *MAP* **key** *DATA*
+ **bpftool map lookup** *MAP* [**key** *DATA*]
Lookup **key** in the map.
**bpftool map getnext** *MAP* [**key** *DATA*]
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 30b92715248d..3c7adc31b567 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -835,11 +835,11 @@ static int do_update(int argc, char **argv)
static int do_lookup(int argc, char **argv)
{
+ void *key = NULL, *value = NULL;
struct bpf_map_info info = {};
__u32 len = sizeof(info);
json_writer_t *btf_wtr;
struct btf *btf = NULL;
- void *key, *value;
int err;
int fd;
@@ -850,9 +850,16 @@ static int do_lookup(int argc, char **argv)
if (fd < 0)
return -1;
- key = malloc(info.key_size);
+ if (info.key_size) {
+ key = malloc(info.key_size);
+ if (!key) {
+ p_err("mem alloc failed");
+ err = -1;
+ goto exit_free;
+ }
+ }
value = alloc_value(&info);
- if (!key || !value) {
+ if (!value) {
p_err("mem alloc failed");
err = -1;
goto exit_free;
@@ -1143,7 +1150,7 @@ static int do_help(int argc, char **argv)
" [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"
+ " %s %s lookup MAP [key DATA]\n"
" %s %s getnext MAP [key DATA]\n"
" %s %s delete MAP key DATA\n"
" %s %s pin MAP FILE\n"
--
2.20.1.415.g653613c723-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf 1/2] bpftool: support queues and stacks in update command
2019-01-03 18:33 [PATCH bpf 1/2] bpftool: support queues and stacks in update command Stanislav Fomichev
2019-01-03 18:33 ` [PATCH bpf 2/2] bpftool: support queues and stacks in lookup command Stanislav Fomichev
@ 2019-01-03 19:43 ` Jakub Kicinski
2019-01-03 20:52 ` Stanislav Fomichev
1 sibling, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2019-01-03 19:43 UTC (permalink / raw)
To: Stanislav Fomichev; +Cc: netdev, davem, ast, daniel, quentin.monnet
On Thu, 3 Jan 2019 10:33:05 -0800, Stanislav Fomichev wrote:
> Bpftool expects both key and value for 'update' operations. For some
> map types, key should not be specified. Support updating those map types.
>
> Before:
> bpftool map create /sys/fs/bpf/q type queue value 4 entries 10 name q
> bpftool map update pinned /sys/fs/bpf/q value 0 1 2 3
> Error: did not find key
>
> After:
> bpftool map create /sys/fs/bpf/q type queue value 4 entries 10 name q
> bpftool map update pinned /sys/fs/bpf/q value 0 1 2 3
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
I guess it doesn't hurt to fix update/lookup, but I'd prefer to see new
separate subcommands to be honest :(
bpftool map push/pop/peek
Could you add those as well? I think most users will be more familiar
with the helpers than the fact that the syscall reuses the old commands.
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 2037e3dc864b..30b92715248d 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -781,11 +781,11 @@ static int do_dump(int argc, char **argv)
>
> static int do_update(int argc, char **argv)
> {
> + void *key = NULL, *value = NULL;
nit: it seems tiny bit more readable to init these in the place you'd
otherwise set key to malloc (in an else clause)
> struct bpf_map_info info = {};
> __u32 len = sizeof(info);
> __u32 *value_fd = NULL;
> __u32 flags = BPF_ANY;
> - void *key, *value;
> int fd, err;
>
> if (argc < 2)
> @@ -795,9 +795,16 @@ static int do_update(int argc, char **argv)
> if (fd < 0)
> return -1;
>
> - key = malloc(info.key_size);
> + if (info.key_size) {
> + key = malloc(info.key_size);
> + if (!key) {
> + p_err("mem alloc failed");
> + err = -1;
> + goto exit_free;
> + }
> + }
> value = alloc_value(&info);
Would you mind taking care of the value as well? So we are ready if
sets are ever added?
> - if (!key || !value) {
> + if (!value) {
> p_err("mem alloc failed");
> err = -1;
> goto exit_free;
I'd consider this -next material TBH, but not strongly.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf 1/2] bpftool: support queues and stacks in update command
2019-01-03 19:43 ` [PATCH bpf 1/2] bpftool: support queues and stacks in update command Jakub Kicinski
@ 2019-01-03 20:52 ` Stanislav Fomichev
0 siblings, 0 replies; 4+ messages in thread
From: Stanislav Fomichev @ 2019-01-03 20:52 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, davem, Alexei Starovoitov, Daniel Borkmann, Quentin Monnet
On Thu, Jan 3, 2019 at 11:43 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Thu, 3 Jan 2019 10:33:05 -0800, Stanislav Fomichev wrote:
> > Bpftool expects both key and value for 'update' operations. For some
> > map types, key should not be specified. Support updating those map types.
> >
> > Before:
> > bpftool map create /sys/fs/bpf/q type queue value 4 entries 10 name q
> > bpftool map update pinned /sys/fs/bpf/q value 0 1 2 3
> > Error: did not find key
> >
> > After:
> > bpftool map create /sys/fs/bpf/q type queue value 4 entries 10 name q
> > bpftool map update pinned /sys/fs/bpf/q value 0 1 2 3
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
>
> I guess it doesn't hurt to fix update/lookup, but I'd prefer to see new
> separate subcommands to be honest :(
>
> bpftool map push/pop/peek
>
> Could you add those as well? I think most users will be more familiar
> with the helpers than the fact that the syscall reuses the old commands.
Sure, I thought about that, but decided to mirror syscall interface initially.
I can do both: support update/lookup and add new commands.
>
> > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > index 2037e3dc864b..30b92715248d 100644
> > --- a/tools/bpf/bpftool/map.c
> > +++ b/tools/bpf/bpftool/map.c
> > @@ -781,11 +781,11 @@ static int do_dump(int argc, char **argv)
> >
> > static int do_update(int argc, char **argv)
> > {
> > + void *key = NULL, *value = NULL;
>
> nit: it seems tiny bit more readable to init these in the place you'd
> otherwise set key to malloc (in an else clause)
Ack, will do.
>
> > struct bpf_map_info info = {};
> > __u32 len = sizeof(info);
> > __u32 *value_fd = NULL;
> > __u32 flags = BPF_ANY;
> > - void *key, *value;
> > int fd, err;
> >
> > if (argc < 2)
> > @@ -795,9 +795,16 @@ static int do_update(int argc, char **argv)
> > if (fd < 0)
> > return -1;
> >
> > - key = malloc(info.key_size);
> > + if (info.key_size) {
> > + key = malloc(info.key_size);
> > + if (!key) {
> > + p_err("mem alloc failed");
> > + err = -1;
> > + goto exit_free;
> > + }
> > + }
> > value = alloc_value(&info);
>
> Would you mind taking care of the value as well? So we are ready if
> sets are ever added?
Sure, makes sense.
>
> > - if (!key || !value) {
> > + if (!value) {
> > p_err("mem alloc failed");
> > err = -1;
> > goto exit_free;
>
> I'd consider this -next material TBH, but not strongly.
My initial thought that these two can go into bpf as 'fixes' because
they technically don't add any new features.
But I'm fine with -next as well, I'll prepare a v2 for bpf-next with
those two changes plus push/pop/peek.
Thank you for a review!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-01-03 20:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-03 18:33 [PATCH bpf 1/2] bpftool: support queues and stacks in update command Stanislav Fomichev
2019-01-03 18:33 ` [PATCH bpf 2/2] bpftool: support queues and stacks in lookup command Stanislav Fomichev
2019-01-03 19:43 ` [PATCH bpf 1/2] bpftool: support queues and stacks in update command Jakub Kicinski
2019-01-03 20:52 ` Stanislav Fomichev
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).