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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 30 Jan 2023 19:35:39 +0200
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>
Cc:     Jiri Olsa <jolsa@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ian Rogers <irogers@...gle.com>,
        linux-perf-users@...r.kernel.org, Leo Yan <leo.yan@...aro.org>,
        James Clark <james.clark@....com>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 0/4] perf intel-pt: Fix the pipe mode (v1)

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@...el.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
>>>
>