Re: [PATCH bpf-next 11/11] tools: bpftool: allow reuse of maps with bpftool prog load
On Fri, 6 Jul 2018 09:16:24 +0200, Daniel Borkmann wrote: > On 07/06/2018 12:57 AM, Jakub Kicinski wrote: > > On Thu, 5 Jul 2018 10:35:24 +0200, Daniel Borkmann wrote: > >> On 07/04/2018 04:54 AM, Jakub Kicinski wrote: > >>> Add map parameter to prog load which will allow reuse of existing > >>> maps instead of creating new ones. > >>> > >>> Signed-off-by: Jakub Kicinski > >>> Reviewed-by: Quentin Monnet > >> [...] > >>> + > >>> + fd = map_parse_fd(, ); > >>> + if (fd < 0) > >>> + goto err_free_reuse_maps; > >>> + > >>> + map_replace = reallocarray(map_replace, old_map_fds + 1, > >>> +sizeof(*map_replace)); > >>> + if (!map_replace) { > >>> + p_err("mem alloc failed"); > >>> + goto err_free_reuse_maps; > >> > >> Series in general looks good to me. However, above reallocarray() doesn't > >> exist and hence build fails, please see below. Is that from newest glibc? > >> > >> You probably need some fallback implementation or in general have something > >> bpftool internal that doesn't make a bet on its availability. > >> > >> # make > >> > >> Auto-detecting system features: > >> ...libbfd: [ on ] > >> ...disassembler-four-args: [ OFF ] > >> > >> CC bpf_jit_disasm.o > >> LINK bpf_jit_disasm > >> CC bpf_dbg.o > >> LINK bpf_dbg > >> CC bpf_asm.o > >> BISONbpf_exp.yacc.c > >> CC bpf_exp.yacc.o > >> FLEX bpf_exp.lex.c > >> CC bpf_exp.lex.o > >> LINK bpf_asm > >> DESCEND bpftool > >> > >> Auto-detecting system features: > y>> ...libbfd: [ on ] > >> ...disassembler-four-args: [ OFF ] > >> > >> CC map_perf_ring.o > >> CC xlated_dumper.o > >> CC perf.o > >> CC prog.o > >> prog.c: In function ‘do_load’: > >> prog.c:785:18: warning: implicit declaration of function ‘reallocarray’ > >> [-Wimplicit-function-declaration] > >> map_replace = reallocarray(map_replace, old_map_fds + 1, > >> ^~~~ > >> prog.c:785:16: warning: assignment makes pointer from integer without a > >> cast [-Wint-conversion] > >> map_replace = reallocarray(map_replace, old_map_fds + 1, > >> ^ > >> CC common.o > >> CC cgroup.o > >> CC main.o > >> CC json_writer.o > >> CC cfg.o > >> CC map.o > >> CC jit_disasm.o > >> CC disasm.o > >> > >> Auto-detecting system features: > >> ...libelf: [ on ] > >> ... bpf: [ on ] > >> > >> Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs > >> from latest version at 'include/uapi/linux/bpf.h' > >> CC libbpf.o > >> CC bpf.o > >> CC nlattr.o > >> CC btf.o > >> LD libbpf-in.o > >> LINK libbpf.a > >> LINK bpftool > >> prog.o: In function `do_load': > >> prog.c:(.text+0x23d): undefined reference to `reallocarray' > >> collect2: error: ld returned 1 exit status > >> Makefile:89: recipe for target 'bpftool' failed > >> make[1]: *** [bpftool] Error 1 > >> Makefile:99: recipe for target 'bpftool' failed > >> make: *** [bpftool] Error 2 > > > > Oh no.. Sorry & thanks for catching this. It would be nice to not > > depend on Glibc version defines, in case someone is using a different > > library. Jiong suggested we can just use the feature detection, so I > > have something like this: > > Yeah, that would be okay to do if you want to go that route, sure. Other > option > I had in mind would have been to import include/linux/overflow.h into the > tools/include/linux/ and have a minor wrapper similar to kmalloc_array() in a > utils.h in bpftool to get to the same for all users. But I think feature test > is totally fine too, and in general some form of reallocarray() would be good > to have rather than plain realloc(). > > So, below complies for me. Although don't we need to define a CFLAG based on > the outcome of the test similar as in feature-disassembler-four-args? > Otherwise > with the below diff the test doesn't really do much, no? Meaning, adding a ... > > ifeq ($(feature-reallocarray), 1) > CFLAGS += -DHAVE_REALLOCARRAY > endif > > ... to the Makefile and in compat.h having it enabled through: > > #ifndef HAVE_REALLOCARRAY > static inline void *reallocarray(void *ptr, size_t nmemb, size_t size) > { > return realloc(ptr, nmemb * size); > } > #endif Ugh, you're very right, reallocarray is a weak alias in glibc, which is probably why I didn't see any errors. > In any case, we could use reallocarray() also in couple of other places in > bpftool and libbpf. I'll look into it, too.
Re: [PATCH bpf-next 11/11] tools: bpftool: allow reuse of maps with bpftool prog load
On 07/06/2018 12:57 AM, Jakub Kicinski wrote: > On Thu, 5 Jul 2018 10:35:24 +0200, Daniel Borkmann wrote: >> On 07/04/2018 04:54 AM, Jakub Kicinski wrote: >>> Add map parameter to prog load which will allow reuse of existing >>> maps instead of creating new ones. >>> >>> Signed-off-by: Jakub Kicinski >>> Reviewed-by: Quentin Monnet >> [...] >>> + >>> + fd = map_parse_fd(, ); >>> + if (fd < 0) >>> + goto err_free_reuse_maps; >>> + >>> + map_replace = reallocarray(map_replace, old_map_fds + 1, >>> + sizeof(*map_replace)); >>> + if (!map_replace) { >>> + p_err("mem alloc failed"); >>> + goto err_free_reuse_maps; >> >> Series in general looks good to me. However, above reallocarray() doesn't >> exist and hence build fails, please see below. Is that from newest glibc? >> >> You probably need some fallback implementation or in general have something >> bpftool internal that doesn't make a bet on its availability. >> >> # make >> >> Auto-detecting system features: >> ...libbfd: [ on ] >> ...disassembler-four-args: [ OFF ] >> >> CC bpf_jit_disasm.o >> LINK bpf_jit_disasm >> CC bpf_dbg.o >> LINK bpf_dbg >> CC bpf_asm.o >> BISONbpf_exp.yacc.c >> CC bpf_exp.yacc.o >> FLEX bpf_exp.lex.c >> CC bpf_exp.lex.o >> LINK bpf_asm >> DESCEND bpftool >> >> Auto-detecting system features: y>> ...libbfd: [ on ] >> ...disassembler-four-args: [ OFF ] >> >> CC map_perf_ring.o >> CC xlated_dumper.o >> CC perf.o >> CC prog.o >> prog.c: In function ‘do_load’: >> prog.c:785:18: warning: implicit declaration of function ‘reallocarray’ >> [-Wimplicit-function-declaration] >> map_replace = reallocarray(map_replace, old_map_fds + 1, >> ^~~~ >> prog.c:785:16: warning: assignment makes pointer from integer without a cast >> [-Wint-conversion] >> map_replace = reallocarray(map_replace, old_map_fds + 1, >> ^ >> CC common.o >> CC cgroup.o >> CC main.o >> CC json_writer.o >> CC cfg.o >> CC map.o >> CC jit_disasm.o >> CC disasm.o >> >> Auto-detecting system features: >> ...libelf: [ on ] >> ... bpf: [ on ] >> >> Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs from >> latest version at 'include/uapi/linux/bpf.h' >> CC libbpf.o >> CC bpf.o >> CC nlattr.o >> CC btf.o >> LD libbpf-in.o >> LINK libbpf.a >> LINK bpftool >> prog.o: In function `do_load': >> prog.c:(.text+0x23d): undefined reference to `reallocarray' >> collect2: error: ld returned 1 exit status >> Makefile:89: recipe for target 'bpftool' failed >> make[1]: *** [bpftool] Error 1 >> Makefile:99: recipe for target 'bpftool' failed >> make: *** [bpftool] Error 2 > > Oh no.. Sorry & thanks for catching this. It would be nice to not > depend on Glibc version defines, in case someone is using a different > library. Jiong suggested we can just use the feature detection, so I > have something like this: Yeah, that would be okay to do if you want to go that route, sure. Other option I had in mind would have been to import include/linux/overflow.h into the tools/include/linux/ and have a minor wrapper similar to kmalloc_array() in a utils.h in bpftool to get to the same for all users. But I think feature test is totally fine too, and in general some form of reallocarray() would be good to have rather than plain realloc(). So, below complies for me. Although don't we need to define a CFLAG based on the outcome of the test similar as in feature-disassembler-four-args? Otherwise with the below diff the test doesn't really do much, no? Meaning, adding a ... ifeq ($(feature-reallocarray), 1) CFLAGS += -DHAVE_REALLOCARRAY endif ... to the Makefile and in compat.h having it enabled through: #ifndef HAVE_REALLOCARRAY static inline void *reallocarray(void *ptr, size_t nmemb, size_t size) { return realloc(ptr, nmemb * size); } #endif In any case, we could use reallocarray() also in couple of other places in bpftool and libbpf. > diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile > index 0911b00b25cc..20a691659381 100644 > --- a/tools/bpf/bpftool/Makefile > +++ b/tools/bpf/bpftool/Makefile > @@ -52,8 +52,8 @@ INSTALL ?= install > RM ?= rm -f > > FEATURE_USER = .bpftool > -FEATURE_TESTS = libbfd disassembler-four-args > -FEATURE_DISPLAY = libbfd disassembler-four-args > +FEATURE_TESTS = libbfd disassembler-four-args reallocarray > +FEATURE_DISPLAY = libbfd disassembler-four-args reallocarray > > check_feat := 1 > NON_CHECK_FEAT_TARGETS :=
Re: [PATCH bpf-next 11/11] tools: bpftool: allow reuse of maps with bpftool prog load
On Thu, 5 Jul 2018 10:35:24 +0200, Daniel Borkmann wrote: > On 07/04/2018 04:54 AM, Jakub Kicinski wrote: > > Add map parameter to prog load which will allow reuse of existing > > maps instead of creating new ones. > > > > Signed-off-by: Jakub Kicinski > > Reviewed-by: Quentin Monnet > [...] > > + > > + fd = map_parse_fd(, ); > > + if (fd < 0) > > + goto err_free_reuse_maps; > > + > > + map_replace = reallocarray(map_replace, old_map_fds + 1, > > + sizeof(*map_replace)); > > + if (!map_replace) { > > + p_err("mem alloc failed"); > > + goto err_free_reuse_maps; > > Series in general looks good to me. However, above reallocarray() doesn't > exist and hence build fails, please see below. Is that from newest glibc? > > You probably need some fallback implementation or in general have something > bpftool internal that doesn't make a bet on its availability. > > # make > > Auto-detecting system features: > ...libbfd: [ on ] > ...disassembler-four-args: [ OFF ] > > CC bpf_jit_disasm.o > LINK bpf_jit_disasm > CC bpf_dbg.o > LINK bpf_dbg > CC bpf_asm.o > BISONbpf_exp.yacc.c > CC bpf_exp.yacc.o > FLEX bpf_exp.lex.c > CC bpf_exp.lex.o > LINK bpf_asm > DESCEND bpftool > > Auto-detecting system features: > ...libbfd: [ on ] > ...disassembler-four-args: [ OFF ] > > CC map_perf_ring.o > CC xlated_dumper.o > CC perf.o > CC prog.o > prog.c: In function ‘do_load’: > prog.c:785:18: warning: implicit declaration of function ‘reallocarray’ > [-Wimplicit-function-declaration] > map_replace = reallocarray(map_replace, old_map_fds + 1, > ^~~~ > prog.c:785:16: warning: assignment makes pointer from integer without a cast > [-Wint-conversion] > map_replace = reallocarray(map_replace, old_map_fds + 1, > ^ > CC common.o > CC cgroup.o > CC main.o > CC json_writer.o > CC cfg.o > CC map.o > CC jit_disasm.o > CC disasm.o > > Auto-detecting system features: > ...libelf: [ on ] > ... bpf: [ on ] > > Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs from > latest version at 'include/uapi/linux/bpf.h' > CC libbpf.o > CC bpf.o > CC nlattr.o > CC btf.o > LD libbpf-in.o > LINK libbpf.a > LINK bpftool > prog.o: In function `do_load': > prog.c:(.text+0x23d): undefined reference to `reallocarray' > collect2: error: ld returned 1 exit status > Makefile:89: recipe for target 'bpftool' failed > make[1]: *** [bpftool] Error 1 > Makefile:99: recipe for target 'bpftool' failed > make: *** [bpftool] Error 2 Oh no.. Sorry & thanks for catching this. It would be nice to not depend on Glibc version defines, in case someone is using a different library. Jiong suggested we can just use the feature detection, so I have something like this: --- diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile index 0911b00b25cc..20a691659381 100644 --- a/tools/bpf/bpftool/Makefile +++ b/tools/bpf/bpftool/Makefile @@ -52,8 +52,8 @@ INSTALL ?= install RM ?= rm -f FEATURE_USER = .bpftool -FEATURE_TESTS = libbfd disassembler-four-args -FEATURE_DISPLAY = libbfd disassembler-four-args +FEATURE_TESTS = libbfd disassembler-four-args reallocarray +FEATURE_DISPLAY = libbfd disassembler-four-args reallocarray check_feat := 1 NON_CHECK_FEAT_TARGETS := clean uninstall doc doc-clean doc-install doc-uninstall diff --git a/tools/bpf/bpftool/compat.h b/tools/bpf/bpftool/compat.h new file mode 100644 index ..7885cedc9efe --- /dev/null +++ b/tools/bpf/bpftool/compat.h @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* Copyright (C) 2018 Netronome Systems, Inc. */ + +#ifndef __BPF_TOOL_COMPAT_H +#define __BPF_TOOL_COMPAT_H + +#define _GNU_SOURCE +#include + +static inline void *reallocarray(void *ptr, size_t nmemb, size_t size) +{ + return realloc(ptr, nmemb * size); +} +#endif diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h index 1a9a2aefa014..2106adb73631 100644 --- a/tools/bpf/bpftool/main.h +++ b/tools/bpf/bpftool/main.h @@ -43,6 +43,7 @@ #include #include +#include "compat.h" #include "json_writer.h" #define ptr_to_u64(ptr)((__u64)(unsigned long)(ptr)) diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile index dac9563b5470..0516259be70f 100644 --- a/tools/build/feature/Makefile +++ b/tools/build/feature/Makefile @@ -14,6 +14,7 @@ FILES= \ test-libaudit.bin \ test-libbfd.bin
Re: [PATCH bpf-next 11/11] tools: bpftool: allow reuse of maps with bpftool prog load
On 07/04/2018 04:54 AM, Jakub Kicinski wrote: > Add map parameter to prog load which will allow reuse of existing > maps instead of creating new ones. > > Signed-off-by: Jakub Kicinski > Reviewed-by: Quentin Monnet [...] > + > + fd = map_parse_fd(, ); > + if (fd < 0) > + goto err_free_reuse_maps; > + > + map_replace = reallocarray(map_replace, old_map_fds + 1, > +sizeof(*map_replace)); > + if (!map_replace) { > + p_err("mem alloc failed"); > + goto err_free_reuse_maps; Series in general looks good to me. However, above reallocarray() doesn't exist and hence build fails, please see below. Is that from newest glibc? You probably need some fallback implementation or in general have something bpftool internal that doesn't make a bet on its availability. # make Auto-detecting system features: ...libbfd: [ on ] ...disassembler-four-args: [ OFF ] CC bpf_jit_disasm.o LINK bpf_jit_disasm CC bpf_dbg.o LINK bpf_dbg CC bpf_asm.o BISONbpf_exp.yacc.c CC bpf_exp.yacc.o FLEX bpf_exp.lex.c CC bpf_exp.lex.o LINK bpf_asm DESCEND bpftool Auto-detecting system features: ...libbfd: [ on ] ...disassembler-four-args: [ OFF ] CC map_perf_ring.o CC xlated_dumper.o CC perf.o CC prog.o prog.c: In function ‘do_load’: prog.c:785:18: warning: implicit declaration of function ‘reallocarray’ [-Wimplicit-function-declaration] map_replace = reallocarray(map_replace, old_map_fds + 1, ^~~~ prog.c:785:16: warning: assignment makes pointer from integer without a cast [-Wint-conversion] map_replace = reallocarray(map_replace, old_map_fds + 1, ^ CC common.o CC cgroup.o CC main.o CC json_writer.o CC cfg.o CC map.o CC jit_disasm.o CC disasm.o Auto-detecting system features: ...libelf: [ on ] ... bpf: [ on ] Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs from latest version at 'include/uapi/linux/bpf.h' CC libbpf.o CC bpf.o CC nlattr.o CC btf.o LD libbpf-in.o LINK libbpf.a LINK bpftool prog.o: In function `do_load': prog.c:(.text+0x23d): undefined reference to `reallocarray' collect2: error: ld returned 1 exit status Makefile:89: recipe for target 'bpftool' failed make[1]: *** [bpftool] Error 1 Makefile:99: recipe for target 'bpftool' failed make: *** [bpftool] Error 2 Thanks, Daniel
[PATCH bpf-next 11/11] tools: bpftool: allow reuse of maps with bpftool prog load
Add map parameter to prog load which will allow reuse of existing maps instead of creating new ones. Signed-off-by: Jakub Kicinski Reviewed-by: Quentin Monnet --- .../bpftool/Documentation/bpftool-prog.rst| 20 ++- tools/bpf/bpftool/bash-completion/bpftool | 67 +++- tools/bpf/bpftool/main.h | 3 + tools/bpf/bpftool/map.c | 4 +- tools/bpf/bpftool/prog.c | 148 -- 5 files changed, 219 insertions(+), 23 deletions(-) diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst index e53e1ad2caf0..64156a16d530 100644 --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst @@ -24,9 +24,10 @@ MAP COMMANDS | **bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes** | **visual**}] | **bpftool** **prog dump jited** *PROG* [{**file** *FILE* | **opcodes**}] | **bpftool** **prog pin** *PROG* *FILE* -| **bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**dev** *NAME*] +| **bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] | **bpftool** **prog help** | +| *MAP* := { **id** *MAP_ID* | **pinned** *FILE* } | *PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* } | *TYPE* := { | **socket** | **kprobe** | **kretprobe** | **classifier** | **action** | @@ -73,10 +74,17 @@ DESCRIPTION Note: *FILE* must be located in *bpffs* mount. - **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**dev** *NAME*] + **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] Load bpf program from binary *OBJ* and pin as *FILE*. **type** is optional, if not specified program type will be inferred from section names. + By default bpftool will create new maps as declared in the ELF + object being loaded. **map** parameter allows for the reuse + of existing maps. It can be specified multiple times, each + time for a different map. *IDX* refers to index of the map + to be replaced in the ELF file counting from 0, while *NAME* + allows to replace a map by name. *MAP* specifies the map to + use, referring to it by **id** or through a **pinned** file. If **dev** *NAME* is specified program will be loaded onto given networking device (offload). @@ -172,6 +180,14 @@ EXAMPLES mov%rbx,0x0(%rbp) 48 89 5d 00 +| +| **# bpftool prog load xdp1_kern.o /sys/fs/bpf/xdp1 type xdp map name rxcnt id 7** +| **# bpftool prog show pinned /sys/fs/bpf/xdp1** +| 9: xdp name xdp_prog1 tag 539ec6ce11b52f98 gpl +| loaded_at 2018-06-25T16:17:31-0700 uid 0 +| xlated 488B jited 336B memlock 4096B map_ids 7 +| **# rm /sys/fs/bpf/xdp1** +| SEE ALSO diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool index 9ae33a73d732..626598964cee 100644 --- a/tools/bpf/bpftool/bash-completion/bpftool +++ b/tools/bpf/bpftool/bash-completion/bpftool @@ -99,6 +99,29 @@ _bpftool_get_prog_tags() command sed -n 's/.*"tag": "\(.*\)",$/\1/p' )" -- "$cur" ) ) } +_bpftool_get_obj_map_names() +{ +local obj + +obj=$1 + +maps=$(objdump -j maps -t $obj 2>/dev/null | \ +command awk '/g . maps/ {print $NF}') + +COMPREPLY+=( $( compgen -W "$maps" -- "$cur" ) ) +} + +_bpftool_get_obj_map_idxs() +{ +local obj + +obj=$1 + +nmaps=$(objdump -j maps -t $obj 2>/dev/null | grep -c 'g . maps') + +COMPREPLY+=( $( compgen -W "$(seq 0 $((nmaps - 1)))" -- "$cur" ) ) +} + _sysfs_get_netdevs() { COMPREPLY+=( $( compgen -W "$( ls /sys/class/net 2>/dev/null )" -- \ @@ -220,12 +243,14 @@ _bpftool() # Completion depends on object and command in use case $object in prog) -case $prev in -id) -_bpftool_get_prog_ids -return 0 -;; -esac +if [[ $command != "load" ]]; then +case $prev in +id) +_bpftool_get_prog_ids +return 0 +;; +esac +fi local PROG_TYPE='id pinned tag' case $command in @@ -268,22 +293,52 @@ _bpftool() return 0 ;; load) +local obj + if [[ ${#words[@]} -lt 6 ]]; then _filedir return 0 fi +