Re: 'perf upgrade' (was: Re: [PATCH v9 00/11] Add support for JSON event files.)

[Sukadev Bhattiprolu]

| 
| * Michael Ellerman <mpe@xxxxxxxxxxxxxx> 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.

Yes, we are trying to solve one really annoying problem without precluding
the ability to do more (aka leave things better than they are even if we
can't solve everything).

Besides, I thought the discussion was where to host the JSON files.
I was not aware of the requirement to download/rebuild perf or not use
JSON at all (i.e generate data structures during build?).


|    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

Thats the part, I am having trouble with.

If the JSON files are like /etc/perfconfig, we don't need to rebuild the
binary when a config file changes right?

Second, if I build perf on my Power8 box, would the perf binary only include
events on my specific revision of the CPU or all Power8 versions?  Would the
perf binary from Power8 include events from all revisions of Power7 and
Power8?

If we include all possible revisions of the CPU, we would bloat perf.

If not we break the ability/convenience of copying rpms from one system
to the other? And we need to rebuild perf if the CPU on my system is
replaced and there are a couple of new event codes in the new CPU?


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

Evil vendor formats, but to be fair, here is what _we_ have today:

	perf stat -e r10068,r20036,r40060,r40ac sleep 1

	 Performance counter stats for 'sleep 1':

		   554,608      r10068                                                      
		    47,948      r20036                                                      
		   491,084      r40060                                                      
		       249      r40ac                                                       

	       1.001561257 seconds time elapsed

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

True, having events like 'cycles', 'branches' makes sense, but there
are so many different flavors of "run cycles" events in just Power8.

    PM_ANY_THRD_RUN_CYC,
    PM_MRK_RUN_CYC,
    PM_RUN_CYC,
    PM_RUN_CYC_SMT2_MODE,
    PM_RUN_CYC_SMT2_SHRD_MODE,
    PM_RUN_CYC_SMT2_SPLIT_MODE,
    PM_RUN_CYC_SMT4_MODE,
    PM_RUN_CYC_SMT8_MODE,
    PM_RUN_CYC_ST_MODE,
    PM_THRD_ALL_RUN_CYC,
    PM_TM_TRANS_RUN_CYC,
    PM_TM_TX_PASS_RUN_CYC,

Are we trying to build a high level structures across architectures?

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

A bit confused.

Have the JSON files in the tree and generate the C structure during
build?

Or, ditch the JSON files and add something like this in say,
tools/perf/arch/powerpc/util/power8-events.h?

static const  struct events power8_events[] = {
    [ 0 ] = {
        .name = "PM_1LPAR_CYC",
        .code = 0x1f05e,
        .brief_desc = " "Number of cycles in single lpar mode. All threads in the core are assigned to the same lpar,",
        .public_desc = "Number of cycles in single lpar mode.,",
      },

If we have the JSON files, would the 'make install' put the JSON files
in ~/.cache/pmu-events or in a standard location?

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

I will try the perf upgrade below,  but the only clear use case I have
is for the event files. I am a little confused about the use case for
downloading and rebuilding perf. 

Building perf requires more packages than simply running perf. Most
users just run perf without needing to rebuild.

Running perf doesn't require root access but if we want to install
JSON files in standard locations (to get latest event codes for
instance) we will need root access.

Or should ~/.cache/pmu-events/*.json complement the event codes builtin
to the perf binary?

Sukadev

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