Message-ID: <YqYTZVa44Y6RQ11W@krava>
Date:   Sun, 12 Jun 2022 18:25:09 +0200
From:   Jiri Olsa <olsajiri@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        "linux-perf-use." <linux-perf-users@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        Ian Rogers <irogers@...gle.com>
Subject: Re: [PATCHv4 bpf-next 0/2] perf tools: Fix prologue generation

On Fri, Jun 10, 2022 at 10:53:57PM +0200, Jiri Olsa wrote:
> On Fri, Jun 10, 2022 at 11:45:48AM -0700, Andrii Nakryiko wrote:
> > On Fri, Jun 10, 2022 at 11:34 AM Arnaldo Carvalho de Melo
> > <acme@...nel.org> wrote:
> > >
> > > Em Thu, Jun 09, 2022 at 01:31:52PM -0700, Andrii Nakryiko escreveu:
> > > > On Fri, Jun 3, 2022 at 1:45 PM Jiri Olsa <jolsa@...nel.org> wrote:
> > > > >
> > > > > hi,
> > > > > sending change we discussed some time ago [1] to get rid of
> > > > > some deprecated functions we use in perf prologue code.
> > > > >
> > > > > Despite the gloomy discussion I think the final code does
> > > > > not look that bad ;-)
> > > > >
> > > > > This patchset removes following libbpf functions from perf:
> > > > >   bpf_program__set_prep
> > > > >   bpf_program__nth_fd
> > > > >   struct bpf_prog_prep_result
> > > > >
> > > > > v4 changes:
> > > > >   - fix typo [Andrii]
> > > > >
> > > > > v3 changes:
> > > > >   - removed R0/R1 zero init in libbpf_prog_prepare_load_fn,
> > > > >     because it's not needed [Andrii]
> > > > >   - rebased/post on top of bpf-next/master which now has
> > > > >     all the needed perf/core changes
> > > > >
> > > > > v2 changes:
> > > > >   - use fallback section prog handler, so we don't need to
> > > > >     use section prefix [Andrii]
> > > > >   - realloc prog->insns array in bpf_program__set_insns [Andrii]
> > > > >   - squash patch 1 from previous version with
> > > > >     bpf_program__set_insns change [Daniel]
> > > > >   - patch 3 already merged [Arnaldo]
> > > > >   - added more comments
> > > > >
> > > > > thanks,
> > > > > jirka
> > > > >
> > > >
> > > > Arnaldo, can I get an ack from you for this patch set? Thank you!
> > >
> > > So, before these patches:
> > >
> > > [acme@...co perf-urgent]$ git log --oneline -5
> > > 22905f78d181f446 (HEAD) libperf evsel: Open shouldn't leak fd on failure
> > > a3c6da3dbd4bdf9c perf test: Fix "perf stat CSV output linter" test on s390
> > > 785cb9e85e8ba66f perf unwind: Fix uninitialized variable
> > > 874c8ca1e60b2c56 netfs: Fix gcc-12 warning by embedding vfs inode in netfs_i_context
> > > 3d9f55c57bc3659f Merge tag 'fs_for_v5.19-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs
> > > [acme@...co perf-urgent]$ sudo su -
> > > [root@...co ~]# perf -v
> > > perf version 5.19.rc1.g22905f78d181
> > > [root@...co ~]# perf test 42
> > >  42: BPF filter                                                      :
> > >  42.1: Basic BPF filtering                                           : Ok
> > >  42.2: BPF pinning                                                   : Ok
> > >  42.3: BPF prologue generation                                       : Ok
> > > [root@...co ~]#
> > >
> > > And after:
> > >
> > > [acme@...co perf-urgent]$ git log --oneline -5
> > > f8ec656242acf2de (HEAD -> perf/urgent) perf tools: Rework prologue generation code
> > > a750a8dd7e5d2d4b perf tools: Register fallback libbpf section handler
> > > d28f2a8ad42af160 libperf evsel: Open shouldn't leak fd on failure
> > > a3c6da3dbd4bdf9c perf test: Fix "perf stat CSV output linter" test on s390
> > > 785cb9e85e8ba66f perf unwind: Fix uninitialized variable
> > > [acme@...co perf-urgent]$ sudo su -
> > > [root@...co ~]# perf -v
> > > perf version 5.19.rc1.gf8ec656242ac
> > > [root@...co ~]# perf test 42
> > >  42: BPF filter                                                      :
> > >  42.1: Basic BPF filtering                                           : FAILED!
> > >  42.2: BPF pinning                                                   : FAILED!
> > >  42.3: BPF prologue generation                                       : Ok
> > > [root@...co ~]#
> > >
> > > Jiri, can you try reproducing these? Do this require some other work
> > > that is in bpf-next/master? Lemme try...
> > >
> > > Further details:
> > >
> > > [acme@...co perf-urgent]$ clang -v
> > > clang version 13.0.0 (five:git/llvm-project d667b96c98438edcc00ec85a3b151ac2dae862f3)
> > > Target: x86_64-unknown-linux-gnu
> > > Thread model: posix
> > > InstalledDir: /usr/local/bin
> > > Found candidate GCC installation: /usr/lib/gcc/x86_64-redhat-linux/12
> > > Selected GCC installation: /usr/lib/gcc/x86_64-redhat-linux/12
> > > Candidate multilib: .;@m64
> > > Candidate multilib: 32;@m32
> > > Selected multilib: .;@m64
> > > [acme@...co perf-urgent]$ cat /etc/fedora-release
> > > Fedora release 36 (Thirty Six)
> > > [acme@...co perf-urgent]$ gcc -v
> > > Using built-in specs.
> > > COLLECT_GCC=/usr/bin/gcc
> > > COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/12/lto-wrapper
> > > OFFLOAD_TARGET_NAMES=nvptx-none
> > > OFFLOAD_TARGET_DEFAULT=1
> > > Target: x86_64-redhat-linux
> > > Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,fortran,objc,obj-c++,ada,go,d,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --enable-libstdcxx-backtrace --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl=/builddir/build/BUILD/gcc-12.1.1-20220507/obj-x86_64-redhat-linux/isl-install --enable-offload-targets=nvptx-none --without-cuda-driver --enable-offload-defaulted --enable-gnu-indirect-function --enable-cet --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux --with-build-config=bootstrap-lto --enable-link-serialization=1
> > > Thread model: posix
> > > Supported LTO compression algorithms: zlib zstd
> > > gcc version 12.1.1 20220507 (Red Hat 12.1.1-1) (GCC)
> > > [acme@...co perf-urgent]$
> > >
> > > [root@...co ~]# perf test -v 42
> > >  42: BPF filter                                                      :
> > >  42.1: Basic BPF filtering                                           :
> > > --- start ---
> > > test child forked, pid 638881
> > > Kernel build dir is set to /lib/modules/5.17.11-300.fc36.x86_64/build
> > > set env: KBUILD_DIR=/lib/modules/5.17.11-300.fc36.x86_64/build
> > > unset env: KBUILD_OPTS
> > > include option is set to -nostdinc -I./arch/x86/include -I./arch/x86/include/generated  -I./include -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/compiler-version.h -include ./include/linux/kconfig.h
> > > set env: NR_CPUS=8
> > > set env: LINUX_VERSION_CODE=0x5110b
> > > set env: CLANG_EXEC=/usr/lib64/ccache/clang
> > > set env: CLANG_OPTIONS=-xc -g
> 
> ok, it's the BTF debug info as Andrii mentioned below,
> I assume you have 'clang-opt=-g' in .perfconfig, right?
> 
> when I add it to mine I can reproduce, perfect
> 
> SNIP
> 
> > > bpf: config 'func=do_epoll_wait' is ok
> > > Looking at the vmlinux_path (8 entries long)
> > > symsrc__init: build id mismatch for vmlinux.
> > > Using /usr/lib/debug/lib/modules/5.17.11-300.fc36.x86_64/vmlinux for symbols
> > > Open Debuginfo file: /usr/lib/debug/.build-id/f2/26f5d75e6842add57095a0615a1e5c16783dd7.debug
> > > Try to find probe point from debuginfo.
> > > Matched function: do_epoll_wait [38063fb]
> > > Probe point found: do_epoll_wait+0
> > > Found 1 probe_trace_events.
> > > Opening /sys/kernel/tracing//kprobe_events write=1
> > > Opening /sys/kernel/tracing//README write=0
> > > Writing event: p:perf_bpf_probe/func _text+3943040
> > > libbpf: map 'flip_table': created successfully, fd=4
> > > libbpf: prog 'bpf_func__SyS_epoll_pwait': BPF program load failed: Invalid argument
> > > libbpf: prog 'bpf_func__SyS_epoll_pwait': -- BEGIN PROG LOAD LOG --
> > > Invalid insn code at line_info[11].insn_off
> > 
> > Mismatched line_info.
> > 
> > Jiri, I think we need to clear func_info and line_info in
> > bpf_program__set_insns() because at that point func/line info can be
> > mismatched and won't correspond to the actual set of instructions.

