[PATCH 0/4] perf intel-pt: Fix the pipe mode (v1)

[PATCH 0/4] perf intel-pt: Fix the pipe mode (v1)

[Namhyung Kim]

Hello,

I found some problems in Intel-PT and auxtrace in general with pipe.
In the past it used to work with pipe, but recent code fails.  As it
also touches the generic code, other auxtrace users like ARM SPE will
be affected too.  I added a test case to verify it works with pipes.

At last, I can run this command without a problem.

  $ perf record -o- -e intel_pt// true | perf inject -b | perf report -i- --itrace=i1000

The code is available at 'perf/auxtrace-pipe-v1' branch in

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung

Namhyung Kim (4):
  perf inject: Use perf_data__read() for auxtrace
  perf intel-pt: Do not try to queue auxtrace data on pipe
  perf session: Avoid calling lseek(2) for pipe
  perf test: Add pipe mode test to the Intel PT test suite

 tools/perf/builtin-inject.c             |  6 +++---
 tools/perf/tests/shell/test_intel_pt.sh | 17 +++++++++++++++++
 tools/perf/util/auxtrace.c              |  3 +++
 tools/perf/util/session.c               |  9 +++++++--
 4 files changed, 30 insertions(+), 5 deletions(-)


base-commit: 5670ebf54bd26482f57a094c53bdc562c106e0a9
prerequisite-patch-id: 4ccdf9c974a3909075051f4ffe498faecab7567b
-- 
2.39.1.456.gfc5497dd1b-goog

[PATCH 1/4] perf inject: Use perf_data__read() for auxtrace

[Namhyung Kim]

In copy_bytes(), it reads the data from the (input) fd and writes it to
the output file.  But it does with the read(2) unconditionally which
caused a problem of mixing buffered vs unbuffered I/O together.

You can see the problem when using pipes.

  $ perf record -e intel_pt// -o- true | perf inject -b > /dev/null
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.000 MB - ]
  0x45c0 [0x30]: failed to process type: 71

It should use perf_data__read() to honor the 'use_stdio' setting.

