linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] perf map: fix overlapped map handling
@ 2019-09-20 19:20 Steve MacLean
  2019-09-20 19:38 ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 5+ messages in thread
From: Steve MacLean @ 2019-09-20 19:20 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Changbin Du, Eric Saint-Etienne, John Keeping, Andi Kleen,
	Song Liu, Davidlohr Bueso, Leo Yan, linux-kernel
  Cc: Brian Robbins, Tom McDonald, John Salem

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 <Steve.MacLean@Microsoft.com>
---
 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 <assert.h>
 #include <errno.h>
 #include <inttypes.h>
 #include <limits.h>
@@ -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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] perf map: fix overlapped map handling
  2019-09-20 19:20 [PATCH] perf map: fix overlapped map handling Steve MacLean
@ 2019-09-20 19:38 ` Arnaldo Carvalho de Melo
  2019-09-20 21:46   ` Steve MacLean
  0 siblings, 1 reply; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-09-20 19:38 UTC (permalink / raw)
  To: Steve MacLean
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Changbin Du, Eric Saint-Etienne,
	John Keeping, Andi Kleen, Song Liu, Davidlohr Bueso, Leo Yan,
	linux-kernel, Brian Robbins, Tom McDonald, John Salem

Em Fri, Sep 20, 2019 at 07:20:18PM +0000, Steve MacLean escreveu:
> 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 <Steve.MacLean@Microsoft.com>
> ---
>  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 <assert.h>
>  #include <errno.h>
>  #include <inttypes.h>
>  #include <limits.h>
> @@ -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);

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?

- Arnaldo

> +			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

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] perf map: fix overlapped map handling
  2019-09-20 19:38 ` Arnaldo Carvalho de Melo
@ 2019-09-20 21:46   ` Steve MacLean
  2019-09-27 15:35     ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 5+ messages in thread
From: Steve MacLean @ 2019-09-20 21:46 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Changbin Du, Eric Saint-Etienne,
	John Keeping, Andi Kleen, Song Liu, Davidlohr Bueso, Leo Yan,
	linux-kernel, Brian Robbins, Tom McDonald, John Salem

>>  			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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] perf map: fix overlapped map handling
  2019-09-20 21:46   ` Steve MacLean
@ 2019-09-27 15:35     ` Arnaldo Carvalho de Melo
  2019-09-28  1:32       ` Steve MacLean
  0 siblings, 1 reply; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2019-09-27 15:35 UTC (permalink / raw)
  To: Steve MacLean
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Changbin Du, Eric Saint-Etienne, John Keeping, Andi Kleen,
	Song Liu, Davidlohr Bueso, Leo Yan, linux-kernel, Brian Robbins,
	Tom McDonald, John Salem

Em Fri, Sep 20, 2019 at 09:46:15PM +0000, Steve MacLean escreveu:
> >>  			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.

Sorry for the delay in continuing with this discussion, I was at
Plumbers in Lisbon and then some vacations, etc. Also I was hoping
someone else would jump here and provide some Reviewed-by tag, etc :-)

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] perf map: fix overlapped map handling
  2019-09-27 15:35     ` Arnaldo Carvalho de Melo
@ 2019-09-28  1:32       ` Steve MacLean
  0 siblings, 0 replies; 5+ messages in thread
From: Steve MacLean @ 2019-09-28  1:32 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Changbin Du, Eric Saint-Etienne,
	John Keeping, Andi Kleen, Song Liu, Davidlohr Bueso, Leo Yan,
	linux-kernel, Brian Robbins, Tom McDonald, John Salem

>> 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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-09-28  1:32 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-20 19:20 [PATCH] perf map: fix overlapped map handling Steve MacLean
2019-09-20 19:38 ` Arnaldo Carvalho de Melo
2019-09-20 21:46   ` Steve MacLean
2019-09-27 15:35     ` Arnaldo Carvalho de Melo
2019-09-28  1:32       ` Steve MacLean

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).