Message-ID: <20180116175806.GA10320@xps15>
Date:   Tue, 16 Jan 2018 10:58:06 -0700
From:   Mathieu Poirier <mathieu.poirier@...aro.org>
To:     Kim Phillips <kim.phillips@....com>
Cc:     Mike Leach <mike.leach@...aro.org>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-kernel@...r.kernel.org,
        Adrian Hunter <adrian.hunter@...el.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        "Jeremiassen, Tor" <tor@...com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Mark Brown <broonie@...nel.org>,
        Mike Leach <mike.leach@....com>, namhyung@...nel.org,
        suzuki.poulosi@....com, Jiri Olsa <jolsa@...hat.com>,
        Ingo Molnar <mingo@...hat.com>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 00/10] perf tools: Add support for CoreSight trace
 decoding

On Tue, Jan 16, 2018 at 11:01:02AM -0600, Kim Phillips wrote:
> On Tue, 16 Jan 2018 12:35:26 +0000
> Mike Leach <mike.leach@...aro.org> wrote:
> 
> > Hi,
> > 
> > On 11 January 2018 at 22:18, Mathieu Poirier <mathieu.poirier@...aro.org> wrote:
> > > On 11 January 2018 at 14:49, Kim Phillips <kim.phillips@....com> wrote:
> > >> On Thu, 11 Jan 2018 14:11:00 -0700
> > >> Mathieu Poirier <mathieu.poirier@...aro.org> wrote:
> > >>
> > >>> On 11 January 2018 at 10:28, Kim Phillips <kim.phillips@....com> wrote:
> > >>> > On Thu, 11 Jan 2018 08:45:21 -0700
> > >>> > Mathieu Poirier <mathieu.poirier@...aro.org> wrote:
> > >>> >
> > >>> >> On 11 January 2018 at 05:23, Mark Brown <broonie@...nel.org> wrote:
> > >>> >> > On Wed, Jan 10, 2018 at 06:08:21PM -0600, Kim Phillips wrote:
> > >>> >> >> Mathieu Poirier <mathieu.poirier@...aro.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.

Again, we don't need to do all that if we keep the library separate.

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

It would be extremely time consuming to start splitting the library by
functionality and then, incrementally start (re)adding the features.  I rather
spend that time working on new functionality and making the CoreSight subsystem
better.

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

Once again I think you're blowing the down side out of proportion.  Like Mark
Brown said we have to start somewhere.  Once we have this in work on proper
packaging will start immediately.

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

There's nothing going out of sync - until we have proper packaging people simply
need to recompile their perf utility.  As pointed out before that wouldn't be
hard to do for people using hardware assisted tracing.

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

Not true: kernel subsystem get out of sync regularly, this wouldn't be any
different.


> 
> Kim

Powered by blists - more mailing lists