2014-06-05 16:06:43

by Stephane Eranian

Subject: [BUG] perf record: pipe mode still broken

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

2014-06-05 17:00:50

by Jiri Olsa

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

On Thu, Jun 05, 2014 at 06:06:41PM +0200, Stephane Eranian wrote:
> 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

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(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 6a3af00..664010b 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -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

2014-06-05 20:21:12

by Arnaldo Carvalho de Melo

Subject: 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:
> On Thu, Jun 05, 2014 at 06:06:41PM +0200, Stephane Eranian wrote:
> > 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
>
> 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)

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

>
> 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(-)
>
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 6a3af00..664010b 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -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

2014-06-05 21:27:50

by Jiri Olsa

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

On Thu, Jun 05, 2014 at 05:21:02PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jun 05, 2014 at 07:00:11PM +0200, Jiri Olsa escreveu:
> > On Thu, Jun 05, 2014 at 06:06:41PM +0200, Stephane Eranian wrote:
> > > 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
> >
> > 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)
>
> 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?

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(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 6a3af00..16c7c11 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -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

2014-06-05 21:35:00

by Stephane Eranian

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

On Thu, Jun 5, 2014 at 10:41 PM, Jiri Olsa <[email protected]> wrote:
> On Thu, Jun 05, 2014 at 05:21:02PM -0300, Arnaldo Carvalho de Melo wrote:
>> Em Thu, Jun 05, 2014 at 07:00:11PM +0200, Jiri Olsa escreveu:
>> > On Thu, Jun 05, 2014 at 06:06:41PM +0200, Stephane Eranian wrote:
>> > > 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
>> >
>> > 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)
>>
>> 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?
>
> aaarhg.. yes ;-) v2 attached
>
Had the same reaction as Arnaldo. Now, it makes more sense...

> 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(-)
>
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 6a3af00..16c7c11 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -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
>

2014-06-12 12:02:02

by tip-bot for Vasyl Gomonovych

Subject: [tip:perf/core] 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]
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/builtin-inject.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 6a3af00..16c7c11 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -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);