Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1314987 > unrolled thread

[PATCHSET 0/7] perf tools: Check error during collapsing hist entries

Back to article view | Back to linux.kernel

Contents

  [PATCHSET 0/7] perf tools: Check error during collapsing hist entries Namhyung Kim <namhyung@kernel.org> - 2016-01-22 14:50 +0100
    [PATCH 1/7] perf callchain: Check return value of add_child() Namhyung Kim <namhyung@kernel.org> - 2016-01-22 14:50 +0100
    [PATCH 2/7] perf callchain: Check return value of fill_node() Namhyung Kim <namhyung@kernel.org> - 2016-01-22 14:50 +0100
    [PATCH 3/7] perf callchain: Add enum match_result for match_chain() Namhyung Kim <namhyung@kernel.org> - 2016-01-22 14:50 +0100
      Re: [PATCH 3/7] perf callchain: Add enum match_result for  match_chain() Jiri Olsa <jolsa@redhat.com> - 2016-01-23 18:10 +0100
        Re: [PATCH 3/7] perf callchain: Add enum match_result for  match_chain() Namhyung Kim <namhyung@kernel.org> - 2016-01-24 05:10 +0100
        [PATCH v2 3/7] perf callchain: Add enum match_result for  match_chain() Namhyung Kim <namhyung@kernel.org> - 2016-01-24 07:00 +0100
    [PATCH 5/7] perf callchain: Check return value of append_chain_children() Namhyung Kim <namhyung@kernel.org> - 2016-01-22 14:50 +0100
    Re: [PATCHSET 0/7] perf tools: Check error during collapsing hist  entries Jiri Olsa <jolsa@redhat.com> - 2016-01-23 18:10 +0100
      Re: [PATCHSET 0/7] perf tools: Check error during collapsing hist  entries Namhyung Kim <namhyung@kernel.org> - 2016-01-24 05:40 +0100
        Re: [PATCHSET 0/7] perf tools: Check error during collapsing hist  entries Jiri Olsa <jolsa@redhat.com> - 2016-01-25 08:20 +0100

#1314987 — [PATCHSET 0/7] 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(-)

-- 
2.6.4

[toc] | [next] | [standalone]

#1314988 — [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 <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/callchain.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 53c43eb9489e..134d88b33fc1 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -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);
 
-- 
2.6.4

[toc] | [prev] | [next] | [standalone]

#1314992 — [PATCH 2/7] perf callchain: Check return value of fill_node()

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 <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/callchain.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 134d88b33fc1..a82ea6f6fc0f 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -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;
-- 
2.6.4

[toc] | [prev] | [next] | [standalone]

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

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 <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/callchain.c | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index a82ea6f6fc0f..7139d438ee6d 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -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,
-- 
2.6.4

[toc] | [prev] | [next] | [standalone]

#1315702 — 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

>  	/* 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;

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

thanks,
jirka


---
diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index 7139d438ee6d..dc08e76aa8d9 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -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;

[toc] | [prev] | [next] | [standalone]

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

Hi Jiri,

On Sat, Jan 23, 2016 at 06:01:10PM +0100, Jiri Olsa wrote:
> On Fri, Jan 22, 2016 at 10:41:36PM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> >  	/* 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;
> 
> so if we want to use the return values like that you
> probably missed 2 other places

Right!

> 
> 
> ---
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index 7139d438ee6d..dc08e76aa8d9 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -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)

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


>  			break;
>  
>  		found = true;

[toc] | [prev] | [next] | [standalone]

#1315830 — [PATCH v2 3/7] perf callchain: Add enum match_result for match_chain()

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 <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/callchain.c | 52 +++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index a82ea6f6fc0f..dab2c1f1e86b 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -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,
-- 
2.6.4

[toc] | [prev] | [next] | [standalone]

#1314999 — [PATCH 5/7] perf callchain: Check return value of append_chain_children()

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

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/callchain.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index fc980323539e..1d97ea6d5d30 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -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);
-- 
2.6.4

[toc] | [prev] | [next] | [standalone]

#1315701 — Re: [PATCHSET 0/7] perf tools: Check error during collapsing hist entries

On Fri, Jan 22, 2016 at 10:41:33PM +0900, Namhyung Kim wrote:
> 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()

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

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1315821 — Re: [PATCHSET 0/7] perf tools: Check error during collapsing hist entries

On Sat, Jan 23, 2016 at 06:01:21PM +0100, Jiri Olsa wrote:
> On Fri, Jan 22, 2016 at 10:41:33PM +0900, Namhyung Kim wrote:
> > 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()
> 
> 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..


>   callchain_node__make_parent_list

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

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

#1316255 — Re: [PATCHSET 0/7] perf tools: Check error during collapsing hist entries

On Sun, Jan 24, 2016 at 01:37:55PM +0900, Namhyung Kim wrote:
> On Sat, Jan 23, 2016 at 06:01:21PM +0100, Jiri Olsa wrote:
> > On Fri, Jan 22, 2016 at 10:41:33PM +0900, Namhyung Kim wrote:
> > > 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()
> > 
> > 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..
> 
> 
> >   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 <jolsa@kernel.org>

thanks,
jirka

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web