Message-ID: <20150415092557.GA14942@gmail.com>
Date:	Wed, 15 Apr 2015 11:25:58 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Michael Ellerman <mpe@...erman.id.au>
Cc:	Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>, mingo@...hat.com,
	ak@...ux.intel.com, Jiri Olsa <jolsa@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	peterz@...radead.org, namhyung@...nel.org,
	linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
	Pekka Enberg <penberg@....fi>,
	Arnaldo Carvalho de Melo <acme@...radead.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Thomas Gleixner <tglx@...utronix.de>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Borislav Petkov <bp@...en8.de>
Subject: Re: 'perf upgrade' (was: Re: [PATCH v9 00/11] Add support for JSON
 event files.)


* Michael Ellerman <mpe@...erman.id.au> wrote:

> > We just merged a patch series that was first sent in 2013. Some 
> > things take time to get right.
> 
> The first attempt to get symbolic event name support into perf was 
> sent in 2010, that's FIVE years ago [1].

kgdb took even longer, I think it was first proposed before 2000, over 
10 years before it got merged?

fs/overlayfs/ took similarly long I think, the first (Unionfs) patches 
I remember were from around 2003 - 11 years before the functionality 
was merged?

> And what complicated feature are we asking for? The ability to map a 
> human readable name to a hex code, it has the complexity of a first 
> year programming assignment.

No, what you are asking for, and which I NAK-ed, is:

 - to add a very limited 'update perf' capability which only updates a
   single issue that you care about, with no ability to do more.
   The 'perf upgrade' prototype I sent can update all or part of perf. 
   (The latest version is attached further below.)

 - to break the 'single binary' property of perf that many people rely on

 - to add JSON parsing overhead to every invocation of perf instead of
   pre-parsing the event tables at build time and putting them into 
   a nice data structure.

 - to blindly follow some poorly constructed vendor format with no 
   high level structure, that IMHO didn't work very well when OProfile 
   was written, and misrepresenting it as 'symbolic event names'.

   Take a look at:

     https://download.01.org/perfmon/HSW/Haswell_core_V17.json

   and weep. How is one supposed to see the higher level structure of
   the events with a format like that?

 - to add an ABI to support those vendor files

And those are IMHO five good technical reasons to disagree with your 
approach.

My suggestion to resolve the technical objections and lift the NAK 
would be:

 - to add the tables to the source code, in a more human readable 
   format and (optionally) structure the event names better into a 
   higher level hierarchy, than the humungous linear dumps with no 
   explanations that you propose - while still supporting the 'raw' 
   vendor event names you want to use, for those people who are used 
   to them.

 - to pre-parse the event descriptions at build time - beyond the 
   speedup this also keeps the 'single binary' property of perf.

 - to upgrade perf as a whole unit: this helps not just your usecase
   but many other usecases as well.

   For those who only want to update event tables: with
   'perf upgrade stable' basically only new event tables (backported 
   to -stable) would be picked up, plus regression fixes: it pretty 
   much does what your proposed 'perf download' solution does, except 
   it's much more capable.

Saying 'no' and suggesting better solutions is my job as a maintainer.

Thanks,

	Ingo

======={ perf-upgrade.sh }===========>

#!/bin/bash
#
# Simple script that fetches the 'perf' utility from Linus's tree, builds and
# installs it, by creating a shallow clone and a sparse checkout for Linux's
# tools/ directory and related dependencies:
#

DIR=~/.perf/git

rm -rf $DIR
mkdir -p $DIR || { echo 'error: No write permissions in the current directory?'; exit -1; }
cd $DIR

#REPO=git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
#BRANCH=HEAD
REPO=git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
BRANCH=perf/core

REFERENCE="."
[ -d ~/linux/.git     ] && REFERENCE=~/linux/
[ -d ~/linux.git/.git ] && REFERENCE=~/linux.git/
[ -d ~/tip/.git       ] && REFERENCE=~/tip/
[ -d ~/tip.git/.git   ] && REFERENCE=~/tip.git/
[ -d ~/git/linux      ] && REFERENCE=~/git/linux/

git clone --reference $REFERENCE --depth 1 --bare $REPO --branch $BRANCH .git
git config --local core.bare false

git config --local core.sparseCheckout true
git remote remove origin
git remote add -f origin $REPO -t $BRANCH

(
  echo '/tools/*'
  echo '/lib/*'
  echo '/include/*'
  echo '/arch/x86/lib/*'
  echo '/arch/x86/include/*'
  echo 'Makefile'
  echo '/scripts/*'

) > .git/info/sparse-checkout

git checkout $BRANCH

make -C tools/perf/ install

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists