Received-SPF: pass (google.com: domain of linux-kernel+bounces-68070-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240212233322.1855161-1-namhyung@kernel.org> <20240213033954.GB81405@debian-dev>
 <CAM9d7ciTwYAgry-nW9z+_VMj+BJ7ZNZnkKH_t_AHvV5joNuWQQ@mail.gmail.com> <20240214101420.GF81405@debian-dev>
In-Reply-To: <20240214101420.GF81405@debian-dev>
From: Namhyung Kim <namhyung@kernel.org>
Date: Thu, 15 Feb 2024 21:19:51 -0800
Message-ID: <CAM9d7cgWXyv1Uy=ZWsT6K=KaztgtszZp0BOxPAbgjuKuX6OzdQ@mail.gmail.com>
Subject: Re: [PATCH] perf tools: Fixup module symbol end address properly
To: Leo Yan <leo.yan@linux.dev>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>, Ian Rogers <irogers@google.com>, Jiri Olsa <jolsa@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, Peter Zijlstra <peterz@infradead.org>, 
	Ingo Molnar <mingo@kernel.org>, LKML <linux-kernel@vger.kernel.org>, 
	linux-perf-users@vger.kernel.org, Will Deacon <will@kernel.org>, 
	Mark Rutland <mark.rutland@arm.com>, John Garry <john.g.garry@oracle.com>, 
	Mike Leach <mike.leach@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Feb 14, 2024 at 2:14=E2=80=AFAM Leo Yan <leo.yan@linux.dev> wrote:
>
> On Tue, Feb 13, 2024 at 10:48:53AM -0800, Namhyung Kim wrote:
> > Hi Leo,
> >
> > Thanks for your review!
> >
> > On Mon, Feb 12, 2024 at 7:40???PM Leo Yan <leo.yan@linux.dev> wrote:
> > >
> > > On Mon, Feb 12, 2024 at 03:33:22PM -0800, Namhyung Kim wrote:
> > > > I got a strange error on ARM to fail on processing FINISHED_ROUND
> > > > record.  It turned out that it was failing in symbol__alloc_hist()
> > > > because the symbol size is too big.
> > > >
> > > > When a sample is captured on a specific BPF program, it failed.  I'=
ve
> > > > added a debug code and found the end address of the symbol is from
> > > > the next module which is placed far way.
> > > >
> > > >   ffff800008795778-ffff80000879d6d8: bpf_prog_1bac53b8aac4bc58_netc=
g_sock    [bpf]
> > > >   ffff80000879d6d8-ffff80000ad656b4: bpf_prog_76867454b5944e15_netc=
g_getsockopt      [bpf]
> > > >   ffff80000ad656b4-ffffd69b7af74048: bpf_prog_1d50286d2eb1be85_hn_e=
gress     [bpf]   <---------- here
> > > >   ffffd69b7af74048-ffffd69b7af74048: $x.5    [sha3_generic]
> > > >   ffffd69b7af74048-ffffd69b7af740b8: crypto_sha3_init        [sha3_=
generic]
> > > >   ffffd69b7af740b8-ffffd69b7af741e0: crypto_sha3_update      [sha3_=
generic]
> > > >
> > > > The logic in symbols__fixup_end() just uses curr->start to update t=
he
> > > > prev->end.  But in this case, it won't work as it's too different.
> > > >
> > > > I think ARM has a different kernel memory layout for modules and BP=
F
> > > > than on x86.  Actually there's a logic to handle kernel and module
> > > > boundary.  Let's do the same for symbols between different modules.
> > >
> > > Even Arm32 and Arm64 kernel have different memory layout for modules
> > > and kernel image.
> > >
> > > eBPF program (JITed) should be allocated from the vmalloc region, for
> > > Arm64, see bpf_jit_alloc_exec() in arch/arm64/net/bpf_jit_comp.c.
> >
> > Ok, so chances are they can fall out far away right?
>
> Yes, this is my understanding.
>
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > ---
> > > >  tools/perf/util/symbol.c | 21 +++++++++++++++++++--
> > > >  1 file changed, 19 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > > > index 35975189999b..9ebdb8e13c0b 100644
> > > > --- a/tools/perf/util/symbol.c
> > > > +++ b/tools/perf/util/symbol.c
> > > > @@ -248,14 +248,31 @@ void symbols__fixup_end(struct rb_root_cached=
 *symbols, bool is_kallsyms)
> > > >                * segment is very big.  Therefore do not fill this g=
ap and do
> > > >                * not assign it to the kernel dso map (kallsyms).
> > > >                *
> > > > +              * Also BPF code can be allocated separately from tex=
t segments
> > > > +              * and modules.  So the last entry in a module should=
 not fill
> > > > +              * the gap too.
> > > > +              *
> > > >                * In kallsyms, it determines module symbols using '[=
' character
> > > >                * like in:
> > > >                *   ffffffffc1937000 T hdmi_driver_init  [snd_hda_co=
dec_hdmi]
> > > >                */
> > > >               if (prev->end =3D=3D prev->start) {
> > > > +                     const char *prev_mod;
> > > > +                     const char *curr_mod;
> > > > +
> > > > +                     if (!is_kallsyms) {
> > > > +                             prev->end =3D curr->start;
> > > > +                             continue;
> > > > +                     }
> > > > +
> > > > +                     prev_mod =3D strchr(prev->name, '[');
> > > > +                     curr_mod =3D strchr(curr->name, '[');
> > > > +
> > > >                       /* Last kernel/module symbol mapped to end of=
 page */
> > > > -                     if (is_kallsyms && (!strchr(prev->name, '[') =
!=3D
> > > > -                                         !strchr(curr->name, '['))=
)
> > > > +                     if (!prev_mod !=3D !curr_mod)
> > > > +                             prev->end =3D roundup(prev->end + 409=
6, 4096);
> > > > +                     /* Last symbol in the previous module */
> > > > +                     else if (prev_mod && strcmp(prev_mod, curr_mo=
d))
> > >
> > > Should two consecutive moudles fall into this case? I think we need t=
o assign
> > > 'prev->end =3D curr->start' for two two consecutive moudles.
> >
> > Yeah I thought about that case but I believe they would be on
> > separate pages (hopefully there's a page gap between them).
> > So I think it should not overlap.  But if you really care we can
> > check it explicitly like this:
> >
> >     prev->end =3D min(roundup(...), curr->start);
>
> I am not concerned that to assign a bigger end value for the 'prev'
> symbol. With an exaggerate end region, it will not cause any
> difficulty for parsing symbols.

Right, but my problem was not in parsing.  It failed to allocate
memory for the symbol because it's too big.

> On the other hand, I am a bit concern
> for a big function (e.g. its code size > 4KiB), we might fail to find
> symbols in this case with the change above.

Yes, it's another problem.  But it cannot know the exact size
so it just assumes it fits in a page.

>
> > > If so, we should use a specific checking for eBPF program, e.g.:
> > >
> > >                         else if (prev_mod && strcmp(prev_mod, curr_mo=
d) &&
> > >                                  (!strcmp(prev->name, "bpf") ||
> > >                                   !strcmp(curr->name, "bpf")))
> >
> > I suspect it can happen on any module boundary so better
> > to handle it in a more general way.
>
> I don't want to introduce over complexity at here. We can apply
> current patch as it is.

Good, can I get your Reviewed-by then? :)

>
> A side topic, when I saw the code is hard coded for 4096 as the page
> size, this is not always true on Arm64 (the page size can be 4KiB,
> 16KiB or 64KiB). We need to consider to extend the environment for
> recording the system's page size.

Sounds good.  But until then, 4K would be the reasonable choice.

Thanks,
Namhyung

> >
> > >
> > > >                               prev->end =3D roundup(prev->end + 409=
6, 4096);
> > > >                       else
> > > >                               prev->end =3D curr->start;
> > > > --
> > > > 2.43.0.687.g38aa6559b0-goog
> > > >