perf record: pipe mode still broken

BUG Superseded Stephane Eranian 4 participants 6 messages Jun 5, 2014 – Jun 12, 2014 perf events

Share & tools

Stephane Eranian

author

2014-06-05 16:06 UTC #0 lore raw

Hi Jiri,

Somehow, I thought you had written a fix to avoid the problem below. But when I try with tip.git, the problem is still there. Could you push your fix ASAP.

Thanks.

$ perf record -o - noploop 2 |  perf inject -b |  perf report -i -
# To display the perf.data header info, please use
--header/--header-only options.
#
noploop for 2 seconds
0x1bd0 [0x28]: failed to process type: 9

Jiri Olsa

2014-06-05 17:00 UTC #1 lore raw

PATCH 1/1 perf tools: Fix pipe check regression in attr event callback

Quoted 6 lines · Stephane Eranian

Hi Jiri,
Somehow, I thought you had written a fix to avoid the problem below. But when I try with tip.git, the problem is still there. Could you push your fix ASAP.

Quoted 8 lines

Thanks.

$ perf record -o - noploop 2 |  perf inject -b |  perf report -i -
# To display the perf.data header info, please use
--header/--header-only options.
#
noploop for 2 seconds
0x1bd0 [0x28]: failed to process type: 9

hum, I remember fixing another issue.. this one is separated one, please try attached patch.

thanks,
jirka

---
The file factoring in builtin-inject.c object introduced regression in attr event callback. The commit is:
3406912 perf inject: Handle output file via perf_data_file object

Following hunk reversed the logic:
- if (!inject->pipe_output)
+ if (&inject->output.is_pipe)

putting it back, following example now works:

  $ perf record -o - kill | perf inject -b | perf report -i -

Reported-by Stephane Eranian <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Corey Ashford <[email protected]>

Cc David Ahern <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/builtin-inject.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/perf/builtin-inject.c +1 -1

index 6a3af00..664010b 100644
@@ -72,7 +72,7 @@ static int perf_event__repipe_attr(struct perf_tool *tool,
 	if (ret)
 		return ret;
 
-	if (&inject->output.is_pipe)
+	if (!&inject->output.is_pipe)
 		return 0;
 
 	return perf_event__repipe_synth(tool, event);

Signature · 1 line

1.8.3.1

Arnaldo Carvalho de Melo

2014-06-05 20:21 UTC #2 lore raw

Re: [PATCH] perf tools: Fix pipe check regression in attr event callback

Em Thu, Jun 05, 2014 at 07:00:11PM +0200, Jiri Olsa escreveu:

Quoted 23 lines · Stephane Eranian

Quoted 6 lines
Hi Jiri,
Somehow, I thought you had written a fix to avoid the problem below. But when I try with tip.git, the problem is still there. Could you push your fix ASAP.
Quoted 8 lines
Thanks.
$ perf record -o - noploop 2 |  perf inject -b |  perf report -i -
# To display the perf.data header info, please use
--header/--header-only options.
#
noploop for 2 seconds
0x1bd0 [0x28]: failed to process type: 9
hum, I remember fixing another issue.. this one is separated one, please try attached patch.
thanks,
jirka

Quoted 8 lines

---
The file factoring in builtin-inject.c object introduced regression in attr event callback. The commit is:
3406912 perf inject: Handle output file via perf_data_file object
Following hunk reversed the logic:
- if (!inject->pipe_output)
+ if (&inject->output.is_pipe)

Why do we need this '&'? this will always evaluate to true, as it will get the address of a variable, also adding a ! before it will just it eval to false always, or am I missing something? :-)

I think this should really be:

            if (!inject->output.is_pipe)

No?

- Arnaldo

Quoted 33 lines

putting it back, following example now works:
  $ perf record -o - kill | perf inject -b | perf report -i -
Reported-by Stephane Eranian <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Corey Ashford <[email protected]>
Cc David Ahern <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Peter Zijlstra <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 tools/perf/builtin-inject.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/builtin-inject.c +1 -1
index 6a3af00..664010b 100644
@@ -72,7 +72,7 @@ static int perf_event__repipe_attr(struct perf_tool *tool,
 	if (ret)
 		return ret;
 
-	if (&inject->output.is_pipe)
+	if (!&inject->output.is_pipe)
 		return 0;
 
 	return perf_event__repipe_synth(tool, event);
Signature · 1 line
1.8.3.1

Jiri Olsa

2014-06-05 21:27 UTC #3 lore raw

PATCH v2 1/1 perf tools: Fix pipe check regression in attr event callback

Quoted 37 lines · Arnaldo Carvalho de Melo

Em Thu, Jun 05, 2014 at 07:00:11PM +0200, Jiri Olsa escreveu:
Quoted 23 lines · Stephane Eranian
Quoted 6 lines
Hi Jiri,
Somehow, I thought you had written a fix to avoid the problem below. But when I try with tip.git, the problem is still there. Could you push your fix ASAP.
Quoted 8 lines
Thanks.
$ perf record -o - noploop 2 |  perf inject -b |  perf report -i -
# To display the perf.data header info, please use
--header/--header-only options.
#
noploop for 2 seconds
0x1bd0 [0x28]: failed to process type: 9
hum, I remember fixing another issue.. this one is separated one, please try attached patch.
thanks,
jirka
Quoted 8 lines
---
The file factoring in builtin-inject.c object introduced regression in attr event callback. The commit is:
3406912 perf inject: Handle output file via perf_data_file object
Following hunk reversed the logic:
- if (!inject->pipe_output)
+ if (&inject->output.is_pipe)
Why do we need this '&'? this will always evaluate to true, as it will get the address of a variable, also adding a ! before it will just it eval to false always, or am I missing something? :-)

