Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1576478 > unrolled thread

Re: [PATCH 10/10] perf, tools, stat: Output JSON MetricExpr metric

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 10/10] perf, tools, stat: Output JSON MetricExpr metric Jiri Olsa <jolsa@redhat.com> - 2017-02-08 12:40 +0100
    Re: [PATCH 10/10] perf, tools, stat: Output JSON MetricExpr metric Andi Kleen <andi@firstfloor.org> - 2017-02-08 23:00 +0100
      Re: [PATCH 10/10] perf, tools, stat: Output JSON MetricExpr metric Jiri Olsa <jolsa@redhat.com> - 2017-02-09 12:50 +0100
        Re: [PATCH 10/10] perf, tools, stat: Output JSON MetricExpr metric Andi Kleen <ak@linux.intel.com> - 2017-02-09 18:20 +0100
          Re: [PATCH 10/10] perf, tools, stat: Output JSON MetricExpr metric Jiri Olsa <jolsa@redhat.com> - 2017-02-09 19:50 +0100
            Re: [PATCH 10/10] perf, tools, stat: Output JSON MetricExpr metric Andi Kleen <andi@firstfloor.org> - 2017-02-09 20:10 +0100
              Re: [PATCH 10/10] perf, tools, stat: Output JSON MetricExpr metric Jiri Olsa <jolsa@redhat.com> - 2017-02-10 09:30 +0100
      Re: [PATCH 10/10] perf, tools, stat: Output JSON MetricExpr metric Jiri Olsa <jolsa@redhat.com> - 2017-02-09 13:00 +0100

#1576478 — Re: [PATCH 10/10] perf, tools, stat: Output JSON MetricExpr metric

On Fri, Jan 27, 2017 at 06:03:45PM -0800, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Add generic infrastructure to perf stat to output ratios for "MetricExpr"
> entries in the event lists. Many events are more useful as ratios
> than in raw form, typically some count in relation to total ticks.
> 
> Transfer the MetricExpr information from the alias to the evsel.
> 
> We mark the events that need to be collected for MetricExpr, and also
> link the events using them with a pointer. The code is careful
> to always prefer the right event in the same group to minimize
> multiplexing errors. At the moment only a single relation is supported.
> 
> Then add a rblist to the stat shadow code that remembers stats based
> on the cpu and context.
> 
> Then finally update and retrieve and print these values similarly to the
> existing hardcoded perf metrics. We use the simple expression parser
> added earlier to evaluate the expression.
> 
> Normally we just output the result without further commentary,
> but for --metric-only this would lead to empty columns. So for this
> case use the original event as description.
> 
> So far there is no attempt to automatically add the MetricExpr event,
> if it is missing, however we suggest it to the user.
> 
> $ perf stat -a -I 1000 -e '{unc_p_clockticks,unc_p_freq_max_os_cycles}'
>      1.000228813        800,139,950      unc_p_clockticks
>      1.000228813        789,833,783      unc_p_freq_max_os_cycles  #     98.7
>      2.000654229        800,308,990      unc_p_clockticks
>      2.000654229        396,214,238      unc_p_freq_max_os_cycles  #     49.5

[jolsa@krava perf]$ ./perf stat -a -I 1000 -e '{unc_p_clockticks,unc_p_freq_max_os_cycles}'
invalid or unsupported event: '{unc_p_clockticks,unc_p_freq_max_os_cycles}'
Run 'perf list' for a list of valid events

could you show an example of the MetricExpr?

it's part of the event record, what if you wanted to have 2 or more metrics defined for event?

who defines those expressions?

what if you dont provide the necessary events needed for the expression?

shouldnt we do it the other way around? like pick an expression
we are interested in and perf will configure the needful..

> 
> $ perf stat -a -I 1000 -e '{unc_p_clockticks,unc_p_freq_max_os_cycles}' --metric-only
>      1.000206740     48.0
>      2.000451543     48.1

how does user know what those number stand for?

is there a way for user to display the metric expression?

it's still feels to me like a hack without much concept behind

jirka

[toc] | [next] | [standalone]

#1577130

