From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752322AbaAOO1n (ORCPT ); Wed, 15 Jan 2014 09:27:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35919 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752297AbaAOO1k (ORCPT ); Wed, 15 Jan 2014 09:27:40 -0500 Date: Wed, 15 Jan 2014 09:27:27 -0500 From: Don Zickus To: Gaurav Jain Cc: "linux-kernel@vger.kernel.org" , Ingo Molnar , Jiri Olsa , Paul Mackerras , Peter Zijlstra , Arun Sharma Subject: Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap Message-ID: <20140115142727.GJ25953@redhat.com> References: <20140113165441.GB25953@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 14, 2014 at 08:48:23PM +0000, Gaurav Jain wrote: > On 1/13/14, 11:54 AM, "Don Zickus" 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. Thanks for the improved problem description. I see it led to a better patch. :-) That is why it is generally a good idea to describe the problem you are trying to solve to see if others have a better solution. Cheers, Don > > >Also style issue below.. > > > >> > >> Signed-off-by: Gaurav Jain > >> Cc: Ingo Molnar > >> Cc: Jiri Olsa > >> Cc: Paul Mackerras > >> Cc: Peter Zijlstra > >> Cc: Don Zickus > >> Cc: Arun Sharma > >> --- > >> 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 >