[RFC 0/4] perf tools: Add hist_entry allocation callbacks

[RFC 0/4] perf tools: Add hist_entry allocation callbacks

[Jiri Olsa]

hi,
this patchset tries to add support provide own allocation
zalloc/free methods for hist_entry object.

The reason is to provide a way to be able to store more
data within hist_entry object in a transparent way to
its current usage by allocating its own hist_entry sub
object.

The user/app which wants to allocate its own hist_entry
sub object provides following ops struct:

  struct hist_entry_ops *ops {
    void*   (*new)(size_t);
    void    (*free)(void *);
  }

via new interface function:

  struct hist_entry *
  hists__add_entry_ops(struct hists *hists,
                       struct hist_entry_ops *ops,
                       ...

I'm using this for c2c code to enlarge hist_entry object
with large stats structure, which has no use to standard
perf usage/commands.

It might be used to lower the hist_entry footproint for
default perf usage by uing this for things like hierarchy
output that adds extra stuff into hist_entry object.

Available at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
  perf/he_ops

thanks for comments,
jirka


---
Jiri Olsa (4):
      perf tools: Introduce hist_entry__init function
      perf tools: Do the error path hist_entry release in hist_entry__new
      perf tools: Introduce hist_entry_ops
      perf tools: Introduce hists__add_entry_ops function

 tools/perf/util/hist.c | 204 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------
 tools/perf/util/hist.h |  11 ++++++
 tools/perf/util/sort.h |   6 ++++
 3 files changed, 148 insertions(+), 73 deletions(-)

[PATCH 1/4] perf tools: Introduce hist_entry__init function

[Jiri Olsa]

Move hist_entry initialization code into separate function.
It'll be useful and more clear for following patches that
introduce allocation callbacks.

Link: http://lkml.kernel.org/n/tip-j8dz6ytir91x8qd1zk4vf7ki@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/hist.c | 142 ++++++++++++++++++++++++++-----------------------
 1 file changed, 75 insertions(+), 67 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index e1fcc8d7c01a..ed7bea9aa68d 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -352,89 +352,97 @@ void hists__delete_entries(struct hists *hists)
  * histogram, sorted on item, collects periods
  */
 
-static struct hist_entry *hist_entry__new(struct hist_entry *template,
-					  bool sample_self)
+static int hist_entry__init(struct hist_entry *he,
+			    struct hist_entry *template,
+			    bool sample_self)
 {
-	size_t callchain_size = 0;
-	struct hist_entry *he;
+	*he = *template;
 
-	if (symbol_conf.use_callchain)
-		callchain_size = sizeof(struct callchain_root);
-
-	he = zalloc(sizeof(*he) + callchain_size);
+	if (symbol_conf.cumulate_callchain) {
+		he->stat_acc = malloc(sizeof(he->stat));
+		if (he->stat_acc == NULL) {
+			free(he);
+			return -ENOMEM;
+		}
+		memcpy(he->stat_acc, &he->stat, sizeof(he->stat));
+		if (!sample_self)
+			memset(&he->stat, 0, sizeof(he->stat));
+	}
 
-	if (he != NULL) {
-		*he = *template;
+	map__get(he->ms.map);
 
-		if (symbol_conf.cumulate_callchain) {
-			he->stat_acc = malloc(sizeof(he->stat));
-			if (he->stat_acc == NULL) {
-				free(he);
-				return NULL;
-			}
-			memcpy(he->stat_acc, &he->stat, sizeof(he->stat));
-			if (!sample_self)
-				memset(&he->stat, 0, sizeof(he->stat));
+	if (he->branch_info) {
+		/*
+		 * This branch info is (a part of) allocated from
+		 * sample__resolve_bstack() and will be freed after
+		 * adding new entries.  So we need to save a copy.
+		 */
+		he->branch_info = malloc(sizeof(*he->branch_info));
+		if (he->branch_info == NULL) {
+			map__zput(he->ms.map);
+			free(he->stat_acc);
+			free(he);
+			return -ENOMEM;
 		}
 
-		map__get(he->ms.map);
+		memcpy(he->branch_info, template->branch_info,
+		       sizeof(*he->branch_info));
 
-		if (he->branch_info) {
-			/*
-			 * This branch info is (a part of) allocated from
-			 * sample__resolve_bstack() and will be freed after
-			 * adding new entries.  So we need to save a copy.
-			 */
-			he->branch_info = malloc(sizeof(*he->branch_info));
-			if (he->branch_info == NULL) {
-				map__zput(he->ms.map);
-				free(he->stat_acc);
-				free(he);
-				return NULL;
-			}
+		map__get(he->branch_info->from.map);
+		map__get(he->branch_info->to.map);
+	}
 
-			memcpy(he->branch_info, template->branch_info,
-			       sizeof(*he->branch_info));
+	if (he->mem_info) {
+		map__get(he->mem_info->iaddr.map);
+		map__get(he->mem_info->daddr.map);
+	}
 
-			map__get(he->branch_info->from.map);
-			map__get(he->branch_info->to.map);
-		}
+	if (symbol_conf.use_callchain)
+		callchain_init(he->callchain);
 
-		if (he->mem_info) {
-			map__get(he->mem_info->iaddr.map);
-			map__get(he->mem_info->daddr.map);
+	if (he->raw_data) {
+		he->raw_data = memdup(he->raw_data, he->raw_size);
+
+		if (he->raw_data == NULL) {
+			map__put(he->ms.map);
+			if (he->branch_info) {
+				map__put(he->branch_info->from.map);
+				map__put(he->branch_info->to.map);
+				free(he->branch_info);
+			}
+			if (he->mem_info) {
+				map__put(he->mem_info->iaddr.map);
+				map__put(he->mem_info->daddr.map);
+			}
+			free(he->stat_acc);
+			free(he);
+			return -ENOMEM;
 		}
+	}
+	INIT_LIST_HEAD(&he->pairs.node);
+	thread__get(he->thread);
 
-		if (symbol_conf.use_callchain)
-			callchain_init(he->callchain);
+	if (!symbol_conf.report_hierarchy)
+		he->leaf = true;
 
-		if (he->raw_data) {
-			he->raw_data = memdup(he->raw_data, he->raw_size);
+	return 0;
+}
 
-			if (he->raw_data == NULL) {
-				map__put(he->ms.map);
-				if (he->branch_info) {
-					map__put(he->branch_info->from.map);
-					map__put(he->branch_info->to.map);
-					free(he->branch_info);
-				}
-				if (he->mem_info) {
-					map__put(he->mem_info->iaddr.map);
-					map__put(he->mem_info->daddr.map);
-				}
-				free(he->stat_acc);
-				free(he);
-				return NULL;
-			}
-		}
-		INIT_LIST_HEAD(&he->pairs.node);
-		thread__get(he->thread);
+static struct hist_entry *hist_entry__new(struct hist_entry *template,
+					  bool sample_self)
+{
+	size_t callchain_size = 0;
+	struct hist_entry *he;
+	int err = 0;
 
-		if (!symbol_conf.report_hierarchy)
-			he->leaf = true;
-	}
+	if (symbol_conf.use_callchain)
+		callchain_size = sizeof(struct callchain_root);
 
-	return he;
+	he = zalloc(sizeof(*he) + callchain_size);
+	if (he)
+		err = hist_entry__init(he, template, sample_self);
+
+	return err ? NULL : he;
 }
 
 static u8 symbol__parent_filter(const struct symbol *parent)
-- 
2.4.11

[PATCH 2/4] perf tools: Do the error path hist_entry release in hist_entry__new

[Jiri Olsa]

It's better to release the hist_entry object in
hist_entry__new function (where it's allocated)
rather than in hist_entry__init.

Link: http://lkml.kernel.org/n/tip-uoatzgsbdk3ebaeu56kdb9ku@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/hist.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index ed7bea9aa68d..04f3b52a319c 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -360,10 +360,8 @@ static int hist_entry__init(struct hist_entry *he,
 
 	if (symbol_conf.cumulate_callchain) {
 		he->stat_acc = malloc(sizeof(he->stat));
-		if (he->stat_acc == NULL) {
-			free(he);
+		if (he->stat_acc == NULL)
 			return -ENOMEM;
-		}
 		memcpy(he->stat_acc, &he->stat, sizeof(he->stat));
 		if (!sample_self)
 			memset(&he->stat, 0, sizeof(he->stat));
@@ -381,7 +379,6 @@ static int hist_entry__init(struct hist_entry *he,
 		if (he->branch_info == NULL) {
 			map__zput(he->ms.map);
 			free(he->stat_acc);
-			free(he);
 			return -ENOMEM;
 		}
 
@@ -415,7 +412,6 @@ static int hist_entry__init(struct hist_entry *he,
 				map__put(he->mem_info->daddr.map);
 			}
 			free(he->stat_acc);
-			free(he);
 			return -ENOMEM;
 		}
 	}
@@ -439,10 +435,13 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template,
 		callchain_size = sizeof(struct callchain_root);
 
 	he = zalloc(sizeof(*he) + callchain_size);
-	if (he)
+	if (he) {
 		err = hist_entry__init(he, template, sample_self);
+		if (err)
+			zfree(&he);
+	}
 
-	return err ? NULL : he;
+	return he;
 }
 
 static u8 symbol__parent_filter(const struct symbol *parent)
-- 
2.4.11

[PATCH 3/4] perf tools: Introduce hist_entry_ops

[Jiri Olsa]

Introducing allocation callbacks, that allows to extend
current hist_entry object into objects with special needs
without polluting the current hist_entry object.

Link: http://lkml.kernel.org/n/tip-yvapb3gmmn01qo7qn9lzl9vr@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/hist.c | 31 +++++++++++++++++++++++++++----
 tools/perf/util/sort.h |  6 ++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 04f3b52a319c..355b7601ddb7 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -424,21 +424,42 @@ static int hist_entry__init(struct hist_entry *he,
 	return 0;
 }
 
+static void *hist_entry__zalloc(size_t size)
+{
+	return zalloc(size + sizeof(struct hist_entry));
+}
+
+static void hist_entry__free(void *ptr)
+{
+	free(ptr);
+}
+
+static struct hist_entry_ops default_ops = {
+	.new	= hist_entry__zalloc,
+	.free	= hist_entry__free,
+};
+
 static struct hist_entry *hist_entry__new(struct hist_entry *template,
 					  bool sample_self)
 {
+	struct hist_entry_ops *ops = template->ops;
 	size_t callchain_size = 0;
 	struct hist_entry *he;
 	int err = 0;
 
+	if (!ops)
+		ops = template->ops = &default_ops;
+
 	if (symbol_conf.use_callchain)
 		callchain_size = sizeof(struct callchain_root);
 
-	he = zalloc(sizeof(*he) + callchain_size);
+	he = ops->new(callchain_size);
 	if (he) {
 		err = hist_entry__init(he, template, sample_self);
-		if (err)
-			zfree(&he);
+		if (err) {
+			ops->free(he);
+			he = NULL;
+		}
 	}
 
 	return he;
@@ -1050,6 +1071,8 @@ hist_entry__collapse(struct hist_entry *left, struct hist_entry *right)
 
 void hist_entry__delete(struct hist_entry *he)
 {
+	struct hist_entry_ops *ops = he->ops;
+
 	thread__zput(he->thread);
 	map__zput(he->ms.map);
 
@@ -1074,7 +1097,7 @@ void hist_entry__delete(struct hist_entry *he)
 	free_callchain(he->callchain);
 	free(he->trace_output);
 	free(he->raw_data);
-	free(he);
+	ops->free(he);
 }
 
 /*
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index ebb59cacd092..6fd0801d58a4 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -67,6 +67,11 @@ struct hist_entry_diff {
 	};
 };
 
+struct hist_entry_ops {
+	void*	(*new)(size_t);
+	void	(*free)(void *);
+};
+
 /**
  * struct hist_entry - histogram entry
  *
@@ -125,6 +130,7 @@ struct hist_entry {
 	void			*trace_output;
 	struct perf_hpp_list	*hpp_list;
 	struct hist_entry	*parent_he;
+	struct hist_entry_ops	*ops;
 	union {
 		/* this is for hierarchical entry structure */
 		struct {
-- 
2.4.11

[PATCH 4/4] perf tools: Introduce hists__add_entry_ops function

[Jiri Olsa]

Introducing hists__add_entry_ops function to allow using
the allocation callbacks externally.

Link: http://lkml.kernel.org/n/tip-r4bumbbg5st7p38hjm2z16b5@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/hist.c | 42 +++++++++++++++++++++++++++++++++++-------
 tools/perf/util/hist.h | 11 +++++++++++
 2 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 355b7601ddb7..a18d142cdca3 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -559,13 +559,15 @@ out:
 	return he;
 }
 
-struct hist_entry *hists__add_entry(struct hists *hists,
-				    struct addr_location *al,
-				    struct symbol *sym_parent,
-				    struct branch_info *bi,
-				    struct mem_info *mi,
-				    struct perf_sample *sample,
-				    bool sample_self)
+static struct hist_entry*
+__hists__add_entry(struct hists *hists,
+		   struct addr_location *al,
+		   struct symbol *sym_parent,
+		   struct branch_info *bi,
+		   struct mem_info *mi,
+		   struct perf_sample *sample,
+		   bool sample_self,
+		   struct hist_entry_ops *ops)
 {
 	struct hist_entry entry = {
 		.thread	= al->thread,
@@ -592,11 +594,37 @@ struct hist_entry *hists__add_entry(struct hists *hists,
 		.transaction = sample->transaction,
 		.raw_data = sample->raw_data,
 		.raw_size = sample->raw_size,
+		.ops = ops,
 	};
 
 	return hists__findnew_entry(hists, &entry, al, sample_self);
 }
 
+struct hist_entry *hists__add_entry(struct hists *hists,
+				    struct addr_location *al,
+				    struct symbol *sym_parent,
+				    struct branch_info *bi,
+				    struct mem_info *mi,
+				    struct perf_sample *sample,
+				    bool sample_self)
+{
+	return __hists__add_entry(hists, al, sym_parent, bi, mi,
+				  sample, sample_self, NULL);
+}
+
+struct hist_entry *hists__add_entry_ops(struct hists *hists,
+					struct hist_entry_ops *ops,
+					struct addr_location *al,
+					struct symbol *sym_parent,
+					struct branch_info *bi,
+					struct mem_info *mi,
+					struct perf_sample *sample,
+					bool sample_self)
+{
+	return __hists__add_entry(hists, al, sym_parent, bi, mi,
+				  sample, sample_self, ops);
+}
+
 static int
 iter_next_nop_entry(struct hist_entry_iter *iter __maybe_unused,
 		    struct addr_location *al __maybe_unused)
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 0a03e08be503..49aa4fac148f 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -10,6 +10,7 @@
 #include "ui/progress.h"
 
 struct hist_entry;
+struct hist_entry_ops;
 struct addr_location;
 struct symbol;
 
@@ -127,6 +128,16 @@ struct hist_entry *hists__add_entry(struct hists *hists,
 				    struct mem_info *mi,
 				    struct perf_sample *sample,
 				    bool sample_self);
+
+struct hist_entry *hists__add_entry_ops(struct hists *hists,
+					struct hist_entry_ops *ops,
+					struct addr_location *al,
+					struct symbol *sym_parent,
+					struct branch_info *bi,
+					struct mem_info *mi,
+					struct perf_sample *sample,
+					bool sample_self);
+
 int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al,
 			 int max_stack_depth, void *arg);
 
-- 
2.4.11

Re: [PATCH 1/4] perf tools: Introduce hist_entry__init function

[Arnaldo Carvalho de Melo]

Em Mon, Jul 04, 2016 at 04:01:36PM +0200, Jiri Olsa escreveu:
> Move hist_entry initialization code into separate function.
> It'll be useful and more clear for following patches that
> introduce allocation callbacks.
> 
> Link: http://lkml.kernel.org/n/tip-j8dz6ytir91x8qd1zk4vf7ki@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/hist.c | 142 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 75 insertions(+), 67 deletions(-)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index e1fcc8d7c01a..ed7bea9aa68d 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -352,89 +352,97 @@ void hists__delete_entries(struct hists *hists)
>   * histogram, sorted on item, collects periods
>   */
>  
> -static struct hist_entry *hist_entry__new(struct hist_entry *template,
> -					  bool sample_self)
> +static int hist_entry__init(struct hist_entry *he,
> +			    struct hist_entry *template,
> +			    bool sample_self)
>  {
> -	size_t callchain_size = 0;
> -	struct hist_entry *he;
> +	*he = *template;
>  
> -	if (symbol_conf.use_callchain)
> -		callchain_size = sizeof(struct callchain_root);
> -
> -	he = zalloc(sizeof(*he) + callchain_size);
> +	if (symbol_conf.cumulate_callchain) {
> +		he->stat_acc = malloc(sizeof(he->stat));
> +		if (he->stat_acc == NULL) {
> +			free(he);
> +			return -ENOMEM;
> +		}
> +		memcpy(he->stat_acc, &he->stat, sizeof(he->stat));
> +		if (!sample_self)
> +			memset(&he->stat, 0, sizeof(he->stat));
> +	}
>  
> -	if (he != NULL) {
> -		*he = *template;
> +	map__get(he->ms.map);
>  
> -		if (symbol_conf.cumulate_callchain) {
> -			he->stat_acc = malloc(sizeof(he->stat));
> -			if (he->stat_acc == NULL) {
> -				free(he);
> -				return NULL;
> -			}
> -			memcpy(he->stat_acc, &he->stat, sizeof(he->stat));
> -			if (!sample_self)
> -				memset(&he->stat, 0, sizeof(he->stat));
> +	if (he->branch_info) {
> +		/*
> +		 * This branch info is (a part of) allocated from
> +		 * sample__resolve_bstack() and will be freed after
> +		 * adding new entries.  So we need to save a copy.
> +		 */
> +		he->branch_info = malloc(sizeof(*he->branch_info));
> +		if (he->branch_info == NULL) {
> +			map__zput(he->ms.map);
> +			free(he->stat_acc);
> +			free(he);
> +			return -ENOMEM;
>  		}
>  
> -		map__get(he->ms.map);
> +		memcpy(he->branch_info, template->branch_info,
> +		       sizeof(*he->branch_info));
>  
> -		if (he->branch_info) {
> -			/*
> -			 * This branch info is (a part of) allocated from
> -			 * sample__resolve_bstack() and will be freed after
> -			 * adding new entries.  So we need to save a copy.
> -			 */
> -			he->branch_info = malloc(sizeof(*he->branch_info));
> -			if (he->branch_info == NULL) {
> -				map__zput(he->ms.map);
> -				free(he->stat_acc);
> -				free(he);
> -				return NULL;
> -			}
> +		map__get(he->branch_info->from.map);
> +		map__get(he->branch_info->to.map);
> +	}
>  
> -			memcpy(he->branch_info, template->branch_info,
> -			       sizeof(*he->branch_info));
> +	if (he->mem_info) {
> +		map__get(he->mem_info->iaddr.map);
> +		map__get(he->mem_info->daddr.map);
> +	}
>  
> -			map__get(he->branch_info->from.map);
> -			map__get(he->branch_info->to.map);
> -		}
> +	if (symbol_conf.use_callchain)
> +		callchain_init(he->callchain);
>  
> -		if (he->mem_info) {
> -			map__get(he->mem_info->iaddr.map);
> -			map__get(he->mem_info->daddr.map);
> +	if (he->raw_data) {
> +		he->raw_data = memdup(he->raw_data, he->raw_size);
> +
> +		if (he->raw_data == NULL) {
> +			map__put(he->ms.map);
> +			if (he->branch_info) {
> +				map__put(he->branch_info->from.map);
> +				map__put(he->branch_info->to.map);
> +				free(he->branch_info);
> +			}
> +			if (he->mem_info) {
> +				map__put(he->mem_info->iaddr.map);
> +				map__put(he->mem_info->daddr.map);
> +			}
> +			free(he->stat_acc);
> +			free(he);
> +			return -ENOMEM;
>  		}
> +	}
> +	INIT_LIST_HEAD(&he->pairs.node);
> +	thread__get(he->thread);
>  
> -		if (symbol_conf.use_callchain)
> -			callchain_init(he->callchain);
> +	if (!symbol_conf.report_hierarchy)
> +		he->leaf = true;
>  
> -		if (he->raw_data) {
> -			he->raw_data = memdup(he->raw_data, he->raw_size);
> +	return 0;
> +}
>  
> -			if (he->raw_data == NULL) {
> -				map__put(he->ms.map);
> -				if (he->branch_info) {
> -					map__put(he->branch_info->from.map);
> -					map__put(he->branch_info->to.map);
> -					free(he->branch_info);
> -				}
> -				if (he->mem_info) {
> -					map__put(he->mem_info->iaddr.map);
> -					map__put(he->mem_info->daddr.map);
> -				}
> -				free(he->stat_acc);
> -				free(he);
> -				return NULL;
> -			}
> -		}
> -		INIT_LIST_HEAD(&he->pairs.node);
> -		thread__get(he->thread);
> +static struct hist_entry *hist_entry__new(struct hist_entry *template,
> +					  bool sample_self)
> +{
> +	size_t callchain_size = 0;
> +	struct hist_entry *he;
> +	int err = 0;
>  
> -		if (!symbol_conf.report_hierarchy)
> -			he->leaf = true;
> -	}
> +	if (symbol_conf.use_callchain)
> +		callchain_size = sizeof(struct callchain_root);
>  
> -	return he;
> +	he = zalloc(sizeof(*he) + callchain_size);
> +	if (he)
> +		err = hist_entry__init(he, template, sample_self);

An init function shouldn't free the object being initialized, I haven't
checked if that is the case, the renaming of new->init made the patch
difficult to read, perhaps its just leaking it here in case init()
returns !0, please check.

I.e. if init() somehow fails, then new() should detect that, and free
what new() allocated ('he' in this case).

> +
> +	return err ? NULL : he;
>  }
>  
>  static u8 symbol__parent_filter(const struct symbol *parent)
> -- 
> 2.4.11

Re: [PATCH 2/4] perf tools: Do the error path hist_entry release in hist_entry__new

[Arnaldo Carvalho de Melo]

Em Mon, Jul 04, 2016 at 04:01:37PM +0200, Jiri Olsa escreveu:
> It's better to release the hist_entry object in
> hist_entry__new function (where it's allocated)
> rather than in hist_entry__init.

Please combine this with the previous patch, that way this all gets
clearer :-\

- Arnaldo
 
> Link: http://lkml.kernel.org/n/tip-uoatzgsbdk3ebaeu56kdb9ku@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/hist.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index ed7bea9aa68d..04f3b52a319c 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -360,10 +360,8 @@ static int hist_entry__init(struct hist_entry *he,
>  
>  	if (symbol_conf.cumulate_callchain) {
>  		he->stat_acc = malloc(sizeof(he->stat));
> -		if (he->stat_acc == NULL) {
> -			free(he);
> +		if (he->stat_acc == NULL)
>  			return -ENOMEM;
> -		}
>  		memcpy(he->stat_acc, &he->stat, sizeof(he->stat));
>  		if (!sample_self)
>  			memset(&he->stat, 0, sizeof(he->stat));
> @@ -381,7 +379,6 @@ static int hist_entry__init(struct hist_entry *he,
>  		if (he->branch_info == NULL) {
>  			map__zput(he->ms.map);
>  			free(he->stat_acc);
> -			free(he);
>  			return -ENOMEM;
>  		}
>  
> @@ -415,7 +412,6 @@ static int hist_entry__init(struct hist_entry *he,
>  				map__put(he->mem_info->daddr.map);
>  			}
>  			free(he->stat_acc);
> -			free(he);
>  			return -ENOMEM;
>  		}
>  	}
> @@ -439,10 +435,13 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template,
>  		callchain_size = sizeof(struct callchain_root);
>  
>  	he = zalloc(sizeof(*he) + callchain_size);
> -	if (he)
> +	if (he) {
>  		err = hist_entry__init(he, template, sample_self);
> +		if (err)
> +			zfree(&he);
> +	}
>  
> -	return err ? NULL : he;
> +	return he;
>  }
>  
>  static u8 symbol__parent_filter(const struct symbol *parent)
> -- 
> 2.4.11

Re: [PATCH 2/4] perf tools: Do the error path hist_entry release in hist_entry__new

[Jiri Olsa]

On Mon, Jul 04, 2016 at 04:09:47PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jul 04, 2016 at 04:01:37PM +0200, Jiri Olsa escreveu:
> > It's better to release the hist_entry object in
> > hist_entry__new function (where it's allocated)
> > rather than in hist_entry__init.
> 
> Please combine this with the previous patch, that way this all gets
> clearer :-\

ok, will change

thanks,
jirka