Messages in this thread | | | From | Gaurav Jain <> | Subject | Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap | Date | Wed, 15 Jan 2014 06:44:39 +0000 |
| |
Hi Namhyung,
On 1/15/14, 12:46 AM, "Namhyung Kim" <namhyung@kernel.org> wrote:
>I'd like to take my ack back - it seems I missed some points.
No worries, looks like the patch wasn’t well thought out.
>On Tue, 14 Jan 2014 20:48:23 +0000, Gaurav Jain wrote: >> On 1/13/14, 11:54 AM, "Don Zickus" <dzickus@redhat.com> wrote: >> >>>On Sat, Jan 11, 2014 at 08:32:14PM -0800, Gaurav Jain wrote: >>>> Anon records usually do not have the 'execname' entry. However if they >>>>are on >>>> the heap, the execname shows up as '[heap]'. The fix considers any >>>>executable >>>> entries in the map that do not have a name or are on the heap as anon >>>>records >>>> and sets the name to '//anon'. >>>> >>>> This fixes JIT profiling for records on the heap. >>> >>>I guess I don't understand the need for this fix. It seems breaking out >>>//anon vs. [heap] would be useful. Your patch is saying otherwise. Can >>>give a description of the problem you are trying to solve? >> >> Thank you for looking at the patch. >> >> We generate a perf map file which includes certain JIT¹ed functions that >> show up as [heap] entries. As a result, I included the executable heap >> entries as anon pages so that it would be handled in >> util/map.c:map__new(). The alternative would be to handle heap entries >>in >> map__new() directly, however I wasn¹t sure if this would break something >> as it seems that heap and stack entries are expected to fail all >> map__find_* functions. Thus I considered executable heap entries as >> //anon, but perhaps there is a better way. > >Hmm.. so the point is that an executable heap mapping should have >/tmp/perf-XXX.map as a file name, right? If so, does something like >below work well for you?
Just gave it a try and it fixed the issue perfectly! Thanks for the help. This looks like a much better solution than treating the heap mapping as an anon record.
Gaurav
>diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c >index 9b9bd719aa19..d52387fe83f1 100644 >--- a/tools/perf/util/map.c >+++ b/tools/perf/util/map.c >@@ -69,7 +69,7 @@ struct map *map__new(struct list_head *dsos__list, u64 >start, u64 len, > map->ino = ino; > map->ino_generation = ino_gen; > >- if (anon) { >+ if (anon || (no_dso && type == MAP__FUNCTION)) { > snprintf(newfilename, sizeof(newfilename), "/tmp/perf-%d.map", pid); > filename = newfilename; > } >@@ -93,7 +93,7 @@ struct map *map__new(struct list_head *dsos__list, u64 >start, u64 len, > * functions still return NULL, and we avoid the > * unnecessary map__load warning. > */ >- if (no_dso) >+ if (no_dso && type != MAP__FUNCTION) > dso__set_loaded(dso, map->type); > } > }
| |