[PATCH] perf tools: Do not call pevent_free_format when deleting tracepoint

[PATCH] perf tools: Do not call pevent_free_format when deleting tracepoint

[Jiri Olsa]

The libtraceevent library's main handle 'struct pevent' holds
pointers of every event that was added to it via functions:
  pevent_parse_format
  pevent_parse_event

We can't release struct event_format (call pevent_free_format)
separately, because that breaks that pointers array mentioned
above and another add_event call could end up with segfault.

All added events are released within the handle cleanup
in pevent_free.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evsel.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 2f9e68025ede..12b4396c7175 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -853,8 +853,6 @@ void perf_evsel__exit(struct perf_evsel *evsel)
 	perf_evsel__free_id(evsel);
 	close_cgroup(evsel->cgrp);
 	zfree(&evsel->group_name);
-	if (evsel->tp_format)
-		pevent_free_format(evsel->tp_format);
 	zfree(&evsel->name);
 	perf_evsel__object.fini(evsel);
 }
-- 
1.9.3

[tip:perf/core] perf evsel: Do not call pevent_free_format when deleting tracepoint

[tip-bot for Jiri Olsa]

Commit-ID:  daa01794a4a36a1da1b09a529adec0c8c0b94ab2
Gitweb:     http://git.kernel.org/tip/daa01794a4a36a1da1b09a529adec0c8c0b94ab2
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Tue, 4 Nov 2014 11:55:38 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 6 Nov 2014 17:47:14 -0300

perf evsel: Do not call pevent_free_format when deleting tracepoint

The libtraceevent library's main handle 'struct pevent' holds pointers
of every event that was added to it via functions:

  pevent_parse_format
  pevent_parse_event

We can't release struct event_format (call pevent_free_format)
separately, because that breaks that pointers array mentioned above and
another add_event call could end up with segfault.

All added events are released within the handle cleanup in pevent_free.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/1415098538-1512-1-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evsel.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 2f9e680..12b4396 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -853,8 +853,6 @@ void perf_evsel__exit(struct perf_evsel *evsel)
 	perf_evsel__free_id(evsel);
 	close_cgroup(evsel->cgrp);
 	zfree(&evsel->group_name);
-	if (evsel->tp_format)
-		pevent_free_format(evsel->tp_format);
 	zfree(&evsel->name);
 	perf_evsel__object.fini(evsel);
 }

[PATCH V8 0/4] perf tools: pmu event new style format fix

[kan.liang]

From: Kan Liang <kan.liang@intel.com>

There are two types of pmu event stytle formats, "pmu_event_name"
or "cpu/pmu_event_name/". However, there is a bug on supporting these
two formats, especially when they mixed with other perf events.
The patch set intends to fix this issue.

The patch set has been tested by perf automated test "./perf test parse -v"
on haswell, ivybridge and Romley platform.
The patch set also has been tested on haswell by the following script.
(Note: please make sure that your test system support TSX and
L1-dcache-loads events. Otherwise, you may want to change the events
to other pmu events.)

[lk@localhost ~]$ cat perf_style_test.sh
#hardware events + kernel pmu event with different style
perf stat -x, -e cycles,mem-stores,tx-start sleep 2
perf stat -x, -e cpu-cycles,cycles-ct,cycles-t sleep 2
perf stat -x, -e cycles,cpu/cycles-ct/,cpu/cycles-t/ sleep 2
perf stat -x, -e instructions,cpu/tx-start/ sleep 2
perf stat -x, -e '{cycles,tx-start}' sleep 2
perf stat -x, -e '{cycles,cpu/tx-start/}' sleep 2

#HW Cache event + kernel pmu event with different style
perf stat -x, -e L1-dcache-loads,cpu/mem-stores/,tx-start sleep 2
perf stat -x, -e L1-dcache-loads,mem-stores,cpu/tx-start/ sleep 2
perf stat -x, -e '{L1-dcache-loads,mem-stores}' sleep 2
perf stat -x, -e '{L1-dcache-loads,cpu/tx-start/}' sleep 2

