Message-ID: <20141118020852.GG12538@two.firstfloor.org>
Date:	Tue, 18 Nov 2014 03:08:52 +0100
From:	Andi Kleen <andi@...stfloor.org>
To:	Namhyung Kim <namhyung@...nel.org>
Cc:	Andi Kleen <andi@...stfloor.org>, jolsa@...hat.com,
	linux-kernel@...r.kernel.org, acme@...nel.org,
	Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH 1/8] perf, tools: Support handling complete branch
 stacks as histograms

> > I considered this. For this example it doesn't make much difference
> > because the functions are so small.
> >
> > But for anything larger I really need the line numbers to make
> > sense of it. 
> >
> > So I prefer to keep them. I'll look into some easy switch
> > to turn them off though.
> 
> Oh, I'm not just removing line numbers - it also removed duplicates (f1
> and f2).  But having both from/to entries, I'm not sure it's worth tho..

The duplicate removal is only for the LBRs. I think it's a sensible
default there.

What would be nice in the future would be to add some kind
of annotation support to the hist entries, so we could say
"removed N iterations" and display it (and possibly some more
LBR information, like mispredict rate). But that's more
work and definitely would be a new patchkit.

> 
> 
> >> > +		if (sort__has_parent && !*parent &&
> >> > +		    symbol__match_regex(al.sym, &parent_regex))
> >> > +			*parent = al.sym;
> >> > +		else if (have_ignore_callees && root_al &&
> >> > +		  symbol__match_regex(al.sym, &ignore_callees_regex)) {
> >> > +			/* Treat this symbol as the root,
> >> > +			   forgetting its callees. */
> >> > +			*root_al = al;
> >> > +			callchain_cursor_reset(&callchain_cursor);
> >> > +		}
> >> > +		if (!symbol_conf.use_callchain)
> >> > +			return -EINVAL;
> >> 
> >> This check already went away.
> >> 
> >> And, to remove duplicates, I think we need to check last callchain
> >> cursor node wrt the callchain_param.key here.
> >
> > I don't understand the comment. I'm not modifying anything
> > that has been already added to the callchain. Just things
> > to be added in the future. So why would I need to check
> > or change the cursor?
> 
> But didn't you already do it (with ips[first_call]) to remove overlaps
> between LBR and normal callchain?

I added the LBRs, but i didn't add the normal call entries yet.


> 
> 
> >
> >> 
> >> Also, by comparing 'from' address, I'd expect you add the from address
> >> alone but you add both of 'from' and 'to'.  Do we really need to do
> >> that?
> >
> > Adding from and to makes it much clearer to the user what happens,
> > especially with conditional branches, so they can follow the 
> > control flow.
> 
> But it could be confusing too - esp. when it moves from LBR to normal
> callchains?  Hmm.. maybe we can print them bit differently.

Yes that would be nice.

> 
> 
> >
> >
> >> And the first address saved in normal callchain is address of the
> >> function itself so it might be 'to' you need to check if sampled before
> >> any branch in a function.
> >
> > I'm checking against the CALL, not the target.
> 
> Yeah, but I'm afraid that it'd always fail to find a match.

It seems to work as far as I can tell.
> >> > +				err = add_callchain_ip(machine, thread,
> >> > +						       parent, root_al,
> >> > +						       -1, be[i].from);
> >> > +			if (err == -EINVAL)
> >> > +				break;
> >> > +			if (err)
> >> > +				return err;
> >> > +		}
> >> > +		chain_nr -= nr;
> >> 
> >> I'm not sure this line is needed.
> >
> > Without that i could exceed the limit.
> 
> What limit?

The limit of max history entries.

-Andi
-- 
ak@...ux.intel.com -- Speaking for myself only.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists