Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
Hi Namhyung, > On Jan 15, 2014, at 8:01 PM, "Namhyung Kim" wrote: > > Hi Gaurav, > >> On Wed, 15 Jan 2014 06:44:39 +, Gaurav Jain wrote: >> Hi Namhyung, >> >> >>> On 1/15/14, 12:46 AM, "Namhyung Kim" wrote: >>> 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)) { > > Hmm.. I think it should check type of anon mapping too (assuming JIT > interface only provides function symbols, no?): > >if ((anon || no_dso) && type == MAP__FUNCTION)) { In that case, shouldn't we filter out all mappings that are not executable as I had intended to do in my original patch? >>>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) > > And it should be simply: > >if (type != MAP__FUNCTION) > > >>>dso__set_loaded(dso, map->type); >>>} >>>} > > I'll update this and send a formal patch soon. Could you please test it with > the new patch then? Sure I'll test the new patch, though please note my previous suggestion. Gaurav-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
Hi Don, On Wed, 15 Jan 2014 09:27:27 -0500, Don Zickus wrote: > On Tue, Jan 14, 2014 at 08:48:23PM +, 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. Yes, thank you very much for pointing it out and helping to resolve this issue! Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
Hi Gaurav, On Wed, 15 Jan 2014 06:44:39 +, Gaurav Jain wrote: > Hi Namhyung, > > > On 1/15/14, 12:46 AM, "Namhyung Kim" wrote: >>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. Thanks for testing! It's good to hear it solved your problem. :) > > 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)) { Hmm.. I think it should check type of anon mapping too (assuming JIT interface only provides function symbols, no?): 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) And it should be simply: if (type != MAP__FUNCTION) >> dso__set_loaded(dso, map->type); >> } >> } I'll update this and send a formal patch soon. Could you please test it with the new patch then? Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
On Tue, Jan 14, 2014 at 08:48:23PM +, 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
On Tue, Jan 14, 2014 at 08:48:23PM +, Gaurav Jain wrote: On 1/13/14, 11:54 AM, Don Zickus dzic...@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. 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 gj...@fb.com Cc: Ingo Molnar mi...@kernel.org Cc: Jiri Olsa jo...@redhat.com Cc: Paul Mackerras pau...@samba.org Cc: Peter Zijlstra a.p.zijls...@chello.nl Cc: Don Zickus dzic...@redhat.com Cc: Arun Sharma asha...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
Hi Gaurav, On Wed, 15 Jan 2014 06:44:39 +, Gaurav Jain wrote: Hi Namhyung, On 1/15/14, 12:46 AM, Namhyung Kim namhy...@kernel.org wrote: 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. Thanks for testing! It's good to hear it solved your problem. :) 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)) { Hmm.. I think it should check type of anon mapping too (assuming JIT interface only provides function symbols, no?): 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) And it should be simply: if (type != MAP__FUNCTION) dso__set_loaded(dso, map-type); } } I'll update this and send a formal patch soon. Could you please test it with the new patch then? Thanks, Namhyung -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
Hi Don, On Wed, 15 Jan 2014 09:27:27 -0500, Don Zickus wrote: On Tue, Jan 14, 2014 at 08:48:23PM +, Gaurav Jain wrote: On 1/13/14, 11:54 AM, Don Zickus dzic...@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. 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. Yes, thank you very much for pointing it out and helping to resolve this issue! Thanks, Namhyung -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
Hi Namhyung, On Jan 15, 2014, at 8:01 PM, Namhyung Kim namhy...@kernel.org wrote: Hi Gaurav, On Wed, 15 Jan 2014 06:44:39 +, Gaurav Jain wrote: Hi Namhyung, On 1/15/14, 12:46 AM, Namhyung Kim namhy...@kernel.org wrote: 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)) { Hmm.. I think it should check type of anon mapping too (assuming JIT interface only provides function symbols, no?): if ((anon || no_dso) type == MAP__FUNCTION)) { In that case, shouldn't we filter out all mappings that are not executable as I had intended to do in my original patch? 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) And it should be simply: if (type != MAP__FUNCTION) dso__set_loaded(dso, map-type); } } I'll update this and send a formal patch soon. Could you please test it with the new patch then? Sure I'll test the new patch, though please note my previous suggestion. Gaurav-- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
Hi Namhyung, On 1/15/14, 12:46 AM, "Namhyung Kim" 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 +, 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. > >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); > } > }
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
Hi Gaurav, I'd like to take my ack back - it seems I missed some points. On Tue, 14 Jan 2014 20:48:23 +, 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. 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? Thanks, Namhyung 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); } } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
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. >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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
On 1/13/14, 11:54 AM, Don Zickus dzic...@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 gj...@fb.com Cc: Ingo Molnar mi...@kernel.org Cc: Jiri Olsa jo...@redhat.com Cc: Paul Mackerras pau...@samba.org Cc: Peter Zijlstra a.p.zijls...@chello.nl Cc: Don Zickus dzic...@redhat.com Cc: Arun Sharma asha...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
Hi Gaurav, I'd like to take my ack back - it seems I missed some points. On Tue, 14 Jan 2014 20:48:23 +, Gaurav Jain wrote: On 1/13/14, 11:54 AM, Don Zickus dzic...@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? Thanks, Namhyung 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); } } -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
Hi Namhyung, On 1/15/14, 12:46 AM, Namhyung Kim namhy...@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 +, Gaurav Jain wrote: On 1/13/14, 11:54 AM, Don Zickus dzic...@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); } }
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
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? 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). Cheers, Don -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
Hi Gaurav, On Sat, 11 Jan 2014 20:32:14 -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. Acked-by: Namhyung Kim Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
Hi Gaurav, On Sat, 11 Jan 2014 20:32:14 -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. Acked-by: Namhyung Kim namhy...@kernel.org Thanks, Namhyung -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf tools: Synthesize anon MMAP records on the heap
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? Also style issue below.. Signed-off-by: Gaurav Jain gj...@fb.com Cc: Ingo Molnar mi...@kernel.org Cc: Jiri Olsa jo...@redhat.com Cc: Paul Mackerras pau...@samba.org Cc: Peter Zijlstra a.p.zijls...@chello.nl Cc: Don Zickus dzic...@redhat.com Cc: Arun Sharma asha...@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). Cheers, Don -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] perf tools: Synthesize anon MMAP records on the heap
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. 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]")) { strcpy(execname, anonstr); + } size = strlen(execname) + 1; memcpy(event->mmap.filename, execname, size); -- 1.8.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] perf tools: Synthesize anon MMAP records on the heap
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. Signed-off-by: Gaurav Jain gj...@fb.com Cc: Ingo Molnar mi...@kernel.org Cc: Jiri Olsa jo...@redhat.com Cc: Paul Mackerras pau...@samba.org Cc: Peter Zijlstra a.p.zijls...@chello.nl Cc: Don Zickus dzic...@redhat.com Cc: Arun Sharma asha...@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])) { strcpy(execname, anonstr); + } size = strlen(execname) + 1; memcpy(event-mmap.filename, execname, size); -- 1.8.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/