From: Ingo Molnar <mingo@kernel.org> To: Michael Ellerman <mpe@ellerman.id.au> Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>, mingo@redhat.com, ak@linux.intel.com, Jiri Olsa <jolsa@redhat.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, peterz@infradead.org, namhyung@kernel.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Pekka Enberg <penberg@iki.fi>, Arnaldo Carvalho de Melo <acme@infradead.org>, Peter Zijlstra <a.p.zijlstra@chello.nl>, Thomas Gleixner <tglx@linutronix.de>, Linus Torvalds <torvalds@linux-foundation.org>, Borislav Petkov <bp@alien8.de> Subject: Re: 'perf upgrade' (was: Re: [PATCH v9 00/11] Add support for JSON event files.) Date: Wed, 15 Apr 2015 11:25:58 +0200 [thread overview] Message-ID: <20150415092557.GA14942@gmail.com> (raw) In-Reply-To: <1429052004.10000.1.camel@ellerman.id.au> * Michael Ellerman <mpe@ellerman.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
WARNING: multiple messages have this Message-ID (diff)
From: Ingo Molnar <mingo@kernel.org> To: Michael Ellerman <mpe@ellerman.id.au> Cc: Arnaldo Carvalho de Melo <acme@infradead.org>, ak@linux.intel.com, Peter Zijlstra <a.p.zijlstra@chello.nl>, peterz@infradead.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Arnaldo Carvalho de Melo <acme@kernel.org>, Linus Torvalds <torvalds@linux-foundation.org>, mingo@redhat.com, Borislav Petkov <bp@alien8.de>, namhyung@kernel.org, Thomas Gleixner <tglx@linutronix.de>, Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>, Jiri Olsa <jolsa@redhat.com>, Pekka Enberg <penberg@iki.fi> Subject: Re: 'perf upgrade' (was: Re: [PATCH v9 00/11] Add support for JSON event files.) Date: Wed, 15 Apr 2015 11:25:58 +0200 [thread overview] Message-ID: <20150415092557.GA14942@gmail.com> (raw) In-Reply-To: <1429052004.10000.1.camel@ellerman.id.au> * Michael Ellerman <mpe@ellerman.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
next prev parent reply other threads:[~2015-04-15 9:26 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-04-14 6:40 [PATCH v9 00/11] Add support for JSON event files Sukadev Bhattiprolu 2015-04-14 6:40 ` Sukadev Bhattiprolu 2015-04-14 6:40 ` [PATCH v9 01/11] perf, tools: Add jsmn `jasmine' JSON parser Sukadev Bhattiprolu 2015-04-14 6:40 ` Sukadev Bhattiprolu 2015-04-14 6:40 ` [PATCH v9 02/11] perf, tools: Add support for text descriptions of events and alias add Sukadev Bhattiprolu 2015-04-14 6:40 ` Sukadev Bhattiprolu 2015-04-14 6:40 ` [PATCH v9 03/11] perf, tools, list: Update perf list to output descriptions Sukadev Bhattiprolu 2015-04-14 6:40 ` Sukadev Bhattiprolu 2015-04-14 6:40 ` [PATCH v9 04/11] perf, tools: Add support for reading JSON event files Sukadev Bhattiprolu 2015-04-14 6:40 ` Sukadev Bhattiprolu 2015-04-14 6:40 ` [PATCH v9 05/11] perf, tools: Automatically look for event file name for cpu Sukadev Bhattiprolu 2015-04-14 6:40 ` Sukadev Bhattiprolu 2015-04-14 6:40 ` [PATCH v9 06/11] powerpc/perf: Implement get_cpu_str() Sukadev Bhattiprolu 2015-04-14 6:40 ` Sukadev Bhattiprolu 2015-04-14 6:41 ` [PATCH v9 07/11] perf, tools: Query terminal width and use in perf list Sukadev Bhattiprolu 2015-04-14 6:41 ` Sukadev Bhattiprolu 2015-04-14 6:41 ` [PATCH v9 08/11] perf, tools: Add a new pmu interface to iterate over all events Sukadev Bhattiprolu 2015-04-14 6:41 ` Sukadev Bhattiprolu 2015-04-14 6:41 ` [PATCH v9 09/11] perf, tools, test: Add test case for alias and JSON parsing Sukadev Bhattiprolu 2015-04-14 6:41 ` Sukadev Bhattiprolu 2015-04-14 6:41 ` [PATCH v9 10/11] perf, tools: Add a --no-desc flag to perf list Sukadev Bhattiprolu 2015-04-14 6:41 ` Sukadev Bhattiprolu 2015-04-14 6:41 ` [PATCH v9 11/11] perf-download: Download the events json file Sukadev Bhattiprolu 2015-04-14 6:41 ` Sukadev Bhattiprolu 2015-04-14 8:55 ` 'perf upgrade' (was: Re: [PATCH v9 00/11] Add support for JSON event files.) Ingo Molnar 2015-04-14 8:55 ` Ingo Molnar 2015-04-14 11:21 ` Michael Ellerman 2015-04-14 11:21 ` Michael Ellerman 2015-04-14 12:58 ` Ingo Molnar 2015-04-14 12:58 ` Ingo Molnar 2015-04-14 18:03 ` Sukadev Bhattiprolu 2015-04-14 18:03 ` Sukadev Bhattiprolu 2015-04-14 22:53 ` Michael Ellerman 2015-04-14 22:53 ` Michael Ellerman 2015-04-15 9:25 ` Ingo Molnar [this message] 2015-04-15 9:25 ` Ingo Molnar 2015-04-15 19:17 ` Andi Kleen 2015-04-15 19:17 ` Andi Kleen 2015-04-15 20:50 ` Sukadev Bhattiprolu 2015-04-15 20:50 ` Sukadev Bhattiprolu 2015-04-17 15:31 ` Jiri Olsa 2015-04-17 15:31 ` Jiri Olsa 2015-04-17 20:09 ` Andi Kleen 2015-04-17 20:09 ` Andi Kleen 2015-04-18 13:05 ` Jiri Olsa 2015-04-18 13:05 ` Jiri Olsa 2015-04-18 13:12 ` Jiri Olsa 2015-04-18 13:12 ` Jiri Olsa 2015-04-14 20:16 ` Andi Kleen 2015-04-14 20:16 ` Andi Kleen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20150415092557.GA14942@gmail.com \ --to=mingo@kernel.org \ --cc=a.p.zijlstra@chello.nl \ --cc=acme@infradead.org \ --cc=acme@kernel.org \ --cc=ak@linux.intel.com \ --cc=bp@alien8.de \ --cc=jolsa@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mingo@redhat.com \ --cc=mpe@ellerman.id.au \ --cc=namhyung@kernel.org \ --cc=penberg@iki.fi \ --cc=peterz@infradead.org \ --cc=sukadev@linux.vnet.ibm.com \ --cc=tglx@linutronix.de \ --cc=torvalds@linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.