* tools build: Unused function, incomplete rename @ 2015-09-18 19:23 Arnaldo Carvalho de Melo 2015-09-18 19:38 ` Arnaldo Carvalho de Melo 2015-09-19 13:35 ` Jiri Olsa 0 siblings, 2 replies; 6+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-09-18 19:23 UTC (permalink / raw) To: Jiri Olsa Cc: Ingo Molnar, Namhyung Kim, Adrian Hunter, David Ahern, Linux Kernel Mailing List Hi Jiri, Ingo, While trying to figure out why the bpf feature test is always triggering the display of the "Auto-detecting system features" I noticed this pattern: [acme@felicio linux]$ egrep '^define|eval' tools/build/Makefile.feature feature_check = $(eval $(feature_check_code)) define feature_check_code feature_set = $(eval $(feature_set_code)) define feature_set_code set_test_all_flags = $(eval $(set_test_all_flags_code)) define set_test_all_flags_code feature_print_status = $(eval $(feature_print_status_code)) $(info $(MSG)) define feature_print_status_code feature_print_text = $(eval $(feature_print_text_code)) $(info $(MSG)) define feature_print_text_code feature_display_check = $(eval $(feature_check_code)) define feature_display_check_code [acme@felicio linux]$ In all but one case the eval matches the following define, except for the last one, don't we need the following patch? $ git diff tools/build/Makefile.feature diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature index 0caeaf2cae5f..072ec879b84f 100644 --- a/tools/build/Makefile.feature +++ b/tools/build/Makefile.feature @@ -142,7 +142,7 @@ ifneq ("$(FEATURE_DUMP)","$(FEATURE_DUMP_FILE)") feature_display := 1 endif -feature_display_check = $(eval $(feature_check_code)) +feature_display_check = $(eval $(feature_check_display_code)) define feature_display_check_code ifneq ($(feature-$(1)), 1) feature_display := 1 --------------------------------------------------------------------- I guess the bug was introduced here: commit 58d4f00ff13f20468f8fa8edcb57a195c31af46d Author: Jiri Olsa <jolsa@kernel.org> Date: Thu Mar 19 20:48:49 2015 +0100 perf build: Fix feature_check name clash We have 2 feature_check functions, which conflict with each other. Fixing it by renaming the latter to feature_display_check. Signed-off-by: Jiri Olsa <jolsa@kernel.org> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com> Cc: David Ahern <david.ahern@oracle.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Paul Mackerras <paulus@samba.org> Cc: Peter Zijlstra <peterz@infradead.org> Link: http://lkml.kernel.org/n/tip-wmyccro6qeffseforipu5kcl@git.kernel.org Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile index e7f83b15fcbf..81d8c2bbc4df 100644 --- a/tools/perf/config/Makefile +++ b/tools/perf/config/Makefile @@ -805,14 +805,14 @@ ifneq ("$(FEATURE_DUMP)","$(FEATURE_DUMP_FILE)") feature_display := 1 endif -feature_check = $(eval $(feature_check_code)) -define feature_check_code +feature_display_check = $(eval $(feature_check_code)) +define feature_display_check_code ifneq ($(feature-$(1)), 1) feature_display := 1 endif endef -$(foreach feat,$(FEATURE_DISPLAY),$(call feature_check,$(feat))) +$(foreach feat,$(FEATURE_DISPLAY),$(call feature_display_check,$(feat))) ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: tools build: Unused function, incomplete rename 2015-09-18 19:23 tools build: Unused function, incomplete rename Arnaldo Carvalho de Melo @ 2015-09-18 19:38 ` Arnaldo Carvalho de Melo 2015-09-18 19:48 ` Arnaldo Carvalho de Melo 2015-09-19 13:35 ` Jiri Olsa 1 sibling, 1 reply; 6+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-09-18 19:38 UTC (permalink / raw) To: Jiri Olsa Cc: Ingo Molnar, Namhyung Kim, Adrian Hunter, David Ahern, Wang Nan, Linux Kernel Mailing List Em Fri, Sep 18, 2015 at 04:23:34PM -0300, Arnaldo Carvalho de Melo escreveu: > Hi Jiri, Ingo, > > While trying to figure out why the bpf feature test is always > triggering the display of the "Auto-detecting system features" I noticed > this pattern: Another problem, this time in how tools/lib/bpf/ specifies which features to test for and which ones should have the feature detection shown, does the following patch makes sense? I think it does because FEATURE_TESTS looks like the ones that will be tested, and FEATURE_DISPLAY the ones that will appear...: diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile index f68d23a0b487..604c12081b4b 100644 --- a/tools/lib/bpf/Makefile +++ b/tools/lib/bpf/Makefile @@ -64,8 +64,8 @@ srctree := $(patsubst %/,%,$(dir $(srctree))) #$(info Determined 'srctree' to be $(srctree)) endif -FEATURE_DISPLAY = libelf libelf-getphdrnum libelf-mmap bpf -FEATURE_TESTS = libelf bpf +FEATURE_TESTS = libelf libelf-getphdrnum libelf-mmap bpf +FEATURE_DISPLAY = libelf bpf INCLUDES = -I. -I$(srctree)/tools/include -I$(srctree)/arch/$(ARCH)/include/uapi -I$(srctree)/include/uapi FEATURE_CHECK_CFLAGS-bpf = $(INCLUDES) ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: tools build: Unused function, incomplete rename 2015-09-18 19:38 ` Arnaldo Carvalho de Melo @ 2015-09-18 19:48 ` Arnaldo Carvalho de Melo 2015-09-18 20:42 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 6+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-09-18 19:48 UTC (permalink / raw) To: Jiri Olsa Cc: Ingo Molnar, Namhyung Kim, Adrian Hunter, David Ahern, Wang Nan, Linux Kernel Mailing List Em Fri, Sep 18, 2015 at 04:38:42PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Fri, Sep 18, 2015 at 04:23:34PM -0300, Arnaldo Carvalho de Melo escreveu: > > While trying to figure out why the bpf feature test is always > > triggering the display of the "Auto-detecting system features" I noticed > > this pattern: > Another problem, this time in how tools/lib/bpf/ specifies which > features to test for and which ones should have the feature detection > shown, does the following patch makes sense? I think it does because > FEATURE_TESTS looks like the ones that will be tested, and > FEATURE_DISPLAY the ones that will appear...: So the original problem seems to be this: [acme@felicio linux]$ cat /tmp/build/perf/FEATURE-DUMP feature-libelf(1) feature-libelf-getphdrnum(1) feature-libelf-mmap(1) feature-bpf(1) This is the content at the end of a build, i.e. the FEATURE-DUMP for tools/lib/ebpf/ usage of the feature detection system, since tools/perf/ uses the same file and selects a different set of features. I think that ebpf should use a separate directory, inside $(OUTPUT), this way we would have $(OUTPUT)/FEATURE-DUMP for perf and $(OUTPUT)/bpf/FEATURE-DUMP for ebpf. - Arnaldo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: tools build: Unused function, incomplete rename 2015-09-18 19:48 ` Arnaldo Carvalho de Melo @ 2015-09-18 20:42 ` Arnaldo Carvalho de Melo 2015-09-19 13:45 ` Jiri Olsa 0 siblings, 1 reply; 6+ messages in thread From: Arnaldo Carvalho de Melo @ 2015-09-18 20:42 UTC (permalink / raw) To: Jiri Olsa Cc: Ingo Molnar, Namhyung Kim, Adrian Hunter, David Ahern, Wang Nan, Linux Kernel Mailing List Em Fri, Sep 18, 2015 at 04:48:52PM -0300, Arnaldo Carvalho de Melo escreveu: > Em Fri, Sep 18, 2015 at 04:38:42PM -0300, Arnaldo Carvalho de Melo escreveu: > > Em Fri, Sep 18, 2015 at 04:23:34PM -0300, Arnaldo Carvalho de Melo escreveu: > > > While trying to figure out why the bpf feature test is always > > > triggering the display of the "Auto-detecting system features" I noticed > > > this pattern: > > > Another problem, this time in how tools/lib/bpf/ specifies which > > features to test for and which ones should have the feature detection > > shown, does the following patch makes sense? I think it does because > > FEATURE_TESTS looks like the ones that will be tested, and > > FEATURE_DISPLAY the ones that will appear...: > > So the original problem seems to be this: > > [acme@felicio linux]$ cat /tmp/build/perf/FEATURE-DUMP > feature-libelf(1) feature-libelf-getphdrnum(1) feature-libelf-mmap(1) feature-bpf(1) > > This is the content at the end of a build, i.e. the FEATURE-DUMP for > tools/lib/ebpf/ usage of the feature detection system, since > tools/perf/ uses the same file and selects a different set of > features. > > I think that ebpf should use a separate directory, inside $(OUTPUT), > this way we would have $(OUTPUT)/FEATURE-DUMP for perf and > $(OUTPUT)/bpf/FEATURE-DUMP for ebpf. [acme@felicio linux]$ ls -la /tmp/build/perf/FEATURE-DUMP -rw-rw-r--. 1 acme acme 338 Sep 18 17:38 /tmp/build/perf/FEATURE-DUMP [acme@felicio linux]$ ls -la /tmp/build/perf/FEATURE-DUMP.libbpf -rw-rw-r--. 1 acme acme 85 Sep 18 17:38 /tmp/build/perf/FEATURE-DUMP.libbpf [acme@felicio linux]$ Ok, patch below fixes this one, now the second run doesn't auto detects things again, i.e. libbpf feature detection doesn't stomps on perf's, I'll get those patches in a patchkit and send over the weekend. If you find anything fishy with it, holler. - Arnaldo diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature index 0caeaf2cae5f..0dedb3d245a1 100644 --- a/tools/build/Makefile.feature +++ b/tools/build/Makefile.feature @@ -123,8 +123,9 @@ define feature_print_text_code MSG = $(shell printf '...%30s: %s' $(1) $(2)) endef +FEATURE_DUMP_FILENAME = $(OUTPUT)FEATURE-DUMP$(FEATURE_USER) FEATURE_DUMP := $(foreach feat,$(FEATURE_DISPLAY),feature-$(feat)($(feature-$(feat)))) -FEATURE_DUMP_FILE := $(shell touch $(OUTPUT)FEATURE-DUMP; cat $(OUTPUT)FEATURE-DUMP) +FEATURE_DUMP_FILE := $(shell touch $(FEATURE_DUMP_FILENAME); cat $(FEATURE_DUMP_FILENAME)) ifeq ($(dwarf-post-unwind),1) FEATURE_DUMP += dwarf-post-unwind($(dwarf-post-unwind-text)) @@ -138,6 +139,6 @@ endif # - VF is enabled ifneq ("$(FEATURE_DUMP)","$(FEATURE_DUMP_FILE)") - $(shell echo "$(FEATURE_DUMP)" > $(OUTPUT)FEATURE-DUMP) + $(shell echo "$(FEATURE_DUMP)" > $(FEATURE_DUMP_FILENAME)) feature_display := 1 endif diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile index f68d23a0b487..01b4ff16a4e0 100644 --- a/tools/lib/bpf/Makefile +++ b/tools/lib/bpf/Makefile @@ -64,8 +64,8 @@ srctree := $(patsubst %/,%,$(dir $(srctree))) #$(info Determined 'srctree' to be $(srctree)) endif +FEATURE_USER = .libbpf FEATURE_DISPLAY = libelf libelf-getphdrnum libelf-mmap bpf FEATURE_TESTS = libelf bpf INCLUDES = -I. -I$(srctree)/tools/include -I$(srctree)/arch/$(ARCH)/include/uapi -I$(srctree)/include/uapi FEATURE_CHECK_CFLAGS-bpf = $(INCLUDES) ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: tools build: Unused function, incomplete rename 2015-09-18 20:42 ` Arnaldo Carvalho de Melo @ 2015-09-19 13:45 ` Jiri Olsa 0 siblings, 0 replies; 6+ messages in thread From: Jiri Olsa @ 2015-09-19 13:45 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Jiri Olsa, Ingo Molnar, Namhyung Kim, Adrian Hunter, David Ahern, Wang Nan, Linux Kernel Mailing List On Fri, Sep 18, 2015 at 05:42:47PM -0300, Arnaldo Carvalho de Melo wrote: > Em Fri, Sep 18, 2015 at 04:48:52PM -0300, Arnaldo Carvalho de Melo escreveu: > > Em Fri, Sep 18, 2015 at 04:38:42PM -0300, Arnaldo Carvalho de Melo escreveu: > > > Em Fri, Sep 18, 2015 at 04:23:34PM -0300, Arnaldo Carvalho de Melo escreveu: > > > > While trying to figure out why the bpf feature test is always > > > > triggering the display of the "Auto-detecting system features" I noticed > > > > this pattern: > > > > > Another problem, this time in how tools/lib/bpf/ specifies which > > > features to test for and which ones should have the feature detection > > > shown, does the following patch makes sense? I think it does because > > > FEATURE_TESTS looks like the ones that will be tested, and > > > FEATURE_DISPLAY the ones that will appear...: > > > > So the original problem seems to be this: > > > > [acme@felicio linux]$ cat /tmp/build/perf/FEATURE-DUMP > > feature-libelf(1) feature-libelf-getphdrnum(1) feature-libelf-mmap(1) feature-bpf(1) > > > > This is the content at the end of a build, i.e. the FEATURE-DUMP for > > tools/lib/ebpf/ usage of the feature detection system, since > > tools/perf/ uses the same file and selects a different set of > > features. > > > > I think that ebpf should use a separate directory, inside $(OUTPUT), > > this way we would have $(OUTPUT)/FEATURE-DUMP for perf and > > $(OUTPUT)/bpf/FEATURE-DUMP for ebpf. > > [acme@felicio linux]$ ls -la /tmp/build/perf/FEATURE-DUMP > -rw-rw-r--. 1 acme acme 338 Sep 18 17:38 /tmp/build/perf/FEATURE-DUMP > [acme@felicio linux]$ ls -la /tmp/build/perf/FEATURE-DUMP.libbpf > -rw-rw-r--. 1 acme acme 85 Sep 18 17:38 /tmp/build/perf/FEATURE-DUMP.libbpf > [acme@felicio linux]$ > > Ok, patch below fixes this one, now the second run doesn't auto detects > things again, i.e. libbpf feature detection doesn't stomps on perf's, > I'll get those patches in a patchkit and send over the weekend. If you > find anything fishy with it, holler. it looks ok.. btw IMO sharing output directory for perf and related libs could bite us in the future.. I think we should base it to tools directory.. I'll try to come up with something thanks, jirka ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: tools build: Unused function, incomplete rename 2015-09-18 19:23 tools build: Unused function, incomplete rename Arnaldo Carvalho de Melo 2015-09-18 19:38 ` Arnaldo Carvalho de Melo @ 2015-09-19 13:35 ` Jiri Olsa 1 sibling, 0 replies; 6+ messages in thread From: Jiri Olsa @ 2015-09-19 13:35 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Jiri Olsa, Ingo Molnar, Namhyung Kim, Adrian Hunter, David Ahern, Linux Kernel Mailing List On Fri, Sep 18, 2015 at 04:23:34PM -0300, Arnaldo Carvalho de Melo wrote: > Hi Jiri, Ingo, > > While trying to figure out why the bpf feature test is always > triggering the display of the "Auto-detecting system features" I noticed > this pattern: > > [acme@felicio linux]$ egrep '^define|eval' tools/build/Makefile.feature > feature_check = $(eval $(feature_check_code)) > define feature_check_code > feature_set = $(eval $(feature_set_code)) > define feature_set_code > set_test_all_flags = $(eval $(set_test_all_flags_code)) > define set_test_all_flags_code > feature_print_status = $(eval $(feature_print_status_code)) $(info $(MSG)) > define feature_print_status_code > feature_print_text = $(eval $(feature_print_text_code)) $(info $(MSG)) > define feature_print_text_code > feature_display_check = $(eval $(feature_check_code)) > define feature_display_check_code > [acme@felicio linux]$ > > > In all but one case the eval matches the following define, except for the last > one, don't we need the following patch? > > $ git diff tools/build/Makefile.feature > diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature > index 0caeaf2cae5f..072ec879b84f 100644 > --- a/tools/build/Makefile.feature > +++ b/tools/build/Makefile.feature > @@ -142,7 +142,7 @@ ifneq ("$(FEATURE_DUMP)","$(FEATURE_DUMP_FILE)") > feature_display := 1 > endif > > -feature_display_check = $(eval $(feature_check_code)) > +feature_display_check = $(eval $(feature_check_display_code)) > define feature_display_check_code > ifneq ($(feature-$(1)), 1) > feature_display := 1 ouch, that is a bug.. nice catch! thanks, jirka ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-19 13:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-09-18 19:23 tools build: Unused function, incomplete rename Arnaldo Carvalho de Melo 2015-09-18 19:38 ` Arnaldo Carvalho de Melo 2015-09-18 19:48 ` Arnaldo Carvalho de Melo 2015-09-18 20:42 ` Arnaldo Carvalho de Melo 2015-09-19 13:45 ` Jiri Olsa 2015-09-19 13:35 ` Jiri Olsa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox