Re: [EXTERNAL] Re: [PATCH v4] perf inject --jit: Remove //anon mmap events

From: Ian Rogers <irogers@google.com>
To: Steve MacLean <Steve.MacLean@microsoft.com>
Cc: Steve MacLean <steve.maclean@linux.microsoft.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
	Stephane Eranian <eranian@google.com>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [EXTERNAL] Re: [PATCH v4] perf inject --jit: Remove //anon mmap events
Date: Thu, 28 May 2020 02:32:55 -0700	[thread overview]
Message-ID: <CAP-5=fV7F4u66388HC-q8btOYWKxtb1gTTi4LK_Besb-zE25Rw@mail.gmail.com> (raw)
In-Reply-To: <CAP-5=fVHo262Lo_Re31wM8Bt0soJ-m51J7MEnwvdTg5P8J6e6A@mail.gmail.com>

On Wed, May 27, 2020 at 1:59 PM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, May 27, 2020 at 12:27 PM Steve MacLean
> <Steve.MacLean@microsoft.com> wrote:
> >
> > >> ** Implemented solution
> > >>
> > >> This patch solves the issue by removing // anon mmap events for any
> > >> process which has a valid jit-<pid>.dump file.
> > >>
> > >> It tracks on a per process basis to handle the case where some running
> > >> apps support jit-<pid>.dump, but some only support perf-<pid>.map.
> > >>
> > >> It adds new assumptions:
> > >> * // anon mmap events are only required for perf-<pid>.map support.
> > >> * An app that uses jit-<pid>.dump, no longer needs perf-<pid>.map
> > >> support. It assumes that any perf-<pid>.map info is inferior.
> > >>
> > >> *** Details
> > >>
> > >> Use thread->priv to store whether a jitdump file has been processed
> > >>
> > >> During "perf inject --jit", discard "//anon*" mmap events for any pid
> > >> which has sucessfully processed a jitdump file.
> > >
> > >
> > > Thanks Steve this is an important fix! As //anon could be for malloc or other uses, should the stripping behavior be behind a flag?
> > >
> > > Ian
> >
> > I hadn't anticipated a need to preserve the //anon mmap events when profiling JIT generated code.
> >
> > As far as I know mmap events are captured by perf only for mapping code to symbols.  File mappings are kept
> > by the change.  Only // anon mappings are stripped.  (Only for processes which emitted jitdump files.)
> > And these are stripped only during the `perf inject --jit` step. I believe the // Anon mapping are only
> > generally useful for mapping JIT code.
> >
> > I suppose if someone was trying to count mmap events it might be confusing, but `perf inject --jit` creates
> > synthetic mmap file events which would also make this scenario confusing.
> >
> > I personally don't see a good reason to add a flag.  I also don't see a simple way either.  Not running `perf inject --jit`
> > would preserve existing behavior w/o jitdump support.  Without stripping the anon events jitdump support is painfully
> > broken....
>
> Agreed that things are broken. In general only executable mappings are
> held onto by perf, so it could be I'm over worrying about //anon
> stripping breaking around memory allocations. We have some other use
> cases for //anon at Google but they aren't impacted by jitdump. We
> have also been trying to migrate jit caches to using memfd_create,
> which has the same problem that this patch fixes for //anon. Fixing
> memfd_create is a separate issue to //anon. I'll try to get a repro
> for Java that demonstrates the problem and then add a Tested-by.
>
> Thanks,
> Ian

So on tip/perf/core with:
1c0cd2dbb993 perf jvmti: Fix jitdump for methods without debug info
3ce17c1e52f4 perf jvmti: remove redundant jitdump line table entries

I've been trying variants of:

Before:
/tmp/perf/perf record -k 1 -e cycles:u -o /tmp/perf.data java
-agentpath:/tmp/perf/libperf-jvmti.so -XX:+PreserveFramePointer
-XX:InitialCodeCacheSize=20M -XX:ReservedCodeCacheSize=1G -jar
dacapo-9.12-bach.jar jython
/tmp/perf/perf inject -i /tmp/perf.data -o /tmp/perf-jit.data -j
/tmp/perf/perf report -i /tmp/perf-jit.data |grep class\ |wc -l
578
/tmp/perf/perf report -i /tmp/perf-jit.data |grep unknown |wc -l
6

After:
/tmp/perf/perf record -k 1 -e cycles:u -o /tmp/perf.data java
-agentpath:/tmp/perf/libperf-jvmti.so -XX:+PreserveFramePointer
-XX:InitialCodeCacheSize=20M -XX:ReservedCodeCacheSize=1G -jar
dacapo-9.12-bach.jar jython
/tmp/perf/perf inject -i /tmp/perf.data -o /tmp/perf-jit.data -j
/tmp/perf/perf report -i /tmp/perf-jit.data |grep class\ |wc -l
589
/tmp/perf/perf report -i /tmp/perf-jit.data |grep unknown |wc -l
60

So maybe the jit cache isn't behaving the way that the patch cures,
the uptick in unknowns appears consistent in my testing though. I
expect user error, is there an obvious explanation I'm missing?

Thanks,
Ian