Re: [PATCH v5 11/16] perf report: properly handle branch count in match_chain

[Arnaldo Carvalho de Melo]

Em Sat, Oct 14, 2017 at 09:30:54PM +0200, Milian Wolff escreveu:
> On Freitag, 13. Oktober 2017 16:08:34 CEST Arnaldo Carvalho de Melo wrote:
> > Em Fri, Oct 13, 2017 at 10:39:03AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Mon, Oct 09, 2017 at 10:33:05PM +0200, Milian Wolff escreveu:
> > > > Some of the code paths I introduced before returned too early
> > > > without running the code to handle a node's branch count.
> > > > By refactoring match_chain to only have one exit point, this
> > > > can be remedied.
> > > 
> > > Fixing up this one now.
> > 
> > Millian, this is all fresher in your mind, can you please take a look at
> > my perf/core branch and check if the change i made to ]PATCH v5 09/16]
> > "perf report: compare symbol name for inlined frames when matching" is
> > ok wrt Ravi's fix and then, please, rebase v5 on top of what is there?
> 
> Regarding the 09/16 patch, I think your change is fine. With your rebase 
> request, do you mean I should rebase the rest of v5 (starting from 11/16, you 
> seem to have applied 10/16 already) and resent that as v6? I can do that, when 
> I get the time.

Yes, can you please do that? As soon as you have the time, if I think it
takes long I'll just move it to a separate branch and continue
processing other patches, just take your time.

Right now I'm processing/testing some perf/urgent bits.

- Arnaldo
 
