Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751487AbeAPR6d (ORCPT + 1 other); Tue, 16 Jan 2018 12:58:33 -0500 Received: from mail-it0-f54.google.com ([209.85.214.54]:41258 "EHLO mail-it0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751238AbeAPR6L (ORCPT ); Tue, 16 Jan 2018 12:58:11 -0500 X-Google-Smtp-Source: ACJfBotaRUWj4oP2Ys/GsuyDYBboWw8uRNU2gKgdp3nyDliWbOdkkqdalrEAyy6+k4GfD9LXdMrEgw== Date: Tue, 16 Jan 2018 10:58:06 -0700 From: Mathieu Poirier To: Kim Phillips Cc: Mike Leach , Peter Zijlstra , linux-kernel@vger.kernel.org, Adrian Hunter , Arnaldo Carvalho de Melo , "Jeremiassen, Tor" , Alexander Shishkin , Mark Brown , Mike Leach , namhyung@kernel.org, suzuki.poulosi@arm.com, Jiri Olsa , Ingo Molnar , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 00/10] perf tools: Add support for CoreSight trace decoding Message-ID: <20180116175806.GA10320@xps15> References: <1513356299-26274-1-git-send-email-mathieu.poirier@linaro.org> <20180110180821.a92368599af8708790e0362b@arm.com> <20180111122317.GA7834@sirena.org.uk> <20180111112835.1511702c2e6ba0077af4112e@arm.com> <20180111154942.37120115a94cc77d8f336368@arm.com> <20180116110102.3bec73e7c3d8d1fc1dfd6c59@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180116110102.3bec73e7c3d8d1fc1dfd6c59@arm.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Tue, Jan 16, 2018 at 11:01:02AM -0600, Kim Phillips wrote: > On Tue, 16 Jan 2018 12:35:26 +0000 > Mike Leach wrote: > > > Hi, > > > > On 11 January 2018 at 22:18, Mathieu Poirier wrote: > > > On 11 January 2018 at 14:49, Kim Phillips wrote: > > >> On Thu, 11 Jan 2018 14:11:00 -0700 > > >> Mathieu Poirier wrote: > > >> > > >>> On 11 January 2018 at 10:28, Kim Phillips wrote: > > >>> > On Thu, 11 Jan 2018 08:45:21 -0700 > > >>> > Mathieu Poirier wrote: > > >>> > > > >>> >> On 11 January 2018 at 05:23, Mark Brown wrote: > > >>> >> > On Wed, Jan 10, 2018 at 06:08:21PM -0600, Kim Phillips wrote: > > >>> >> >> Mathieu Poirier 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