Quoted 5 lines

I think this should really be:
            if (!inject->output.is_pipe)
No?

aaarhg.. yes ;-) v2 attached

thanks,
jirka

---
The file factoring in builtin-inject.c object introduced regression in attr event callback. The commit is:
3406912 perf inject: Handle output file via perf_data_file object

Following hunk reversed the logic:
- if (!inject->pipe_output)
+ if (&inject->output.is_pipe)

putting it back, following example now works:

  $ perf record -o - kill | perf inject -b | perf report -i -

Plus removing extra '&' (kudos to Arnaldo)

Reported-by Stephane Eranian <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Corey Ashford <[email protected]>

Cc David Ahern <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/builtin-inject.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/perf/builtin-inject.c +1 -1

index 6a3af00..16c7c11 100644
@@ -72,7 +72,7 @@ static int perf_event__repipe_attr(struct perf_tool *tool,
 	if (ret)
 		return ret;
 
-	if (&inject->output.is_pipe)
+	if (!inject->output.is_pipe)
 		return 0;
 
 	return perf_event__repipe_synth(tool, event);

Signature · 1 line

1.8.3.1

Stephane Eranian

author

2014-06-05 21:35 UTC #4 lore raw

Re: [PATCHv2] perf tools: Fix pipe check regression in attr event callback

Quoted 38 lines · Jiri Olsa

Quoted 37 lines · Arnaldo Carvalho de Melo
Em Thu, Jun 05, 2014 at 07:00:11PM +0200, Jiri Olsa escreveu:
Quoted 23 lines · Stephane Eranian
Quoted 6 lines
Hi Jiri,
Somehow, I thought you had written a fix to avoid the problem below. But when I try with tip.git, the problem is still there. Could you push your fix ASAP.
Quoted 8 lines
Thanks.
$ perf record -o - noploop 2 |  perf inject -b |  perf report -i -
# To display the perf.data header info, please use
--header/--header-only options.
#
noploop for 2 seconds
0x1bd0 [0x28]: failed to process type: 9
hum, I remember fixing another issue.. this one is separated one, please try attached patch.
thanks,
jirka
Quoted 8 lines
---
The file factoring in builtin-inject.c object introduced regression in attr event callback. The commit is:
3406912 perf inject: Handle output file via perf_data_file object
Following hunk reversed the logic:
- if (!inject->pipe_output)
+ if (&inject->output.is_pipe)
Why do we need this '&'? this will always evaluate to true, as it will get the address of a variable, also adding a ! before it will just it eval to false always, or am I missing something? :-)

Quoted 8 lines

Quoted 5 lines
I think this should really be:
            if (!inject->output.is_pipe)
No?
aaarhg.. yes ;-) v2 attached

Had the same reaction as Arnaldo. Now, it makes more sense...

Quoted 47 lines

thanks,
jirka
---
The file factoring in builtin-inject.c object introduced regression in attr event callback. The commit is:
3406912 perf inject: Handle output file via perf_data_file object
Following hunk reversed the logic:
- if (!inject->pipe_output)
+ if (&inject->output.is_pipe)
putting it back, following example now works:
  $ perf record -o - kill | perf inject -b | perf report -i -
Plus removing extra '&' (kudos to Arnaldo)
Reported-by Stephane Eranian <[email protected]>
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Corey Ashford <[email protected]>
Cc David Ahern <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Peter Zijlstra <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 tools/perf/builtin-inject.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/perf/builtin-inject.c +1 -2
index 6a3af00..16c7c11 100644
@@ -72,7 +72,7 @@ static int perf_event__repipe_attr(struct perf_tool *tool,
        if (ret)
                return ret;
 
-       if (&inject->output.is_pipe)
+       if (!inject->output.is_pipe)
                return 0;
 
        return perf_event__repipe_synth(tool, event);
--
1.8.3.1

tip-bot for Jiri Olsa

merge bot

2014-06-12 12:02 UTC #5 lore raw

[tip:perf/core] perf tools: Fix pipe check regression in attr event callback

In reply to Jiri Olsa

perf tools: Fix pipe check regression in attr event callback

Commit-ID:  a261e4a09a073451057e9dbe17783255ea94598d
Gitweb:     http://git.kernel.org/tip/a261e4a09a073451057e9dbe17783255ea94598d
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 5 Jun 2014 18:51:44 +0200
Committer:  Jiri Olsa <[email protected]>
CommitDate: Mon, 9 Jun 2014 12:20:34 +0200

perf tools: Fix pipe check regression in attr event callback

The file factoring in builtin-inject.c object introduced regression in attr event callback. The commit is:
3406912 perf inject: Handle output file via perf_data_file object

Following hunk reversed the logic:
- if (!inject->pipe_output)
+ if (&inject->output.is_pipe)

putting it back, following example now works:

  $ perf record -o - kill | perf inject -b | perf report -i -

Plus removing extra '&' (kudos to Arnaldo)

Reported-by Stephane Eranian <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Corey Ashford <[email protected]>

Cc David Ahern <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Peter Zijlstra <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/builtin-inject.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/perf/builtin-inject.c +1 -1

index 6a3af00..16c7c11 100644
@@ -72,7 +72,7 @@ static int perf_event__repipe_attr(struct perf_tool *tool,
 	if (ret)
 		return ret;
 
-	if (&inject->output.is_pipe)
+	if (!inject->output.is_pipe)
 		return 0;
 
 	return perf_event__repipe_synth(tool, event);