From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jakub Kicinski Subject: Re: [PATCH bpf-next 11/11] tools: bpftool: allow reuse of maps with bpftool prog load Date: Fri, 6 Jul 2018 08:30:18 -0700 Message-ID: <20180706083018.22dd69b2@cakuba.netronome.com> References: <20180704025417.8848-1-jakub.kicinski@netronome.com> <20180704025417.8848-12-jakub.kicinski@netronome.com> <3db866ff-5712-c308-9eef-18deaadf490d@iogearbox.net> <20180705155718.35b9bdd1@cakuba.netronome.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: alexei.starovoitov@gmail.com, oss-drivers@netronome.com, netdev@vger.kernel.org To: Daniel Borkmann Return-path: Received: from mail-pf0-f195.google.com ([209.85.192.195]:37039 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753632AbeGFPaX (ORCPT ); Fri, 6 Jul 2018 11:30:23 -0400 Received: by mail-pf0-f195.google.com with SMTP id h20-v6so8731945pfn.4 for ; Fri, 06 Jul 2018 08:30:23 -0700 (PDT) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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: =20 > >> On 07/04/2018 04:54 AM, Jakub Kicinski wrote: =20 > >>> 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 =20 > >> [...] =20 > >>> + > >>> + fd =3D map_parse_fd(&argc, &argv); > >>> + if (fd < 0) > >>> + goto err_free_reuse_maps; > >>> + > >>> + map_replace =3D reallocarray(map_replace, old_map_fds + 1, > >>> + sizeof(*map_replace)); > >>> + if (!map_replace) { > >>> + p_err("mem alloc failed"); > >>> + goto err_free_reuse_maps; =20 > >> > >> Series in general looks good to me. However, above reallocarray() does= n't > >> exist and hence build fails, please see below. Is that from newest gli= bc? > >> > >> You probably need some fallback implementation or in general have some= thing > >> 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 > >> BISON bpf_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 =E2=80=98do_load=E2=80=99: > >> prog.c:785:18: warning: implicit declaration of function =E2=80=98real= locarray=E2=80=99 [-Wimplicit-function-declaration] > >> map_replace =3D reallocarray(map_replace, old_map_fds + 1, > >> ^~~~~~~~~~~~ > >> prog.c:785:16: warning: assignment makes pointer from integer without = a cast [-Wint-conversion] > >> map_replace =3D 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 =20 > >=20 > > 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: =20 >=20 > 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(). >=20 > 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? Oth= erwise > with the below diff the test doesn't really do much, no? Meaning, adding = a ... >=20 > ifeq ($(feature-reallocarray), 1) > CFLAGS +=3D -DHAVE_REALLOCARRAY > endif >=20 > ... to the Makefile and in compat.h having it enabled through: >=20 > #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.