Message-ID: <CAP-5=fXrhjv8obOPKpXThsrOKjb5kKFTpoTRO3tDk5MoJyhdjw@mail.gmail.com>
Date:   Mon, 14 Sep 2020 17:03:25 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Stephane Eranian <eranian@...gle.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Jiri Olsa <jolsa@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Michael Petlan <mpetlan@...hat.com>,
        Song Liu <songliubraving@...com>,
        "Frank Ch. Eigler" <fche@...hat.com>,
        Alexey Budankov <alexey.budankov@...ux.intel.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Adrian Hunter <adrian.hunter@...el.com>
Subject: Re: [PATCH 02/26] perf: Introduce mmap3 version of mmap event

On Mon, Sep 14, 2020 at 10:26 AM Stephane Eranian <eranian@...gle.com> wrote:
>
> On Mon, Sep 14, 2020 at 2:08 AM <peterz@...radead.org> wrote:
> >
> > On Sun, Sep 13, 2020 at 11:41:00PM -0700, Stephane Eranian wrote:
> > > On Sun, Sep 13, 2020 at 2:03 PM Jiri Olsa <jolsa@...nel.org> wrote:
> > > what happens if I set mmap3 and mmap2?
> > >
> > > I think using mmap3 for every mmap may be overkill as you add useless
> > > 20 bytes to an mmap record.
> > > I am not sure if your code handles the case where mmap3 is not needed
> > > because there is no buildid, e.g, anonymous memory.
> > > It seems to me you've written the patch in such a way that if the user
> > > tool supports mmap3, then it supersedes mmap2, and thus
> > > you need all the fields of mmap2. But if could be more interesting to
> > > return either MMAP2 or MMAP3 depending on tool support
> > > and type of mmap, that would certainly save 20 bytes on any anon mmap.
> > > But maybe that logic is already in your patch and I missed it.
> >
> > That, and what if you don't want any of that buildid nonsense at all? I
> > always kill that because it makes perf pointlessly slow and has
> > absolutely no upsides for me.
> >
> I have seen situations where the perf tool takes a visibly significant
> amount of time (many seconds) to inject the buildids at the end of the
> collection
> of perf record (same if using perf inject -b). That is because it
> needs to go through all the records in the perf.data to find MMAP
> records and then read
> the buildids from the filesystem. This has caused some problems in our
> environment. Having the kernel add the buildid to *relevant* mmaps
> would avoid
> a lot of that penalty, by avoiding having to parse the perf.data file
> and leveraging the fact that the buildid may be in memory already.
> Although my concern on
> this has to do with large pages and the impact they have on alignment
> of sections in memory.  I think Ian can comment better on this.

I believe this is a problem we have and that is going away. For
context, we map huge pages and move executable code to them, not from
a file, but using anonymous memory or other sources of huge pages. By
definition we will fail to find build ids for such anonymous memory,
but we may also break the non file backed hugepage case if the
alignment is such that the ELF header is on the hugepage and for some
reason not in the page cache. File backed huge pages solve this
problem.

Thanks,
Ian

> I think this patch series is useful if it can demonstrate a speedup
> during recording (perf record or perf record | perf inject -b). But it
> needs to be
> optimized to minimize the volume of useless info returned. And Jiri
> needs to decide if MMAP3 is a replacement of MMAP2, or a different
> kind of record
> targeted at ELF images only in which case some of the fields may be
> removed. My tendency would be to go for the latter.

Powered by blists - more mailing lists