Re: [PATCH 00/10] perf tools: Add support for CoreSight trace decoding

[Kim Phillips <kim.phillips@arm.com>]

On Tue, 16 Jan 2018 12:35:26 +0000
Mike Leach <mike.leach@linaro.org> wrote:

> Hi,
> 
> On 11 January 2018 at 22:18, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> > On 11 January 2018 at 14:49, Kim Phillips <kim.phillips@arm.com> wrote:
> >> On Thu, 11 Jan 2018 14:11:00 -0700
> >> Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> >>
> >>> On 11 January 2018 at 10:28, Kim Phillips <kim.phillips@arm.com> wrote:
> >>> > On Thu, 11 Jan 2018 08:45:21 -0700
> >>> > Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> >>> >
> >>> >> On 11 January 2018 at 05:23, Mark Brown <broonie@kernel.org> wrote:
> >>> >> > On Wed, Jan 10, 2018 at 06:08:21PM -0600, Kim Phillips wrote:
> >>> >> >> Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> >>> >> >
> >>> >> >> > Instructions on how to build and install the openCSD library are provided
> >>> >> >> > in the HOWTO.md of the project repository.
> >>> >> >
> >>> >> >> Usually when a perf builder sees something they need "on," they - or,
> >>> >> >> at least I - start querying the host's package manager for something
> >>> >> >> that provides it (e.g., apt search/install libopencsd), but since no
> >>> >> >> distro provides libopencsd, this is bad because it misleads the user.
> >>> >> >
> >>> >> > It's on the radar to push this at distros fairly soon.
> >>> >
> >>> > Adding packages to distros takes years, this patchset is being
> >>> > submitted for inclusion *now*.  So until then, it would greatly
> >>> > facilitate users if the relevant libopencsd source files were
> >>> > self-contained within perf from the get go.
> >>>
> >>> I do not agree with you on the front that it takes years.  On the flip
> >>> side it would take a significant amount of time and effort to refactor
> >>> the openCSD library so that it can be added to the kernel tree.  This
> >>
> >> The dtc wasn't refactored before it was added to the kernel tree.
> 
> The openCSD library consists of c. 140 C++ source and header files,
> plus a few headers to form the libraries C-API, formatted in what is
> an accepted standard within ARM - indent of 4 spaces and no tabs.
> Now, afaik, this is unlikely to be acceptable to the kernel tree.

like I said, the refactoring (restyling?) shouldn't have to be
necessary, as was with the dtc.  Plus, only the necessary files would
be imported by the update script: a figure an order of magnitude less
than 140.

> The decision to use C++ was made at the start of this project - the

I don't think that matters either.

> Unlike intel PT, that has a single protocol, tightly bound to the core
> it is run on, (similar to SPE in this respect), the library supports
> multiple trace protocols, which are configured and controlled
> independently of the cores that generate them. The library supports
> all extant protocols - ETMv3 & 4, PTM and STM, plus the ability to
> de-multiplex individual trace streams from the CS trace frames,
> decoding these into a common generic standard, removing the need for
> tooling to aware of the underlying protocol. Furthermore, given that

So if we limited the first submission to just the relevant ETMv4 files
imported, we'd start off in the same place as Intel PT: a single
protocol.  Not that it's a crime to extend it later to newer versions
of the ETM protocol, and possibly backport the older stuff at a later
date.  The object here is to enable the current-generation ETM user wrt
perf decoding as seamlessly and as easily as possible.

> CoreSight hardware can support proprietary trace protocols (e.g. DSPs)

Likewise, we're not concerned about these cases in the first submission
of the ETM decoder patch to perf.

> The library currently supports ETMv4.1 -> we have a requirement to
> support v4.3 once these cores begin to appear, and it is inevitable
> that the trace protocols will develop as the ARM architecture
> develops. Adding the decoder library to the kernel tree will result in
> code churn that is unnecessary for the kernel tree as the library is
> developed, especially if functionality is added that is not used by A
> class cores (see comment below re data trace).

Generally, if new hardware needs to be supported, the kernel tree will
accept patches to support it.

During development, the developers are free to maintain public git
repos with their work-in-progress, as usual, for those seeking early
access to the new feature.

> >>> patchset is available now with a solution that follows what has
> >>> already been done for dozens of other external library.  There is no
> >>> point in delaying the inclusion of the functionality when an
> >>> end-to-end solution exists.
> >>
> >> See above: I'm not necessarily suggesting the code get refactored.
> >>
> >>> >> >  Part of the
> >>> >> > discussion was wanting to get things to the point where the tools using
> >>> >> > the library were far enough along that we could be reasonably sure that
> >>> >
> >>> > Curious, what other tools are there?
> >>>
> >>> Ask around at ARM.
> >>
> >> I'm asking the person that claimed it.
> 
> The intent is to use openCSD as a decoder within existing ARM tools -
> though the library will need to be extended to decode Data trace
> before this is possible. (ARM A class processors are instruction trace
> only. )

So there are no other users in the linux ecosystem, meaning nothing
is gained by packaging it in distros, and all the more reason to enable
it by directly importing it into perf, so users get all the benefit
without the wait and without the hassle of having to manually install
the library and worry about compatibility between perf.

> Additionally the library test program itself is in fact a useful tool
> for investigating trace capture / corruption / setup errors in
> hardware. It is capable of listing trace packets from snapshots
> captured by DS-5 or the open source CoreSight Access Library.

Like I said above, the libopencsd files perf doesn't need don't need to
be imported by the update script.

> >>> >> > there weren't any problems that were going to require ABI breaks to fix
> >>> >> > before pushing the library at distros since ABI churn isn't nice for
> >>> >> > packagers to deal with.
> >>> >
> >>> > Why make perf the guinea pig?  Whatever, this doesn't preclude
> >>> > adding the code into the tree; it can be removed years from now when
> >>> > libopencsd becomes ubiquitous among distros.
> >>>
> >>> The same can be said about proceeding the other way around - the
> >>> openCSD library can be added to the kernel tree later if it is deemed
> >>> necessary.  Until then I really don't see why we'd prevent people from
> >>> accessing the functionality.
> >>
> >> Again, I'm not suggesting the code be refactored...
> >>
> >>> >> > There's also a bit of a chicken and egg problem
> >>> >> > in that it's a lot easier to get distros to package libraries that have
> >>> >> > users available (some are not really bothered about this of course but
> >>> >> > it still helps).
> >>> >>
> >>> >> Moreover including in the kernel tree every library that can
> >>> >> potentially be used by the perf tools simply doesn't scale.
> >>> >
> >>> > This is a trace decoder library we're talking about:  there are no
> >>> > others in perf's system features autodetection list.  And why wouldn't
> >>> > adding such libraries scale?
> >>>
> >>> I don't see why a decoder library and say, libelf, need to be treated
> >>> differently.
> >>
> >> libelf is a mature library based on an industry-wide standard, not to
> >> mention already packaged by most (all?) distros.
> 
> We re-factored the library name and the layout of the C-API necessary
> header files in the latest release (v0.8) to allow for installation
> into a linux system and the build changes made to perf.
> Now, while it is true that we need to be careful that users are aware
> they need to use the latest release, in general if the library is not
> present on a system; if is not part of the distribution, it is very
> easy to find using a simple search.

The way it's being done in this patchseries, users are going to query
their package managers for libopencsd and not find it: very
misleading.  To avoid having to manually install the library and
inevitably have it go out of sync with perf, it'd behoove us to
include the sources directly as of now.

> Frankly whenever I appear to be
> missing a library in linux the first thing I do is a web search, to
> figure out what package I am missing (since it is not always obvious
> to me what libraries lie in which packages).
> 
> Google 'libopencsd' and you see the github HOWTO.md for OpenCSD very quickly.
> 
> I think users interested in using the library will have the know-how
> to find an install it if the perf build process marks it as missing.
> The HOWTO will be maintained as part of the library to explain the
> installation and integration with perf.

We can avoid all of this.

> Given all the above, I cannot see any engineering benefit from adding
> the source to the kernel tree. We have a stable and maintainable
> solution with the library added to the perf build as an external
> dependency.

That's not true: the perf and libopencsd sources can get out of sync,
whereas if the perf-dependent files of libopencsd were maintained along
with the perf source itself, that would not be possible.

Kim