Re: [PATCH 6/6] perf parse events: demystify memory allocations

2013-07-07 Thread Jiri Olsa 
On Sun, Jul 07, 2013 at 10:45:13AM -0600, David Ahern wrote:
> On 7/7/13 9:26 AM, Jiri Olsa wrote:
> >On Tue, Jul 02, 2013 at 01:27:25PM -0600, David Ahern wrote:
> >>List heads are currently allocated way down the function chain in 
> >>__add_event
> >>and add_tracepoint and then freed when the scanner code calls
> >>parse_events_update_lists.
> >>
> >>Be more explicit with where memory is allocated and who should free it. With
> >>this patch the list_head is allocated in the scanner code and freed when the
> >>scanner code calls parse_events_update_lists.
> >>
> >
> >SNIP
> >
> >>@@ -266,9 +279,10 @@ event_legacy_mem:
> >>  PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
> >>  {
> >>struct parse_events_evlist *data = _data;
> >>-   struct list_head *list = NULL;
> >>+   struct list_head *list;
> >>
> >>-   ABORT_ON(parse_events_add_breakpoint(, >idx,
> >>+   ALLOC_LIST(list);
> >>+   ABORT_ON(parse_events_add_breakpoint(list, >idx,
> >> (void *) $2, $4));
> >>$$ = list;
> >>  }
> >>@@ -276,9 +290,10 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
> >>  PE_PREFIX_MEM PE_VALUE sep_dc
> >>  {
> >>struct parse_events_evlist *data = _data;
> >>-   struct list_head *list = NULL;
> >>+   struct list_head *list;
> >>
> >>-   ABORT_ON(parse_events_add_breakpoint(, >idx,
> >>+   ALLOC_LIST(list);
> >>+   ABORT_ON(parse_events_add_breakpoint(list, >idx,
> >> (void *) $2, NULL));
> >
> >so who now frees the list if there's an error
> >in parse_events_add_breakpoint?
> 
> According to valgrind that memory is not freed prior to this patch,
> so this one does not introduce new leaks.

I thought this hunk did:


evsel = perf_evsel__new(attr, (*idx)++);
-   if (!evsel) {
-   free(list);


but I might have missed other cases..

jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] perf parse events: demystify memory allocations

2013-07-07 Thread David Ahern 

On 7/7/13 9:26 AM, Jiri Olsa wrote:

On Tue, Jul 02, 2013 at 01:27:25PM -0600, David Ahern wrote:

List heads are currently allocated way down the function chain in __add_event
and add_tracepoint and then freed when the scanner code calls
parse_events_update_lists.

Be more explicit with where memory is allocated and who should free it. With
this patch the list_head is allocated in the scanner code and freed when the
scanner code calls parse_events_update_lists.



SNIP


@@ -266,9 +279,10 @@ event_legacy_mem:
  PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
  {
struct parse_events_evlist *data = _data;
-   struct list_head *list = NULL;
+   struct list_head *list;

-   ABORT_ON(parse_events_add_breakpoint(, >idx,
+   ALLOC_LIST(list);
+   ABORT_ON(parse_events_add_breakpoint(list, >idx,
 (void *) $2, $4));
$$ = list;
  }
@@ -276,9 +290,10 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
  PE_PREFIX_MEM PE_VALUE sep_dc
  {
struct parse_events_evlist *data = _data;
-   struct list_head *list = NULL;
+   struct list_head *list;

-   ABORT_ON(parse_events_add_breakpoint(, >idx,
+   ALLOC_LIST(list);
+   ABORT_ON(parse_events_add_breakpoint(list, >idx,
 (void *) $2, NULL));


so who now frees the list if there's an error
in parse_events_add_breakpoint?


According to valgrind that memory is not freed prior to this patch, so 
this one does not introduce new leaks.




ditto for other ABORT_ON cases


I will whip up a patch to free memory on failure paths.

David

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] perf parse events: demystify memory allocations

2013-07-07 Thread Jiri Olsa 
On Tue, Jul 02, 2013 at 01:27:25PM -0600, David Ahern wrote:
> List heads are currently allocated way down the function chain in __add_event
> and add_tracepoint and then freed when the scanner code calls
> parse_events_update_lists.
> 
> Be more explicit with where memory is allocated and who should free it. With
> this patch the list_head is allocated in the scanner code and freed when the
> scanner code calls parse_events_update_lists.
> 

SNIP

> @@ -266,9 +279,10 @@ event_legacy_mem:
>  PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
>  {
>   struct parse_events_evlist *data = _data;
> - struct list_head *list = NULL;
> + struct list_head *list;
>  
> - ABORT_ON(parse_events_add_breakpoint(, >idx,
> + ALLOC_LIST(list);
> + ABORT_ON(parse_events_add_breakpoint(list, >idx,
>(void *) $2, $4));
>   $$ = list;
>  }
> @@ -276,9 +290,10 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
>  PE_PREFIX_MEM PE_VALUE sep_dc
>  {
>   struct parse_events_evlist *data = _data;
> - struct list_head *list = NULL;
> + struct list_head *list;
>  
> - ABORT_ON(parse_events_add_breakpoint(, >idx,
> + ALLOC_LIST(list);
> + ABORT_ON(parse_events_add_breakpoint(list, >idx,
>(void *) $2, NULL));

so who now frees the list if there's an error
in parse_events_add_breakpoint?

ditto for other ABORT_ON cases

jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] perf parse events: demystify memory allocations

2013-07-07 Thread Jiri Olsa 
On Tue, Jul 02, 2013 at 01:27:25PM -0600, David Ahern wrote:
 List heads are currently allocated way down the function chain in __add_event
 and add_tracepoint and then freed when the scanner code calls
 parse_events_update_lists.
 
 Be more explicit with where memory is allocated and who should free it. With
 this patch the list_head is allocated in the scanner code and freed when the
 scanner code calls parse_events_update_lists.
 

SNIP

 @@ -266,9 +279,10 @@ event_legacy_mem:
  PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
  {
   struct parse_events_evlist *data = _data;
 - struct list_head *list = NULL;
 + struct list_head *list;
  
 - ABORT_ON(parse_events_add_breakpoint(list, data-idx,
 + ALLOC_LIST(list);
 + ABORT_ON(parse_events_add_breakpoint(list, data-idx,
(void *) $2, $4));
   $$ = list;
  }
 @@ -276,9 +290,10 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
  PE_PREFIX_MEM PE_VALUE sep_dc
  {
   struct parse_events_evlist *data = _data;
 - struct list_head *list = NULL;
 + struct list_head *list;
  
 - ABORT_ON(parse_events_add_breakpoint(list, data-idx,
 + ALLOC_LIST(list);
 + ABORT_ON(parse_events_add_breakpoint(list, data-idx,
(void *) $2, NULL));

so who now frees the list if there's an error
in parse_events_add_breakpoint?

ditto for other ABORT_ON cases

jirka
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] perf parse events: demystify memory allocations

2013-07-07 Thread David Ahern 

On 7/7/13 9:26 AM, Jiri Olsa wrote:

On Tue, Jul 02, 2013 at 01:27:25PM -0600, David Ahern wrote:

List heads are currently allocated way down the function chain in __add_event
and add_tracepoint and then freed when the scanner code calls
parse_events_update_lists.

Be more explicit with where memory is allocated and who should free it. With
this patch the list_head is allocated in the scanner code and freed when the
scanner code calls parse_events_update_lists.



SNIP


@@ -266,9 +279,10 @@ event_legacy_mem:
  PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
  {
struct parse_events_evlist *data = _data;
-   struct list_head *list = NULL;
+   struct list_head *list;

-   ABORT_ON(parse_events_add_breakpoint(list, data-idx,
+   ALLOC_LIST(list);
+   ABORT_ON(parse_events_add_breakpoint(list, data-idx,
 (void *) $2, $4));
$$ = list;
  }
@@ -276,9 +290,10 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
  PE_PREFIX_MEM PE_VALUE sep_dc
  {
struct parse_events_evlist *data = _data;
-   struct list_head *list = NULL;
+   struct list_head *list;

-   ABORT_ON(parse_events_add_breakpoint(list, data-idx,
+   ALLOC_LIST(list);
+   ABORT_ON(parse_events_add_breakpoint(list, data-idx,
 (void *) $2, NULL));


so who now frees the list if there's an error
in parse_events_add_breakpoint?


According to valgrind that memory is not freed prior to this patch, so 
this one does not introduce new leaks.




ditto for other ABORT_ON cases


I will whip up a patch to free memory on failure paths.

David

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] perf parse events: demystify memory allocations

2013-07-07 Thread Jiri Olsa 
On Sun, Jul 07, 2013 at 10:45:13AM -0600, David Ahern wrote:
 On 7/7/13 9:26 AM, Jiri Olsa wrote:
 On Tue, Jul 02, 2013 at 01:27:25PM -0600, David Ahern wrote:
 List heads are currently allocated way down the function chain in 
 __add_event
 and add_tracepoint and then freed when the scanner code calls
 parse_events_update_lists.
 
 Be more explicit with where memory is allocated and who should free it. With
 this patch the list_head is allocated in the scanner code and freed when the
 scanner code calls parse_events_update_lists.
 
 
 SNIP
 
 @@ -266,9 +279,10 @@ event_legacy_mem:
   PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
   {
 struct parse_events_evlist *data = _data;
 -   struct list_head *list = NULL;
 +   struct list_head *list;
 
 -   ABORT_ON(parse_events_add_breakpoint(list, data-idx,
 +   ALLOC_LIST(list);
 +   ABORT_ON(parse_events_add_breakpoint(list, data-idx,
  (void *) $2, $4));
 $$ = list;
   }
 @@ -276,9 +290,10 @@ PE_PREFIX_MEM PE_VALUE ':' PE_MODIFIER_BP sep_dc
   PE_PREFIX_MEM PE_VALUE sep_dc
   {
 struct parse_events_evlist *data = _data;
 -   struct list_head *list = NULL;
 +   struct list_head *list;
 
 -   ABORT_ON(parse_events_add_breakpoint(list, data-idx,
 +   ALLOC_LIST(list);
 +   ABORT_ON(parse_events_add_breakpoint(list, data-idx,
  (void *) $2, NULL));
 
 so who now frees the list if there's an error
 in parse_events_add_breakpoint?
 
 According to valgrind that memory is not freed prior to this patch,
 so this one does not introduce new leaks.

I thought this hunk did:


evsel = perf_evsel__new(attr, (*idx)++);
-   if (!evsel) {
-   free(list);


but I might have missed other cases..

jirka
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 6/6] perf parse events: demystify memory allocations

2013-07-02 Thread David Ahern 
List heads are currently allocated way down the function chain in __add_event
and add_tracepoint and then freed when the scanner code calls
parse_events_update_lists.

Be more explicit with where memory is allocated and who should free it. With
this patch the list_head is allocated in the scanner code and freed when the
scanner code calls parse_events_update_lists.

Signed-off-by: David Ahern 
Cc: Jiri Olsa 
Cc: Ingo Molnar 
Cc: Frederic Weisbecker 
Cc: Peter Zijlstra 
Cc: Namhyung Kim 
Cc: Adrian Hunter 
---
 tools/perf/util/parse-events.c |   52 +++--
 tools/perf/util/parse-events.h |   10 +++
 tools/perf/util/parse-events.y |   62 ++--
 3 files changed, 61 insertions(+), 63 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 3a34f7b..8e3e270 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -264,40 +264,29 @@ const char *event_type(int type)
 
 
 
-static int __add_event(struct list_head **_list, int *idx,
+static int __add_event(struct list_head *list, int *idx,
   struct perf_event_attr *attr,
   char *name, struct cpu_map *cpus)
 {
struct perf_evsel *evsel;
-   struct list_head *list = *_list;
-
-   if (!list) {
-   list = malloc(sizeof(*list));
-   if (!list)
-   return -ENOMEM;
-   INIT_LIST_HEAD(list);
-   }
 
event_attr_init(attr);
 
evsel = perf_evsel__new(attr, (*idx)++);
-   if (!evsel) {
-   free(list);
+   if (!evsel)
return -ENOMEM;
-   }
 
evsel->cpus = cpus;
if (name)
evsel->name = strdup(name);
list_add_tail(>node, list);
-   *_list = list;
return 0;
 }
 
-static int add_event(struct list_head **_list, int *idx,
+static int add_event(struct list_head *list, int *idx,
 struct perf_event_attr *attr, char *name)
 {
-   return __add_event(_list, idx, attr, name, NULL);
+   return __add_event(list, idx, attr, name, NULL);
 }
 
 static int parse_aliases(char *str, const char 
*names[][PERF_EVSEL__MAX_ALIASES], int size)
@@ -318,7 +307,7 @@ static int parse_aliases(char *str, const char 
*names[][PERF_EVSEL__MAX_ALIASES]
return -1;
 }
 
-int parse_events_add_cache(struct list_head **list, int *idx,
+int parse_events_add_cache(struct list_head *list, int *idx,
   char *type, char *op_result1, char *op_result2)
 {
struct perf_event_attr attr;
@@ -379,31 +368,21 @@ int parse_events_add_cache(struct list_head **list, int 
*idx,
return add_event(list, idx, , name);
 }
 
-static int add_tracepoint(struct list_head **listp, int *idx,
+static int add_tracepoint(struct list_head *list, int *idx,
  char *sys_name, char *evt_name)
 {
struct perf_evsel *evsel;
-   struct list_head *list = *listp;
-
-   if (!list) {
-   list = malloc(sizeof(*list));
-   if (!list)
-   return -ENOMEM;
-   INIT_LIST_HEAD(list);
-   }
 
evsel = perf_evsel__newtp(sys_name, evt_name, (*idx)++);
-   if (!evsel) {
-   free(list);
+   if (!evsel)
return -ENOMEM;
-   }
 
list_add_tail(>node, list);
-   *listp = list;
+
return 0;
 }
 
-static int add_tracepoint_multi_event(struct list_head **list, int *idx,
+static int add_tracepoint_multi_event(struct list_head *list, int *idx,
  char *sys_name, char *evt_name)
 {
char evt_path[MAXPATHLEN];
@@ -435,7 +414,7 @@ static int add_tracepoint_multi_event(struct list_head 
**list, int *idx,
return ret;
 }
 
-static int add_tracepoint_event(struct list_head **list, int *idx,
+static int add_tracepoint_event(struct list_head *list, int *idx,
char *sys_name, char *evt_name)
 {
return strpbrk(evt_name, "*?") ?
@@ -443,7 +422,7 @@ static int add_tracepoint_event(struct list_head **list, 
int *idx,
   add_tracepoint(list, idx, sys_name, evt_name);
 }
 
-static int add_tracepoint_multi_sys(struct list_head **list, int *idx,
+static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
char *sys_name, char *evt_name)
 {
struct dirent *events_ent;
@@ -475,7 +454,7 @@ static int add_tracepoint_multi_sys(struct list_head 
**list, int *idx,
return ret;
 }
 
-int parse_events_add_tracepoint(struct list_head **list, int *idx,
+int parse_events_add_tracepoint(struct list_head *list, int *idx,
char *sys, char *event)
 {
int ret;
@@ -530,7 +509,7 @@ do {\
return 0;
 }
 
-int parse_events_add_breakpoint(struct list_head **list, int *idx,
+int

[PATCH 6/6] perf parse events: demystify memory allocations

2013-07-02 Thread David Ahern 
List heads are currently allocated way down the function chain in __add_event
and add_tracepoint and then freed when the scanner code calls
parse_events_update_lists.

Be more explicit with where memory is allocated and who should free it. With
this patch the list_head is allocated in the scanner code and freed when the
scanner code calls parse_events_update_lists.

Signed-off-by: David Ahern dsah...@gmail.com
Cc: Jiri Olsa jo...@redhat.com
Cc: Ingo Molnar mi...@kernel.org
Cc: Frederic Weisbecker fweis...@gmail.com
Cc: Peter Zijlstra pet...@infradead.org
Cc: Namhyung Kim namhy...@kernel.org
Cc: Adrian Hunter adrian.hun...@intel.com
---
 tools/perf/util/parse-events.c |   52 +++--
 tools/perf/util/parse-events.h |   10 +++
 tools/perf/util/parse-events.y |   62 ++--
 3 files changed, 61 insertions(+), 63 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 3a34f7b..8e3e270 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -264,40 +264,29 @@ const char *event_type(int type)
 
 
 
-static int __add_event(struct list_head **_list, int *idx,
+static int __add_event(struct list_head *list, int *idx,
   struct perf_event_attr *attr,
   char *name, struct cpu_map *cpus)
 {
struct perf_evsel *evsel;
-   struct list_head *list = *_list;
-
-   if (!list) {
-   list = malloc(sizeof(*list));
-   if (!list)
-   return -ENOMEM;
-   INIT_LIST_HEAD(list);
-   }
 
event_attr_init(attr);
 
evsel = perf_evsel__new(attr, (*idx)++);
-   if (!evsel) {
-   free(list);
+   if (!evsel)
return -ENOMEM;
-   }
 
evsel-cpus = cpus;
if (name)
evsel-name = strdup(name);
list_add_tail(evsel-node, list);
-   *_list = list;
return 0;
 }
 
-static int add_event(struct list_head **_list, int *idx,
+static int add_event(struct list_head *list, int *idx,
 struct perf_event_attr *attr, char *name)
 {
-   return __add_event(_list, idx, attr, name, NULL);
+   return __add_event(list, idx, attr, name, NULL);
 }
 
 static int parse_aliases(char *str, const char 
*names[][PERF_EVSEL__MAX_ALIASES], int size)
@@ -318,7 +307,7 @@ static int parse_aliases(char *str, const char 
*names[][PERF_EVSEL__MAX_ALIASES]
return -1;
 }
 
-int parse_events_add_cache(struct list_head **list, int *idx,
+int parse_events_add_cache(struct list_head *list, int *idx,
   char *type, char *op_result1, char *op_result2)
 {
struct perf_event_attr attr;
@@ -379,31 +368,21 @@ int parse_events_add_cache(struct list_head **list, int 
*idx,
return add_event(list, idx, attr, name);
 }
 
-static int add_tracepoint(struct list_head **listp, int *idx,
+static int add_tracepoint(struct list_head *list, int *idx,
  char *sys_name, char *evt_name)
 {
struct perf_evsel *evsel;
-   struct list_head *list = *listp;
-
-   if (!list) {
-   list = malloc(sizeof(*list));
-   if (!list)
-   return -ENOMEM;
-   INIT_LIST_HEAD(list);
-   }
 
evsel = perf_evsel__newtp(sys_name, evt_name, (*idx)++);
-   if (!evsel) {
-   free(list);
+   if (!evsel)
return -ENOMEM;
-   }
 
list_add_tail(evsel-node, list);
-   *listp = list;
+
return 0;
 }
 
-static int add_tracepoint_multi_event(struct list_head **list, int *idx,
+static int add_tracepoint_multi_event(struct list_head *list, int *idx,
  char *sys_name, char *evt_name)
 {
char evt_path[MAXPATHLEN];
@@ -435,7 +414,7 @@ static int add_tracepoint_multi_event(struct list_head 
**list, int *idx,
return ret;
 }
 
-static int add_tracepoint_event(struct list_head **list, int *idx,
+static int add_tracepoint_event(struct list_head *list, int *idx,
char *sys_name, char *evt_name)
 {
return strpbrk(evt_name, *?) ?
@@ -443,7 +422,7 @@ static int add_tracepoint_event(struct list_head **list, 
int *idx,
   add_tracepoint(list, idx, sys_name, evt_name);
 }
 
-static int add_tracepoint_multi_sys(struct list_head **list, int *idx,
+static int add_tracepoint_multi_sys(struct list_head *list, int *idx,
char *sys_name, char *evt_name)
 {
struct dirent *events_ent;
@@ -475,7 +454,7 @@ static int add_tracepoint_multi_sys(struct list_head 
**list, int *idx,
return ret;
 }
 
-int parse_events_add_tracepoint(struct list_head **list, int *idx,
+int parse_events_add_tracepoint(struct list_head *list, int *idx,
char *sys, char *event)
 {
int ret;
@@ -530,7 +509,7 @@ do {