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:

quoted

Em Mon, Oct 09, 2017 at 10:33:05PM +0200, Milian Wolff escreveu:

quoted

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.

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.

quoted

quoted

Cc: Arnaldo Carvalho de Melo <redacted>
Cc: David Ahern <redacted>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <redacted>
Cc: Yao Jin <redacted>
Signed-off-by: Milian Wolff <redacted>
---

 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@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH&Co KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts