Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Date:   Fri, 27 Sep 2019 12:35:40 -0300
To:     Steve MacLean <Steve.MacLean@microsoft.com>
Cc:     Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Changbin Du <changbin.du@intel.com>,
        Eric Saint-Etienne <eric.saint.etienne@oracle.com>,
        John Keeping <john@metanate.com>,
        Andi Kleen <ak@linux.intel.com>,
        Song Liu <songliubraving@fb.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Leo Yan <leo.yan@linaro.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Brian Robbins <brianrob@microsoft.com>,
        Tom McDonald <Thomas.McDonald@microsoft.com>,
        John Salem <josalem@microsoft.com>
Subject: Re: [PATCH] perf map: fix overlapped map handling
Message-ID: <20190927153540.GB20644@kernel.org>
References: <BN8PR21MB136261C1A4BB2C884F10FCECF7880@BN8PR21MB1362.namprd21.prod.outlook.com>
 <20190920193852.GI4865@kernel.org>
 <BN8PR21MB1362B1921DF8ABF3A19B43A5F7880@BN8PR21MB1362.namprd21.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BN8PR21MB1362B1921DF8ABF3A19B43A5F7880@BN8PR21MB1362.namprd21.prod.outlook.com>
User-Agent: Mutt/1.12.1 (2019-06-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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