RE: [EXTERNAL] Re: [PATCH v4] perf inject --jit: Remove //anon mmap events
>>> Hi Ian, >>> I tried this as well with latest perf/core. The difference is that >>> unresolved addresses currently look like: >>> >>> 0.00% java [JIT] tid 221782 [.] 0x451499a4 >>> 0.00% java [JIT] tid 221782 [.] 0x4514f3e8 >>> 0.00% java [JIT] tid 221782 [.] 0x45149394 >>> >>> But after Steve's patch this becomes: >>> >>> 0.00% java [unknown] [.] 0x58557d14 >>> 0.00% java [unknown] [.] 0x785c03b4 >>> 0.00% java [unknown] [.] 0x58386520 >>> >>> I couldn't see any events that were symbolised before but are no >>> longer symbolised after this patch. >> >> I see this, thanks for digging into the explanation! Were you able to >> get a test case where the unknowns went down? For example, by forcing >> the code cache size to be small? This is the result I'd expect to see. > >I tried the same Dacapo benchmark as you with different values of >InitialCodeCacheSize and grepped for -e '\[unknown\]' -e '\[JIT\]'. > > Base Patched > 100M 338373 > 50M 333315 > 25M 323368 > 15M 1238 309 > 10M 2600 333 > 1M6035 337 > >This looks fairly convincing to me: the cliff at 15M is where the code cache >starts needing to be enlarged. > Removing the anonymous mappings causes a small regression. Specifically, the reporting of the module name goes from "[JIT] tid " to "[unknown]". This occurs when the JIT fails to report memory used in jitdump before it is used. However there is also confirmation that JAVA does see the reported issue when using a small code cache. The current patch resolves the issue in this case. I see two options: + Accept the regression. Since this is a regression for a jit dump reporting synchronization error, this may be a reasonable option. + Design a more complicated patch. Either + Only strip parts of // anon mmap events overlapping existing jitted--.so mmap events. + Only strip parts of // anon mmap events overlapping prior // anon mmap events Any opinions?
RE: [EXTERNAL] Re: [PATCH v4] perf inject --jit: Remove //anon mmap events
>/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 Its also possible the `InitialCodeCacheSize=20M` argument is masking the problem. Something like 4K would make the problem much easier to see. I see the problem every time .NET grows the cache across a 64K page boundary. 20M may mean you are allocating huge pages or something which might mask the problem. We may be allocating code pages 64K at a time.
RE: [EXTERNAL] Re: [PATCH v4] perf inject --jit: Remove //anon mmap events
> 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? That is certainly disturbing. I don't see an obvious user error, but I haven't played with java jitdump Couple of ideas come to mind. + When I was drafting .NET jitdump support, I looked briefly at the java agent. My impression was the java agent support wasn't thread safe. The jitdump file format requires that certain records are collocated to simplify the reader. I wasn't sure how that was being accomplished by the writer if JAVA's JIT was multithreaded (like CoreCLR JIT is). + The way I implemented jitdump on CoreCLR was to hook into the existing system to write perf-map files. So I am pretty confident there is a 1:1 correspondence of perf-map records with jitdump records. Is it possible that Java choose to only emit interesting jitdump records? Perhaps skipping thunks/stubs/trampolines? + There are still some `unknown` records in CoreCLR, but I don't believe there is an increase. I am pretty sure some of our stubs are not being generated. They were present in our before case too. + Your methodology would be more repeatable if you record once, and analyze multiple times. record, report, inject / report, inject / report + I was focusing on eliminating duplicate symbols for the same address. So having the module name in the report allowed me to see if the symbol was from jitdump or from perf-map. In the before case I could see duplicate symbols for the same address. This was how the problem was first observed. + I would be more interested in looking at the diff of the reports. Possibly sorted by address. Probably with zero context. If I were to guess, I would think Java choose to optimize jitdump and only record interesting code. So if we need to support that scenario the approach of this patch wouldn't work. We would need to use a different approach. Ideas... + Keep anon mappings from overwriting jitdump mappings. Introduce a weak add map method, that would only fill empty/anon. + Move the anon mapping timestamp to the beginning of time, so they are processed first + Fix the kernel perf map events + Keep the anon mappings in a separate fall back map I kind of like the add weak map approach.
RE: [EXTERNAL] Re: [PATCH v4] perf inject --jit: Remove //anon mmap events
>> ** Implemented solution >> >> This patch solves the issue by removing // anon mmap events for any >> process which has a valid jit-.dump file. >> >> It tracks on a per process basis to handle the case where some running >> apps support jit-.dump, but some only support perf-.map. >> >> It adds new assumptions: >> * // anon mmap events are only required for perf-.map support. >> * An app that uses jit-.dump, no longer needs perf-.map >> support. It assumes that any perf-.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
[PATCH v4] perf inject --jit: Remove //anon mmap events
From: Steve MacLean
**perf-.map and jit-.dump designs:
When a JIT generates code to be executed, it must allocate memory and
mark it executable using an mmap call.
*** perf-.map design
The perf-.map assumes that any sample recorded in an anonymous
memory page is JIT code. It then tries to resolve the symbol name by
looking at the process' perf-.map.
*** jit-.dump design
The jit-.dump mechanism takes a different approach. It requires a JIT
to write a `/jit-.dump` file. This file must also be mmapped
so that perf inject -jit can find the file. The JIT must also add
JIT_CODE_LOAD records for any functions it generates. The records are
timestamped using a clock which can be correlated to the perf record
clock.
After perf record, the `perf inject -jit` pass parses the recording
looking for a `/jit-.dump` file. When it finds the file, it
parses it and for each JIT_CODE_LOAD record:
* creates an elf file `/jitted--.so
* injects a new mmap record mapping the new elf file into the process.
*** Coexistence design
The kernel and perf support both of these mechanisms. We need to make
sure perf works on an app supporting either or both of these mechanisms.
Both designs rely on mmap records to determine how to resolve an ip
address.
The mmap records of both techniques by definition overlap. When the JIT
compiles a method, it must:
* allocate memory (mmap)
* add execution privilege (mprotect or mmap. either will
generate an mmap event form the kernel to perf)
* compile code into memory
* add a function record to perf-.map and/or jit-.dump
Because the jit-.dump mechanism supports greater capabilities, perf
prefers the symbols from jit-.dump. It implements this based on
timestamp ordering of events. There is an implicit ASSUMPTION that the
JIT_CODE_LOAD record timestamp will be after the // anon mmap event that
was generated during memory allocation or adding the execution privilege
setting.
*** Problems with the ASSUMPTION
The ASSUMPTION made in the Coexistence design section above is violated
in the following scenario.
*** Scenario
While a JIT is jitting code it will eventually need to commit more
pages and change these pages to executable permissions. Typically the
JIT will want these collocated to minimize branch displacements.
The kernel will coalesce these anonymous mapping with identical
permissions before sending an MMAP event for the new pages. The address
range of the new mmap will not be just the most recently mmap pages.
It will include the entire coalesced mmap region.
See mm/mmap.c
unsigned long mmap_region(struct file *file, unsigned long addr,
unsigned long len, vm_flags_t vm_flags, unsigned long pgoff,
struct list_head *uf)
{
...
/*
* Can we just expand an old mapping?
*/
...
perf_event_mmap(vma);
...
}
*** Symptoms
The coalesced // anon mmap event will be timestamped after the
JIT_CODE_LOAD records. This means it will be used as the most recent
mapping for that entire address range. For remaining events it will look at the
inferior perf-.map for symbols.
If both mechanisms are supported, the symbol will appear twice with
different module names. This causes weird behavior in reporting.
If only jit-.dump is supported, the symbol will no longer be resolved.
** Implemented solution
This patch solves the issue by removing // anon mmap events for any
process which has a valid jit-.dump file.
It tracks on a per process basis to handle the case where some running
apps support jit-.dump, but some only support perf-.map.
It adds new assumptions:
* // anon mmap events are only required for perf-.map support.
* An app that uses jit-.dump, no longer needs
perf-.map support. It assumes that any perf-.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.
** Committer testing:
// jitdump case
perf record
perf inject --jit --input perf.data --output perfjit.data
// verify mmap "//anon" events present initially
perf script --input perf.data --show-mmap-events | grep '//anon'
// verify mmap "//anon" events removed
perf script --input perfjit.data --show-mmap-events | grep '//anon'
// no jitdump case
perf record
perf inject --jit --input perf.data --output perfjit.data
// verify mmap "//anon" events present initially
perf script --input perf.data --show-mmap-events | grep '//anon'
// verify mmap "//anon" events not removed
perf script --input perfjit.data --show-mmap-events | grep '//anon'
** Repro:
This issue was discovered while testing the initial CoreCLR jitdump
implementation. https://github.com/dotnet/coreclr/pull/26897.
** Alternate solutions considered
These were also briefly considered
* Change kernel to not coalesce mmap regions.
* Change kernel reporting of coalesced mmap region
[PATCH RESEND] perf inject --jit: Remove //anon mmap events
From: Steve MacLean
While a JIT is jitting code it will eventually need to commit more pages and
change these pages to executable permissions.
Typically the JIT will want these colocated to minimize branch displacements.
The kernel will coalesce these anonymous mapping with identical permissions
before sending an MMAP event for the new pages. This means the mmap event for
the new pages will include the older pages.
These anonymous mmap events will obscure the jitdump injected pseudo events.
This means that the jitdump generated symbols, machine code, debugging info,
and unwind info will no longer be used.
Observations:
When a process emits a jit dump marker and a jitdump file, the perf-xxx.map
file represents inferior information which has been superceded by the
jitdump jit-xxx.dump file.
Further the '//anon*' mmap events are only required for the legacy
perf-xxx.map mapping.
Summary:
Add rbtree to track which pids have sucessfully injected a jitdump file.
During "perf inject --jit", discard "//anon*" mmap events for any pid which
has sucessfully processed a jitdump file.
Committer testing:
// jitdump case
perf record
perf inject --jit --input perf.data --output perfjit.data
// verify mmap "//anon" events present initially
perf script --input perf.data --show-mmap-events | grep '//anon'
// verify mmap "//anon" events removed
perf script --input perfjit.data --show-mmap-events | grep '//anon'
// no jitdump case
perf record
perf inject --jit --input perf.data --output perfjit.data
// verify mmap "//anon" events present initially
perf script --input perf.data --show-mmap-events | grep '//anon'
// verify mmap "//anon" events not removed
perf script --input perfjit.data --show-mmap-events | grep '//anon'
Repro:
This issue was discovered while testing the initial CoreCLR jitdump
implementation. https://github.com/dotnet/coreclr/pull/26897.
Cc: Peter Zijlstra
Cc: Ingo Molnar
Cc: Arnaldo Carvalho de Melo
Cc: Mark Rutland
Cc: Alexander Shishkin
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Stephane Eranian
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Steve MacLean
---
tools/perf/builtin-inject.c | 4 +--
tools/perf/util/jitdump.c | 63 +
2 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 372ecb3..0f38862 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -263,7 +263,7 @@ static int perf_event__jit_repipe_mmap(struct perf_tool
*tool,
* if jit marker, then inject jit mmaps and generate ELF images
*/
ret = jit_process(inject->session, >output, machine,
- event->mmap.filename, sample->pid, );
+ event->mmap.filename, event->mmap.pid, );
if (ret < 0)
return ret;
if (ret) {
@@ -301,7 +301,7 @@ static int perf_event__jit_repipe_mmap2(struct perf_tool
*tool,
* if jit marker, then inject jit mmaps and generate ELF images
*/
ret = jit_process(inject->session, >output, machine,
- event->mmap2.filename, sample->pid, );
+ event->mmap2.filename, event->mmap2.pid, );
if (ret < 0)
return ret;
if (ret) {
diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index e3ccb0c..6d891d1 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -749,6 +749,59 @@ static int jit_repipe_debug_info(struct jit_buf_desc *jd,
union jr_entry *jr)
return 0;
}
+struct pid_rbtree
+{
+ struct rb_node node;
+ pid_t pid;
+};
+
+static void jit_add_pid(struct rb_root *root, pid_t pid)
+{
+ struct rb_node **new = &(root->rb_node), *parent = NULL;
+ struct pid_rbtree* data = NULL;
+
+ /* Figure out where to put new node */
+ while (*new) {
+ struct pid_rbtree *this = container_of(*new, struct pid_rbtree,
node);
+ pid_t nodePid = this->pid;
+
+ parent = *new;
+ if (pid < nodePid)
+ new = &((*new)->rb_left);
+ else if (pid > nodePid)
+ new = &((*new)->rb_right);
+ else
+ return;
+ }
+
+ data = malloc(sizeof(struct pid_rbtree));
+ data->pid = pid;
+
+ /* Add new node and rebalance tree. */
+ rb_link_node(>node, parent, new);
+ rb_insert_color(>node, root);
+
+ return;
+}
+
+static bool jit_has_pid(struct rb_root *root, pid_t pid)
+{
+ struct rb_node *node = root->rb_node;
+
+ while (node) {
+ struct pid_rbtree *this = container_of(node, struct pid_rbtree,
node);
+ pid_t nodePid = this->pid;
+
+ if
[tip: perf/urgent] perf docs: Correct and clarify jitdump spec
The following commit has been merged into the perf/urgent branch of tip: Commit-ID: 2657983b4c0d81632c6a73bae469951b0d341251 Gitweb: https://git.kernel.org/tip/2657983b4c0d81632c6a73bae469951b0d341251 Author:Steve MacLean AuthorDate:Sat, 28 Sep 2019 01:53:08 Committer: Arnaldo Carvalho de Melo CommitterDate: Mon, 30 Sep 2019 17:29:51 -03:00 perf docs: Correct and clarify jitdump spec Specification claims latest version of jitdump file format is 2. Current jit dump reading code treats 1 as the latest version. Correct spec to match code. The original language made it unclear the value to be written in the magic field. Revise language that the writer always writes the same value. Specify that the reader uses the value to detect endian mismatches. Signed-off-by: Steve MacLean Acked-by: Stephane Eranian Cc: Alexander Shishkin Cc: Andi Kleen Cc: Brian Robbins Cc: Davidlohr Bueso Cc: Eric Saint-Etienne Cc: Jiri Olsa Cc: John Keeping Cc: John Salem Cc: Leo Yan Cc: Mark Rutland Cc: Namhyung Kim Cc: Peter Zijlstra Cc: Song Liu Cc: Tom McDonald Link: http://lore.kernel.org/lkml/bn8pr21mb1362f63cde7ac69736fc7f9ef7...@bn8pr21mb1362.namprd21.prod.outlook.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/Documentation/jitdump-specification.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/perf/Documentation/jitdump-specification.txt b/tools/perf/Documentation/jitdump-specification.txt index 4c62b07..52152d1 100644 --- a/tools/perf/Documentation/jitdump-specification.txt +++ b/tools/perf/Documentation/jitdump-specification.txt @@ -36,8 +36,8 @@ III/ Jitdump file header format Each jitdump file starts with a fixed size header containing the following fields in order: -* uint32_t magic : a magic number tagging the file type. The value is 4-byte long and represents the string "JiTD" in ASCII form. It is 0x4A695444 or 0x4454694a depending on the endianness. The field can be used to detect the endianness of the file -* uint32_t version : a 4-byte value representing the format version. It is currently set to 2 +* uint32_t magic : a magic number tagging the file type. The value is 4-byte long and represents the string "JiTD" in ASCII form. It written is as 0x4A695444. The reader will detect an endian mismatch when it reads 0x4454694a. +* uint32_t version : a 4-byte value representing the format version. It is currently set to 1 * uint32_t total_size: size in bytes of file header * uint32_t elf_mach : ELF architecture encoding (ELF e_machine value as specified in /usr/include/elf.h) * uint32_t pad1 : padding. Reserved for future use
[tip: perf/urgent] perf map: Fix overlapped map handling
The following commit has been merged into the perf/urgent branch of tip: Commit-ID: ee212d6ea20887c0ef352be8563ca13dbf965906 Gitweb: https://git.kernel.org/tip/ee212d6ea20887c0ef352be8563ca13dbf965906 Author:Steve MacLean AuthorDate:Sat, 28 Sep 2019 01:39:00 Committer: Arnaldo Carvalho de Melo CommitterDate: Mon, 30 Sep 2019 17:29:46 -03:00 perf map: Fix overlapped map handling Whenever an mmap/mmap2 event occurs, the map tree must be updated to add a new entry. If a new map overlaps a previous map, the overlapped section of the previous map is effectively unmapped, but the non-overlapping sections are still valid. maps__fixup_overlappings() is responsible for creating any new map entries from the previously overlapped map. It optionally creates a before and an after map. When creating the after map the existing code failed to adjust the map.pgoff. This meant the new after map would incorrectly calculate the file offset for the ip. This results in incorrect symbol name resolution for any ip in the after region. Make maps__fixup_overlappings() correctly populate map.pgoff. Add an assert that new mapping matches old mapping at the beginning of the after map. Committer-testing: Validated correct parsing of libcoreclr.so symbols from .NET Core 3.0 preview9 (which didn't strip symbols). Preparation: ~/dotnet3.0-preview9/dotnet new webapi -o perfSymbol cd perfSymbol ~/dotnet3.0-preview9/dotnet publish perf record ~/dotnet3.0-preview9/dotnet \ bin/Debug/netcoreapp3.0/publish/perfSymbol.dll ^C Before: perf script --show-mmap-events 2>&1 | grep -e MMAP -e unknown |\ grep libcoreclr.so | head -n 4 dotnet 1907 373352.698780: PERF_RECORD_MMAP2 1907/1907: \ [0x7fe615726000(0x768000) @ 0 08:02 5510620 765057155]: \ r-xp .../3.0.0-preview9-19423-09/libcoreclr.so dotnet 1907 373352.701091: PERF_RECORD_MMAP2 1907/1907: \ [0x7fe615974000(0x1000) @ 0x24e000 08:02 5510620 765057155]: \ rwxp .../3.0.0-preview9-19423-09/libcoreclr.so dotnet 1907 373352.701241: PERF_RECORD_MMAP2 1907/1907: \ [0x7fe615c42000(0x1000) @ 0x51c000 08:02 5510620 765057155]: \ rwxp .../3.0.0-preview9-19423-09/libcoreclr.so dotnet 1907 373352.705249: 25 cpu-clock: \ 7fe6159a1f99 [unknown] \ (.../3.0.0-preview9-19423-09/libcoreclr.so) After: perf script --show-mmap-events 2>&1 | grep -e MMAP -e unknown |\ grep libcoreclr.so | head -n 4 dotnet 1907 373352.698780: PERF_RECORD_MMAP2 1907/1907: \ [0x7fe615726000(0x768000) @ 0 08:02 5510620 765057155]: \ r-xp .../3.0.0-preview9-19423-09/libcoreclr.so dotnet 1907 373352.701091: PERF_RECORD_MMAP2 1907/1907: \ [0x7fe615974000(0x1000) @ 0x24e000 08:02 5510620 765057155]: \ rwxp .../3.0.0-preview9-19423-09/libcoreclr.so dotnet 1907 373352.701241: PERF_RECORD_MMAP2 1907/1907: \ [0x7fe615c42000(0x1000) @ 0x51c000 08:02 5510620 765057155]: \ rwxp .../3.0.0-preview9-19423-09/libcoreclr.so All the [unknown] symbols were resolved. Signed-off-by: Steve MacLean Tested-by: Brian Robbins Acked-by: Jiri Olsa Cc: Alexander Shishkin Cc: Andi Kleen Cc: Davidlohr Bueso Cc: Eric Saint-Etienne Cc: John Keeping Cc: John Salem Cc: Leo Yan Cc: Mark Rutland Cc: Namhyung Kim Cc: Peter Zijlstra Cc: Song Liu Cc: Stephane Eranian Cc: Tom McDonald Link: http://lore.kernel.org/lkml/bn8pr21mb136270949f22a6a02335c238f7...@bn8pr21mb1362.namprd21.prod.outlook.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/map.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c index 5b83ed1..eec9b28 100644 --- a/tools/perf/util/map.c +++ b/tools/perf/util/map.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 #include "symbol.h" +#include #include #include #include @@ -850,6 +851,8 @@ static int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp } after->start = map->end; + after->pgoff += map->end - pos->start; + assert(pos->map_ip(pos, map->end) == after->map_ip(after, map->end)); __map_groups__insert(pos->groups, after); if (verbose >= 2 && !use_browser) map__fprintf(after, fp);
[tip: perf/urgent] perf inject jit: Fix JIT_CODE_MOVE filename
The following commit has been merged into the perf/urgent branch of tip:
Commit-ID: b59711e9b0d22fd47abfa00602fd8c365cdd3ab7
Gitweb:
https://git.kernel.org/tip/b59711e9b0d22fd47abfa00602fd8c365cdd3ab7
Author:Steve MacLean
AuthorDate:Sat, 28 Sep 2019 01:41:18
Committer: Arnaldo Carvalho de Melo
CommitterDate: Mon, 30 Sep 2019 17:29:49 -03:00
perf inject jit: Fix JIT_CODE_MOVE filename
During perf inject --jit, JIT_CODE_MOVE records were injecting MMAP records
with an incorrect filename. Specifically it was missing the ".so" suffix.
Further the JIT_CODE_LOAD record were silently truncating the
jr->load.code_index field to 32 bits before generating the filename.
Make both records emit the same filename based on the full 64 bit
code_index field.
Fixes: 9b07e27f88b9 ("perf inject: Add jitdump mmap injection support")
Cc: sta...@vger.kernel.org # v4.6+
Signed-off-by: Steve MacLean
Acked-by: Jiri Olsa
Cc: Alexander Shishkin
Cc: Andi Kleen
Cc: Brian Robbins
Cc: Davidlohr Bueso
Cc: Eric Saint-Etienne
Cc: John Keeping
Cc: John Salem
Cc: Leo Yan
Cc: Mark Rutland
Cc: Namhyung Kim
Cc: Peter Zijlstra
Cc: Song Liu
Cc: Stephane Eranian
Cc: Tom McDonald
Link:
http://lore.kernel.org/lkml/bn8pr21mb1362ff8f127b31dbf4121528f7...@bn8pr21mb1362.namprd21.prod.outlook.com
Signed-off-by: Arnaldo Carvalho de Melo
---
tools/perf/util/jitdump.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index 1bdf4c6..e3ccb0c 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -395,7 +395,7 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd,
union jr_entry *jr)
size_t size;
u16 idr_size;
const char *sym;
- uint32_t count;
+ uint64_t count;
int ret, csize, usize;
pid_t pid, tid;
struct {
@@ -418,7 +418,7 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd,
union jr_entry *jr)
return -1;
filename = event->mmap2.filename;
- size = snprintf(filename, PATH_MAX, "%s/jitted-%d-%u.so",
+ size = snprintf(filename, PATH_MAX, "%s/jitted-%d-%" PRIu64 ".so",
jd->dir,
pid,
count);
@@ -529,7 +529,7 @@ static int jit_repipe_code_move(struct jit_buf_desc *jd,
union jr_entry *jr)
return -1;
filename = event->mmap2.filename;
- size = snprintf(filename, PATH_MAX, "%s/jitted-%d-%"PRIu64,
+ size = snprintf(filename, PATH_MAX, "%s/jitted-%d-%" PRIu64 ".so",
jd->dir,
pid,
jr->move.code_index);
[PATCH 3/4] perf inject --jit: Remove //anon mmap events
From: Steve MacLean
While a JIT is jitting code it will eventually need to commit more pages and
change these pages to executable permissions.
Typically the JIT will want these colocated to minimize branch displacements.
The kernel will coalesce these anonymous mapping with identical permissions
before sending an MMAP event for the new pages. This means the mmap event for
the new pages will include the older pages.
These anonymous mmap events will obscure the jitdump injected pseudo events.
This means that the jitdump generated symbols, machine code, debugging info,
and unwind info will no longer be used.
Observations:
When a process emits a jit dump marker and a jitdump file, the perf-xxx.map
file represents inferior information which has been superseded by the
jitdump jit-xxx.dump file.
Further the '//anon*' mmap events are only required for the legacy
perf-xxx.map mapping.
Summary:
Add rbtree to track which pids have successfully injected a jitdump file.
During "perf inject --jit", discard "//anon*" mmap events for any pid which
has successfully processed a jitdump file.
Committer testing:
// jitdump case
perf record
perf inject --jit --input perf.data --output perfjit.data
// verify mmap "//anon" events present initially
perf script --input perf.data --show-mmap-events | grep '//anon'
// verify mmap "//anon" events removed
perf script --input perfjit.data --show-mmap-events | grep '//anon'
// no jitdump case
perf record
perf inject --jit --input perf.data --output perfjit.data
// verify mmap "//anon" events present initially
perf script --input perf.data --show-mmap-events | grep '//anon'
// verify mmap "//anon" events not removed
perf script --input perfjit.data --show-mmap-events | grep '//anon'
Repro:
This issue was discovered while testing the initial CoreCLR jitdump
implementation. https://github.com/dotnet/coreclr/pull/26897.
Cc: Peter Zijlstra
Cc: Ingo Molnar
Cc: Arnaldo Carvalho de Melo
Cc: Mark Rutland
Cc: Alexander Shishkin
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Stephane Eranian
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Steve MacLean
---
tools/perf/builtin-inject.c | 4 +--
tools/perf/util/jitdump.c | 63 +
2 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index c14f40b8..4c921e0 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -261,7 +261,7 @@ static int perf_event__jit_repipe_mmap(struct perf_tool
*tool,
* if jit marker, then inject jit mmaps and generate ELF images
*/
ret = jit_process(inject->session, >output, machine,
- event->mmap.filename, sample->pid, );
+ event->mmap.filename, event->mmap.pid, );
if (ret < 0)
return ret;
if (ret) {
@@ -299,7 +299,7 @@ static int perf_event__jit_repipe_mmap2(struct perf_tool
*tool,
* if jit marker, then inject jit mmaps and generate ELF images
*/
ret = jit_process(inject->session, >output, machine,
- event->mmap2.filename, sample->pid, );
+ event->mmap2.filename, event->mmap2.pid, );
if (ret < 0)
return ret;
if (ret) {
diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index 22d09c4..6a1563f 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -751,6 +751,59 @@ static int jit_repipe_debug_info(struct jit_buf_desc *jd,
union jr_entry *jr)
return 0;
}
+struct pid_rbtree
+{
+ struct rb_node node;
+ pid_t pid;
+};
+
+static void jit_add_pid(struct rb_root *root, pid_t pid)
+{
+ struct rb_node **new = &(root->rb_node), *parent = NULL;
+ struct pid_rbtree* data = NULL;
+
+ /* Figure out where to put new node */
+ while (*new) {
+ struct pid_rbtree *this = container_of(*new, struct pid_rbtree,
node);
+ pid_t nodePid = this->pid;
+
+ parent = *new;
+ if (pid < nodePid)
+ new = &((*new)->rb_left);
+ else if (pid > nodePid)
+ new = &((*new)->rb_right);
+ else
+ return;
+ }
+
+ data = malloc(sizeof(struct pid_rbtree));
+ data->pid = pid;
+
+ /* Add new node and rebalance tree. */
+ rb_link_node(>node, parent, new);
+ rb_insert_color(>node, root);
+
+ return;
+}
+
+static bool jit_has_pid(struct rb_root *root, pid_t pid)
+{
+ struct rb_node *node = root->rb_node;
+
+ while (node) {
+ struct pid_rbtree *this = container_of(node, struct pid_rbtree,
node);
+ pid_t nodePid = this->pid;
+
+ if
RE: [PATCH 3/4 RESEND] perf inject --jit: Remove //anon mmap events
> looks like Andi is right, I'm still getting malformed patch error > the patch has extra characters '=20' and broken lines, like: My apologies, I was trying to get by with an inappropriate mailer. I have an sendmail account now, and will resend.
[PATCH 3/4 RESEND] perf inject --jit: Remove //anon mmap events
While a JIT is jitting code it will eventually need to commit more pages and
change these pages to executable permissions.
Typically the JIT will want these collocated to minimize branch displacements.
The kernel will coalesce these anonymous mapping with identical permissions
before sending an mmap event for the new pages. This means the mmap event for
the new pages will include the older pages.
These anonymous mmap events will obscure the jitdump injected pseudo events.
This means that the jitdump generated symbols, machine code, debugging info,
and unwind info will no longer be used.
Observations:
When a process emits a jit dump marker and a jitdump file, the perf-xxx.map
file represents inferior information which has been superseded by the
jitdump jit-xxx.dump file.
Further the '//anon*' mmap events are only required for the legacy
perf-xxx.map mapping.
Summary:
Add rbtree to track which pids have successfully injected a jitdump file.
During "perf inject --jit", discard "//anon*" mmap events for any pid which
has successfully processed a jitdump file.
Committer testing:
// jitdump case
perf record
perf inject --jit --input perf.data --output perfjit.data
// verify mmap "//anon" events present initially
perf script --input perf.data --show-mmap-events | grep '//anon'
// verify mmap "//anon" events removed
perf script --input perfjit.data --show-mmap-events | grep '//anon'
// no jitdump case
perf record
perf inject --jit --input perf.data --output perfjit.data
// verify mmap "//anon" events present initially
perf script --input perf.data --show-mmap-events | grep '//anon'
// verify mmap "//anon" events not removed
perf script --input perfjit.data --show-mmap-events | grep '//anon'
Repro:
This issue was discovered while testing the initial CoreCLR jitdump
implementation. https://github.com/dotnet/coreclr/pull/26897.
Cc: Peter Zijlstra
Cc: Ingo Molnar
Cc: Arnaldo Carvalho de Melo
Cc: Mark Rutland
Cc: Alexander Shishkin
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Stephane Eranian
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Steve MacLean
---
tools/perf/builtin-inject.c | 4 +--
tools/perf/util/jitdump.c | 63 +
2 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index c14f40b8..4c921e0 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -261,7 +261,7 @@ static int perf_event__jit_repipe_mmap(struct perf_tool
*tool,
* if jit marker, then inject jit mmaps and generate ELF images
*/
ret = jit_process(inject->session, >output, machine,
- event->mmap.filename, sample->pid, );
+ event->mmap.filename, event->mmap.pid, );
if (ret < 0)
return ret;
if (ret) {
@@ -299,7 +299,7 @@ static int perf_event__jit_repipe_mmap2(struct perf_tool
*tool,
* if jit marker, then inject jit mmaps and generate ELF images
*/
ret = jit_process(inject->session, >output, machine,
- event->mmap2.filename, sample->pid, );
+ event->mmap2.filename, event->mmap2.pid, );
if (ret < 0)
return ret;
if (ret) {
diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index 22d09c4..6a1563f 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -751,6 +751,59 @@ jit_detect(char *mmap_name, pid_t pid)
return 0;
}
+struct pid_rbtree
+{
+ struct rb_node node;
+ pid_t pid;
+};
+
+static void jit_add_pid(struct rb_root *root, pid_t pid)
+{
+ struct rb_node **new = &(root->rb_node), *parent = NULL;
+ struct pid_rbtree* data = NULL;
+
+ /* Figure out where to put new node */
+ while (*new) {
+ struct pid_rbtree *this = container_of(*new, struct pid_rbtree,
node);
+ pid_t nodePid = this->pid;
+
+ parent = *new;
+ if (pid < nodePid)
+ new = &((*new)->rb_left);
+ else if (pid > nodePid)
+ new = &((*new)->rb_right);
+ else
+ return;
+ }
+
+ data = malloc(sizeof(struct pid_rbtree));
+ data->pid = pid;
+
+ /* Add new node and rebalance tree. */
+ rb_link_node(>node, parent, new);
+ rb_insert_color(>node, root);
+
+ return;
+}
+
+static bool jit_has_pid(struct rb_root *root, pid_t pid)
+{
+ struct rb_node *node = root->rb_node;
+
+ while (node) {
+ struct pid_rbtree *this = container_of(node, struct pid_rbtree,
node);
+ pid_t nodePid = this->pid;
+
+ if (pid < nodePid)
+ node = node-
RE: [PATCH 3/4] perf inject --jit: Remove //anon mmap events
SNIP > I can't apply this one: > patching file builtin-inject.c > Hunk #1 FAILED at 263. > 1 out of 1 hunk FAILED -- saving rejects to file builtin-inject.c.rej I assume this is because I based my patches on the wrong tip. > patching file util/jitdump.c > patch: malformed patch at line 236: btree, node); This doesn't make sense to me. The patch doesn't try to inject near line 236. There aren't 236 lines in the e-mail Looking at the MAINTAINERS file, it looks like I should have based my changes on: git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git perf/core I'll rebase the patch and resend.
[PATCH 4/4] perf docs: Correct and clarify jitdump spec
Specification claims latest version of jitdump file format is 2. Current jit dump reading code treats 1 as the latest version. Correct spec to match code. The original language made it unclear the value to be written in the magic field. Revise language that the writer always writes the same value. Specify that the reader uses the value to detect endian mismatches. Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Arnaldo Carvalho de Melo Cc: Mark Rutland Cc: Alexander Shishkin Cc: Jiri Olsa Cc: Namhyung Kim Cc: Stephane Eranian Cc: linux-kernel@vger.kernel.org Signed-off-by: Steve MacLean --- tools/perf/Documentation/jitdump-specification.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/perf/Documentation/jitdump-specification.txt b/tools/perf/Documentation/jitdump-specification.txt index 4c62b07..52152d1 100644 --- a/tools/perf/Documentation/jitdump-specification.txt +++ b/tools/perf/Documentation/jitdump-specification.txt @@ -36,8 +36,8 @@ III/ Jitdump file header format Each jitdump file starts with a fixed size header containing the following fields in order: -* uint32_t magic : a magic number tagging the file type. The value is 4-byte long and represents the string "JiTD" in ASCII form. It is 0x4A695444 or 0x4454694a depending on the endianness. The field can be used to detect the endianness of the file -* uint32_t version : a 4-byte value representing the format version. It is currently set to 2 +* uint32_t magic : a magic number tagging the file type. The value is 4-byte long and represents the string "JiTD" in ASCII form. It written is as 0x4A695444. The reader will detect an endian mismatch when it reads 0x4454694a. +* uint32_t version : a 4-byte value representing the format version. It is currently set to 1 * uint32_t total_size: size in bytes of file header * uint32_t elf_mach : ELF architecture encoding (ELF e_machine value as specified in /usr/include/elf.h) * uint32_t pad1 : padding. Reserved for future use -- 2.7.4
[PATCH 3/4] perf inject --jit: Remove //anon mmap events
While a JIT is jitting code it will eventually need to commit more pages and
change these pages to executable permissions.
Typically the JIT will want these co-located to minimize branch displacements.
The kernel will coalesce these anonymous mapping with identical permissions
before sending an mmap event for the new pages. This means the mmap event for
the new pages will include the older pages.
These anonymous mmap events will obscure the jitdump injected pseudo events.
This means that the jitdump generated symbols, machine code, debugging info,
and unwind info will no longer be used.
Observations:
When a process emits a jit dump marker and a jitdump file, the perf-xxx.map
file represents inferior information which has been superseded by the
jitdump jit-xxx.dump file.
Further the '//anon*' mmap events are only required for the legacy
perf-xxx.map mapping.
Summary:
Add rbtree to track which pids have successfully injected a jitdump file.
During "perf inject --jit", discard "//anon*" mmap events for any pid which
has successfully processed a jitdump file.
Committer testing:
// jitdump case
perf record
perf inject --jit --input perf.data --output perfjit.data
// verify mmap "//anon" events present initially
perf script --input perf.data --show-mmap-events | grep '//anon'
// verify mmap "//anon" events removed
perf script --input perfjit.data --show-mmap-events | grep '//anon'
// no jitdump case
perf record
perf inject --jit --input perf.data --output perfjit.data
// verify mmap "//anon" events present initially
perf script --input perf.data --show-mmap-events | grep '//anon'
// verify mmap "//anon" events not removed
perf script --input perfjit.data --show-mmap-events | grep '//anon'
Repro:
This issue was discovered while testing the initial CoreCLR jitdump
implementation. https://github.com/dotnet/coreclr/pull/26897.
Cc: Peter Zijlstra
Cc: Ingo Molnar
Cc: Arnaldo Carvalho de Melo
Cc: Mark Rutland
Cc: Alexander Shishkin
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Stephane Eranian
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Steve MacLean
---
tools/perf/builtin-inject.c | 4 +--
tools/perf/util/jitdump.c | 63 +
2 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 372ecb3..0f38862 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -263,7 +263,7 @@ static int perf_event__jit_repipe_mmap(struct perf_tool
*tool,
* if jit marker, then inject jit mmaps and generate ELF images
*/
ret = jit_process(inject->session, >output, machine,
- event->mmap.filename, sample->pid, );
+ event->mmap.filename, event->mmap.pid, );
if (ret < 0)
return ret;
if (ret) {
@@ -301,7 +301,7 @@ static int perf_event__jit_repipe_mmap2(struct perf_tool
*tool,
* if jit marker, then inject jit mmaps and generate ELF images
*/
ret = jit_process(inject->session, >output, machine,
- event->mmap2.filename, sample->pid, );
+ event->mmap2.filename, event->mmap2.pid, );
if (ret < 0)
return ret;
if (ret) {
diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index e3ccb0c..6d891d1 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -749,6 +749,59 @@ jit_detect(char *mmap_name, pid_t pid)
return 0;
}
+struct pid_rbtree
+{
+ struct rb_node node;
+ pid_t pid;
+};
+
+static void jit_add_pid(struct rb_root *root, pid_t pid)
+{
+ struct rb_node **new = &(root->rb_node), *parent = NULL;
+ struct pid_rbtree* data = NULL;
+
+ /* Figure out where to put new node */
+ while (*new) {
+ struct pid_rbtree *this = container_of(*new, struct pid_rbtree,
node);
+ pid_t nodePid = this->pid;
+
+ parent = *new;
+ if (pid < nodePid)
+ new = &((*new)->rb_left);
+ else if (pid > nodePid)
+ new = &((*new)->rb_right);
+ else
+ return;
+ }
+
+ data = malloc(sizeof(struct pid_rbtree));
+ data->pid = pid;
+
+ /* Add new node and rebalance tree. */
+ rb_link_node(>node, parent, new);
+ rb_insert_color(>node, root);
+
+ return;
+}
+
+static bool jit_has_pid(struct rb_root *root, pid_t pid)
+{
+ struct rb_node *node = root->rb_node;
+
+ while (node) {
+ struct pid_rbtree *this = container_of(node, struct pid_rbtree,
node);
+ pid_t nodePid = this->pid;
+
+ if (pid < nodePid)
+ node = node-
[PATCH 2/4] perf inject jit: Fix JIT_CODE_MOVE filename
During perf inject --jit, JIT_CODE_MOVE records were injecting MMAP records
with an incorrect filename. Specifically it was missing the ".so" suffix.
Further the JIT_CODE_LOAD record were silently truncating the
jr->load.code_index field to 32 bits before generating the filename.
Make both records emit the same filename based on the full 64 bit
code_index field.
Cc: Peter Zijlstra
Cc: Ingo Molnar
Cc: Arnaldo Carvalho de Melo
Cc: Mark Rutland
Cc: Alexander Shishkin
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Stephane Eranian
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Steve MacLean
---
tools/perf/util/jitdump.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index 1bdf4c6..e3ccb0c 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -395,7 +395,7 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd,
union jr_entry *jr)
size_t size;
u16 idr_size;
const char *sym;
- uint32_t count;
+ uint64_t count;
int ret, csize, usize;
pid_t pid, tid;
struct {
@@ -418,7 +418,7 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd,
union jr_entry *jr)
return -1;
filename = event->mmap2.filename;
- size = snprintf(filename, PATH_MAX, "%s/jitted-%d-%u.so",
+ size = snprintf(filename, PATH_MAX, "%s/jitted-%d-%" PRIu64 ".so",
jd->dir,
pid,
count);
@@ -529,7 +529,7 @@ static int jit_repipe_code_move(struct jit_buf_desc *jd,
union jr_entry *jr)
return -1;
filename = event->mmap2.filename;
- size = snprintf(filename, PATH_MAX, "%s/jitted-%d-%"PRIu64,
+ size = snprintf(filename, PATH_MAX, "%s/jitted-%d-%" PRIu64 ".so",
jd->dir,
pid,
jr->move.code_index);
--
2.7.4
[PATCH 1/4] perf map: fix overlapped map handling
Whenever an mmap/mmap2 event occurs, the map tree must be updated to add a new entry. If a new map overlaps a previous map, the overlapped section of the previous map is effectively unmapped, but the non-overlapping sections are still valid. maps__fixup_overlappings() is responsible for creating any new map entries from the previously overlapped map. It optionally creates a before and an after map. When creating the after map the existing code failed to adjust the map.pgoff. This meant the new after map would incorrectly calculate the file offset for the ip. This results in incorrect symbol name resolution for any ip in the after region. Make maps__fixup_overlappings() correctly populate map.pgoff. Add an assert that new mapping matches old mapping at the beginning of the after map. Committer-testing: Validated correct parsing of libcoreclr.so symbols from .NET Core 3.0 preview9 (which didn't strip symbols). Preparation: ~/dotnet3.0-preview9/dotnet new webapi -o perfSymbol cd perfSymbol ~/dotnet3.0-preview9/dotnet publish perf record ~/dotnet3.0-preview9/dotnet \ bin/Debug/netcoreapp3.0/publish/perfSymbol.dll ^C Before: perf script --show-mmap-events 2>&1 | grep -e MMAP -e unknown |\ grep libcoreclr.so | head -n 4 dotnet 1907 373352.698780: PERF_RECORD_MMAP2 1907/1907: \ [0x7fe615726000(0x768000) @ 0 08:02 5510620 765057155]: \ r-xp .../3.0.0-preview9-19423-09/libcoreclr.so dotnet 1907 373352.701091: PERF_RECORD_MMAP2 1907/1907: \ [0x7fe615974000(0x1000) @ 0x24e000 08:02 5510620 765057155]: \ rwxp .../3.0.0-preview9-19423-09/libcoreclr.so dotnet 1907 373352.701241: PERF_RECORD_MMAP2 1907/1907: \ [0x7fe615c42000(0x1000) @ 0x51c000 08:02 5510620 765057155]: \ rwxp .../3.0.0-preview9-19423-09/libcoreclr.so dotnet 1907 373352.705249: 25 cpu-clock: \ 7fe6159a1f99 [unknown] \ (.../3.0.0-preview9-19423-09/libcoreclr.so) After: perf script --show-mmap-events 2>&1 | grep -e MMAP -e unknown |\ grep libcoreclr.so | head -n 4 dotnet 1907 373352.698780: PERF_RECORD_MMAP2 1907/1907: \ [0x7fe615726000(0x768000) @ 0 08:02 5510620 765057155]: \ r-xp .../3.0.0-preview9-19423-09/libcoreclr.so dotnet 1907 373352.701091: PERF_RECORD_MMAP2 1907/1907: \ [0x7fe615974000(0x1000) @ 0x24e000 08:02 5510620 765057155]: \ rwxp .../3.0.0-preview9-19423-09/libcoreclr.so dotnet 1907 373352.701241: PERF_RECORD_MMAP2 1907/1907: \ [0x7fe615c42000(0x1000) @ 0x51c000 08:02 5510620 765057155]: \ rwxp .../3.0.0-preview9-19423-09/libcoreclr.so All the [unknown] symbols were resolved. Tested-by: Brian Robbins Cc: Peter Zijlstra Cc: Ingo Molnar Cc: Arnaldo Carvalho de Melo Cc: Mark Rutland Cc: Alexander Shishkin Cc: Jiri Olsa Cc: Namhyung Kim Cc: Stephane Eranian Cc: linux-kernel@vger.kernel.org Signed-off-by: Steve MacLean --- tools/perf/util/map.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c index 5b83ed1..eec9b28 100644 --- a/tools/perf/util/map.c +++ b/tools/perf/util/map.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 #include "symbol.h" +#include #include #include #include @@ -850,6 +851,8 @@ static int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp } after->start = map->end; + after->pgoff += map->end - pos->start; + assert(pos->map_ip(pos, map->end) == after->map_ip(after, map->end)); __map_groups__insert(pos->groups, after); if (verbose >= 2 && !use_browser) map__fprintf(after, fp); -- 2.7.4
RE: [PATCH] perf map: fix overlapped map handling
>> An earlier version of this patch used: >> after->start = map->end; >> +after->pgoff += map->end - pos->start; >> >> Instead of the newer Functionally equivalent: >> after->start = map->end; >> +after->pgoff = pos->map_ip(pos, map->end); >> >> I preferred the latter form as it made more sense with the assertion that >> the mapping of map->end should match in pos and after. > > So, if they are equivalent then I think its better to use code that > ressembles the kernel as much as possible, so that when in doubt we can > compare the tools/perf calcs with how the kernel does it, filtering out > things like the PAGE_SHIFT, can we go that way? > > Also do you have some reproducer, if you have one then we can try and have > this as a 'perf test' entry, bolting some more checks into > tools/perf/tests/perf-record.c or using it as a start for a test that > stresses this code. > > This is not a prerequisite for having your fix on, but would help checking > that perf doesn't regresses in this area. > > - Arnaldo I have updated the patch to use the earlier version, which more closely matches the kernel. I have updated the commit message to include the repro info. I am including a few other patches I have generated while adding support for perf jitdump to coreclr.
RE: [PATCH] perf map: fix overlapped map handling
>> after->start = map->end;
>> +after->pgoff = pos->map_ip(pos, map->end);
>
> So is this equivalent to what __split_vma() does in the kernel, i.e.:
>
>if (new_below)
>new->vm_end = addr;
>else {
>new->vm_start = addr;
>new->vm_pgoff += ((addr - vma->vm_start) >> PAGE_SHIFT);
>}
>
> where new->vm_pgoff starts equal to the vm_pgoff of the mmap being split?
It is roughly equivalent. The pgoff in struct map is stored in bytes not in
pages, so it doesn't include the shift.
An earlier version of this patch used:
after->start = map->end;
+ after->pgoff += map->end - pos->start;
Instead of the newer Functionally equivalent:
after->start = map->end;
+ after->pgoff = pos->map_ip(pos, map->end);
I preferred the latter form as it made more sense with the assertion that the
mapping of map->end should match in pos and after.
Steve
[PATCH] perf map: fix overlapped map handling
Whenever an mmap/mmap2 event occurs, the map tree must be updated to add a new entry. If a new map overlaps a previous map, the overlapped section of the previous map is effectively unmapped, but the non-overlapping sections are still valid. maps__fixup_overlappings() is responsible for creating any new map entries from the previously overlapped map. It optionally creates a before and an after map. When creating the after map the existing code failed to adjust the map.pgoff. This meant the new after map would incorrectly calculate the file offset for the ip. This results in incorrect symbol name resolution for any ip in the after region. Make maps__fixup_overlappings() correctly populate map.pgoff. Add an assert that new mapping matches old mapping at the beginning of the after map. Signed-off-by: Steve MacLean --- tools/perf/util/map.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c index 5b83ed1..73870d7 100644 --- a/tools/perf/util/map.c +++ b/tools/perf/util/map.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 #include "symbol.h" +#include #include #include #include @@ -850,6 +851,8 @@ static int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp } after->start = map->end; + after->pgoff = pos->map_ip(pos, map->end); + assert(pos->map_ip(pos, map->end) == after->map_ip(after, map->end)); __map_groups__insert(pos->groups, after); if (verbose >= 2 && !use_browser) map__fprintf(after, fp); -- 2.7.4