so the problem is that we prepend init proglogue instructions
for each program not just for the one that needs it, so it will
mismatch later on.. the fix below makes it work for me

Arnaldo,
I squashed and pushed the change below changes to my bpf/depre
branch, could you please retest?

thanks,
jirka


---
diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index 30d0e688beec..6bd7c288e820 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -107,6 +107,17 @@ static int bpf_perf_object__add(struct bpf_object *obj)
 	return perf_obj ? 0 : -ENOMEM;
 }
 
+static void *program_priv(const struct bpf_program *prog)
+{
+	void *priv;
+
+	if (IS_ERR_OR_NULL(bpf_program_hash))
+		return NULL;
+	if (!hashmap__find(bpf_program_hash, prog, &priv))
+		return NULL;
+	return priv;
+}
+
 static struct bpf_insn prologue_init_insn[] = {
 	BPF_MOV64_IMM(BPF_REG_2, 0),
 	BPF_MOV64_IMM(BPF_REG_3, 0),
@@ -120,9 +131,18 @@ static int libbpf_prog_prepare_load_fn(struct bpf_program *prog,
 {
 	size_t init_size_cnt = ARRAY_SIZE(prologue_init_insn);
 	size_t orig_insn_cnt, insn_cnt, init_size, orig_size;
+	struct bpf_prog_priv *priv = program_priv(prog);
 	const struct bpf_insn *orig_insn;
 	struct bpf_insn *insn;
 
+	if (IS_ERR_OR_NULL(priv)) {
+		pr_debug("bpf: failed to get private field\n");
+		return -BPF_LOADER_ERRNO__INTERNAL;
+	}
+
+	if (!priv->need_prologue)
+		return 0;
+
 	/* prepend initialization code to program instructions */
 	orig_insn = bpf_program__insns(prog);
 	orig_insn_cnt = bpf_program__insn_cnt(prog);
@@ -312,17 +332,6 @@ static bool ptr_equal(const void *key1, const void *key2,
 	return key1 == key2;
 }
 
-static void *program_priv(const struct bpf_program *prog)
-{
-	void *priv;
-
-	if (IS_ERR_OR_NULL(bpf_program_hash))
-		return NULL;
-	if (!hashmap__find(bpf_program_hash, prog, &priv))
-		return NULL;
-	return priv;
-}
-
 static int program_set_priv(struct bpf_program *prog, void *priv)
 {
 	void *old_priv;

Powered by blists - more mailing lists