> > Ravi, please take a look at this as well, to see if with these changes
> > your fix remains valid, ok?
> > 
> > Thanks,
> 
> Thanks for the review and rebase.
> 
> > > > Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > > > Cc: David Ahern <dsahern@xxxxxxxxx>
> > > > Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> > > > Cc: Peter Zijlstra <a.p.zijlstra@xxxxxxxxx>
> > > > Cc: Yao Jin <yao.jin@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Milian Wolff <milian.wolff@xxxxxxxx>
> > > > ---
> > > > 
> > > >  tools/perf/util/callchain.c | 117
> > > >  +++++++++++++++++++++++--------------------- 1 file changed, 60
> > > >  insertions(+), 57 deletions(-)
> > > > 
> > > > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > > > index 3f1431bf71bd..782de047c902 100644
> > > > --- a/tools/perf/util/callchain.c
> > > > +++ b/tools/perf/util/callchain.c
> > > > @@ -666,78 +666,81 @@ static enum match_result match_chain_strings(const
> > > > char *left,> > 
> > > >  	return ret;
> > > >  
> > > >  }
> > > > 
> > > > +static enum match_result match_address(u64 left, u64 right)
> > > > +{
> > > > +	if (left == right)
> > > > +		return MATCH_EQ;
> > > > +	else if (left < right)
> > > > +		return MATCH_LT;
> > > > +	else
> > > > +		return MATCH_GT;
> > > > +}
> > > > +
> > > > 
> > > >  static enum match_result match_chain(struct callchain_cursor_node
> > > >  *node,
> > > >  
> > > >  				     struct callchain_list *cnode)
> > > >  
> > > >  {
> > > > 
> > > > -	struct symbol *sym = node->sym;
> > > > -	enum match_result match;
> > > > -	u64 left, right;
> > > > +	enum match_result match = MATCH_ERROR;
> > > > 
> > > > -	if (callchain_param.key == CCKEY_SRCLINE) {
> > > > +	switch (callchain_param.key) {
> > > > 
> > > > +	case CCKEY_SRCLINE:
> > > >  		match = match_chain_strings(cnode->srcline, node->srcline);
> > > > 
> > > > -
> > > > -		/* if no srcline is available, fallback to symbol name */
> > > > -		if (match == MATCH_ERROR && cnode->ms.sym && node->sym)
> > > > -			match = match_chain_strings(cnode->ms.sym->name,
> > > > -						    node->sym->name);
> > > > -
> > > > 
> > > >  		if (match != MATCH_ERROR)
> > > > 
> > > > -			return match;
> > > > -
> > > > -		/* otherwise fall-back to IP-based comparison below */
> > > > -	}
> > > > -
> > > > -	if (cnode->ms.sym && sym && callchain_param.key == CCKEY_FUNCTION) {
> > > > -		/* compare inlined frames based on their symbol name because
> > > > -		 * different inlined frames will have the same symbol start
> > > > -		 */
> > > > -		if (cnode->ms.sym->inlined || node->sym->inlined)
> > > > -			return match_chain_strings(cnode->ms.sym->name,
> > > > -						   node->sym->name);
> > > > -
> > > > -		left = cnode->ms.sym->start;
> > > > -		right = sym->start;
> > > > -	} else {
> > > > -		left = cnode->ip;
> > > > -		right = node->ip;
> > > > +			break;
> > > > +		__fallthrough;
> > > > +	case CCKEY_FUNCTION:
> > > > +		if (node->sym && cnode->ms.sym) {
> > > > +			/* compare inlined frames based on their symbol name
> > > > +			 * because different inlined frames will have the same
> > > > +			 * symbol start. otherwise do a faster comparison based
> > > > +			 * on the symbol start address
> > > > +			 */
> > > > +			if (cnode->ms.sym->inlined || node->sym->inlined)
> > > > +				match = match_chain_strings(cnode->ms.sym->name,
> > > > +							    node->sym->name);
> > > > +			else
> > > > +				match = match_address(cnode->ms.sym->start,
> > > > +						      node->sym->start);
> > > > +			if (match != MATCH_ERROR)
> > > > +				break;
> > > > +		}
> > > > +		__fallthrough;
> > > > +	case CCKEY_ADDRESS:
> > > > +	default:
> > > > +		match = match_address(cnode->ip, node->ip);
> > > > +		break;
> > > > 
> > > >  	}
> > > > 
> > > > -	if (left == right) {
> > > > -		if (node->branch) {
> > > > -			cnode->branch_count++;
> > > > +	if (match == MATCH_EQ && node->branch) {
> > > > +		cnode->branch_count++;
> > > > 
> > > > -			if (node->branch_from) {
> > > > -				/*
> > > > -				 * It's "to" of a branch
> > > > -				 */
> > > > -				cnode->brtype_stat.branch_to = true;
> > > > +		if (node->branch_from) {
> > > > +			/*
> > > > +			 * It's "to" of a branch
> > > > +			 */
> > > > +			cnode->brtype_stat.branch_to = true;
> > > > 
> > > > -				if (node->branch_flags.predicted)
> > > > -					cnode->predicted_count++;
> > > > +			if (node->branch_flags.predicted)
> > > > +				cnode->predicted_count++;
> > > > 
> > > > -				if (node->branch_flags.abort)
> > > > -					cnode->abort_count++;
> > > > +			if (node->branch_flags.abort)
> > > > +				cnode->abort_count++;
> > > > 
> > > > -				branch_type_count(&cnode->brtype_stat,
> > > > -						  &node->branch_flags,
> > > > -						  node->branch_from,
> > > > -						  node->ip);
> > > > -			} else {
> > > > -				/*
> > > > -				 * It's "from" of a branch
> > > > -				 */
> > > > -				cnode->brtype_stat.branch_to = false;
> > > > -				cnode->cycles_count +=
> > > > -					node->branch_flags.cycles;
> > > > -				cnode->iter_count += node->nr_loop_iter;
> > > > -				cnode->iter_cycles += node->iter_cycles;
> > > > -			}
> > > > +			branch_type_count(&cnode->brtype_stat,
> > > > +					  &node->branch_flags,
> > > > +					  node->branch_from,
> > > > +					  node->ip);
> > > > +		} else {
> > > > +			/*
> > > > +			 * It's "from" of a branch
> > > > +			 */
> > > > +			cnode->brtype_stat.branch_to = false;
> > > > +			cnode->cycles_count += node->branch_flags.cycles;
> > > > +			cnode->iter_count += node->nr_loop_iter;
> > > > +			cnode->iter_cycles += node->iter_cycles;
> > > > 
> > > >  		}
> > > > 
> > > > -
> > > > -		return MATCH_EQ;
> > > > 
> > > >  	}
> > > > 
> > > > -	return left > right ? MATCH_GT : MATCH_LT;
> > > > +	return match;
> > > > 
> > > >  }
> > > >  
> > > >  /*
> 
> 
> -- 
> Milian Wolff | milian.wolff@xxxxxxxx | Senior Software Engineer
> KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
> Tel: +49-30-521325470
> KDAB - The Qt Experts
>