| Message ID | 20544b6e19128021e7a25b5a649630aef20c2731.1547830062.git.pabeni@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] bpftool: fix percpu maps updating | expand |
2019-01-18 17:50 UTC+0100 ~ Paolo Abeni <pabeni@redhat.com> > When updating a percpu map, bpftool currently copies the provided > value only into the first per CPU copy of the specified value, > all others instances are left zeroed. > > This change explicitly copies the user-provided bytes to all the > per CPU instances, keeping the sub-command syntax unchanged. > > Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > tools/bpf/bpftool/map.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c > index 2037e3dc864b..a73b5bb33ddf 100644 > --- a/tools/bpf/bpftool/map.c > +++ b/tools/bpf/bpftool/map.c > @@ -347,6 +347,20 @@ static char **parse_bytes(char **argv, const char *name, unsigned char *val, > return argv + i; > } > > +/* on per cpu maps we must copy the provided value on all value instances */ > +static void fill_value(struct bpf_map_info *info, void *value, __u32 value_size) > +{ > + unsigned int i, n, step; > + > + if (!map_is_per_cpu(info->type)) > + return; > + > + n = get_possible_cpus(); > + step = round_up(info->value_size, 8); > + for (i = 1; i < n; i++) > + memcpy(value + i * step, value, info->value_size); > +} > + > static int parse_elem(char **argv, struct bpf_map_info *info, > void *key, void *value, __u32 key_size, __u32 value_size, > __u32 *flags, __u32 **value_fd) > @@ -426,6 +440,8 @@ static int parse_elem(char **argv, struct bpf_map_info *info, > argv = parse_bytes(argv, "value", value, value_size); > if (!argv) > return -1; > + > + fill_value(info, value, value_size); Hi Paolo, The patch looks good to me, thanks for it! One minor nit though, could we make it more obvious in parse_elem() that fill_value() is only for per-CPU maps? Reading parse_elem()'s code it seems that we need to fill a value for all map types, this is a bit confusing. I was thinking about either renaming the function ("fill_per_cpu_value()" or something like this?), or moving the check on map_is_per_cpu() before the function is called. What do you think? > } > > return parse_elem(argv, info, key, NULL, key_size, value_size, >
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c index 2037e3dc864b..a73b5bb33ddf 100644 --- a/tools/bpf/bpftool/map.c +++ b/tools/bpf/bpftool/map.c @@ -347,6 +347,20 @@ static char **parse_bytes(char **argv, const char *name, unsigned char *val, return argv + i; } +/* on per cpu maps we must copy the provided value on all value instances */ +static void fill_value(struct bpf_map_info *info, void *value, __u32 value_size) +{ + unsigned int i, n, step; + + if (!map_is_per_cpu(info->type)) + return; + + n = get_possible_cpus(); + step = round_up(info->value_size, 8); + for (i = 1; i < n; i++) + memcpy(value + i * step, value, info->value_size); +} + static int parse_elem(char **argv, struct bpf_map_info *info, void *key, void *value, __u32 key_size, __u32 value_size, __u32 *flags, __u32 **value_fd) @@ -426,6 +440,8 @@ static int parse_elem(char **argv, struct bpf_map_info *info, argv = parse_bytes(argv, "value", value, value_size); if (!argv) return -1; + + fill_value(info, value, value_size); } return parse_elem(argv, info, key, NULL, key_size, value_size,
When updating a percpu map, bpftool currently copies the provided value only into the first per CPU copy of the specified value, all others instances are left zeroed. This change explicitly copies the user-provided bytes to all the per CPU instances, keeping the sub-command syntax unchanged. Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- tools/bpf/bpftool/map.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)