On Wed, Feb 08, 2017 at 12:31:34PM +0100, Jiri Olsa wrote:
> On Fri, Jan 27, 2017 at 06:03:45PM -0800, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > Add generic infrastructure to perf stat to output ratios for "MetricExpr"
> > entries in the event lists. Many events are more useful as ratios
> > than in raw form, typically some count in relation to total ticks.
> > 
> > Transfer the MetricExpr information from the alias to the evsel.
> > 
> > We mark the events that need to be collected for MetricExpr, and also
> > link the events using them with a pointer. The code is careful
> > to always prefer the right event in the same group to minimize
> > multiplexing errors. At the moment only a single relation is supported.
> > 
> > Then add a rblist to the stat shadow code that remembers stats based
> > on the cpu and context.
> > 
> > Then finally update and retrieve and print these values similarly to the
> > existing hardcoded perf metrics. We use the simple expression parser
> > added earlier to evaluate the expression.
> > 
> > Normally we just output the result without further commentary,
> > but for --metric-only this would lead to empty columns. So for this
> > case use the original event as description.
> > 
> > So far there is no attempt to automatically add the MetricExpr event,
> > if it is missing, however we suggest it to the user.
> > 
> > $ perf stat -a -I 1000 -e '{unc_p_clockticks,unc_p_freq_max_os_cycles}'
> >      1.000228813        800,139,950      unc_p_clockticks
> >      1.000228813        789,833,783      unc_p_freq_max_os_cycles  #     98.7
> >      2.000654229        800,308,990      unc_p_clockticks
> >      2.000654229        396,214,238      unc_p_freq_max_os_cycles  #     49.5
> 
> [jolsa@krava perf]$ ./perf stat -a -I 1000 -e '{unc_p_clockticks,unc_p_freq_max_os_cycles}'
> invalid or unsupported event: '{unc_p_clockticks,unc_p_freq_max_os_cycles}'
> Run 'perf list' for a list of valid events
> 
> could you show an example of the MetricExpr?

It's in the event list branch

https://git.kernel.org/cgit/linux/kernel/git/ak/linux-misc.git/log/?h=perf/intel-uncore-json-files-3

All the metrics currently do is just the same as DividedBy earlier:
generate a percentage out of a count, typically based on clock ticks.

+        "MetricExpr": "(UNC_M_POWER_CHANNEL_PPD / UNC_M_CLOCKTICKS) *
100.",



> 
> it's part of the event record, what if you wanted to have 2 or more metrics defined for event?

Would need multiple copies of the event.

> 
> who defines those expressions?

It's metrics used internally by Intel.

> 
> what if you dont provide the necessary events needed for the expression?

Then perf prints a warning suggesting the events.

It's currently a TODO to add them automatically. Could be added,
but the patch was already complex, so I didn't add it.

It's somewhat complicated because you would need to avoid
duplicates and have to handle groups correctly. perf stat
doesn't have the necessarily knowledge to fully understand
the constraints on groups. 

Then the extra event may not fit into the group, and it seemed
saner to let the user decide what to do then, instead of
generating a possible unschedulable group.

So it's not that easy to do it automatically.

> > 
> > $ perf stat -a -I 1000 -e '{unc_p_clockticks,unc_p_freq_max_os_cycles}' --metric-only
> >      1.000206740     48.0
> >      2.000451543     48.1
> 
> how does user know what those number stand for?

I assume it was obvious enough that it is the percentage. Could
add something more to the event description.

> 
> is there a way for user to display the metric expression?

Only in the source, but could be added to perf list -v

For most users metrics are much more useful than raw numbers.
I think they would rather consider raw numbers to be a "hack
with no concept"

-Andi

[toc] | [prev] | [next] | [standalone]

#1577495

On Wed, Feb 08, 2017 at 01:51:05PM -0800, Andi Kleen wrote:

SNIP

> > 
> > could you show an example of the MetricExpr?
> 
> It's in the event list branch
> 
> https://git.kernel.org/cgit/linux/kernel/git/ak/linux-misc.git/log/?h=perf/intel-uncore-json-files-3
> 
> All the metrics currently do is just the same as DividedBy earlier:
> generate a percentage out of a count, typically based on clock ticks.
> 
> +        "MetricExpr": "(UNC_M_POWER_CHANNEL_PPD / UNC_M_CLOCKTICKS) *
> 100.",
> 
> 
> 
> > 
> > it's part of the event record, what if you wanted to have 2 or more metrics defined for event?
> 
> Would need multiple copies of the event.

