Messages in this thread | | | From | Gaurav Jain <> | Subject | Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap | Date | Tue, 14 Jan 2014 20:48:23 +0000 |
| |
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.
>Also style issue below.. > >> >> Signed-off-by: Gaurav Jain <gjain@fb.com> >> Cc: Ingo Molnar <mingo@kernel.org> >> Cc: Jiri Olsa <jolsa@redhat.com> >> Cc: Paul Mackerras <paulus@samba.org> >> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> >> Cc: Don Zickus <dzickus@redhat.com> >> Cc: Arun Sharma <asharma@fb.com> >> --- >> tools/perf/util/event.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c >> index bb788c1..ae9c55b 100644 >> --- a/tools/perf/util/event.c >> +++ b/tools/perf/util/event.c >> @@ -224,10 +224,9 @@ static int >>perf_event__synthesize_mmap_events(struct perf_tool *tool, >> continue; >> >> event->header.misc |= PERF_RECORD_MISC_MMAP_DATA; >> - } >> - >> - if (!strcmp(execname, "")) >> + } if (!strcmp(execname, "") || !strcmp(execname, "[heap]")) { > >The '} if' part should seperate the 'if' on to its own line (unless you >meant an 'else' in there).
Bah yes, I intended 'else if'. I apologize for that. Does the fact that I filtered anon entries to only those marked as executable break the existing behavior?
Thanks,
Gaurav
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |