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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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