Re: [PATCH 2/2] btf_encoder: Fix function generation

From: Jiri Olsa <jolsa@redhat.com>
To: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	Jiri Olsa <jolsa@kernel.org>,
	dwarves@vger.kernel.org, bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andriin@fb.com>, Yonghong Song <yhs@fb.com>,
	Hao Luo <haoluo@google.com>
Subject: Re: [PATCH 2/2] btf_encoder: Fix function generation
Date: Mon, 16 Nov 2020 20:40:08 +0100	[thread overview]
Message-ID: <20201116194008.GA1216482@krava> (raw)
In-Reply-To: <20201116192221.GB614220@kernel.org>

On Mon, Nov 16, 2020 at 04:22:21PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Nov 16, 2020 at 04:15:44PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Nov 16, 2020 at 07:21:45PM +0100, Jiri Olsa escreveu:
> > > On Mon, Nov 16, 2020 at 10:50:16AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Em Fri, Nov 13, 2020 at 01:43:47PM -0800, Andrii Nakryiko escreveu:
> > > > > On Fri, Nov 13, 2020 at 1:29 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > > > 
> > > > > > On Fri, Nov 13, 2020 at 12:56:40PM -0800, Andrii Nakryiko wrote:
> > > > > > > On Fri, Nov 13, 2020 at 7:13 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > 
> > > > > > > > Current conditions for picking up function records break
> > > > > > > > BTF data on some gcc versions.
> > > > 
> > > > > > > > Some function records can appear with no arguments but with
> > > > > > > > declaration tag set, so moving the 'fn->declaration' in front
> > > > > > > > of other checks.
> > > > 
> > > > > > > > Then checking if argument names are present and finally checking
> > > > > > > > ftrace filter if it's present. If ftrace filter is not available,
> > > > > > > > using the external tag to filter out non external functions.
> > > > 
> > > > > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > 
> > > > > > > I tested locally, all seems to work fine. Left few suggestions below,
> > > > > > > but those could be done in follow ups (or argued to not be done).
> > > > 
> > > > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > > 
> > > > > > > BTW, for some stats.
> > > > 
> > > > > > > BEFORE allowing static funcs:
> > > > 
> > > > 
> > > > Nowhere in the last patchkit comments is some explanation for the
> > > > inclusion of static functions :-\ After the first patch in the last
> > > > series I get:
> > > > 
> > > >   $ llvm-objcopy --remove-section=.BTF vmlinux
> > > >   $ readelf -SW vmlinux  | grep BTF
> > > >   $ pahole -J vmlinux
> > > >   $ bpftool btf dump file ./vmlinux | grep 'FUNC '| cut -d\' -f2 | sort > before.bpftool
> > > >   $ cp vmlinux vmlinux.before.all
> > > >   $ wc -l before.bpftool
> > > >   28829 before.bpftool
> > > 
> > > I think you see the original number of functions, because without
> > > the 'not merged' kernel patch, that added the special init section,
> > > pahole will fail to detect vmlinux and fall back to checking dwarf
> > > declarations
> > 
> > Indeed, I moved the verbose/force setting to the beggining of the
> > encoder and:
> > 
> > ------------
> > Found 352 per-CPU variables!
> > vmlinux not detected, falling back to dwarf data
> > File vmlinux:
> > search cu '/home/acme/git/linux/arch/x86/kernel/head_64.S' for percpu global variables.
> > -----------------
> > 
> > Now I have to read that code to figure out what that "vmlinux not
> > detected, falling back to dwarf data" message means, as vmlinux is where
> > DWARF data is, so what is that isn't being "detected", /me checks...
> 
> So with some debugging I see, the message is just confusing:
> 
> "vmlinux not detected, falling back to dwarf data (functions_cnt=53238, has_all_symbols(&fl)=0"

how about:

"ftrace data not detected, falling back to dwarf data"

> 
> It finds the ELF symtab, finds the percpu variables there, tons of
> functions, matching the number after this approach of marking BPF init
> functions was dropped its just that vague "has_all_symbols()" routine
> that fails to find all the symbols it needs in the vmlinux file.

we collect functions and other symbols in one loop over the symtab,
so thats why we have all those collected and still can decide to fall back

before we needed also the init section symbols, now with this patch
we need to know only mcount section begin/end

jirka

> 
> - Arnaldo
>