Message-ID: <2384048.5gd4jAH0Hk@agathebauer>
Date:   Mon, 29 May 2017 20:36:25 +0200
From:   Milian Wolff <milian.wolff@...b.com>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     Linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        kernel-team@....com
Subject: Re: [PATCH 0/7] generate full callchain cursor entries for inlined frames

On Mittwoch, 24. Mai 2017 17:02:37 CEST Namhyung Kim wrote:
> On Wed, May 24, 2017 at 03:42:59PM +0200, Milian Wolff wrote:
> > On Wednesday, May 24, 2017 1:46:04 PM CEST Milian Wolff wrote:
> > > On Monday, May 22, 2017 11:06:43 AM CEST Namhyung Kim wrote:
> > > > Why not making the fake symbol has start addr of the sample IP and
> > > > length of 1.  The histogram sort code also compares the sym->start
> > > > which might confuse the output of the children mode too IMHO.
> > > 
> > > I can try that out, thank you for the suggestion. But I think it can
> > > easily
> > > break in different ways. I.e. when the same inline function gets used at
> > > different IPs, it should actually be considered to be the same function
> > > when we group/merge/aggregate. I updated the `match_chain` function
> > > accordingly, to do a symname / srcline comparison on inlined frames,
> > > instead of relying on the symbol start/end. I think using the IP for
> > > the fake symbols won't be more reliable here, don't you think?
> > > 
> > > In the end, I think we'll always have to special-case inlined fake
> > > symbols
> > > when we aggregate data, since the sym start/end is always going to be
> > > some
> > > arbitrary value that may or may not be what we want it to be. Doing the
> > > explicit comparison on e.g. srcline/symname is always going to be the
> > > most
> > > reliable option, as it also directly results in a proper aggregation
> > > based
> > > on the strings that the user will see in the end.
> > 
> > I haven't yet tried it out, but I think I can come up with a way to break
> > your approach easily. Assume the following pseudo-code:
> > 
> > void tail()
> > {
> > 
> >     instr1; // IP1
> >     instr2; // IP2
> > 
> > }
> > 
> > void mid()
> > {
> > 
> >     tail();
> > 
> > }
> > 
> > void main()
> > {
> > 
> >     mid();
> > 
> > }
> > 
> > Now, assume both `tail` and `mid` get inlined into `main`. If we get one
> > sample each for both IP1 and IP2, we want the following merged structure
> > if we merge based on symbol:
> > 
> > sym  | incl | self
> > main | 2    | 0
> > mid  | 2    | 0
> > tail | 2    | 2
> > 
> > If we would give the inlined fake-symbols a start of the IP, i.e. either
> > IP1 or IP2, then we would end up with this (unexpected) behavior instead:
> > 
> > sym  | incl | self
> > main | 2    | 0
> > mid  | 1    | 0
> > mid  | 1    | 0
> > tail | 1    | 1
> > tail | 1    | 1
> > 
> > The reason is that the fake symbols for the inlined frames would be
> > considered to be different functions since their start/end are not equal.
> > This is "wrong" in my eyes - we really have to do symbol name comparisons
> > for inlined frames, and also include srcline if that is desired.
> 
> That would depend on how we treat inlined function instances.  Each
> instance might be considered as same or not - but I think it'd be
> better treating them as same for simplicity.
> 
> Also currently perf aggregates samples using symbol name, but a new
> sort key might be added to use symbol address later.  Thus it'd be
> better to be prepared for such change.
> 
> > If you think the above is not a valid assessment, I'll try to change my
> > patch series to use the IP + 1 trick you suggest. But I really don't
> > think it's going to work.
> 
> So I agree that we should do symbol name comparison, but I still
> prefer setting fake symbol address to [IP, +1].  That would reduce
> memory space for annotate as well.

Can you expand on this? I can implement this, but without having a way to test 
I don't know whether I'm doing it right or not ;-)

Note that fake symbols cannot be annotated from `perf report` currently. The 
browser just does nothing - no error, nothing. So I'm really unsure how this 
would influence the "memory space for annotate".

Thanks

-- 
Milian Wolff | milian.wolff@...b.com | Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts
Download attachment "smime.p7s" of type "application/pkcs7-signature" (5903 bytes)

Powered by blists - more mailing lists