#Raw event + kernel pmu event with different style:
perf stat -x, -e cpu/event=0xc0,umask=0x00/,mem-loads,cpu/mem-stores/ sleep 2
perf stat -x, -e cpu/event=0xc0,umask=0x00/,tx-start,cpu/el-start/ sleep 2
perf stat -x, -e '{cpu/event=0xc0,umask=0x00/,tx-start}' sleep 2

Changes since V1:
Read kernel PMU events from sysfs at runtime

Changes since V2:
Use strlcpy to replace strncpy

Changes since V3:
rebase to git.kernel.org/pub/scm/linux/kernel/git/acme/linux perf/core

Changes since V4:
scan kernel pmu events from sysfs only needed
rename the init/check/clenup functions and related struct.
allocate each symbol string separatelly
Use ALLOC_LIST

Changes since V5:
Using perf_pmu__find to instead of perf_pmu__scan
Don't scan all the time if the system doesn't support kernel pmu events

Changes since V6:
Add test case in automated tests suite
Use strdup and macro to refine the code
Add sep_dc for PMU event to support PE_MODIFIER_EVENT
Some minor changes for code style

Changes since V7:
Add allocation failure check

Kan Liang (4):
  Revert "perf tools: Default to cpu// for events v5"
  perf tools: parse the pmu event prefix and suffix
  perf tools: Add support to new style format of kernel PMU event
  perf tools: Add test case for pmu event new style format

 tools/perf/tests/parse-events.c        |  36 +++++++++
 tools/perf/util/include/linux/string.h |   1 -
 tools/perf/util/parse-events.c         | 133 +++++++++++++++++++++++++++------
 tools/perf/util/parse-events.h         |  14 ++++
 tools/perf/util/parse-events.l         |  30 +++++++-
 tools/perf/util/parse-events.y         |  40 ++++++++++
 tools/perf/util/pmu.c                  |  10 ---
 tools/perf/util/pmu.h                  |  10 +++
 tools/perf/util/string.c               |  24 ------
 9 files changed, 240 insertions(+), 58 deletions(-)

-- 
1.8.3.2

RE: [PATCH V8 0/4] perf tools: pmu event new style format fix

[Liang, Kan]

Hi Jolsa,

Does the new patch set work on your machine?
I tested the V8 patch set on haswell, ivybridge and Romley platform,
I cannot reproduce the issue you mentioned.
Could you please try the latest V8 patch? 

Thanks,
Kan

