perf tools: Check error during collapsing hist entries

PATCH In review Namhyung Kim 2 participants 14 messages Jan 22, 2016 – Jan 25, 2016 perf events

lore

Share & tools

lore

Namhyung Kim

author maintainer

2016-01-22 13:42 UTC #0 lore raw

cover perf tools: Check error during collapsing hist entries

Hi,

This patchset checks error case during the process of collapsing hist entries. It's a preparation of upcoming hierarchy patchset which adds more work in the collapsing path. If there's an error during this stage, it'll stop processing and show warning to user.

Thanks,
Namhyung

Namhyung Kim (7):
perf callchain: Check return value of add_child() perf callchain: Check return value of fill_node() perf callchain: Add enum match_result for match_chain() perf callchain: Check return value of split_add_child() perf callchain: Check return value of append_chain_children() perf hists: Return error from hists__collapse_resort() perf report: Check error during report__collapse_hists()

 tools/perf/builtin-report.c | 14 +++++--
 tools/perf/util/callchain.c | 94 +++++++++++++++++++++++++++++++++------------
 tools/perf/util/hist.c      | 27 ++++++++-----
 tools/perf/util/hist.h      |  4 +-
 4 files changed, 100 insertions(+), 39 deletions(-)

Signature · 1 line

2.6.4

Namhyung Kim

author maintainer

2016-01-22 13:43 UTC #1 lore raw

PATCH 1/7 perf callchain: Check return value of add_child()

The create_child() in add_child() can return NULL in case of memory allocation failure. So check the return value and bail out. The proper error handling will be added later.

Cc Frederic Weisbecker <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/util/callchain.c | 8 ++++++++
 1 file changed, 8 insertions(+)

tools/perf/util/callchain.c +8 -0

