Re: [PATCH] perf map: fix overlapped map handling

[Arnaldo Carvalho de Melo]

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