> From: Kan Liang <kan.liang@intel.com>
> 
> There are two types of pmu event stytle formats, "pmu_event_name"
> or "cpu/pmu_event_name/". However, there is a bug on supporting these
> two formats, especially when they mixed with other perf events.
> The patch set intends to fix this issue.
> 
> The patch set has been tested by perf automated test "./perf test parse -v"
> on haswell, ivybridge and Romley platform.
> The patch set also has been tested on haswell by the following script.
> (Note: please make sure that your test system support TSX and L1-dcache-
> loads events. Otherwise, you may want to change the events to other pmu
> events.)
> 
> [lk@localhost ~]$ cat perf_style_test.sh #hardware events + kernel pmu
> event with different style perf stat -x, -e cycles,mem-stores,tx-start sleep 2
> perf stat -x, -e cpu-cycles,cycles-ct,cycles-t sleep 2 perf stat -x, -e
> cycles,cpu/cycles-ct/,cpu/cycles-t/ sleep 2 perf stat -x, -e instructions,cpu/tx-
> start/ sleep 2 perf stat -x, -e '{cycles,tx-start}' sleep 2 perf stat -x, -e
> '{cycles,cpu/tx-start/}' sleep 2
> 
> #HW Cache event + kernel pmu event with different style perf stat -x, -e L1-
> dcache-loads,cpu/mem-stores/,tx-start sleep 2 perf stat -x, -e L1-dcache-
> loads,mem-stores,cpu/tx-start/ sleep 2 perf stat -x, -e '{L1-dcache-
> loads,mem-stores}' sleep 2 perf stat -x, -e '{L1-dcache-loads,cpu/tx-start/}'
> sleep 2
> 
> #Raw event + kernel pmu event with different style:
> perf stat -x, -e cpu/event=0xc0,umask=0x00/,mem-loads,cpu/mem-stores/
> sleep 2 perf stat -x, -e cpu/event=0xc0,umask=0x00/,tx-start,cpu/el-start/
> sleep 2 perf stat -x, -e '{cpu/event=0xc0,umask=0x00/,tx-start}' sleep 2
> 
> Changes since V1:
> Read kernel PMU events from sysfs at runtime
> 
> Changes since V2:
> Use strlcpy to replace strncpy
> 
> Changes since V3:
> rebase to git.kernel.org/pub/scm/linux/kernel/git/acme/linux perf/core
> 
> Changes since V4:
> scan kernel pmu events from sysfs only needed rename the
> init/check/clenup functions and related struct.
> allocate each symbol string separatelly
> Use ALLOC_LIST
> 
> Changes since V5:
> Using perf_pmu__find to instead of perf_pmu__scan Don't scan all the time
> if the system doesn't support kernel pmu events
> 
> Changes since V6:
> Add test case in automated tests suite
> Use strdup and macro to refine the code
> Add sep_dc for PMU event to support PE_MODIFIER_EVENT Some minor
> changes for code style
> 
> Changes since V7:
> Add allocation failure check
> 
> Kan Liang (4):
>   Revert "perf tools: Default to cpu// for events v5"
>   perf tools: parse the pmu event prefix and suffix
>   perf tools: Add support to new style format of kernel PMU event
>   perf tools: Add test case for pmu event new style format
> 
>  tools/perf/tests/parse-events.c        |  36 +++++++++
>  tools/perf/util/include/linux/string.h |   1 -
>  tools/perf/util/parse-events.c         | 133 +++++++++++++++++++++++++++-
> -----
>  tools/perf/util/parse-events.h         |  14 ++++
>  tools/perf/util/parse-events.l         |  30 +++++++-
>  tools/perf/util/parse-events.y         |  40 ++++++++++
>  tools/perf/util/pmu.c                  |  10 ---
>  tools/perf/util/pmu.h                  |  10 +++
>  tools/perf/util/string.c               |  24 ------
>  9 files changed, 240 insertions(+), 58 deletions(-)
> 
> --
> 1.8.3.2

[PATCH] perf tools: Do not call pevent_free_format when deleting tracepoint

[Jiri Olsa]

On Mon, Oct 13, 2014 at 03:43:18PM +0000, Liang, Kan wrote:
> Hi Jolsa,
> 
> Does the new patch set work on your machine?
> I tested the V8 patch set on haswell, ivybridge and Romley platform,
> I cannot reproduce the issue you mentioned.
> Could you please try the latest V8 patch? 

actually it still does, but it looks like another issue,
I think we need patch below.. I'll now check rest of your
patches.

Steven,
does the patch below make sense to you? I dont think we want
to allow freeing of the event_format that pevent keeps in
the array.

jirka


---
The libtraceevent library's main handle 'struct pevent' holds
pointers of every event that was added to it via functions:
  pevent_parse_format
  pevent_parse_event

All added events are released within the handle cleanup
in pevent_free.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evsel.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index e0868a901c4a..f5cc101428be 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -824,8 +824,6 @@ void perf_evsel__delete(struct perf_evsel *evsel)
 	perf_evsel__exit(evsel);
 	close_cgroup(evsel->cgrp);
 	zfree(&evsel->group_name);
-	if (evsel->tp_format)
-		pevent_free_format(evsel->tp_format);
 	zfree(&evsel->name);
 	free(evsel);
 }
-- 
1.9.3