On Thu, Oct 5, 2023 at 2:04 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote: > > On 05/10/2023 08:19, Jiri Olsa wrote: > > On Wed, Oct 04, 2023 at 10:21:12AM -0700, Andrii Nakryiko wrote: > >> On Wed, Oct 4, 2023 at 1:27 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote: > >>> > >>> On Tue, Oct 03, 2023 at 05:17:49PM -0700, Andrii Nakryiko wrote: > >>>> Add support for building selftests with -O2 level of optimization, which > >>>> allows more compiler warnings detection (like lots of potentially > >>>> uninitialized usage), but also is useful to have a faster-running test > >>>> for some CPU-intensive tests. > >>>> > >>>> One can build optimized versions of libbpf and selftests by running: > >>>> > >>>> $ make RELEASE=1 > >>>> > >>>> There is a measurable speed up of about 10 seconds for me locally, > >>>> though it's mostly capped by non-parallelized serial tests. User CPU > >>>> time goes down by total 40 seconds, from 1m10s to 0m28s. > >>>> > >>>> Unoptimized build (-O0) > >>>> ======================= > >>>> Summary: 430/3544 PASSED, 25 SKIPPED, 4 FAILED > >>>> > >>>> real 1m59.937s > >>>> user 1m10.877s > >>>> sys 3m14.880s > >>>> > >>>> Optimized build (-O2) > >>>> ===================== > >>>> Summary: 425/3543 PASSED, 25 SKIPPED, 9 FAILED > >>>> > >>>> real 1m50.540s > >>>> user 0m28.406s > >>>> sys 3m13.198s > >>> > >>> hi, > >>> I get following error when running selftest compiled with RELEASE=1 > >>> > >>> # ./test_progs -t attach_probe/manual-legacy > >>> test_attach_probe:PASS:skel_open 0 nsec > >>> test_attach_probe:PASS:skel_load 0 nsec > >>> test_attach_probe:PASS:check_bss 0 nsec > >>> test_attach_probe:PASS:uprobe_ref_ctr_cleanup 0 nsec > >>> test_attach_probe_manual:PASS:skel_kprobe_manual_open_and_load 0 nsec > >>> test_attach_probe_manual:PASS:uprobe_offset 0 nsec > >>> test_attach_probe_manual:PASS:attach_kprobe 0 nsec > >>> test_attach_probe_manual:PASS:attach_kretprobe 0 nsec > >>> test_attach_probe_manual:PASS:attach_uprobe 0 nsec > >>> test_attach_probe_manual:PASS:attach_uretprobe 0 nsec > >>> libbpf: failed to add legacy uprobe event for /proc/self/exe:0x19020: -17 > >>> libbpf: prog 'handle_uprobe_byname': failed to create uprobe '/proc/self/exe:0x19020' perf event: File exists > >>> test_attach_probe_manual:FAIL:attach_uprobe_byname unexpected error: -17 > >>> #8/2 attach_probe/manual-legacy:FAIL > >>> #8 attach_probe:FAIL > >>> > >>> > >>> it looks like -O2 can merge some of the trigger functions: > >>> > >>> [root@qemu bpf]# nm test_progs | grep trigger_func > >>> 0000000000558f30 t autoattach_trigger_func.constprop.0 > >>> 000000000041d240 t trigger_func > >>> 0000000000419020 t trigger_func > >>> 0000000000420e70 t trigger_func > >>> 0000000000507aa0 t trigger_func > >>> 0000000000419020 t trigger_func2 > >>> 0000000000419020 t trigger_func3 > >>> 0000000000419030 t trigger_func4 > >>> [root@qemu bpf]# nm test_progs | grep 0000000000419020 > >>> 0000000000419020 t trigger_func > >>> 0000000000419020 t trigger_func2 > >>> 0000000000419020 t trigger_func3 > >>> > >>> I got more tests fails, but I suspect it's all for similar > >>> reason like above > >>> > >> > > This one is an interesting failure that definitely seems worth fixing; > as above trigger_func2 and trigger_func3 were merged it looks like, and > the legacy perf_event_kprobe_open_legacy()-based attach failed due to > add_uprobe_event_legacy() failing. The latter seems to be down to the > way that gen_uprobe_legacy_event_name() constructs legacy event names > via pid, binary_path and offset. In this case it will construct the > same name for trigger_func2 and trigger_func3, hence the -EEXIST. > > It _seems_ like a fix here would be to add an optional func_name to > gen_uprobe_legacy_event_name(). At least it appears that the non-legacy > attach handles this case, which is great. Regardless, we can follow > up with some of this stuff later. Yeah, let's fix this up as a follow up. I'm not sure about using function name as part of uprobe name, mostly because these symbol names can be super long, and I don't know what's kernel size limits. So we probably want to keep them bounded in size. Having said that, seems like we use binary path and we also don't sanitize that. So I think we should try to fix all that as a follow up: sanitize and maybe truncate binary_path, and generally make sure that each uprobe name is unique (probably with atomic counter). This atomic static counter will take care of all such issues, right? Maybe we should drop the binary path from the uprobe name altogether with that? Either way, thanks for taking a deeper look into failures! > > >> yes, I didn't say that -O2 version passes all tests :) at least there > >> are complicated USDT cases under -O2 which libbpf can't support (if I > >> remember correctly, it was offset relative to global symbol case). But > >> it's the first step. And once we have ability to build with RELEASE=1, > >> we can add it as a separate test in CI and catch more of these > >> uninitialized usage errors. Initially we can denylist tests that are > >> broken due to -O2 and work to fix them. > > > > Sounds good to me; it's a great change as we're more likely to hit > more problems that users run into. yep > > For the series: > > Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx> > > > I see ;-) sounds good > > > > Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx> > > > > jirka > > > >> > >>> jirka > >>> > >>> > >>>> > >>>> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > >>>> --- > >>>> tools/testing/selftests/bpf/Makefile | 14 ++++++++------ > >>>> 1 file changed, 8 insertions(+), 6 deletions(-) > >>>> > >>>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > >>>> index a25e262dbc69..55d1b1848e6c 100644 > >>>> --- a/tools/testing/selftests/bpf/Makefile > >>>> +++ b/tools/testing/selftests/bpf/Makefile > >>>> @@ -27,7 +27,9 @@ endif > >>>> BPF_GCC ?= $(shell command -v bpf-gcc;) > >>>> SAN_CFLAGS ?= > >>>> SAN_LDFLAGS ?= $(SAN_CFLAGS) > >>>> -CFLAGS += -g -O0 -rdynamic \ > >>>> +RELEASE ?= > >>>> +OPT_FLAGS ?= $(if $(RELEASE),-O2,-O0) > >>>> +CFLAGS += -g $(OPT_FLAGS) -rdynamic \ > >>>> -Wall -Werror \ > >>>> $(GENFLAGS) $(SAN_CFLAGS) \ > >>>> -I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR) \ > >>>> @@ -241,7 +243,7 @@ $(OUTPUT)/runqslower: $(BPFOBJ) | $(DEFAULT_BPFTOOL) $(RUNQSLOWER_OUTPUT) > >>>> BPFTOOL_OUTPUT=$(HOST_BUILD_DIR)/bpftool/ \ > >>>> BPFOBJ_OUTPUT=$(BUILD_DIR)/libbpf \ > >>>> BPFOBJ=$(BPFOBJ) BPF_INCLUDE=$(INCLUDE_DIR) \ > >>>> - EXTRA_CFLAGS='-g -O0 $(SAN_CFLAGS)' \ > >>>> + EXTRA_CFLAGS='-g $(OPT_FLAGS) $(SAN_CFLAGS)' \ > >>>> EXTRA_LDFLAGS='$(SAN_LDFLAGS)' && \ > >>>> cp $(RUNQSLOWER_OUTPUT)runqslower $@ > >>>> > >>>> @@ -279,7 +281,7 @@ $(DEFAULT_BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile) \ > >>>> $(HOST_BPFOBJ) | $(HOST_BUILD_DIR)/bpftool > >>>> $(Q)$(MAKE) $(submake_extras) -C $(BPFTOOLDIR) \ > >>>> ARCH= CROSS_COMPILE= CC="$(HOSTCC)" LD="$(HOSTLD)" \ > >>>> - EXTRA_CFLAGS='-g -O0' \ > >>>> + EXTRA_CFLAGS='-g $(OPT_FLAGS)' \ > >>>> OUTPUT=$(HOST_BUILD_DIR)/bpftool/ \ > >>>> LIBBPF_OUTPUT=$(HOST_BUILD_DIR)/libbpf/ \ > >>>> LIBBPF_DESTDIR=$(HOST_SCRATCH_DIR)/ \ > >>>> @@ -290,7 +292,7 @@ $(CROSS_BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile) \ > >>>> $(BPFOBJ) | $(BUILD_DIR)/bpftool > >>>> $(Q)$(MAKE) $(submake_extras) -C $(BPFTOOLDIR) \ > >>>> ARCH=$(ARCH) CROSS_COMPILE=$(CROSS_COMPILE) \ > >>>> - EXTRA_CFLAGS='-g -O0' \ > >>>> + EXTRA_CFLAGS='-g $(OPT_FLAGS)' \ > >>>> OUTPUT=$(BUILD_DIR)/bpftool/ \ > >>>> LIBBPF_OUTPUT=$(BUILD_DIR)/libbpf/ \ > >>>> LIBBPF_DESTDIR=$(SCRATCH_DIR)/ \ > >>>> @@ -313,7 +315,7 @@ $(BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile) \ > >>>> $(APIDIR)/linux/bpf.h \ > >>>> | $(BUILD_DIR)/libbpf > >>>> $(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) OUTPUT=$(BUILD_DIR)/libbpf/ \ > >>>> - EXTRA_CFLAGS='-g -O0 $(SAN_CFLAGS)' \ > >>>> + EXTRA_CFLAGS='-g $(OPT_FLAGS) $(SAN_CFLAGS)' \ > >>>> EXTRA_LDFLAGS='$(SAN_LDFLAGS)' \ > >>>> DESTDIR=$(SCRATCH_DIR) prefix= all install_headers > >>>> > >>>> @@ -322,7 +324,7 @@ $(HOST_BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile) \ > >>>> $(APIDIR)/linux/bpf.h \ > >>>> | $(HOST_BUILD_DIR)/libbpf > >>>> $(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) \ > >>>> - EXTRA_CFLAGS='-g -O0' ARCH= CROSS_COMPILE= \ > >>>> + EXTRA_CFLAGS='-g $(OPT_FLAGS)' ARCH= CROSS_COMPILE= \ > >>>> OUTPUT=$(HOST_BUILD_DIR)/libbpf/ \ > >>>> CC="$(HOSTCC)" LD="$(HOSTLD)" \ > >>>> DESTDIR=$(HOST_SCRATCH_DIR)/ prefix= all install_headers > >>>> -- > >>>> 2.34.1 > >>>> > >>>> > >