this ...

> 
> > 
> > who defines those expressions?
> 
> It's metrics used internally by Intel.
> 
> > 
> > what if you dont provide the necessary events needed for the expression?
> 
> Then perf prints a warning suggesting the events.
> 
> It's currently a TODO to add them automatically. Could be added,
> but the patch was already complex, so I didn't add it.
> 
> It's somewhat complicated because you would need to avoid
> duplicates and have to handle groups correctly. perf stat
> doesn't have the necessarily knowledge to fully understand
> the constraints on groups. 
> 
> Then the extra event may not fit into the group, and it seemed
> saner to let the user decide what to do then, instead of
> generating a possible unschedulable group.
> 
> So it's not that easy to do it automatically.

and this makes me think, that this is not the right approach

adding extra copy of an event when you want to add new expression?

why can't we have another list/file of those expressions
from which point we could point and configure events we need

jirka

[toc] | [prev] | [next] | [standalone]

#1577802

On Thu, Feb 09, 2017 at 12:39:37PM +0100, Jiri Olsa wrote:
> and this makes me think, that this is not the right approach
> 
> adding extra copy of an event when you want to add new expression?

I don't want to add new expressions.

I don't even need arbitrary expressions, just DividedBy
to get percentages, you just forced me to do the expressions.


> why can't we have another list/file of those expressions

The last time I proposed separate files Ingo vetoed it.
He wanted everything built in.

> from which point we could point and configure events we need

If you want full flexibility you can use your perf stat report
approach, or what most people do is to just run a script/spreadsheet
over the the -x; output. This all continues to work.

This is just a minimum approach to provide some convenience
integrated with the event list to provide something similar
as the built in expressions in stat-shadow. 

It's not trying to build the great perf scripting language.

-Andi

[toc] | [prev] | [next] | [standalone]

#1577862

On Thu, Feb 09, 2017 at 09:00:35AM -0800, Andi Kleen wrote:
> On Thu, Feb 09, 2017 at 12:39:37PM +0100, Jiri Olsa wrote:
> > and this makes me think, that this is not the right approach
> > 
> > adding extra copy of an event when you want to add new expression?
> 
> I don't want to add new expressions.
> 
> I don't even need arbitrary expressions, just DividedBy
> to get percentages, you just forced me to do the expressions.
> 
> 
> > why can't we have another list/file of those expressions
> 
> The last time I proposed separate files Ingo vetoed it.
> He wanted everything built in.

sure, he veto it for event files.. expressions could be built
in same way as we have events now

> > from which point we could point and configure events we need
> 
> If you want full flexibility you can use your perf stat report
> approach, or what most people do is to just run a script/spreadsheet
> over the the -x; output. This all continues to work.
> 
> This is just a minimum approach to provide some convenience
> integrated with the event list to provide something similar
> as the built in expressions in stat-shadow. 
> 
> It's not trying to build the great perf scripting language.

yea I understand that but can't ack that based on the points
I descibed in my other email

jirka

[toc] | [prev] | [next] | [standalone]

#1577875

On Thu, Feb 09, 2017 at 07:37:55PM +0100, Jiri Olsa wrote:
> > The last time I proposed separate files Ingo vetoed it.
> > He wanted everything built in.
> 
> sure, he veto it for event files.. expressions could be built
> in same way as we have events now

That's exactly what I implemented. The expression is part
of the JSON file, which seems like the logical place.

You just want it in a separate file in the source?


> 
> > > from which point we could point and configure events we need
> > 
> > If you want full flexibility you can use your perf stat report
> > approach, or what most people do is to just run a script/spreadsheet
> > over the the -x; output. This all continues to work.
> > 
> > This is just a minimum approach to provide some convenience
> > integrated with the event list to provide something similar
> > as the built in expressions in stat-shadow. 
> > 
> > It's not trying to build the great perf scripting language.
> 
> yea I understand that but can't ack that based on the points
> I descibed in my other email