index 53c43eb9489e..134d88b33fc1 100644
@@ -453,6 +453,9 @@ add_child(struct callchain_node *parent,
 	struct callchain_node *new;
 
 	new = create_child(parent, false);
+	if (new == NULL)
+		return NULL;
+
 	fill_node(new, cursor);
 
 	new->children_hit = 0;

@@ -524,6 +527,8 @@ split_add_child(struct callchain_node *parent,
 
 		node = callchain_cursor_current(cursor);
 		new = add_child(parent, cursor, period);
+		if (new == NULL)
+			return;
 
 		/*
 		 * This is second child since we moved parent's children

@@ -585,6 +590,9 @@ append_chain_children(struct callchain_node *root,
 	}
 	/* nothing in children, add to the current node */
 	rnode = add_child(root, cursor, period);
+	if (rnode == NULL)
+		return;
+
 	rb_link_node(&rnode->rb_node_in, parent, p);
 	rb_insert_color(&rnode->rb_node_in, &root->rb_root_in);

Signature · 1 line

2.6.4

Namhyung Kim

author maintainer

2016-01-22 13:43 UTC #2 lore raw

PATCH 2/7 perf callchain: Check return value of fill_node()

In reply to Namhyung Kim

Memory allocation in the fill_node() can fail so change its return type to int and check it in add_child() too.

Cc Frederic Weisbecker <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/util/callchain.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

tools/perf/util/callchain.c +13 -3

index 134d88b33fc1..a82ea6f6fc0f 100644
@@ -416,7 +416,7 @@ create_child(struct callchain_node *parent, bool inherit_children)
 /*
  * Fill the node with callchain values
  */
-static void
+static int
 fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
 {
 	struct callchain_cursor_node *cursor_node;

@@ -433,7 +433,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
 		call = zalloc(sizeof(*call));
 		if (!call) {
 			perror("not enough memory for the code path tree");
-			return;
+			return -1;
 		}
 		call->ip = cursor_node->ip;
 		call->ms.sym = cursor_node->sym;

@@ -443,6 +443,7 @@ fill_node(struct callchain_node *node, struct callchain_cursor *cursor)
 		callchain_cursor_advance(cursor);
 		cursor_node = callchain_cursor_current(cursor);
 	}
+	return 0;
 }
 
 static struct callchain_node *

@@ -456,7 +457,16 @@ add_child(struct callchain_node *parent,
 	if (new == NULL)
 		return NULL;
 
-	fill_node(new, cursor);
+	if (fill_node(new, cursor) < 0) {
+		struct callchain_list *call, *tmp;
+
+		list_for_each_entry_safe(call, tmp, &new->val, list) {
+			list_del(&call->list);
+			free(call);
+		}
+		free(new);
+		return NULL;
+	}
 
 	new->children_hit = 0;
 	new->hit = period;

Signature · 1 line

2.6.4

Namhyung Kim

author maintainer

2016-01-22 13:43 UTC #3 lore raw

PATCH 3/7 perf callchain: Add enum match_result for match_chain()

In reply to Namhyung Kim

The append_chain() might return either result of match_chain() or other (error) code. But match_chain() can return any value in s64 type so it's hard to check the error case. Add new enum match_result and make match_chain() return non-negative values only so that we can check the error cases.

Cc Frederic Weisbecker <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/util/callchain.c | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

tools/perf/util/callchain.c +30 ~~-14~~

index a82ea6f6fc0f..7139d438ee6d 100644
@@ -475,16 +475,32 @@ add_child(struct callchain_node *parent,
 	return new;
 }
 
+enum match_result {
+	MATCH_ERROR  = -1,
+	MATCH_EQ,
+	MATCH_LT,
+	MATCH_GT,
+};
+
 static s64 match_chain(struct callchain_cursor_node *node,
 		      struct callchain_list *cnode)
 {
 	struct symbol *sym = node->sym;
+	u64 left, right;
 
 	if (cnode->ms.sym && sym &&
-	    callchain_param.key == CCKEY_FUNCTION)
-		return cnode->ms.sym->start - sym->start;
-	else
-		return cnode->ip - node->ip;
+	    callchain_param.key == CCKEY_FUNCTION) {
+		left = cnode->ms.sym->start;
+		right = sym->start;
+	} else {
+		left = cnode->ip;
+		right = node->ip;
+	}
+
+	if (left == right)
+		return MATCH_EQ;
+
+	return left > right ? MATCH_GT : MATCH_LT;
 }
 
 /*

@@ -562,7 +578,7 @@ split_add_child(struct callchain_node *parent,
 	}
 }
 
-static int
+static enum match_result
 append_chain(struct callchain_node *root,
 	     struct callchain_cursor *cursor,
 	     u64 period);

@@ -583,17 +599,17 @@ append_chain_children(struct callchain_node *root,
 
 	/* lookup in childrens */
 	while (*p) {
-		s64 ret;
+		enum match_result ret;
 
 		parent = *p;
 		rnode = rb_entry(parent, struct callchain_node, rb_node_in);
 
 		/* If at least first entry matches, rely to children */
 		ret = append_chain(rnode, cursor, period);
-		if (ret == 0)
+		if (ret == MATCH_EQ)
 			goto inc_children_hit;
 
-		if (ret < 0)
+		if (ret == MATCH_LT)
 			p = &parent->rb_left;
 		else
 			p = &parent->rb_right;

@@ -611,7 +627,7 @@ inc_children_hit:
 	root->children_count++;
 }
 
-static int
+static enum match_result
 append_chain(struct callchain_node *root,
 	     struct callchain_cursor *cursor,
 	     u64 period)

@@ -620,7 +636,7 @@ append_chain(struct callchain_node *root,
 	u64 start = cursor->pos;
 	bool found = false;
 	u64 matches;
-	int cmp = 0;
+	enum match_result cmp = MATCH_ERROR;
 
 	/*
 	 * Lookup in the current node

@@ -646,7 +662,7 @@ append_chain(struct callchain_node *root,
 
 	/* matches not, relay no the parent */
 	if (!found) {
-		WARN_ONCE(!cmp, "Chain comparison error\n");
+		WARN_ONCE(cmp == MATCH_ERROR, "Chain comparison error\n");
 		return cmp;
 	}

@@ -655,20 +671,20 @@ append_chain(struct callchain_node *root,
 	/* we match only a part of the node. Split it and add the new chain */
 	if (matches < root->val_nr) {
 		split_add_child(root, cursor, cnode, start, matches, period);
-		return 0;
+		return MATCH_EQ;
 	}
 
 	/* we match 100% of the path, increment the hit */
 	if (matches == root->val_nr && cursor->pos == cursor->nr) {
 		root->hit += period;
 		root->count++;
-		return 0;
+		return MATCH_EQ;
 	}
 
 	/* We match the node and still have a part remaining */
 	append_chain_children(root, cursor, period);
 
-	return 0;
+	return MATCH_EQ;
 }
 
 int callchain_append(struct callchain_root *root,

Signature · 1 line

2.6.4

Jiri Olsa

2016-01-23 17:01 UTC #4 lore raw

Re: [PATCH 3/7] perf callchain: Add enum match_result for match_chain()

On Fri, Jan 22, 2016 at 10:41:36PM +0900, Namhyung Kim wrote:

SNIP

Quoted 9 lines

(quoted diff) +1 -1

 	/* lookup in childrens */
 	while (*p) {
-		s64 ret;
+		enum match_result ret;
 
 		parent = *p;
 		rnode = rb_entry(parent, struct callchain_node, rb_node_in);
 
 		/* If at least first entry matches, rely to children */

Review

Quoted 10 lines

(quoted diff) +2 -2

 		ret = append_chain(rnode, cursor, period);
-		if (ret == 0)
+		if (ret == MATCH_EQ)
 			goto inc_children_hit;
 
-		if (ret < 0)
+		if (ret == MATCH_LT)
 			p = &parent->rb_left;
 		else
 			p = &parent->rb_right;

so if we want to use the return values like that you probably missed 2 other places

thanks,
jirka

---

tools/perf/util/callchain.c +2 -2

index 7139d438ee6d..dc08e76aa8d9 100644
@@ -565,7 +565,7 @@ split_add_child(struct callchain_node *parent,
 		cnode = list_first_entry(&first->val, struct callchain_list,
 					 list);
 
-		if (match_chain(node, cnode) < 0)
+		if (match_chain(node, cnode) == MATCH_LT)
 			pp = &p->rb_left;
 		else
 			pp = &p->rb_right;

@@ -652,7 +652,7 @@ append_chain(struct callchain_node *root,
 			break;
 
 		cmp = match_chain(node, cnode);
-		if (cmp)
+		if (cmp != MATCH_EQ)
 			break;
 
 		found = true;

Namhyung Kim

author maintainer

2016-01-24 04:06 UTC #5 lore raw

Re: [PATCH 3/7] perf callchain: Add enum match_result for match_chain()

Hi Jiri,

Quoted 18 lines · Jiri Olsa

On Fri, Jan 22, 2016 at 10:41:36PM +0900, Namhyung Kim wrote:

SNIP

Quoted 14 lines

(quoted diff) +2 -2

 	/* lookup in childrens */
 	while (*p) {
-		s64 ret;
+		enum match_result ret;
 
 		parent = *p;
 		rnode = rb_entry(parent, struct callchain_node, rb_node_in);
 
 		/* If at least first entry matches, rely to children */
 		ret = append_chain(rnode, cursor, period);
-		if (ret == 0)
+		if (ret == MATCH_EQ)
 			goto inc_children_hit;

Quoted 8 lines

Review
Quoted 5 lines
(quoted diff) +1 -1
-		if (ret < 0)
+		if (ret == MATCH_LT)
 			p = &parent->rb_left;
 		else
 			p = &parent->rb_right;
so if we want to use the return values like that you probably missed 2 other places

Right!

Quoted 17 lines

---

tools/perf/util/callchain.c +1 -1

index 7139d438ee6d..dc08e76aa8d9 100644
@@ -565,7 +565,7 @@ split_add_child(struct callchain_node *parent,
 		cnode = list_first_entry(&first->val, struct callchain_list,
 					 list);
 
-		if (match_chain(node, cnode) < 0)
+		if (match_chain(node, cnode) == MATCH_LT)
 			pp = &p->rb_left;
 		else
 			pp = &p->rb_right;

tools/perf/util/callchain.c append_chain(struct callchain_node *root, · line 655

Quoted 5 lines

tools/perf/util/callchain.c +1 -1

@@ -652,7 +652,7 @@ append_chain(struct callchain_node *root,
 			break;
 
 		cmp = match_chain(node, cnode);
-		if (cmp)
+		if (cmp != MATCH_EQ)

This has same effect since I chose 0 for MATCH_EQ intentionally. But yes, it'd be better making it explicit. Will change.

Thanks,
Namhyung

Quoted 3 lines

tools/perf/util/callchain.c +0 -0
 			break;
 
 		found = true;

Namhyung Kim

author maintainer

2016-01-24 05:50 UTC #6 lore raw

PATCH v2 3/7 perf callchain: Add enum match_result for match_chain()

In reply to Jiri Olsa

Cc Frederic Weisbecker <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/util/callchain.c | 52 +++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 18 deletions(-)

tools/perf/util/callchain.c +34 ~~-18~~

index a82ea6f6fc0f..dab2c1f1e86b 100644
@@ -475,16 +475,32 @@ add_child(struct callchain_node *parent,
 	return new;
 }
 
-static s64 match_chain(struct callchain_cursor_node *node,
-		      struct callchain_list *cnode)
+enum match_result {
+	MATCH_ERROR  = -1,
+	MATCH_EQ,
+	MATCH_LT,
+	MATCH_GT,
+};
+
+static enum match_result match_chain(struct callchain_cursor_node *node,
+				     struct callchain_list *cnode)
 {
 	struct symbol *sym = node->sym;
+	u64 left, right;
 
 	if (cnode->ms.sym && sym &&
-	    callchain_param.key == CCKEY_FUNCTION)
-		return cnode->ms.sym->start - sym->start;
-	else
-		return cnode->ip - node->ip;
+	    callchain_param.key == CCKEY_FUNCTION) {
+		left = cnode->ms.sym->start;
+		right = sym->start;
+	} else {
+		left = cnode->ip;
+		right = node->ip;
+	}
+
+	if (left == right)
+		return MATCH_EQ;
+
+	return left > right ? MATCH_GT : MATCH_LT;
 }
 
 /*

@@ -549,7 +565,7 @@ split_add_child(struct callchain_node *parent,
 		cnode = list_first_entry(&first->val, struct callchain_list,
 					 list);
 
-		if (match_chain(node, cnode) < 0)
+		if (match_chain(node, cnode) == MATCH_LT)
 			pp = &p->rb_left;
 		else
 			pp = &p->rb_right;

@@ -562,7 +578,7 @@ split_add_child(struct callchain_node *parent,
 	}
 }
 
-static int
+static enum match_result
 append_chain(struct callchain_node *root,
 	     struct callchain_cursor *cursor,
 	     u64 period);

@@ -583,17 +599,17 @@ append_chain_children(struct callchain_node *root,
 
 	/* lookup in childrens */
 	while (*p) {
-		s64 ret;
+		enum match_result ret;
 
 		parent = *p;
 		rnode = rb_entry(parent, struct callchain_node, rb_node_in);
 
 		/* If at least first entry matches, rely to children */
 		ret = append_chain(rnode, cursor, period);
-		if (ret == 0)
+		if (ret == MATCH_EQ)
 			goto inc_children_hit;
 
-		if (ret < 0)
+		if (ret == MATCH_LT)
 			p = &parent->rb_left;
 		else
 			p = &parent->rb_right;

@@ -611,7 +627,7 @@ inc_children_hit:
 	root->children_count++;
 }
 
-static int
+static enum match_result
 append_chain(struct callchain_node *root,
 	     struct callchain_cursor *cursor,
 	     u64 period)

@@ -620,7 +636,7 @@ append_chain(struct callchain_node *root,
 	u64 start = cursor->pos;
 	bool found = false;
 	u64 matches;
-	int cmp = 0;
+	enum match_result cmp = MATCH_ERROR;
 
 	/*
 	 * Lookup in the current node

@@ -636,7 +652,7 @@ append_chain(struct callchain_node *root,
 			break;
 
 		cmp = match_chain(node, cnode);
-		if (cmp)
+		if (cmp != MATCH_EQ)
 			break;
 
 		found = true;

@@ -646,7 +662,7 @@ append_chain(struct callchain_node *root,
 
 	/* matches not, relay no the parent */
 	if (!found) {
-		WARN_ONCE(!cmp, "Chain comparison error\n");
+		WARN_ONCE(cmp == MATCH_ERROR, "Chain comparison error\n");
 		return cmp;
 	}

@@ -655,20 +671,20 @@ append_chain(struct callchain_node *root,
 	/* we match only a part of the node. Split it and add the new chain */
 	if (matches < root->val_nr) {
 		split_add_child(root, cursor, cnode, start, matches, period);
-		return 0;
+		return MATCH_EQ;
 	}
 
 	/* we match 100% of the path, increment the hit */
 	if (matches == root->val_nr && cursor->pos == cursor->nr) {
 		root->hit += period;
 		root->count++;
-		return 0;
+		return MATCH_EQ;
 	}
 
 	/* We match the node and still have a part remaining */
 	append_chain_children(root, cursor, period);
 
-	return 0;
+	return MATCH_EQ;
 }
 
 int callchain_append(struct callchain_root *root,

Signature · 1 line

2.6.4

Namhyung Kim

author maintainer

2016-01-22 13:43 UTC #7 lore raw

PATCH 4/7 perf callchain: Check return value of split_add_child()

In reply to Namhyung Kim

Now create_child() and add_child() return errors so check and pass it to the caller.

Cc Frederic Weisbecker <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/util/callchain.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

tools/perf/util/callchain.c +9 -3

index 7139d438ee6d..fc980323539e 100644
@@ -508,7 +508,7 @@ static s64 match_chain(struct callchain_cursor_node *node,
  * give a part of its callchain to the created child.
  * Then create another child to host the given callchain of new branch
  */
-static void
+static int
 split_add_child(struct callchain_node *parent,
 		struct callchain_cursor *cursor,
 		struct callchain_list *to_split,

@@ -520,6 +520,8 @@ split_add_child(struct callchain_node *parent,
 
 	/* split */
 	new = create_child(parent, true);
+	if (new == NULL)
+		return -1;
 
 	/* split the callchain and move a part to the new child */
 	old_tail = parent->val.prev;

@@ -554,7 +556,7 @@ split_add_child(struct callchain_node *parent,
 		node = callchain_cursor_current(cursor);
 		new = add_child(parent, cursor, period);
 		if (new == NULL)
-			return;
+			return -1;
 
 		/*
 		 * This is second child since we moved parent's children

@@ -576,6 +578,7 @@ split_add_child(struct callchain_node *parent,
 		parent->hit = period;
 		parent->count = 1;
 	}
+	return 0;
 }
 
 static enum match_result

@@ -670,7 +673,10 @@ append_chain(struct callchain_node *root,
 
 	/* we match only a part of the node. Split it and add the new chain */
 	if (matches < root->val_nr) {
-		split_add_child(root, cursor, cnode, start, matches, period);
+		if (split_add_child(root, cursor, cnode, start, matches,
+				    period) < 0)
+			return MATCH_ERROR;
+
 		return MATCH_EQ;
 	}

Signature · 1 line

2.6.4

Namhyung Kim

author maintainer

2016-01-22 13:43 UTC #8 lore raw

PATCH 5/7 perf callchain: Check return value of append_chain_children()

In reply to Namhyung Kim

Now it can check the error case, so check and pass it to the caller.

Cc Frederic Weisbecker <[email protected]>

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/util/callchain.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

tools/perf/util/callchain.c +12 -6

index fc980323539e..1d97ea6d5d30 100644
@@ -586,7 +586,7 @@ append_chain(struct callchain_node *root,
 	     struct callchain_cursor *cursor,
 	     u64 period);
 
-static void
+static int
 append_chain_children(struct callchain_node *root,
 		      struct callchain_cursor *cursor,
 		      u64 period)

@@ -598,7 +598,7 @@ append_chain_children(struct callchain_node *root,
 
 	node = callchain_cursor_current(cursor);
 	if (!node)
-		return;
+		return -1;
 
 	/* lookup in childrens */
 	while (*p) {

@@ -611,6 +611,8 @@ append_chain_children(struct callchain_node *root,
 		ret = append_chain(rnode, cursor, period);
 		if (ret == MATCH_EQ)
 			goto inc_children_hit;
+		if (ret == MATCH_ERROR)
+			return -1;
 
 		if (ret == MATCH_LT)
 			p = &parent->rb_left;

@@ -620,7 +622,7 @@ append_chain_children(struct callchain_node *root,
 	/* nothing in children, add to the current node */
 	rnode = add_child(root, cursor, period);
 	if (rnode == NULL)
-		return;
+		return -1;
 
 	rb_link_node(&rnode->rb_node_in, parent, p);
 	rb_insert_color(&rnode->rb_node_in, &root->rb_root_in);

@@ -628,6 +630,7 @@ append_chain_children(struct callchain_node *root,
 inc_children_hit:
 	root->children_hit += period;
 	root->children_count++;
+	return 0;
 }
 
 static enum match_result

@@ -688,7 +691,8 @@ append_chain(struct callchain_node *root,
 	}
 
 	/* We match the node and still have a part remaining */
-	append_chain_children(root, cursor, period);
+	if (append_chain_children(root, cursor, period) < 0)
+		return MATCH_ERROR;
 
 	return MATCH_EQ;
 }

@@ -702,7 +706,8 @@ int callchain_append(struct callchain_root *root,
 
 	callchain_cursor_commit(cursor);
 
-	append_chain_children(&root->node, cursor, period);
+	if (append_chain_children(&root->node, cursor, period) < 0)
+		return -1;
 
 	if (cursor->nr > root->max_depth)
 		root->max_depth = cursor->nr;

@@ -730,7 +735,8 @@ merge_chain_branch(struct callchain_cursor *cursor,
 
 	if (src->hit) {
 		callchain_cursor_commit(cursor);
-		append_chain_children(dst, cursor, src->hit);
+		if (append_chain_children(dst, cursor, src->hit) < 0)
+			return -1;
 	}
 
 	n = rb_first(&src->rb_root_in);

Signature · 1 line

2.6.4

Namhyung Kim

author maintainer

2016-01-22 13:44 UTC #9 lore raw

PATCH 6/7 perf hists: Return error from hists__collapse_resort()

In reply to Namhyung Kim

Currently hists__collapse_resort() and hists__collapse_insert_entry() don't return error code. Now callchain_merge() can check error case, abort and pass the error to the user. Later patch can add more work which can be failed too.

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/util/hist.c | 27 +++++++++++++++++----------
 tools/perf/util/hist.h |  4 ++--
 2 files changed, 19 insertions(+), 12 deletions(-)

tools/perf/util/hist.c +17 ~~-10~~

index 81ce0aff69d1..89fde7feb88b 100644
@@ -1009,8 +1009,8 @@ void hist_entry__delete(struct hist_entry *he)
  * collapse the histogram
  */
 
-bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
-				  struct rb_root *root, struct hist_entry *he)
+int hists__collapse_insert_entry(struct hists *hists, struct rb_root *root,
+				 struct hist_entry *he)
 {
 	struct rb_node **p = &root->rb_node;
 	struct rb_node *parent = NULL;

@@ -1030,12 +1030,13 @@ bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
 
 			if (symbol_conf.use_callchain) {
 				callchain_cursor_reset(&callchain_cursor);
-				callchain_merge(&callchain_cursor,
-						iter->callchain,
-						he->callchain);
+				if (callchain_merge(&callchain_cursor,
+						    iter->callchain,
+						    he->callchain) < 0)
+					return -1;
 			}
 			hist_entry__delete(he);
-			return false;
+			return 0;
 		}
 
 		if (cmp < 0)

@@ -1047,7 +1048,7 @@ bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
 
 	rb_link_node(&he->rb_node_in, parent, p);
 	rb_insert_color(&he->rb_node_in, root);
-	return true;
+	return 1;
 }
 
 struct rb_root *hists__get_rotate_entries_in(struct hists *hists)

@@ -1073,14 +1074,15 @@ static void hists__apply_filters(struct hists *hists, struct hist_entry *he)
 	hists__filter_entry_by_socket(hists, he);
 }
 
-void hists__collapse_resort(struct hists *hists, struct ui_progress *prog)
+int hists__collapse_resort(struct hists *hists, struct ui_progress *prog)
 {
 	struct rb_root *root;
 	struct rb_node *next;
 	struct hist_entry *n;
+	int ret;
 
 	if (!sort__need_collapse)
-		return;
+		return 0;
 
 	hists->nr_entries = 0;

@@ -1095,7 +1097,11 @@ void hists__collapse_resort(struct hists *hists, struct ui_progress *prog)
 		next = rb_next(&n->rb_node_in);
 
 		rb_erase(&n->rb_node_in, root);
-		if (hists__collapse_insert_entry(hists, &hists->entries_collapsed, n)) {
+		ret = hists__collapse_insert_entry(hists, &hists->entries_collapsed, n);
+		if (ret < 0)
+			return -1;
+
+		if (ret) {
 			/*
 			 * If it wasn't combined with one of the entries already
 			 * collapsed, we need to apply the filters that may have

@@ -1106,6 +1112,7 @@ void hists__collapse_resort(struct hists *hists, struct ui_progress *prog)
 		if (prog)
 			ui_progress__update(prog, 1);
 	}
+	return 0;
 }
 
 static int hist_entry__sort(struct hist_entry *a, struct hist_entry *b)

tools/perf/util/hist.h +2 -2

index d4ec4822a103..9c72f8331d1c 100644
@@ -129,7 +129,7 @@ int hist_entry__sort_snprintf(struct hist_entry *he, char *bf, size_t size,
 void hist_entry__delete(struct hist_entry *he);
 
 void hists__output_resort(struct hists *hists, struct ui_progress *prog);
-void hists__collapse_resort(struct hists *hists, struct ui_progress *prog);
+int hists__collapse_resort(struct hists *hists, struct ui_progress *prog);
 
 void hists__decay_entries(struct hists *hists, bool zap_user, bool zap_kernel);
 void hists__delete_entries(struct hists *hists);

@@ -188,7 +188,7 @@ int hists__init(void);
 int __hists__init(struct hists *hists);
 
 struct rb_root *hists__get_rotate_entries_in(struct hists *hists);
-bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
+int hists__collapse_insert_entry(struct hists *hists,
 				  struct rb_root *root, struct hist_entry *he);
 
 struct perf_hpp {

Signature · 1 line

2.6.4

Namhyung Kim

author maintainer

2016-01-22 13:44 UTC #10 lore raw

PATCH 7/7 perf report: Check error during report__collapse_hists()

In reply to Namhyung Kim

If it returns an error, warn user and bail out instead of silently ignoring.

Signed-off-by Namhyung Kim <[email protected]>

---
 tools/perf/builtin-report.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

tools/perf/builtin-report.c +11 -3

index 2bf537f190a0..7b933a9cf84f 100644
@@ -466,10 +466,11 @@ static int report__browse_hists(struct report *rep)
 	return ret;
 }
 
-static void report__collapse_hists(struct report *rep)
+static int report__collapse_hists(struct report *rep)
 {
 	struct ui_progress prog;
 	struct perf_evsel *pos;
+	int ret = 0;
 
 	ui_progress__init(&prog, rep->nr_entries, "Merging related events...");

@@ -481,7 +482,9 @@ static void report__collapse_hists(struct report *rep)
 
 		hists->socket_filter = rep->socket_filter;
 
-		hists__collapse_resort(hists, &prog);
+		ret = hists__collapse_resort(hists, &prog);
+		if (ret < 0)
+			break;
 
 		/* Non-group events are considered as leader */
 		if (symbol_conf.event_group &&

@@ -494,6 +497,7 @@ static void report__collapse_hists(struct report *rep)
 	}
 
 	ui_progress__finish();
+	return ret;
 }
 
 static void report__output_resort(struct report *rep)

@@ -561,7 +565,11 @@ static int __cmd_report(struct report *rep)
 		}
 	}
 
-	report__collapse_hists(rep);
+	ret = report__collapse_hists(rep);
+	if (ret) {
+		ui__error("failed to process hist entry\n");
+		return ret;
+	}
 
 	if (session_done())
 		return 0;

Signature · 1 line

2.6.4

Jiri Olsa

2016-01-23 17:01 UTC #11 lore raw

In reply to Namhyung Kim

Quoted 11 lines · Namhyung Kim

Hi,
This patchset checks error case during the process of collapsing hist entries. It's a preparation of upcoming hierarchy patchset which adds more work in the collapsing path. If there's an error during this stage, it'll stop processing and show warning to user.
Thanks,
Namhyung

Quoted 8 lines

Namhyung Kim (7):
perf callchain: Check return value of add_child() perf callchain: Check return value of fill_node() perf callchain: Add enum match_result for match_chain() perf callchain: Check return value of split_add_child() perf callchain: Check return value of append_chain_children() perf hists: Return error from hists__collapse_resort() perf report: Check error during report__collapse_hists()

I saw 2 other functions allocating memory and not checked:
callchain_cursor_append callchain_node__make_parent_list

thanks,
jirka

Namhyung Kim

author maintainer

2016-01-24 04:38 UTC #12 lore raw

Quoted 21 lines · Jiri Olsa

Quoted 11 lines · Namhyung Kim
Hi,
This patchset checks error case during the process of collapsing hist entries. It's a preparation of upcoming hierarchy patchset which adds more work in the collapsing path. If there's an error during this stage, it'll stop processing and show warning to user.
Thanks,
Namhyung
Quoted 8 lines
Namhyung Kim (7):
perf callchain: Check return value of add_child() perf callchain: Check return value of fill_node() perf callchain: Add enum match_result for match_chain() perf callchain: Check return value of split_add_child() perf callchain: Check return value of append_chain_children() perf hists: Return error from hists__collapse_resort() perf report: Check error during report__collapse_hists()

Quoted 2 lines

I saw 2 other functions allocating memory and not checked:
callchain_cursor_append

Ok, but this function is basically for the 'addition' path. Well it's also used by the 'collapsing' path but it never allocates new node since it reuses the existing ones. I'll prepare a different patchset for the 'addition' path later..

Quoted 1 line

callchain_node__make_parent_list

It seems not called in the collapsing path. It should be handled by a separate patchset.

Thanks,
Namhyung

Jiri Olsa

Acked-by

2016-01-25 07:12 UTC #13 lore raw

Quoted 31 lines · Namhyung Kim

Quoted 21 lines · Jiri Olsa
Quoted 11 lines · Namhyung Kim
Hi,
This patchset checks error case during the process of collapsing hist entries. It's a preparation of upcoming hierarchy patchset which adds more work in the collapsing path. If there's an error during this stage, it'll stop processing and show warning to user.
Thanks,
Namhyung
Quoted 8 lines
Namhyung Kim (7):
perf callchain: Check return value of add_child() perf callchain: Check return value of fill_node() perf callchain: Add enum match_result for match_chain() perf callchain: Check return value of split_add_child() perf callchain: Check return value of append_chain_children() perf hists: Return error from hists__collapse_resort() perf report: Check error during report__collapse_hists()
Quoted 2 lines
I saw 2 other functions allocating memory and not checked:
callchain_cursor_append
Ok, but this function is basically for the 'addition' path. Well it's also used by the 'collapsing' path but it never allocates new node since it reuses the existing ones. I'll prepare a different patchset for the 'addition' path later..

Quoted 4 lines

Quoted 1 line
callchain_node__make_parent_list
It seems not called in the collapsing path. It should be handled by a separate patchset.

ok, with the v2 for patch 3

Acked-by Jiri Olsa <[email protected]>

thanks,
jirka