| Message ID | 20200515065624.21658-5-irogers@google.com |
|---|---|
| State | Superseded |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Copy hashmap to libapi, use in perf expr | expand |
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 > ... > 000000000002088a t hashmap_add_entry > 000000000001712a t hashmap__append > 0000000000020aa3 T hashmap__capacity > 000000000002099c T hashmap__clear > 00000000000208b3 t hashmap_del_entry > 0000000000020fc1 T hashmap__delete > 0000000000020f29 T hashmap__find > 0000000000020c6c t hashmap_find_entry > 0000000000020a61 T hashmap__free > 0000000000020b08 t hashmap_grow > 00000000000208dd T hashmap__init > 0000000000020d35 T hashmap__insert > 0000000000020ab5 t hashmap_needs_to_grow > 0000000000020947 T hashmap__new > 0000000000000775 t hashmap__set > 00000000000212f8 t hashmap__set > 0000000000020a91 T hashmap__size > ... > > After: > $ nm libbpf.a > ... > 000000000002088a t hashmap_add_entry > 000000000001712a t hashmap__append > 0000000000020aa3 t hashmap__capacity > 000000000002099c t hashmap__clear > 00000000000208b3 t hashmap_del_entry > 0000000000020fc1 t hashmap__delete > 0000000000020f29 t hashmap__find > 0000000000020c6c t hashmap_find_entry > 0000000000020a61 t hashmap__free > 0000000000020b08 t hashmap_grow > 00000000000208dd t hashmap__init > 0000000000020d35 t hashmap__insert > 0000000000020ab5 t hashmap_needs_to_grow > 0000000000020947 t hashmap__new > 0000000000000775 t hashmap__set > 00000000000212f8 t hashmap__set > 0000000000020a91 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
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 > > ... > > 000000000002088a t hashmap_add_entry > > 000000000001712a t hashmap__append > > 0000000000020aa3 T hashmap__capacity > > 000000000002099c T hashmap__clear > > 00000000000208b3 t hashmap_del_entry > > 0000000000020fc1 T hashmap__delete > > 0000000000020f29 T hashmap__find > > 0000000000020c6c t hashmap_find_entry > > 0000000000020a61 T hashmap__free > > 0000000000020b08 t hashmap_grow > > 00000000000208dd T hashmap__init > > 0000000000020d35 T hashmap__insert > > 0000000000020ab5 t hashmap_needs_to_grow > > 0000000000020947 T hashmap__new > > 0000000000000775 t hashmap__set > > 00000000000212f8 t hashmap__set > > 0000000000020a91 T hashmap__size > > ... > > > > After: > > $ nm libbpf.a > > ... > > 000000000002088a t hashmap_add_entry > > 000000000001712a t hashmap__append > > 0000000000020aa3 t hashmap__capacity > > 000000000002099c t hashmap__clear > > 00000000000208b3 t hashmap_del_entry > > 0000000000020fc1 t hashmap__delete > > 0000000000020f29 t hashmap__find > > 0000000000020c6c t hashmap_find_entry > > 0000000000020a61 t hashmap__free > > 0000000000020b08 t hashmap_grow > > 00000000000208dd t hashmap__init > > 0000000000020d35 t hashmap__insert > > 0000000000020ab5 t hashmap_needs_to_grow > > 0000000000020947 t hashmap__new > > 0000000000000775 t hashmap__set > > 00000000000212f8 t hashmap__set > > 0000000000020a91 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
On Fri, May 15, 2020 at 7:29 AM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> 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 > > > ... > > > 000000000002088a t hashmap_add_entry > > > 000000000001712a t hashmap__append > > > 0000000000020aa3 T hashmap__capacity > > > 000000000002099c T hashmap__clear > > > 00000000000208b3 t hashmap_del_entry > > > 0000000000020fc1 T hashmap__delete > > > 0000000000020f29 T hashmap__find > > > 0000000000020c6c t hashmap_find_entry > > > 0000000000020a61 T hashmap__free > > > 0000000000020b08 t hashmap_grow > > > 00000000000208dd T hashmap__init > > > 0000000000020d35 T hashmap__insert > > > 0000000000020ab5 t hashmap_needs_to_grow > > > 0000000000020947 T hashmap__new > > > 0000000000000775 t hashmap__set > > > 00000000000212f8 t hashmap__set > > > 0000000000020a91 T hashmap__size > > > ... > > > > > > After: > > > $ nm libbpf.a > > > ... > > > 000000000002088a t hashmap_add_entry > > > 000000000001712a t hashmap__append > > > 0000000000020aa3 t hashmap__capacity > > > 000000000002099c t hashmap__clear > > > 00000000000208b3 t hashmap_del_entry > > > 0000000000020fc1 t hashmap__delete > > > 0000000000020f29 t hashmap__find > > > 0000000000020c6c t hashmap_find_entry > > > 0000000000020a61 t hashmap__free > > > 0000000000020b08 t hashmap_grow > > > 00000000000208dd t hashmap__init > > > 0000000000020d35 t hashmap__insert > > > 0000000000020ab5 t hashmap_needs_to_grow > > > 0000000000020947 t hashmap__new > > > 0000000000000775 t hashmap__set > > > 00000000000212f8 t hashmap__set > > > 0000000000020a91 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
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 > <arnaldo.melo@gmail.com> 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 > > > > ... > > > > 000000000002088a t hashmap_add_entry > > > > 000000000001712a t hashmap__append > > > > 0000000000020aa3 T hashmap__capacity > > > > 000000000002099c T hashmap__clear > > > > 00000000000208b3 t hashmap_del_entry > > > > 0000000000020fc1 T hashmap__delete > > > > 0000000000020f29 T hashmap__find > > > > 0000000000020c6c t hashmap_find_entry > > > > 0000000000020a61 T hashmap__free > > > > 0000000000020b08 t hashmap_grow > > > > 00000000000208dd T hashmap__init > > > > 0000000000020d35 T hashmap__insert > > > > 0000000000020ab5 t hashmap_needs_to_grow > > > > 0000000000020947 T hashmap__new > > > > 0000000000000775 t hashmap__set > > > > 00000000000212f8 t hashmap__set > > > > 0000000000020a91 T hashmap__size > > > > ... > > > > > > > > After: > > > > $ nm libbpf.a > > > > ... > > > > 000000000002088a t hashmap_add_entry > > > > 000000000001712a t hashmap__append > > > > 0000000000020aa3 t hashmap__capacity > > > > 000000000002099c t hashmap__clear > > > > 00000000000208b3 t hashmap_del_entry > > > > 0000000000020fc1 t hashmap__delete > > > > 0000000000020f29 t hashmap__find > > > > 0000000000020c6c t hashmap_find_entry > > > > 0000000000020a61 t hashmap__free > > > > 0000000000020b08 t hashmap_grow > > > > 00000000000208dd t hashmap__init > > > > 0000000000020d35 t hashmap__insert > > > > 0000000000020ab5 t hashmap_needs_to_grow > > > > 0000000000020947 t hashmap__new > > > > 0000000000000775 t hashmap__set > > > > 00000000000212f8 t hashmap__set > > > > 0000000000020a91 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
On Fri, May 15, 2020 at 9:31 AM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> 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 > > <arnaldo.melo@gmail.com> 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 > > > > > ... > > > > > 000000000002088a t hashmap_add_entry > > > > > 000000000001712a t hashmap__append > > > > > 0000000000020aa3 T hashmap__capacity > > > > > 000000000002099c T hashmap__clear > > > > > 00000000000208b3 t hashmap_del_entry > > > > > 0000000000020fc1 T hashmap__delete > > > > > 0000000000020f29 T hashmap__find > > > > > 0000000000020c6c t hashmap_find_entry > > > > > 0000000000020a61 T hashmap__free > > > > > 0000000000020b08 t hashmap_grow > > > > > 00000000000208dd T hashmap__init > > > > > 0000000000020d35 T hashmap__insert > > > > > 0000000000020ab5 t hashmap_needs_to_grow > > > > > 0000000000020947 T hashmap__new > > > > > 0000000000000775 t hashmap__set > > > > > 00000000000212f8 t hashmap__set > > > > > 0000000000020a91 T hashmap__size > > > > > ... > > > > > > > > > > After: > > > > > $ nm libbpf.a > > > > > ... > > > > > 000000000002088a t hashmap_add_entry > > > > > 000000000001712a t hashmap__append > > > > > 0000000000020aa3 t hashmap__capacity > > > > > 000000000002099c t hashmap__clear > > > > > 00000000000208b3 t hashmap_del_entry > > > > > 0000000000020fc1 t hashmap__delete > > > > > 0000000000020f29 t hashmap__find > > > > > 0000000000020c6c t hashmap_find_entry > > > > > 0000000000020a61 t hashmap__free > > > > > 0000000000020b08 t hashmap_grow > > > > > 00000000000208dd t hashmap__init > > > > > 0000000000020d35 t hashmap__insert > > > > > 0000000000020ab5 t hashmap_needs_to_grow > > > > > 0000000000020947 t hashmap__new > > > > > 0000000000000775 t hashmap__set > > > > > 00000000000212f8 t hashmap__set > > > > > 0000000000020a91 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-irogers@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
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 \
Localize the hashmap__* symbols in libbpf.a. To allow for a version in libapi. Before: $ nm libbpf.a ... 000000000002088a t hashmap_add_entry 000000000001712a t hashmap__append 0000000000020aa3 T hashmap__capacity 000000000002099c T hashmap__clear 00000000000208b3 t hashmap_del_entry 0000000000020fc1 T hashmap__delete 0000000000020f29 T hashmap__find 0000000000020c6c t hashmap_find_entry 0000000000020a61 T hashmap__free 0000000000020b08 t hashmap_grow 00000000000208dd T hashmap__init 0000000000020d35 T hashmap__insert 0000000000020ab5 t hashmap_needs_to_grow 0000000000020947 T hashmap__new 0000000000000775 t hashmap__set 00000000000212f8 t hashmap__set 0000000000020a91 T hashmap__size ... After: $ nm libbpf.a ... 000000000002088a t hashmap_add_entry 000000000001712a t hashmap__append 0000000000020aa3 t hashmap__capacity 000000000002099c t hashmap__clear 00000000000208b3 t hashmap_del_entry 0000000000020fc1 t hashmap__delete 0000000000020f29 t hashmap__find 0000000000020c6c t hashmap_find_entry 0000000000020a61 t hashmap__free 0000000000020b08 t hashmap_grow 00000000000208dd t hashmap__init 0000000000020d35 t hashmap__insert 0000000000020ab5 t hashmap_needs_to_grow 0000000000020947 t hashmap__new 0000000000000775 t hashmap__set 00000000000212f8 t hashmap__set 0000000000020a91 t hashmap__size ... Signed-off-by: Ian Rogers <irogers@google.com> --- tools/lib/bpf/Makefile | 2 ++ 1 file changed, 2 insertions(+)