Re: [PATCH 4/8] libbpf hashmap: Localize static hashmap__* symbols
On Fri, May 15, 2020 at 9:31 AM Arnaldo Carvalho de Melo wrote: > > Em Fri, May 15, 2020 at 07:53:33AM -0700, Ian Rogers escreveu: > > On Fri, May 15, 2020 at 7:29 AM Arnaldo Carvalho de Melo > > wrote: > > > > > > Em Fri, May 15, 2020 at 11:17:07AM +0200, Jiri Olsa escreveu: > > > > On Thu, May 14, 2020 at 11:56:20PM -0700, Ian Rogers wrote: > > > > > Localize the hashmap__* symbols in libbpf.a. To allow for a version in > > > > > libapi. > > > > > > > > > > Before: > > > > > $ nm libbpf.a > > > > > ... > > > > > 0002088a t hashmap_add_entry > > > > > 0001712a t hashmap__append > > > > > 00020aa3 T hashmap__capacity > > > > > 0002099c T hashmap__clear > > > > > 000208b3 t hashmap_del_entry > > > > > 00020fc1 T hashmap__delete > > > > > 00020f29 T hashmap__find > > > > > 00020c6c t hashmap_find_entry > > > > > 00020a61 T hashmap__free > > > > > 00020b08 t hashmap_grow > > > > > 000208dd T hashmap__init > > > > > 00020d35 T hashmap__insert > > > > > 00020ab5 t hashmap_needs_to_grow > > > > > 00020947 T hashmap__new > > > > > 0775 t hashmap__set > > > > > 000212f8 t hashmap__set > > > > > 00020a91 T hashmap__size > > > > > ... > > > > > > > > > > After: > > > > > $ nm libbpf.a > > > > > ... > > > > > 0002088a t hashmap_add_entry > > > > > 0001712a t hashmap__append > > > > > 00020aa3 t hashmap__capacity > > > > > 0002099c t hashmap__clear > > > > > 000208b3 t hashmap_del_entry > > > > > 00020fc1 t hashmap__delete > > > > > 00020f29 t hashmap__find > > > > > 00020c6c t hashmap_find_entry > > > > > 00020a61 t hashmap__free > > > > > 00020b08 t hashmap_grow > > > > > 000208dd t hashmap__init > > > > > 00020d35 t hashmap__insert > > > > > 00020ab5 t hashmap_needs_to_grow > > > > > 00020947 t hashmap__new > > > > > 0775 t hashmap__set > > > > > 000212f8 t hashmap__set > > > > > 00020a91 t hashmap__size > > > > > ... > > > > > > > > I think this will break bpf selftests which use hashmap, > > > > we need to find some other way to include this > > > > > > > > either to use it from libbpf directly, or use the api version > > > > only if the libbpf is not compiled in perf, we could use > > > > following to detect that: > > > > > > > > CFLAGS += -DHAVE_LIBBPF_SUPPORT > > > > $(call detected,CONFIG_LIBBPF) > > > > > > And have it in tools/perf/util/ instead? > > > *sigh* > > > $ make -C tools/testing/selftests/bpf test_hashmap > > make: Entering directory > > '/usr/local/google/home/irogers/kernel-trees/kernel.org/tip/tools/testing/s > > elftests/bpf' > > BINARY test_hashmap > > /usr/bin/ld: /tmp/ccEGGNw5.o: in function `test_hashmap_generic': > > /usr/local/google/home/irogers/kernel-trees/kernel.org/tip/tools/testing/selftests/bpf/test_hashmap. > > c:61: undefined reference to `hashmap__new' > > ... > > > My preference was to make hashmap a sharable API in tools, to benefit > > That is my preference as well, I'm not defending having it in > tools/perf/util/, just saying that that is a possible way to make > progress with the current situation... Thanks, it'd be nice to be expedient as both Jiri and myself are changing code in this area. v2 is up for review here: https://lore.kernel.org/lkml/20200515165007.217120-8-irog...@google.com/ An ifdef when the hashmap.h is used, and one in the build. It could be worse. Thanks, Ian > > not just perf but say things like libsymbol, libperf, etc. Moving it > > into perf and using conditional compilation is kinda gross but having > > libbpf tests depend on libapi also isn't ideal I guess. It is tempting > > to just cut a hashmap from fresh cloth to avoid this and to share > > among tools/. I don't know if the bpf folks have opinions? > > > > I'll do a v2 using conditional compilation to see how bad it looks. > > Cool, lets see how it looks. > > - Arnaldo
Re: [PATCH 4/8] libbpf hashmap: Localize static hashmap__* symbols
Em Fri, May 15, 2020 at 07:53:33AM -0700, Ian Rogers escreveu: > On Fri, May 15, 2020 at 7:29 AM Arnaldo Carvalho de Melo > wrote: > > > > Em Fri, May 15, 2020 at 11:17:07AM +0200, Jiri Olsa escreveu: > > > On Thu, May 14, 2020 at 11:56:20PM -0700, Ian Rogers wrote: > > > > Localize the hashmap__* symbols in libbpf.a. To allow for a version in > > > > libapi. > > > > > > > > Before: > > > > $ nm libbpf.a > > > > ... > > > > 0002088a t hashmap_add_entry > > > > 0001712a t hashmap__append > > > > 00020aa3 T hashmap__capacity > > > > 0002099c T hashmap__clear > > > > 000208b3 t hashmap_del_entry > > > > 00020fc1 T hashmap__delete > > > > 00020f29 T hashmap__find > > > > 00020c6c t hashmap_find_entry > > > > 00020a61 T hashmap__free > > > > 00020b08 t hashmap_grow > > > > 000208dd T hashmap__init > > > > 00020d35 T hashmap__insert > > > > 00020ab5 t hashmap_needs_to_grow > > > > 00020947 T hashmap__new > > > > 0775 t hashmap__set > > > > 000212f8 t hashmap__set > > > > 00020a91 T hashmap__size > > > > ... > > > > > > > > After: > > > > $ nm libbpf.a > > > > ... > > > > 0002088a t hashmap_add_entry > > > > 0001712a t hashmap__append > > > > 00020aa3 t hashmap__capacity > > > > 0002099c t hashmap__clear > > > > 000208b3 t hashmap_del_entry > > > > 00020fc1 t hashmap__delete > > > > 00020f29 t hashmap__find > > > > 00020c6c t hashmap_find_entry > > > > 00020a61 t hashmap__free > > > > 00020b08 t hashmap_grow > > > > 000208dd t hashmap__init > > > > 00020d35 t hashmap__insert > > > > 00020ab5 t hashmap_needs_to_grow > > > > 00020947 t hashmap__new > > > > 0775 t hashmap__set > > > > 000212f8 t hashmap__set > > > > 00020a91 t hashmap__size > > > > ... > > > > > > I think this will break bpf selftests which use hashmap, > > > we need to find some other way to include this > > > > > > either to use it from libbpf directly, or use the api version > > > only if the libbpf is not compiled in perf, we could use > > > following to detect that: > > > > > > CFLAGS += -DHAVE_LIBBPF_SUPPORT > > > $(call detected,CONFIG_LIBBPF) > > > > And have it in tools/perf/util/ instead? > *sigh* > $ make -C tools/testing/selftests/bpf test_hashmap > make: Entering directory > '/usr/local/google/home/irogers/kernel-trees/kernel.org/tip/tools/testing/s > elftests/bpf' > BINARY test_hashmap > /usr/bin/ld: /tmp/ccEGGNw5.o: in function `test_hashmap_generic': > /usr/local/google/home/irogers/kernel-trees/kernel.org/tip/tools/testing/selftests/bpf/test_hashmap. > c:61: undefined reference to `hashmap__new' > ... > My preference was to make hashmap a sharable API in tools, to benefit That is my preference as well, I'm not defending having it in tools/perf/util/, just saying that that is a possible way to make progress with the current situation... > not just perf but say things like libsymbol, libperf, etc. Moving it > into perf and using conditional compilation is kinda gross but having > libbpf tests depend on libapi also isn't ideal I guess. It is tempting > to just cut a hashmap from fresh cloth to avoid this and to share > among tools/. I don't know if the bpf folks have opinions? > > I'll do a v2 using conditional compilation to see how bad it looks. Cool, lets see how it looks. - Arnaldo
Re: [PATCH 4/8] libbpf hashmap: Localize static hashmap__* symbols
On Fri, May 15, 2020 at 7:29 AM Arnaldo Carvalho de Melo wrote: > > Em Fri, May 15, 2020 at 11:17:07AM +0200, Jiri Olsa escreveu: > > On Thu, May 14, 2020 at 11:56:20PM -0700, Ian Rogers wrote: > > > Localize the hashmap__* symbols in libbpf.a. To allow for a version in > > > libapi. > > > > > > Before: > > > $ nm libbpf.a > > > ... > > > 0002088a t hashmap_add_entry > > > 0001712a t hashmap__append > > > 00020aa3 T hashmap__capacity > > > 0002099c T hashmap__clear > > > 000208b3 t hashmap_del_entry > > > 00020fc1 T hashmap__delete > > > 00020f29 T hashmap__find > > > 00020c6c t hashmap_find_entry > > > 00020a61 T hashmap__free > > > 00020b08 t hashmap_grow > > > 000208dd T hashmap__init > > > 00020d35 T hashmap__insert > > > 00020ab5 t hashmap_needs_to_grow > > > 00020947 T hashmap__new > > > 0775 t hashmap__set > > > 000212f8 t hashmap__set > > > 00020a91 T hashmap__size > > > ... > > > > > > After: > > > $ nm libbpf.a > > > ... > > > 0002088a t hashmap_add_entry > > > 0001712a t hashmap__append > > > 00020aa3 t hashmap__capacity > > > 0002099c t hashmap__clear > > > 000208b3 t hashmap_del_entry > > > 00020fc1 t hashmap__delete > > > 00020f29 t hashmap__find > > > 00020c6c t hashmap_find_entry > > > 00020a61 t hashmap__free > > > 00020b08 t hashmap_grow > > > 000208dd t hashmap__init > > > 00020d35 t hashmap__insert > > > 00020ab5 t hashmap_needs_to_grow > > > 00020947 t hashmap__new > > > 0775 t hashmap__set > > > 000212f8 t hashmap__set > > > 00020a91 t hashmap__size > > > ... > > > > I think this will break bpf selftests which use hashmap, > > we need to find some other way to include this > > > > either to use it from libbpf directly, or use the api version > > only if the libbpf is not compiled in perf, we could use > > following to detect that: > > > > CFLAGS += -DHAVE_LIBBPF_SUPPORT > > $(call detected,CONFIG_LIBBPF) > > And have it in tools/perf/util/ instead? > > > - Arnaldo *sigh* $ make -C tools/testing/selftests/bpf test_hashmap make: Entering directory '/usr/local/google/home/irogers/kernel-trees/kernel.org/tip/tools/testing/s elftests/bpf' BINARY test_hashmap /usr/bin/ld: /tmp/ccEGGNw5.o: in function `test_hashmap_generic': /usr/local/google/home/irogers/kernel-trees/kernel.org/tip/tools/testing/selftests/bpf/test_hashmap. c:61: undefined reference to `hashmap__new' ... My preference was to make hashmap a sharable API in tools, to benefit not just perf but say things like libsymbol, libperf, etc. Moving it into perf and using conditional compilation is kinda gross but having libbpf tests depend on libapi also isn't ideal I guess. It is tempting to just cut a hashmap from fresh cloth to avoid this and to share among tools/. I don't know if the bpf folks have opinions? I'll do a v2 using conditional compilation to see how bad it looks. Thanks, Ian
Re: [PATCH 4/8] libbpf hashmap: Localize static hashmap__* symbols
Em Fri, May 15, 2020 at 11:17:07AM +0200, Jiri Olsa escreveu: > On Thu, May 14, 2020 at 11:56:20PM -0700, Ian Rogers wrote: > > Localize the hashmap__* symbols in libbpf.a. To allow for a version in > > libapi. > > > > Before: > > $ nm libbpf.a > > ... > > 0002088a t hashmap_add_entry > > 0001712a t hashmap__append > > 00020aa3 T hashmap__capacity > > 0002099c T hashmap__clear > > 000208b3 t hashmap_del_entry > > 00020fc1 T hashmap__delete > > 00020f29 T hashmap__find > > 00020c6c t hashmap_find_entry > > 00020a61 T hashmap__free > > 00020b08 t hashmap_grow > > 000208dd T hashmap__init > > 00020d35 T hashmap__insert > > 00020ab5 t hashmap_needs_to_grow > > 00020947 T hashmap__new > > 0775 t hashmap__set > > 000212f8 t hashmap__set > > 00020a91 T hashmap__size > > ... > > > > After: > > $ nm libbpf.a > > ... > > 0002088a t hashmap_add_entry > > 0001712a t hashmap__append > > 00020aa3 t hashmap__capacity > > 0002099c t hashmap__clear > > 000208b3 t hashmap_del_entry > > 00020fc1 t hashmap__delete > > 00020f29 t hashmap__find > > 00020c6c t hashmap_find_entry > > 00020a61 t hashmap__free > > 00020b08 t hashmap_grow > > 000208dd t hashmap__init > > 00020d35 t hashmap__insert > > 00020ab5 t hashmap_needs_to_grow > > 00020947 t hashmap__new > > 0775 t hashmap__set > > 000212f8 t hashmap__set > > 00020a91 t hashmap__size > > ... > > I think this will break bpf selftests which use hashmap, > we need to find some other way to include this > > either to use it from libbpf directly, or use the api version > only if the libbpf is not compiled in perf, we could use > following to detect that: > > CFLAGS += -DHAVE_LIBBPF_SUPPORT > $(call detected,CONFIG_LIBBPF) And have it in tools/perf/util/ instead? - Arnaldo
Re: [PATCH 4/8] libbpf hashmap: Localize static hashmap__* symbols
On Thu, May 14, 2020 at 11:56:20PM -0700, Ian Rogers wrote: > Localize the hashmap__* symbols in libbpf.a. To allow for a version in > libapi. > > Before: > $ nm libbpf.a > ... > 0002088a t hashmap_add_entry > 0001712a t hashmap__append > 00020aa3 T hashmap__capacity > 0002099c T hashmap__clear > 000208b3 t hashmap_del_entry > 00020fc1 T hashmap__delete > 00020f29 T hashmap__find > 00020c6c t hashmap_find_entry > 00020a61 T hashmap__free > 00020b08 t hashmap_grow > 000208dd T hashmap__init > 00020d35 T hashmap__insert > 00020ab5 t hashmap_needs_to_grow > 00020947 T hashmap__new > 0775 t hashmap__set > 000212f8 t hashmap__set > 00020a91 T hashmap__size > ... > > After: > $ nm libbpf.a > ... > 0002088a t hashmap_add_entry > 0001712a t hashmap__append > 00020aa3 t hashmap__capacity > 0002099c t hashmap__clear > 000208b3 t hashmap_del_entry > 00020fc1 t hashmap__delete > 00020f29 t hashmap__find > 00020c6c t hashmap_find_entry > 00020a61 t hashmap__free > 00020b08 t hashmap_grow > 000208dd t hashmap__init > 00020d35 t hashmap__insert > 00020ab5 t hashmap_needs_to_grow > 00020947 t hashmap__new > 0775 t hashmap__set > 000212f8 t hashmap__set > 00020a91 t hashmap__size > ... I think this will break bpf selftests which use hashmap, we need to find some other way to include this either to use it from libbpf directly, or use the api version only if the libbpf is not compiled in perf, we could use following to detect that: CFLAGS += -DHAVE_LIBBPF_SUPPORT $(call detected,CONFIG_LIBBPF) jirka
[PATCH 4/8] libbpf hashmap: Localize static hashmap__* symbols
Localize the hashmap__* symbols in libbpf.a. To allow for a version in libapi. Before: $ nm libbpf.a ... 0002088a t hashmap_add_entry 0001712a t hashmap__append 00020aa3 T hashmap__capacity 0002099c T hashmap__clear 000208b3 t hashmap_del_entry 00020fc1 T hashmap__delete 00020f29 T hashmap__find 00020c6c t hashmap_find_entry 00020a61 T hashmap__free 00020b08 t hashmap_grow 000208dd T hashmap__init 00020d35 T hashmap__insert 00020ab5 t hashmap_needs_to_grow 00020947 T hashmap__new 0775 t hashmap__set 000212f8 t hashmap__set 00020a91 T hashmap__size ... After: $ nm libbpf.a ... 0002088a t hashmap_add_entry 0001712a t hashmap__append 00020aa3 t hashmap__capacity 0002099c t hashmap__clear 000208b3 t hashmap_del_entry 00020fc1 t hashmap__delete 00020f29 t hashmap__find 00020c6c t hashmap_find_entry 00020a61 t hashmap__free 00020b08 t hashmap_grow 000208dd t hashmap__init 00020d35 t hashmap__insert 00020ab5 t hashmap_needs_to_grow 00020947 t hashmap__new 0775 t hashmap__set 000212f8 t hashmap__set 00020a91 t hashmap__size ... Signed-off-by: Ian Rogers --- tools/lib/bpf/Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile index aee7f1a83c77..4a1cdbceb04e 100644 --- a/tools/lib/bpf/Makefile +++ b/tools/lib/bpf/Makefile @@ -20,6 +20,7 @@ srctree := $(patsubst %/,%,$(dir $(srctree))) endif INSTALL = install +OBJCOPY ?= objcopy # Use DESTDIR for installing into a different root directory. # This is useful for building a package. The program will be @@ -181,6 +182,7 @@ $(BPF_IN_SHARED): force elfdep zdep bpfdep $(BPF_HELPER_DEFS) $(BPF_IN_STATIC): force elfdep zdep bpfdep $(BPF_HELPER_DEFS) $(Q)$(MAKE) $(build)=libbpf OUTPUT=$(STATIC_OBJDIR) + $(Q)$(OBJCOPY) -w -L hashmap__\* $@ $(BPF_HELPER_DEFS): $(srctree)/tools/include/uapi/linux/bpf.h $(QUIET_GEN)$(srctree)/scripts/bpf_helpers_doc.py --header \ -- 2.26.2.761.g0e0b3e54be-goog