Fixes: 601366678c93 ("perf data: Allow to use stdio functions for pipe mode")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-inject.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 3f4e4dd5abf3..f8182417b734 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -215,14 +215,14 @@ static int perf_event__repipe_event_update(struct perf_tool *tool,
 
 #ifdef HAVE_AUXTRACE_SUPPORT
 
-static int copy_bytes(struct perf_inject *inject, int fd, off_t size)
+static int copy_bytes(struct perf_inject *inject, struct perf_data *data, off_t size)
 {
 	char buf[4096];
 	ssize_t ssz;
 	int ret;
 
 	while (size > 0) {
-		ssz = read(fd, buf, min(size, (off_t)sizeof(buf)));
+		ssz = perf_data__read(data, buf, min(size, (off_t)sizeof(buf)));
 		if (ssz < 0)
 			return -errno;
 		ret = output_bytes(inject, buf, ssz);
@@ -260,7 +260,7 @@ static s64 perf_event__repipe_auxtrace(struct perf_session *session,
 		ret = output_bytes(inject, event, event->header.size);
 		if (ret < 0)
 			return ret;
-		ret = copy_bytes(inject, perf_data__fd(session->data),
+		ret = copy_bytes(inject, session->data,
 				 event->auxtrace.size);
 	} else {
 		ret = output_bytes(inject, event,
-- 
2.39.1.456.gfc5497dd1b-goog

[PATCH 2/4] perf intel-pt: Do not try to queue auxtrace data on pipe

[Namhyung Kim]

When it processes AUXTRACE_INFO, it calls to auxtrace_queue_data() to
collect AUXTRACE data first.  That won't work with pipe since it needs
lseek() to read the scattered aux data.

  $ perf record -o- -e intel_pt// true | perf report -i- --itrace=i100
  # To display the perf.data header info, please use --header/--header-only options.
  #
  0x4118 [0xa0]: failed to process type: 70
  Error:
  failed to process sample

For the pipe mode, it can handle the aux data as it gets.

Fixes: dbd134322e74 ("perf intel-pt: Add support for decoding AUX area samples")
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/auxtrace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index c2e323cd7d49..d4b04fa07a11 100644
--- a/tools/perf/util/auxtrace.c
+++ b/tools/perf/util/auxtrace.c
@@ -1133,6 +1133,9 @@ int auxtrace_queue_data(struct perf_session *session, bool samples, bool events)
 	if (auxtrace__dont_decode(session))
 		return 0;
 
+	if (perf_data__is_pipe(session->data))
+		return 0;
+
 	if (!session->auxtrace || !session->auxtrace->queue_data)
 		return -EINVAL;
 
-- 
2.39.1.456.gfc5497dd1b-goog

[PATCH 3/4] perf session: Avoid calling lseek(2) for pipe

[Namhyung Kim]

We should not call lseek(2) for pipes as it won't work.  And we already
in the proper place to read the data for AUXTRACE.  Add the comment like
in the PERF_RECORD_HEADER_TRACING_DATA.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/session.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 7c021c6cedb9..fdfe772f2699 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1699,8 +1699,13 @@ static s64 perf_session__process_user_event(struct perf_session *session,
 	case PERF_RECORD_AUXTRACE_INFO:
 		return tool->auxtrace_info(session, event);
 	case PERF_RECORD_AUXTRACE:
-		/* setup for reading amidst mmap */
-		lseek(fd, file_offset + event->header.size, SEEK_SET);
+		/*
+		 * Setup for reading amidst mmap, but only when we
+		 * are in 'file' mode.  The 'pipe' fd is in proper
+		 * place already.
+		 */
+		if (!perf_data__is_pipe(session->data))
+			lseek(fd, file_offset + event->header.size, SEEK_SET);
 		return tool->auxtrace(session, event);
 	case PERF_RECORD_AUXTRACE_ERROR:
 		perf_session__auxtrace_error_inc(session, event);
-- 
2.39.1.456.gfc5497dd1b-goog

[PATCH 4/4] perf test: Add pipe mode test to the Intel PT test suite

[Namhyung Kim]

The test_pipe() function will check perf report and perf inject with
pipe input.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/test_intel_pt.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/perf/tests/shell/test_intel_pt.sh b/tools/perf/tests/shell/test_intel_pt.sh
index f5ed7b1af419..4ddb17cb83c5 100755
--- a/tools/perf/tests/shell/test_intel_pt.sh
+++ b/tools/perf/tests/shell/test_intel_pt.sh
@@ -620,6 +620,22 @@ test_event_trace()
 	return 0
 }
 
+test_pipe()
+{
+	echo "--- Test with pipe mode ---"
+	# Check if it works with pipe
+	if ! perf_record_no_bpf -o- -e intel_pt//u uname | perf report -q -i- --itrace=i10000 ; then
+		echo "perf record + report failed with pipe mode"
+		return 1
+	fi
+	if ! perf_record_no_bpf -o- -e intel_pt//u uname | perf inject -b > /dev/null ; then
+		echo "perf record + inject failed with pipe mode"
+		return 1
+	fi
+	echo OK
+	return 0
+}
+
 count_result()
 {
 	if [ "$1" -eq 2 ] ; then
@@ -647,6 +663,7 @@ test_virtual_lbr			|| ret=$? ; count_result $ret ; ret=0
 test_power_event			|| ret=$? ; count_result $ret ; ret=0
 test_no_tnt				|| ret=$? ; count_result $ret ; ret=0
 test_event_trace			|| ret=$? ; count_result $ret ; ret=0
+test_pipe				|| ret=$? ; count_result $ret ; ret=0
 
 cleanup
 
-- 
2.39.1.456.gfc5497dd1b-goog

Re: [PATCH 0/4] perf intel-pt: Fix the pipe mode (v1)

[Adrian Hunter]

On 27/01/23 02:19, Namhyung Kim wrote:
> Hello,
> 
> I found some problems in Intel-PT and auxtrace in general with pipe.
> In the past it used to work with pipe, but recent code fails.

Pipe mode is a problem for Intel PT and possibly other auxtrace users.
Essentially the auxtrace buffers do not behave like the regular perf
event buffers.  That is because the head and tail are updated by
software, but in the auxtrace case the data is written by hardware.
So the head and tail do not get updated as data is written.  In the
Intel PT case, the head and tail are updated only when the trace is
disabled by software, for example:
    - full-trace, system wide : when buffer passes watermark
    - full-trace, not system-wide : when buffer passes watermark or
    context switches
    - snapshot mode : as above but also when a snapshot is made
    - sample mode : as above but also when a sample is made

That means finished-round ordering doesn't work.  An auxtrace buffer
can turn up that has data that extends back in time, possibly to the
very beginning of tracing.

For a perf.data file, that problem is solved by going through the trace
and queuing up the auxtrace buffers in advance.

For pipe mode, the order of events and timestamps can presumably
be messed up.

For Intel PT, it is a bit of a surprise that there is not 
validation to error out in pipe mode.

At the least, a warning is needed, and the above explanation needs
to be added to the documentation.

>                                                                As it
> also touches the generic code, other auxtrace users like ARM SPE will
> be affected too.  I added a test case to verify it works with pipes.
> 
> At last, I can run this command without a problem.
> 
>   $ perf record -o- -e intel_pt// true | perf inject -b | perf report -i- --itrace=i1000
> 
> The code is available at 'perf/auxtrace-pipe-v1' branch in
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> 
> Thanks,
> Namhyung
> 
> Namhyung Kim (4):
>   perf inject: Use perf_data__read() for auxtrace
>   perf intel-pt: Do not try to queue auxtrace data on pipe
>   perf session: Avoid calling lseek(2) for pipe
>   perf test: Add pipe mode test to the Intel PT test suite
> 
>  tools/perf/builtin-inject.c             |  6 +++---
>  tools/perf/tests/shell/test_intel_pt.sh | 17 +++++++++++++++++
>  tools/perf/util/auxtrace.c              |  3 +++
>  tools/perf/util/session.c               |  9 +++++++--
>  4 files changed, 30 insertions(+), 5 deletions(-)
> 
> 
> base-commit: 5670ebf54bd26482f57a094c53bdc562c106e0a9
> prerequisite-patch-id: 4ccdf9c974a3909075051f4ffe498faecab7567b

Re: [PATCH 0/4] perf intel-pt: Fix the pipe mode (v1)

[James Clark]

On 27/01/2023 07:22, Adrian Hunter wrote:
> On 27/01/23 02:19, Namhyung Kim wrote:
>> Hello,
>>
>> I found some problems in Intel-PT and auxtrace in general with pipe.
>> In the past it used to work with pipe, but recent code fails.
> 
> Pipe mode is a problem for Intel PT and possibly other auxtrace users.

Just some info from my side: For Arm Coresight we ended up deprecating
pipe mode, then not supporting it altogether. First was when we added an
optional step to peek through all of the data to help with an edge case.
Then we added a requirement to receive a HW_ID packet before decoding
which necessitated the peek. You can't peek in pipe mode because you
need to be able to seek, so it's not supported at all anymore.

For Arm SPE I never tested it with piped data. I suppose I could add a
test at some point, but I don't really see the usecase.

James

> Essentially the auxtrace buffers do not behave like the regular perf
> event buffers.  That is because the head and tail are updated by
> software, but in the auxtrace case the data is written by hardware.
> So the head and tail do not get updated as data is written.  In the
> Intel PT case, the head and tail are updated only when the trace is
> disabled by software, for example:
>     - full-trace, system wide : when buffer passes watermark
>     - full-trace, not system-wide : when buffer passes watermark or
>     context switches
>     - snapshot mode : as above but also when a snapshot is made
>     - sample mode : as above but also when a sample is made
> 
> That means finished-round ordering doesn't work.  An auxtrace buffer
> can turn up that has data that extends back in time, possibly to the
> very beginning of tracing.
> 
> For a perf.data file, that problem is solved by going through the trace
> and queuing up the auxtrace buffers in advance.
> 
> For pipe mode, the order of events and timestamps can presumably
> be messed up.
> 
> For Intel PT, it is a bit of a surprise that there is not 
> validation to error out in pipe mode.
> 
> At the least, a warning is needed, and the above explanation needs
> to be added to the documentation.
> 
>>                                                                As it
>> also touches the generic code, other auxtrace users like ARM SPE will
>> be affected too.  I added a test case to verify it works with pipes.
>>
>> At last, I can run this command without a problem.
>>
>>   $ perf record -o- -e intel_pt// true | perf inject -b | perf report -i- --itrace=i1000
>>
>> The code is available at 'perf/auxtrace-pipe-v1' branch in
>>
>>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
>>
>> Thanks,
>> Namhyung
>>
>> Namhyung Kim (4):
>>   perf inject: Use perf_data__read() for auxtrace
>>   perf intel-pt: Do not try to queue auxtrace data on pipe
>>   perf session: Avoid calling lseek(2) for pipe
>>   perf test: Add pipe mode test to the Intel PT test suite
>>
>>  tools/perf/builtin-inject.c             |  6 +++---
>>  tools/perf/tests/shell/test_intel_pt.sh | 17 +++++++++++++++++
>>  tools/perf/util/auxtrace.c              |  3 +++
>>  tools/perf/util/session.c               |  9 +++++++--
>>  4 files changed, 30 insertions(+), 5 deletions(-)
>>
>>
>> base-commit: 5670ebf54bd26482f57a094c53bdc562c106e0a9
>> prerequisite-patch-id: 4ccdf9c974a3909075051f4ffe498faecab7567b
> 

Re: [PATCH 0/4] perf intel-pt: Fix the pipe mode (v1)

[James Clark]

On 27/01/2023 00:19, Namhyung Kim wrote:
> Hello,
> 
> I found some problems in Intel-PT and auxtrace in general with pipe.
> In the past it used to work with pipe, but recent code fails.  As it
> also touches the generic code, other auxtrace users like ARM SPE will
> be affected too.  I added a test case to verify it works with pipes.
> 
> At last, I can run this command without a problem.
> 
>   $ perf record -o- -e intel_pt// true | perf inject -b | perf report -i- --itrace=i1000
> 
> The code is available at 'perf/auxtrace-pipe-v1' branch in
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> 
> Thanks,
> Namhyung
> 
> Namhyung Kim (4):
>   perf inject: Use perf_data__read() for auxtrace
>   perf intel-pt: Do not try to queue auxtrace data on pipe
>   perf session: Avoid calling lseek(2) for pipe
>   perf test: Add pipe mode test to the Intel PT test suite
> 
>  tools/perf/builtin-inject.c             |  6 +++---
>  tools/perf/tests/shell/test_intel_pt.sh | 17 +++++++++++++++++
>  tools/perf/util/auxtrace.c              |  3 +++
>  tools/perf/util/session.c               |  9 +++++++--
>  4 files changed, 30 insertions(+), 5 deletions(-)
> 
> 
For the whole set:

Reviewed-by: James Clark <james.clark@arm.com>

> base-commit: 5670ebf54bd26482f57a094c53bdc562c106e0a9
> prerequisite-patch-id: 4ccdf9c974a3909075051f4ffe498faecab7567b

Re: [PATCH 3/4] perf session: Avoid calling lseek(2) for pipe

[James Clark]

On 27/01/2023 00:19, Namhyung Kim wrote:
> We should not call lseek(2) for pipes as it won't work.  And we already
> in the proper place to read the data for AUXTRACE.  Add the comment like
> in the PERF_RECORD_HEADER_TRACING_DATA.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/session.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 7c021c6cedb9..fdfe772f2699 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1699,8 +1699,13 @@ static s64 perf_session__process_user_event(struct perf_session *session,
>  	case PERF_RECORD_AUXTRACE_INFO:
>  		return tool->auxtrace_info(session, event);
>  	case PERF_RECORD_AUXTRACE:
> -		/* setup for reading amidst mmap */
> -		lseek(fd, file_offset + event->header.size, SEEK_SET);
> +		/*
> +		 * Setup for reading amidst mmap, but only when we
> +		 * are in 'file' mode.  The 'pipe' fd is in proper
> +		 * place already.
> +		 */
> +		if (!perf_data__is_pipe(session->data))
> +			lseek(fd, file_offset + event->header.size, SEEK_SET);

I'm not sure if it means anything, but Arm SPE works both with and
without this change, although I did have to skip the build-id inject part:

  perf record -o- -e arm_spe// stress -c 1 -t 1 | \
    perf report -i- --itrace=i1000


>  		return tool->auxtrace(session, event);
>  	case PERF_RECORD_AUXTRACE_ERROR:
>  		perf_session__auxtrace_error_inc(session, event);

Re: [PATCH 0/4] perf intel-pt: Fix the pipe mode (v1)

[Namhyung Kim]

Hi Adrian,

On Thu, Jan 26, 2023 at 11:22 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 27/01/23 02:19, Namhyung Kim wrote:
> > Hello,
> >
> > I found some problems in Intel-PT and auxtrace in general with pipe.
> > In the past it used to work with pipe, but recent code fails.
>
> Pipe mode is a problem for Intel PT and possibly other auxtrace users.
> Essentially the auxtrace buffers do not behave like the regular perf
> event buffers.  That is because the head and tail are updated by
> software, but in the auxtrace case the data is written by hardware.
> So the head and tail do not get updated as data is written.  In the
> Intel PT case, the head and tail are updated only when the trace is
> disabled by software, for example:
>     - full-trace, system wide : when buffer passes watermark
>     - full-trace, not system-wide : when buffer passes watermark or
>     context switches
>     - snapshot mode : as above but also when a snapshot is made
>     - sample mode : as above but also when a sample is made
>
> That means finished-round ordering doesn't work.  An auxtrace buffer
> can turn up that has data that extends back in time, possibly to the
> very beginning of tracing.

Ok, IIUC we want to process the main buffer and auxtrace buffer
together in time order but there's no guarantee to get the auxtrace
data in time, right?

I wonder if it's possible to use 2 pass processing for pipe mode.
We may keep the events in the ordered queue and auxtrace queue
in the first pass, and process together from the beginning in the
second pass. But I guess the data size would be a problem.

Or, assuming that the auxtrace buffer comes later than (or equal to)
the main buffer, we may start processing the main buffer as soon as
every auxtrace queue gets some data.  Thoughts?

>
> For a perf.data file, that problem is solved by going through the trace
> and queuing up the auxtrace buffers in advance.
>
> For pipe mode, the order of events and timestamps can presumably
> be messed up.
>
> For Intel PT, it is a bit of a surprise that there is not
> validation to error out in pipe mode.

What kind of validation do you have in mind?  Checking pid/tid?

>
> At the least, a warning is needed, and the above explanation needs
> to be added to the documentation.

Thanks, I'll add it to the documentation.

How about showing something like this for pipe mode?

  WARNING: Intel-PT with pipe mode may not work correctly.

Thanks,
Namhyung


>
> >                                                                As it
> > also touches the generic code, other auxtrace users like ARM SPE will
> > be affected too.  I added a test case to verify it works with pipes.
> >
> > At last, I can run this command without a problem.
> >
> >   $ perf record -o- -e intel_pt// true | perf inject -b | perf report -i- --itrace=i1000
> >
> > The code is available at 'perf/auxtrace-pipe-v1' branch in
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> >
> > Thanks,
> > Namhyung
> >
> > Namhyung Kim (4):
> >   perf inject: Use perf_data__read() for auxtrace
> >   perf intel-pt: Do not try to queue auxtrace data on pipe
> >   perf session: Avoid calling lseek(2) for pipe
> >   perf test: Add pipe mode test to the Intel PT test suite
> >
> >  tools/perf/builtin-inject.c             |  6 +++---
> >  tools/perf/tests/shell/test_intel_pt.sh | 17 +++++++++++++++++
> >  tools/perf/util/auxtrace.c              |  3 +++
> >  tools/perf/util/session.c               |  9 +++++++--
> >  4 files changed, 30 insertions(+), 5 deletions(-)
> >
> >
> > base-commit: 5670ebf54bd26482f57a094c53bdc562c106e0a9
> > prerequisite-patch-id: 4ccdf9c974a3909075051f4ffe498faecab7567b
>

Re: [PATCH 0/4] perf intel-pt: Fix the pipe mode (v1)

[Namhyung Kim]

Hi James,

On Fri, Jan 27, 2023 at 6:42 AM James Clark <james.clark@arm.com> wrote:
>
>
>
> On 27/01/2023 07:22, Adrian Hunter wrote:
> > On 27/01/23 02:19, Namhyung Kim wrote:
> >> Hello,
> >>
> >> I found some problems in Intel-PT and auxtrace in general with pipe.
> >> In the past it used to work with pipe, but recent code fails.
> >
> > Pipe mode is a problem for Intel PT and possibly other auxtrace users.
>
> Just some info from my side: For Arm Coresight we ended up deprecating
> pipe mode, then not supporting it altogether. First was when we added an
> optional step to peek through all of the data to help with an edge case.
> Then we added a requirement to receive a HW_ID packet before decoding
> which necessitated the peek. You can't peek in pipe mode because you
> need to be able to seek, so it's not supported at all anymore.
>
> For Arm SPE I never tested it with piped data. I suppose I could add a
> test at some point, but I don't really see the usecase.

Yeah, it'd be great if we can have a test for Arm SPE.

Anyway, my work env (Google) requires the pipe mode due to the
restriction in disk usage.  Without the pipe support, it's not possible
to run `perf record` in production.

Thanks,
Namhyung

Re: [PATCH 0/4] perf intel-pt: Fix the pipe mode (v1)

[James Clark]

On 27/01/2023 23:08, Namhyung Kim wrote:
> Hi James,
> 
> On Fri, Jan 27, 2023 at 6:42 AM James Clark <james.clark@arm.com> wrote:
>>
>>
>>
>> On 27/01/2023 07:22, Adrian Hunter wrote:
>>> On 27/01/23 02:19, Namhyung Kim wrote:
>>>> Hello,
>>>>
>>>> I found some problems in Intel-PT and auxtrace in general with pipe.
>>>> In the past it used to work with pipe, but recent code fails.
>>>
>>> Pipe mode is a problem for Intel PT and possibly other auxtrace users.
>>
>> Just some info from my side: For Arm Coresight we ended up deprecating
>> pipe mode, then not supporting it altogether. First was when we added an
>> optional step to peek through all of the data to help with an edge case.
>> Then we added a requirement to receive a HW_ID packet before decoding
>> which necessitated the peek. You can't peek in pipe mode because you
>> need to be able to seek, so it's not supported at all anymore.
>>
>> For Arm SPE I never tested it with piped data. I suppose I could add a
>> test at some point, but I don't really see the usecase.
> 
> Yeah, it'd be great if we can have a test for Arm SPE.
> 

Ok thanks I will put it on the list of things to do.

> Anyway, my work env (Google) requires the pipe mode due to the
> restriction in disk usage.  Without the pipe support, it's not possible
> to run `perf record` in production.
> 

Makes sense. Unfortunately at the moment with Coresight, because of the
lack of appropriate timestamps we're waiting for the end of the file
before starting decoding. So you're not really any better off using
piped mode, unless you have a lot more memory than disk space?

Since this commit [1] and Arm v8.4 we can actually start making use of
the timestamps and do a streaming decode again. So I will also add it to
the list to look into that for Coresight again. Are you using an old
version of Perf or not using Coresight at all? I know Denis at Google is
using Coresight, but only with files rather than pipes.

One other thing, have you used the --switch-output mode to perf record
before? I would have said it would give you some of the benefits of
piped mode, but is more likely to work with Coresight. But last time I
checked it's not working either. Not very helpful I know, but something
to keep in mind.

James

[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=a7fe9a443b6064c68f86a2ee09bdfa7736660ef3

Re: [PATCH 0/4] perf intel-pt: Fix the pipe mode (v1)

[Arnaldo Carvalho de Melo]

Em Fri, Jan 27, 2023 at 02:54:36PM -0800, Namhyung Kim escreveu:
> Hi Adrian,
> 
> On Thu, Jan 26, 2023 at 11:22 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >
> > On 27/01/23 02:19, Namhyung Kim wrote:
> > > Hello,
> > >
> > > I found some problems in Intel-PT and auxtrace in general with pipe.
> > > In the past it used to work with pipe, but recent code fails.
> >
> > Pipe mode is a problem for Intel PT and possibly other auxtrace users.
> > Essentially the auxtrace buffers do not behave like the regular perf
> > event buffers.  That is because the head and tail are updated by
> > software, but in the auxtrace case the data is written by hardware.
> > So the head and tail do not get updated as data is written.  In the
> > Intel PT case, the head and tail are updated only when the trace is
> > disabled by software, for example:
> >     - full-trace, system wide : when buffer passes watermark
> >     - full-trace, not system-wide : when buffer passes watermark or
> >     context switches
> >     - snapshot mode : as above but also when a snapshot is made
> >     - sample mode : as above but also when a sample is made
> >
> > That means finished-round ordering doesn't work.  An auxtrace buffer
> > can turn up that has data that extends back in time, possibly to the
> > very beginning of tracing.
> 
> Ok, IIUC we want to process the main buffer and auxtrace buffer
> together in time order but there's no guarantee to get the auxtrace
> data in time, right?
> 
> I wonder if it's possible to use 2 pass processing for pipe mode.
> We may keep the events in the ordered queue and auxtrace queue
> in the first pass, and process together from the beginning in the
> second pass. But I guess the data size would be a problem.
> 
> Or, assuming that the auxtrace buffer comes later than (or equal to)
> the main buffer, we may start processing the main buffer as soon as
> every auxtrace queue gets some data.  Thoughts?
> 
> >
> > For a perf.data file, that problem is solved by going through the trace
> > and queuing up the auxtrace buffers in advance.
> >
> > For pipe mode, the order of events and timestamps can presumably
> > be messed up.
> >
> > For Intel PT, it is a bit of a surprise that there is not
> > validation to error out in pipe mode.
> 
> What kind of validation do you have in mind?  Checking pid/tid?
> 
> >
> > At the least, a warning is needed, and the above explanation needs
> > to be added to the documentation.
> 
> Thanks, I'll add it to the documentation.

Ok, so I'll wait for v2 of this patch series, Adrian, apart from what
you mentioned, are you ok with the patches, or a subset of them? The
first ones looks ok, right?

- Arnaldo
 
> How about showing something like this for pipe mode?
> 
>   WARNING: Intel-PT with pipe mode may not work correctly.
> 
> Thanks,
> Namhyung
> 
> 
> >
> > >                                                                As it
> > > also touches the generic code, other auxtrace users like ARM SPE will
> > > be affected too.  I added a test case to verify it works with pipes.
> > >
> > > At last, I can run this command without a problem.
> > >
> > >   $ perf record -o- -e intel_pt// true | perf inject -b | perf report -i- --itrace=i1000
> > >
> > > The code is available at 'perf/auxtrace-pipe-v1' branch in
> > >
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> > >
> > > Thanks,
> > > Namhyung
> > >
> > > Namhyung Kim (4):
> > >   perf inject: Use perf_data__read() for auxtrace
> > >   perf intel-pt: Do not try to queue auxtrace data on pipe
> > >   perf session: Avoid calling lseek(2) for pipe
> > >   perf test: Add pipe mode test to the Intel PT test suite
> > >
> > >  tools/perf/builtin-inject.c             |  6 +++---
> > >  tools/perf/tests/shell/test_intel_pt.sh | 17 +++++++++++++++++
> > >  tools/perf/util/auxtrace.c              |  3 +++
> > >  tools/perf/util/session.c               |  9 +++++++--
> > >  4 files changed, 30 insertions(+), 5 deletions(-)
> > >
> > >
> > > base-commit: 5670ebf54bd26482f57a094c53bdc562c106e0a9
> > > prerequisite-patch-id: 4ccdf9c974a3909075051f4ffe498faecab7567b
> >

-- 

- Arnaldo

Re: [PATCH 0/4] perf intel-pt: Fix the pipe mode (v1)

[Adrian Hunter]

On 30/01/23 16:15, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jan 27, 2023 at 02:54:36PM -0800, Namhyung Kim escreveu:
>> Hi Adrian,
>>
>> On Thu, Jan 26, 2023 at 11:22 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>
>>> On 27/01/23 02:19, Namhyung Kim wrote:
>>>> Hello,
>>>>
>>>> I found some problems in Intel-PT and auxtrace in general with pipe.
>>>> In the past it used to work with pipe, but recent code fails.
>>>
>>> Pipe mode is a problem for Intel PT and possibly other auxtrace users.
>>> Essentially the auxtrace buffers do not behave like the regular perf
>>> event buffers.  That is because the head and tail are updated by
>>> software, but in the auxtrace case the data is written by hardware.
>>> So the head and tail do not get updated as data is written.  In the
>>> Intel PT case, the head and tail are updated only when the trace is
>>> disabled by software, for example:
>>>     - full-trace, system wide : when buffer passes watermark
>>>     - full-trace, not system-wide : when buffer passes watermark or
>>>     context switches
>>>     - snapshot mode : as above but also when a snapshot is made
>>>     - sample mode : as above but also when a sample is made
>>>
>>> That means finished-round ordering doesn't work.  An auxtrace buffer
>>> can turn up that has data that extends back in time, possibly to the
>>> very beginning of tracing.
>>
>> Ok, IIUC we want to process the main buffer and auxtrace buffer
>> together in time order but there's no guarantee to get the auxtrace
>> data in time, right?

Yes

>>
>> I wonder if it's possible to use 2 pass processing for pipe mode.
>> We may keep the events in the ordered queue and auxtrace queue
>> in the first pass, and process together from the beginning in the
>> second pass. But I guess the data size would be a problem.
>>
>> Or, assuming that the auxtrace buffer comes later than (or equal to)
>> the main buffer, we may start processing the main buffer as soon as
>> every auxtrace queue gets some data.  Thoughts?

That sounds like it would require figuring out a timestamp up to
which there is Intel PT trace data in all queues.  That would
be very complicated.

>>
>>>
>>> For a perf.data file, that problem is solved by going through the trace
>>> and queuing up the auxtrace buffers in advance.
>>>
>>> For pipe mode, the order of events and timestamps can presumably
>>> be messed up.
>>>
>>> For Intel PT, it is a bit of a surprise that there is not
>>> validation to error out in pipe mode.
>>
>> What kind of validation do you have in mind?  Checking pid/tid?

Validation to kill pipe mode for Intel PT entirely.  But a warning
is ok.

>>
>>>
>>> At the least, a warning is needed, and the above explanation needs
>>> to be added to the documentation.
>>
>> Thanks, I'll add it to the documentation.
> 
> Ok, so I'll wait for v2 of this patch series, Adrian, apart from what
> you mentioned, are you ok with the patches, or a subset of them? The
> first ones looks ok, right?

Yes they are ok.

> 
> - Arnaldo
>  
>> How about showing something like this for pipe mode?
>>
>>   WARNING: Intel-PT with pipe mode may not work correctly.

Perhaps:

WARNING: Intel PT with pipe mode is not recommended. The output cannot be relied upon. In particular, time stamps and the order of events may be incorrect.

>>
>> Thanks,
>> Namhyung
>>
>>
>>>
>>>>                                                                As it
>>>> also touches the generic code, other auxtrace users like ARM SPE will
>>>> be affected too.  I added a test case to verify it works with pipes.
>>>>
>>>> At last, I can run this command without a problem.
>>>>
>>>>   $ perf record -o- -e intel_pt// true | perf inject -b | perf report -i- --itrace=i1000
>>>>
>>>> The code is available at 'perf/auxtrace-pipe-v1' branch in
>>>>
>>>>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
>>>>
>>>> Thanks,
>>>> Namhyung
>>>>
>>>> Namhyung Kim (4):
>>>>   perf inject: Use perf_data__read() for auxtrace
>>>>   perf intel-pt: Do not try to queue auxtrace data on pipe
>>>>   perf session: Avoid calling lseek(2) for pipe
>>>>   perf test: Add pipe mode test to the Intel PT test suite
>>>>
>>>>  tools/perf/builtin-inject.c             |  6 +++---
>>>>  tools/perf/tests/shell/test_intel_pt.sh | 17 +++++++++++++++++
>>>>  tools/perf/util/auxtrace.c              |  3 +++
>>>>  tools/perf/util/session.c               |  9 +++++++--
>>>>  4 files changed, 30 insertions(+), 5 deletions(-)
>>>>
>>>>
>>>> base-commit: 5670ebf54bd26482f57a094c53bdc562c106e0a9
>>>> prerequisite-patch-id: 4ccdf9c974a3909075051f4ffe498faecab7567b
>>>
> 

Re: [PATCH 0/4] perf intel-pt: Fix the pipe mode (v1)

[Namhyung Kim]

On Mon, Jan 30, 2023 at 2:56 AM James Clark <james.clark@arm.com> wrote:
>
>
>
> On 27/01/2023 23:08, Namhyung Kim wrote:
> > Hi James,
> >
> > On Fri, Jan 27, 2023 at 6:42 AM James Clark <james.clark@arm.com> wrote:
> >>
> >>
> >>
> >> On 27/01/2023 07:22, Adrian Hunter wrote:
> >>> On 27/01/23 02:19, Namhyung Kim wrote:
> >>>> Hello,
> >>>>
> >>>> I found some problems in Intel-PT and auxtrace in general with pipe.
> >>>> In the past it used to work with pipe, but recent code fails.
> >>>
> >>> Pipe mode is a problem for Intel PT and possibly other auxtrace users.
> >>
> >> Just some info from my side: For Arm Coresight we ended up deprecating
> >> pipe mode, then not supporting it altogether. First was when we added an
> >> optional step to peek through all of the data to help with an edge case.
> >> Then we added a requirement to receive a HW_ID packet before decoding
> >> which necessitated the peek. You can't peek in pipe mode because you
> >> need to be able to seek, so it's not supported at all anymore.
> >>
> >> For Arm SPE I never tested it with piped data. I suppose I could add a
> >> test at some point, but I don't really see the usecase.
> >
> > Yeah, it'd be great if we can have a test for Arm SPE.
> >
>
> Ok thanks I will put it on the list of things to do.
>
> > Anyway, my work env (Google) requires the pipe mode due to the
> > restriction in disk usage.  Without the pipe support, it's not possible
> > to run `perf record` in production.
> >
>
> Makes sense. Unfortunately at the moment with Coresight, because of the
> lack of appropriate timestamps we're waiting for the end of the file
> before starting decoding. So you're not really any better off using
> piped mode, unless you have a lot more memory than disk space?
>
> Since this commit [1] and Arm v8.4 we can actually start making use of
> the timestamps and do a streaming decode again. So I will also add it to
> the list to look into that for Coresight again. Are you using an old
> version of Perf or not using Coresight at all? I know Denis at Google is
> using Coresight, but only with files rather than pipes.

I'm not aware of usage of Coresight yet in my boundary, but others
may be using it.

>
> One other thing, have you used the --switch-output mode to perf record
> before? I would have said it would give you some of the benefits of
> piped mode, but is more likely to work with Coresight. But last time I
> checked it's not working either. Not very helpful I know, but something
> to keep in mind.

I don't think it'd work because it still occupies the same space.
So far, the pipe mode worked well but I think it needs some
more improvements.

Anyway, thanks for your suggestion and review!

Thanks,
Namhyung

Re: [PATCH 0/4] perf intel-pt: Fix the pipe mode (v1)

[Namhyung Kim]

On Mon, Jan 30, 2023 at 9:36 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 30/01/23 16:15, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Jan 27, 2023 at 02:54:36PM -0800, Namhyung Kim escreveu:
> >> Hi Adrian,
> >>
> >> On Thu, Jan 26, 2023 at 11:22 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>
> >>> On 27/01/23 02:19, Namhyung Kim wrote:
> >>>> Hello,
> >>>>
> >>>> I found some problems in Intel-PT and auxtrace in general with pipe.
> >>>> In the past it used to work with pipe, but recent code fails.
> >>>
> >>> Pipe mode is a problem for Intel PT and possibly other auxtrace users.
> >>> Essentially the auxtrace buffers do not behave like the regular perf
> >>> event buffers.  That is because the head and tail are updated by
> >>> software, but in the auxtrace case the data is written by hardware.
> >>> So the head and tail do not get updated as data is written.  In the
> >>> Intel PT case, the head and tail are updated only when the trace is
> >>> disabled by software, for example:
> >>>     - full-trace, system wide : when buffer passes watermark
> >>>     - full-trace, not system-wide : when buffer passes watermark or
> >>>     context switches
> >>>     - snapshot mode : as above but also when a snapshot is made
> >>>     - sample mode : as above but also when a sample is made
> >>>
> >>> That means finished-round ordering doesn't work.  An auxtrace buffer
> >>> can turn up that has data that extends back in time, possibly to the
> >>> very beginning of tracing.
> >>
> >> Ok, IIUC we want to process the main buffer and auxtrace buffer
> >> together in time order but there's no guarantee to get the auxtrace
> >> data in time, right?
>
> Yes
>
> >>
> >> I wonder if it's possible to use 2 pass processing for pipe mode.
> >> We may keep the events in the ordered queue and auxtrace queue
> >> in the first pass, and process together from the beginning in the
> >> second pass. But I guess the data size would be a problem.
> >>
> >> Or, assuming that the auxtrace buffer comes later than (or equal to)
> >> the main buffer, we may start processing the main buffer as soon as
> >> every auxtrace queue gets some data.  Thoughts?
>
> That sounds like it would require figuring out a timestamp up to
> which there is Intel PT trace data in all queues.  That would
> be very complicated.

Yeah, I don't think it's a simple change.  Just think out loud how
we can handle it.  I'll think about it more..

>
> >>
> >>>
> >>> For a perf.data file, that problem is solved by going through the trace
> >>> and queuing up the auxtrace buffers in advance.
> >>>
> >>> For pipe mode, the order of events and timestamps can presumably
> >>> be messed up.
> >>>
> >>> For Intel PT, it is a bit of a surprise that there is not
> >>> validation to error out in pipe mode.
> >>
> >> What kind of validation do you have in mind?  Checking pid/tid?
>
> Validation to kill pipe mode for Intel PT entirely.  But a warning
> is ok.

I see.

>
> >>
> >>>
> >>> At the least, a warning is needed, and the above explanation needs
> >>> to be added to the documentation.
> >>
> >> Thanks, I'll add it to the documentation.
> >
> > Ok, so I'll wait for v2 of this patch series, Adrian, apart from what
> > you mentioned, are you ok with the patches, or a subset of them? The
> > first ones looks ok, right?
>
> Yes they are ok.
>
> >
> > - Arnaldo
> >
> >> How about showing something like this for pipe mode?
> >>
> >>   WARNING: Intel-PT with pipe mode may not work correctly.
>
> Perhaps:
>
> WARNING: Intel PT with pipe mode is not recommended. The output cannot be relied upon. In particular, time stamps and the order of events may be incorrect.

Ok, will add this.

Thanks,
Namhyung

[PATCH 3/4] perf session: Avoid calling lseek(2) for pipe

[Namhyung Kim]

We should not call lseek(2) for pipes as it won't work.  And we already
in the proper place to read the data for AUXTRACE.  Add the comment like
in the PERF_RECORD_HEADER_TRACING_DATA.

Reviewed-by: James Clark <james.clark@arm.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/session.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 7c021c6cedb9..fdfe772f2699 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1699,8 +1699,13 @@ static s64 perf_session__process_user_event(struct perf_session *session,
 	case PERF_RECORD_AUXTRACE_INFO:
 		return tool->auxtrace_info(session, event);
 	case PERF_RECORD_AUXTRACE:
-		/* setup for reading amidst mmap */
-		lseek(fd, file_offset + event->header.size, SEEK_SET);
+		/*
+		 * Setup for reading amidst mmap, but only when we
+		 * are in 'file' mode.  The 'pipe' fd is in proper
+		 * place already.
+		 */
+		if (!perf_data__is_pipe(session->data))
+			lseek(fd, file_offset + event->header.size, SEEK_SET);
 		return tool->auxtrace(session, event);
 	case PERF_RECORD_AUXTRACE_ERROR:
 		perf_session__auxtrace_error_inc(session, event);
-- 
2.39.1.456.gfc5497dd1b-goog