So what are the crucial points that prevent you?

- You want better column descriptions? 

I suppose could add another field for this.

- You want multiple expressions per event 
(even though they are not needed today)?

It could be implemented, but seems like unnecessary
complexity and overengineering to me at this point.
If nobody ever uses it would it be time spent well?
If someone really uses it they could add the support at that
time.

- You want automatic group creation?

It has nasty corner cases.
It would be possible to build something that works
for simple cases.

Anything I missed?

-Andi

[toc] | [prev] | [next] | [standalone]

#1578273

On Thu, Feb 09, 2017 at 10:59:43AM -0800, Andi Kleen wrote:
> On Thu, Feb 09, 2017 at 07:37:55PM +0100, Jiri Olsa wrote:
> > > The last time I proposed separate files Ingo vetoed it.
> > > He wanted everything built in.
> > 
> > sure, he veto it for event files.. expressions could be built
> > in same way as we have events now
> 
> That's exactly what I implemented. The expression is part
> of the JSON file, which seems like the logical place.
> 
> You just want it in a separate file in the source?
> 
> 
> > 
> > > > from which point we could point and configure events we need
> > > 
> > > If you want full flexibility you can use your perf stat report
> > > approach, or what most people do is to just run a script/spreadsheet
> > > over the the -x; output. This all continues to work.
> > > 
> > > This is just a minimum approach to provide some convenience
> > > integrated with the event list to provide something similar
> > > as the built in expressions in stat-shadow. 
> > > 
> > > It's not trying to build the great perf scripting language.
> > 
> > yea I understand that but can't ack that based on the points
> > I descibed in my other email
> 
> So what are the crucial points that prevent you?
> 
> - You want better column descriptions? 
> 
> I suppose could add another field for this.
> 
> - You want multiple expressions per event 
> (even though they are not needed today)?
> 
> It could be implemented, but seems like unnecessary
> complexity and overengineering to me at this point.
> If nobody ever uses it would it be time spent well?
> If someone really uses it they could add the support at that
> time.

ok, I checked optimization doc and you might be right about this one,
I couldn't find an example that'd prove you wrong ;-)

however I dont think expression should be annonymous number
tied to an event. For example there's this one:

  B.7.9.2  
  Fast Synchronization Penalty
  50. Locked Operations Impact: (L1D_CACHE_LOCK_DURATION + 20 * L1D_CACHE_LOCK.MESI) / CPU_CLK_UNHALTED.CORE * 100

  Fast synchronization is frequently implemented usin
  g locked memory accesses. A high value for Locked 
  Operations Impact indicates that locked operations us
  ed in the workload have high penalty. The latency 
  of a locked operation depends on the location of the 
  data: L1 data cache, L2 cache, other core cache or 
  memory.

There's a name and description.

which event will pick this expression?  L1D_CACHE_LOCK_DURATION or L1D_CACHE_LOCK?

How about we add:

"MetricExpr":      "(L1D_CACHE_LOCK_DURATION + 20 * L1D_CACHE_LOCK.MESI) / CPU_CLK_UNHALTED.CORE * 100"
"MetricExprName":  "Fast Synchronization Penalty"
"MetricExprDoc":   "Fast synchronization is frequently implemented... "

then it could be part of the event record and we can display it
like use the name in column and doc in the perf list

We could append number or something (MetricExpr2..) if there'll
be multiple expressions in some case.. or something like that

> 
> - You want automatic group creation?
> 
> It has nasty corner cases.
> It would be possible to build something that works
> for simple cases.

that would be nice.. when user says perf stat -e 'Fast Synchronization Penalty' ...
it'd create necessary events for him.. or some shorted name probably

jirka

[toc] | [prev] | [next] | [standalone]

#1577501

On Wed, Feb 08, 2017 at 01:51:05PM -0800, Andi Kleen wrote:

SNIP

> Only in the source, but could be added to perf list -v
> 
> For most users metrics are much more useful than raw numbers.
> I think they would rather consider raw numbers to be a "hack
> with no concept"

well, we can always fix that by showing the column header,
but that's not what I meant

jirka

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web