Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
Em Tue, Jul 28, 2020 at 03:01:00PM +0200, Jiri Olsa escreveu: > On Tue, Jul 28, 2020 at 02:54:56PM +0200, Jiri Olsa wrote: > > On Tue, Jul 28, 2020 at 09:39:55AM -0300, Arnaldo Carvalho de Melo wrote: > > > Em Mon, Jul 20, 2020 at 09:16:25PM +0200, Jiri Olsa escreveu: > > > > On Mon, Jul 20, 2020 at 02:32:40PM +0530, kajoljain wrote: > > > > > Hi jiri, > > > > > We need power9 lpar machine and need to make sure > > > > > `CONFIG_HV_PERF_CTRS` is > > > > > enabled. > > > > could you please try with following patch on top? > > > So, can you point me to the cset that this should be merged into? Or can > > > it come as a separate patch? I'll put what I have in the tmp.perf/core > > > branch, and will do testing, please let me know if you want to fold it > > > or as a followup patch. > > sorry for delay.. I planned to squash the change in, but if you already > > pushed something out, I'll rebase on top, I will post new version tomorrow > > 1st 7 patches are good to go in any case > ok, I just saw it's pushed out, I'll post a patch on top of it See, the tmp.perf/core experimentation is about making it available what would be just my local tree for wider consumption, what I have in perf/core is what is non-rebase material, I try to push things there only after testing and then recording the tests performed as signed tags, see: https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tag/?h=perf-tools-tests-2020-07-17 So if we find something wrong with material made available via my tmp.perf/core branch, we can go back in time and fix it, to try and have a more solid bisection base. It may be confusing (it seems so from how you responded to this thread), and if it ends up being so confusing, I'll just ditch it and go back to having just local branches, would be a pity as I'm keen on having a good bisection history so I'll probably take more time to publish things. Opinions about this, of course, more than welcome. Back to this specific case, when sending fixes for things in tmp.perf/core, please state what is the cset that needs folding into. Thanks, - Arnaldo
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
On Tue, Jul 28, 2020 at 02:54:56PM +0200, Jiri Olsa wrote: > On Tue, Jul 28, 2020 at 09:39:55AM -0300, Arnaldo Carvalho de Melo wrote: > > Em Mon, Jul 20, 2020 at 09:16:25PM +0200, Jiri Olsa escreveu: > > > On Mon, Jul 20, 2020 at 02:32:40PM +0530, kajoljain wrote: > > > > > > > > > > > > On 7/20/20 1:49 PM, Jiri Olsa wrote: > > > > > On Mon, Jul 20, 2020 at 01:39:24PM +0530, kajoljain wrote: > > > > > > > > > > SNIP > > > > > > > > > >> This is with your perf/metric branch: > > > > >> command# ./perf stat -M PowerBUS_Frequency -C 0 -I 1000 > > > > >> assertion failed at util/metricgroup.c:709 > > > > >> # time counts unit events > > > > >> 1.54545 7,807,505 hv_24x7/pm_pb_cyc,chip=0/ # > > > > >> 2.0 GHz PowerBUS_Frequency_0 > > > > >> 1.54545 7,807,485 hv_24x7/pm_pb_cyc,chip=1/ > > > > >> > > > > >> 2.000232761 7,807,500 hv_24x7/pm_pb_cyc,chip=0/ # > > > > >> 2.0 GHz PowerBUS_Frequency_0 > > > > >> 2.000232761 7,807,478 hv_24x7/pm_pb_cyc,chip=1/ > > > > >> > > > > >> 3.000363762 7,799,665 hv_24x7/pm_pb_cyc,chip=0/ # > > > > >> 1.9 GHz PowerBUS_Frequency_0 > > > > >> 3.000363762 7,807,502 hv_24x7/pm_pb_cyc,chip=1/ > > > > >> > > > > >> ^C 3.259418599 2,022,150 hv_24x7/pm_pb_cyc,chip=0/ > > > > >> # 0.5 GHz PowerBUS_Frequency_0 > > > > >> 3.259418599 2,022,164 hv_24x7/pm_pb_cyc,chip=1/ > > > > >> > > > > >> > > > > >> Performance counter stats for 'CPU(s) 0': > > > > >> > > > > >> 25,436,820 hv_24x7/pm_pb_cyc,chip=0/ # 6.4 GHz > > > > >> PowerBUS_Frequency_0 > > > > >> 25,444,629 hv_24x7/pm_pb_cyc,chip=1/ > > > > >> > > > > >> > > > > >>3.259505529 seconds time elapsed > > > > > > > > > > I found the bug, we are not adding runtime metrics as standalone ones, > > > > > but as referenced metrics.. will fix and try to add test for that > > > > > > > > > > as for testing.. do I need some special ppc server to have support > > > > > for this? > > > > > > > > Hi jiri, > > > > We need power9 lpar machine and need to make sure > > > > `CONFIG_HV_PERF_CTRS` is > > > > enabled. > > > > > > could you please try with following patch on top? > > > > So, can you point me to the cset that this should be merged into? Or can > > it come as a separate patch? I'll put what I have in the tmp.perf/core > > branch, and will do testing, please let me know if you want to fold it > > or as a followup patch. > > sorry for delay.. I planned to squash the change in, but if you already > pushed something out, I'll rebase on top, I will post new version tomorrow > > 1st 7 patches are good to go in any case ok, I just saw it's pushed out, I'll post a patch on top of it jirka
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
On Tue, Jul 28, 2020 at 09:39:55AM -0300, Arnaldo Carvalho de Melo wrote: > Em Mon, Jul 20, 2020 at 09:16:25PM +0200, Jiri Olsa escreveu: > > On Mon, Jul 20, 2020 at 02:32:40PM +0530, kajoljain wrote: > > > > > > > > > On 7/20/20 1:49 PM, Jiri Olsa wrote: > > > > On Mon, Jul 20, 2020 at 01:39:24PM +0530, kajoljain wrote: > > > > > > > > SNIP > > > > > > > >> This is with your perf/metric branch: > > > >> command# ./perf stat -M PowerBUS_Frequency -C 0 -I 1000 > > > >> assertion failed at util/metricgroup.c:709 > > > >> # time counts unit events > > > >> 1.54545 7,807,505 hv_24x7/pm_pb_cyc,chip=0/ # > > > >>2.0 GHz PowerBUS_Frequency_0 > > > >> 1.54545 7,807,485 hv_24x7/pm_pb_cyc,chip=1/ > > > >> > > > >> 2.000232761 7,807,500 hv_24x7/pm_pb_cyc,chip=0/ # > > > >>2.0 GHz PowerBUS_Frequency_0 > > > >> 2.000232761 7,807,478 hv_24x7/pm_pb_cyc,chip=1/ > > > >> > > > >> 3.000363762 7,799,665 hv_24x7/pm_pb_cyc,chip=0/ # > > > >>1.9 GHz PowerBUS_Frequency_0 > > > >> 3.000363762 7,807,502 hv_24x7/pm_pb_cyc,chip=1/ > > > >> > > > >> ^C 3.259418599 2,022,150 hv_24x7/pm_pb_cyc,chip=0/ # > > > >> 0.5 GHz PowerBUS_Frequency_0 > > > >> 3.259418599 2,022,164 hv_24x7/pm_pb_cyc,chip=1/ > > > >> > > > >> > > > >> Performance counter stats for 'CPU(s) 0': > > > >> > > > >> 25,436,820 hv_24x7/pm_pb_cyc,chip=0/ # 6.4 GHz > > > >> PowerBUS_Frequency_0 > > > >> 25,444,629 hv_24x7/pm_pb_cyc,chip=1/ > > > >> > > > >> > > > >>3.259505529 seconds time elapsed > > > > > > > > I found the bug, we are not adding runtime metrics as standalone ones, > > > > but as referenced metrics.. will fix and try to add test for that > > > > > > > > as for testing.. do I need some special ppc server to have support for > > > > this? > > > > > > Hi jiri, > > > We need power9 lpar machine and need to make sure > > > `CONFIG_HV_PERF_CTRS` is > > > enabled. > > > > could you please try with following patch on top? > > So, can you point me to the cset that this should be merged into? Or can > it come as a separate patch? I'll put what I have in the tmp.perf/core > branch, and will do testing, please let me know if you want to fold it > or as a followup patch. sorry for delay.. I planned to squash the change in, but if you already pushed something out, I'll rebase on top, I will post new version tomorrow 1st 7 patches are good to go in any case thanks, jirka
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
Em Thu, Jul 23, 2020 at 10:59:58AM -0500, Paul A. Clarke escreveu:
> On Wed, Jul 22, 2020 at 08:11:58PM +0200, Jiri Olsa wrote:
> > On Tue, Jul 21, 2020 at 09:48:48AM -0500, Paul A. Clarke wrote:
> > > On Sun, Jul 19, 2020 at 08:13:01PM +0200, Jiri Olsa wrote:
> > > > hi,
> > > > this patchset is adding the support to reused metric in
> > > > another metric.
> > > >
> > > > For example, to define IPC by using CPI with change like:
> > > >
> > > > {
> > > > "BriefDescription": "Instructions Per Cycle (per Logical
> > > > Processor)",
> > > > -"MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> > > > +"MetricExpr": "1/CPI",
> > > > "MetricGroup": "TopDownL1",
> > > > "MetricName": "IPC"
> > > > },
> > > >
> > > > I won't be able to find all the possible places we could
> > > > use this at, so I wonder you guys (who was asking for this)
> > > > would try it and come up with comments if there's something
> > > > missing or we could already use it at some places.
> > > >
> > > > It's based on Arnaldo's tmp.perf/core.
> > > >
> > > > v3 changes:
> > > > - added some acks
> > > > - some patches got merged
> > > > - added missing zalloc include [John Garry]
> > > > - added ids array outside the egroup object [Ian]
> > > > - removed wrong m->has_constraint assignment [Ian]
> > > > - renamed 'list' to 'metric_list' [Ian]
> > > > - fixed group metric and added test for it [Paul A. Clarke]
> > > > - fixed memory leak [Arnaldo]
> > > > - using lowercase keys for metrics in hashmap, because jevents
> > > > converts metric_expr to lowercase
> > > >
> > > > Also available in here:
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > > > perf/metric
> > >
> > > These changes seem to be mostly working for me.
> > >
> > > I attempted to exploit the new capability in the metrics definitions in
> > > tools/perf/pmu-events/arch/powerpc/power9/metrics.json. Those changes
> > > are included below.
> > >
> > > The one problem I found is with the "cpi_breakdown" metric group, as it
> > > no longer works:
> > > ```
> > > # perf stat --metrics cpi_breakdown ./command
> > > Cannot find metric or group `cpi_breakdown'
> > > ```
> > >
> > > "cpi_breakdown" does show up in `perf list --metricgroup`, and all of the
> > > (95!) metrics listed in that group are usable, so it's not obvious whether
> > > my changes have a problem, or merely provoke one.
> >
> > I underestimated the recursion depth setup for groups,
> > your change is working for me with following change:
> >
> > -#define RECURSION_ID_MAX 100
> > +#define RECURSION_ID_MAX 1000
>
> That indeed addressed the issue.
Thanks, I made that change on the cset that added this define.
- Arnaldo
> Is there a point where that limit was being hit and the code silently fails?
> If so, should that failure be less silent?
>
> PC
--
- Arnaldo
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
Em Mon, Jul 20, 2020 at 09:16:25PM +0200, Jiri Olsa escreveu:
> On Mon, Jul 20, 2020 at 02:32:40PM +0530, kajoljain wrote:
> >
> >
> > On 7/20/20 1:49 PM, Jiri Olsa wrote:
> > > On Mon, Jul 20, 2020 at 01:39:24PM +0530, kajoljain wrote:
> > >
> > > SNIP
> > >
> > >> This is with your perf/metric branch:
> > >> command# ./perf stat -M PowerBUS_Frequency -C 0 -I 1000
> > >> assertion failed at util/metricgroup.c:709
> > >> # time counts unit events
> > >> 1.54545 7,807,505 hv_24x7/pm_pb_cyc,chip=0/ #
> > >> 2.0 GHz PowerBUS_Frequency_0
> > >> 1.54545 7,807,485 hv_24x7/pm_pb_cyc,chip=1/
> > >>
> > >> 2.000232761 7,807,500 hv_24x7/pm_pb_cyc,chip=0/ #
> > >> 2.0 GHz PowerBUS_Frequency_0
> > >> 2.000232761 7,807,478 hv_24x7/pm_pb_cyc,chip=1/
> > >>
> > >> 3.000363762 7,799,665 hv_24x7/pm_pb_cyc,chip=0/ #
> > >> 1.9 GHz PowerBUS_Frequency_0
> > >> 3.000363762 7,807,502 hv_24x7/pm_pb_cyc,chip=1/
> > >>
> > >> ^C 3.259418599 2,022,150 hv_24x7/pm_pb_cyc,chip=0/ #
> > >>0.5 GHz PowerBUS_Frequency_0
> > >> 3.259418599 2,022,164 hv_24x7/pm_pb_cyc,chip=1/
> > >>
> > >>
> > >> Performance counter stats for 'CPU(s) 0':
> > >>
> > >> 25,436,820 hv_24x7/pm_pb_cyc,chip=0/ # 6.4 GHz
> > >> PowerBUS_Frequency_0
> > >> 25,444,629 hv_24x7/pm_pb_cyc,chip=1/
> > >>
> > >>
> > >>3.259505529 seconds time elapsed
> > >
> > > I found the bug, we are not adding runtime metrics as standalone ones,
> > > but as referenced metrics.. will fix and try to add test for that
> > >
> > > as for testing.. do I need some special ppc server to have support for
> > > this?
> >
> > Hi jiri,
> > We need power9 lpar machine and need to make sure `CONFIG_HV_PERF_CTRS`
> > is
> > enabled.
>
> could you please try with following patch on top?
So, can you point me to the cset that this should be merged into? Or can
it come as a separate patch? I'll put what I have in the tmp.perf/core
branch, and will do testing, please let me know if you want to fold it
or as a followup patch.
- Arnaldo
> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 6f179b9903a0..03aa4bd4a38b 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -820,11 +820,11 @@ static int add_metric(struct list_head *metric_list,
> struct expr_id *parent,
> struct expr_ids *ids);
>
> -static int resolve_metric(struct metric *m,
> - bool metric_no_group,
> - struct list_head *metric_list,
> - struct pmu_events_map *map,
> - struct expr_ids *ids)
> +static int __resolve_metric(struct metric *m,
> + bool metric_no_group,
> + struct list_head *metric_list,
> + struct pmu_events_map *map,
> + struct expr_ids *ids)
> {
> struct hashmap_entry *cur;
> size_t bkt;
> @@ -869,6 +869,23 @@ static int resolve_metric(struct metric *m,
> return 0;
> }
>
> +static int resolve_metric(bool metric_no_group,
> + struct list_head *metric_list,
> + struct pmu_events_map *map,
> + struct expr_ids *ids)
> +{
> + struct metric *m;
> + int err;
> +
> + list_for_each_entry(m, metric_list, nd) {
> + err = __resolve_metric(m, metric_no_group, metric_list, map,
> ids);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> static int add_metric(struct list_head *metric_list,
> struct pmu_event *pe,
> bool metric_no_group,
> @@ -876,6 +893,7 @@ static int add_metric(struct list_head *metric_list,
> struct expr_id *parent,
> struct expr_ids *ids)
> {
> + struct metric *orig = *m;
> int ret = 0;
>
> pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
> @@ -892,7 +910,7 @@ static int add_metric(struct list_head *metric_list,
>* those events to metric_list.
>*/
>
> - for (j = 0; j < count && !ret; j++) {
> + for (j = 0; j < count && !ret; j++, *m = orig) {
> ret = __add_metric(metric_list, pe, metric_no_group, j,
> m, parent, ids);
> }
> }
> @@ -907,8 +925,8 @@ static int metricgroup__add_metric(const char *metric,
> bool metric_no_group,
>
> {
> struct expr_ids ids = { 0 };
> +
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
On 7/25/20 5:21 PM, Jiri Olsa wrote:
> On Fri, Jul 24, 2020 at 11:22:28AM +0530, kajoljain wrote:
>
> SNIP
>
>>
>> Hi Jiri,
>>The change looks good to me. I tried with adding this patch on top of
>> your perf/metric branch. It did resolve the issue of not printing
>> all chips data. And now I can see proper values for hv-24x7 metric events.
>>
>> I was also trying by adding new metric using the feature added in this
>> patchset with something like this:
>>
>> diff --git a/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
>> b/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
>> index 8383a37647ad..dfe4bd63b587 100644
>> --- a/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
>> +++ b/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
>> @@ -16,6 +16,11 @@
>> "MetricName": "PowerBUS_Frequency",
>> "ScaleUnit": "2.5e-7GHz"
>> },
>> +{
>> + "MetricExpr": "Memory_WR_BW_Chip + Memory_RD_BW_Chip",
>> +"MetricName": "Total_Memory_BW",
>> +"ScaleUnit": "1.6e-2MB"
>> +},
>
> hum, we'll need special case this.. because Memory_WR_BW_Chip will
> unwind to Memory_WR_BW_Chip_[01] and Total_Memory_BW is not aware of
> that.. what's the expected behaviour in here?
>
> have Total_Memory_BW_[01] for each runtime arg?
Hi Jiri,
Yes right. So we want Total_Memory_BW to show sum results of both chip 0
and 1 seperately, which is missing here.
>
> I think this will need to come on top of this changes,
> it's already too big
>
Yes make sense. We can send separate patches on top of this patch set for this
use case.
Other then that the whole patchset looks good to me with the change to rectify
Paul A. Clarke concern.
Tested/Reviewed-By : Kajol Jain
Thanks,
Kajol Jain
> thanks,
> jirka
>
>>
>> I guess as we have dependency on '?' symbol, I am not able to see all chips
>> data for Total_Memory_BW.
>> I am not sure if Its expected behavior?
>>
>> This is what I am getting:
>>
>> [root@ltc-zz189-lp4 perf]# ./perf stat --metric-only -M
>> Total_Memory_BW,Memory_WR_BW_Chip,Memory_RD_BW_Chip -I 1000 -C 0
>> # time MB Total_Memory_BW MB Memory_RD_BW_Chip_1 MB
>> Memory_WR_BW_Chip_1 MB Memory_WR_BW_Chip_0 MB Memory_RD_BW_Chip_0
>> 1.67388 36.4 0.2
>> 36.3 65.0 72.1
>> 2.000374276 36.2 0.3
>> 35.9 65.4 77.9
>> 3.000543202 36.3 0.3
>> 36.0 68.7 81.2
>> 4.000702855 36.3 0.3
>> 36.0 70.9 93.3
>> 5.000856837 36.0 0.2
>> 35.8 67.4 81.5
>> ^C 5.367865273 13.2 0.1
>> 13.1 23.5 28.3
>> Performance counter stats for 'CPU(s) 0':
>>194.4 1.3193.1
>> 361.0434.3
>>5.368039176 seconds time elapsed
>>
>> We can only get single chip data's sum in Total_Memory_BW. Please let me
>> know if I am missing something.
>
> SNIP
>
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
On Thu, Jul 23, 2020 at 10:59:58AM -0500, Paul A. Clarke wrote:
> On Wed, Jul 22, 2020 at 08:11:58PM +0200, Jiri Olsa wrote:
> > On Tue, Jul 21, 2020 at 09:48:48AM -0500, Paul A. Clarke wrote:
> > > On Sun, Jul 19, 2020 at 08:13:01PM +0200, Jiri Olsa wrote:
> > > > hi,
> > > > this patchset is adding the support to reused metric in
> > > > another metric.
> > > >
> > > > For example, to define IPC by using CPI with change like:
> > > >
> > > > {
> > > > "BriefDescription": "Instructions Per Cycle (per Logical
> > > > Processor)",
> > > > -"MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> > > > +"MetricExpr": "1/CPI",
> > > > "MetricGroup": "TopDownL1",
> > > > "MetricName": "IPC"
> > > > },
> > > >
> > > > I won't be able to find all the possible places we could
> > > > use this at, so I wonder you guys (who was asking for this)
> > > > would try it and come up with comments if there's something
> > > > missing or we could already use it at some places.
> > > >
> > > > It's based on Arnaldo's tmp.perf/core.
> > > >
> > > > v3 changes:
> > > > - added some acks
> > > > - some patches got merged
> > > > - added missing zalloc include [John Garry]
> > > > - added ids array outside the egroup object [Ian]
> > > > - removed wrong m->has_constraint assignment [Ian]
> > > > - renamed 'list' to 'metric_list' [Ian]
> > > > - fixed group metric and added test for it [Paul A. Clarke]
> > > > - fixed memory leak [Arnaldo]
> > > > - using lowercase keys for metrics in hashmap, because jevents
> > > > converts metric_expr to lowercase
> > > >
> > > > Also available in here:
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > > > perf/metric
> > >
> > > These changes seem to be mostly working for me.
> > >
> > > I attempted to exploit the new capability in the metrics definitions in
> > > tools/perf/pmu-events/arch/powerpc/power9/metrics.json. Those changes
> > > are included below.
> > >
> > > The one problem I found is with the "cpi_breakdown" metric group, as it
> > > no longer works:
> > > ```
> > > # perf stat --metrics cpi_breakdown ./command
> > > Cannot find metric or group `cpi_breakdown'
> > > ```
> > >
> > > "cpi_breakdown" does show up in `perf list --metricgroup`, and all of the
> > > (95!) metrics listed in that group are usable, so it's not obvious whether
> > > my changes have a problem, or merely provoke one.
> >
> > I underestimated the recursion depth setup for groups,
> > your change is working for me with following change:
> >
> > -#define RECURSION_ID_MAX 100
> > +#define RECURSION_ID_MAX 1000
>
> That indeed addressed the issue.
>
> Is there a point where that limit was being hit and the code silently fails?
yes
> If so, should that failure be less silent?
I'll make it more verbose
thanks,
jirka
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
On Fri, Jul 24, 2020 at 11:22:28AM +0530, kajoljain wrote:
SNIP
>
> Hi Jiri,
>The change looks good to me. I tried with adding this patch on top of
> your perf/metric branch. It did resolve the issue of not printing
> all chips data. And now I can see proper values for hv-24x7 metric events.
>
> I was also trying by adding new metric using the feature added in this
> patchset with something like this:
>
> diff --git a/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
> b/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
> index 8383a37647ad..dfe4bd63b587 100644
> --- a/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
> +++ b/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
> @@ -16,6 +16,11 @@
> "MetricName": "PowerBUS_Frequency",
> "ScaleUnit": "2.5e-7GHz"
> },
> +{
> + "MetricExpr": "Memory_WR_BW_Chip + Memory_RD_BW_Chip",
> +"MetricName": "Total_Memory_BW",
> +"ScaleUnit": "1.6e-2MB"
> +},
hum, we'll need special case this.. because Memory_WR_BW_Chip will
unwind to Memory_WR_BW_Chip_[01] and Total_Memory_BW is not aware of
that.. what's the expected behaviour in here?
have Total_Memory_BW_[01] for each runtime arg?
I think this will need to come on top of this changes,
it's already too big
thanks,
jirka
>
> I guess as we have dependency on '?' symbol, I am not able to see all chips
> data for Total_Memory_BW.
> I am not sure if Its expected behavior?
>
> This is what I am getting:
>
> [root@ltc-zz189-lp4 perf]# ./perf stat --metric-only -M
> Total_Memory_BW,Memory_WR_BW_Chip,Memory_RD_BW_Chip -I 1000 -C 0
> # time MB Total_Memory_BW MB Memory_RD_BW_Chip_1 MB
> Memory_WR_BW_Chip_1 MB Memory_WR_BW_Chip_0 MB Memory_RD_BW_Chip_0
> 1.67388 36.4 0.2
> 36.3 65.0 72.1
> 2.000374276 36.2 0.3
> 35.9 65.4 77.9
> 3.000543202 36.3 0.3
> 36.0 68.7 81.2
> 4.000702855 36.3 0.3
> 36.0 70.9 93.3
> 5.000856837 36.0 0.2
> 35.8 67.4 81.5
> ^C 5.367865273 13.2 0.1
>13.1 23.5 28.3
> Performance counter stats for 'CPU(s) 0':
>194.4 1.3193.1
> 361.0434.3
>5.368039176 seconds time elapsed
>
> We can only get single chip data's sum in Total_Memory_BW. Please let me know
> if I am missing something.
SNIP
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
On 7/21/20 12:46 AM, Jiri Olsa wrote:
> On Mon, Jul 20, 2020 at 02:32:40PM +0530, kajoljain wrote:
>>
>>
>> On 7/20/20 1:49 PM, Jiri Olsa wrote:
>>> On Mon, Jul 20, 2020 at 01:39:24PM +0530, kajoljain wrote:
>>>
>>> SNIP
>>>
This is with your perf/metric branch:
command# ./perf stat -M PowerBUS_Frequency -C 0 -I 1000
assertion failed at util/metricgroup.c:709
# time counts unit events
1.54545 7,807,505 hv_24x7/pm_pb_cyc,chip=0/ #
2.0 GHz PowerBUS_Frequency_0
1.54545 7,807,485 hv_24x7/pm_pb_cyc,chip=1/
2.000232761 7,807,500 hv_24x7/pm_pb_cyc,chip=0/ #
2.0 GHz PowerBUS_Frequency_0
2.000232761 7,807,478 hv_24x7/pm_pb_cyc,chip=1/
3.000363762 7,799,665 hv_24x7/pm_pb_cyc,chip=0/ #
1.9 GHz PowerBUS_Frequency_0
3.000363762 7,807,502 hv_24x7/pm_pb_cyc,chip=1/
^C 3.259418599 2,022,150 hv_24x7/pm_pb_cyc,chip=0/ #
0.5 GHz PowerBUS_Frequency_0
3.259418599 2,022,164 hv_24x7/pm_pb_cyc,chip=1/
Performance counter stats for 'CPU(s) 0':
25,436,820 hv_24x7/pm_pb_cyc,chip=0/ # 6.4 GHz
PowerBUS_Frequency_0
25,444,629 hv_24x7/pm_pb_cyc,chip=1/
3.259505529 seconds time elapsed
>>>
>>> I found the bug, we are not adding runtime metrics as standalone ones,
>>> but as referenced metrics.. will fix and try to add test for that
>>>
>>> as for testing.. do I need some special ppc server to have support for
>>> this?
>>
>> Hi jiri,
>> We need power9 lpar machine and need to make sure `CONFIG_HV_PERF_CTRS`
>> is
>> enabled.
>
> could you please try with following patch on top?
Hi Jiri,
The change looks good to me. I tried with adding this patch on top of
your perf/metric branch. It did resolve the issue of not printing
all chips data. And now I can see proper values for hv-24x7 metric events.
I was also trying by adding new metric using the feature added in this patchset
with something like this:
diff --git a/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
b/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
index 8383a37647ad..dfe4bd63b587 100644
--- a/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
+++ b/tools/perf/pmu-events/arch/powerpc/power9/nest_metrics.json
@@ -16,6 +16,11 @@
"MetricName": "PowerBUS_Frequency",
"ScaleUnit": "2.5e-7GHz"
},
+{
+ "MetricExpr": "Memory_WR_BW_Chip + Memory_RD_BW_Chip",
+"MetricName": "Total_Memory_BW",
+"ScaleUnit": "1.6e-2MB"
+},
I guess as we have dependency on '?' symbol, I am not able to see all chips
data for Total_Memory_BW.
I am not sure if Its expected behavior?
This is what I am getting:
[root@ltc-zz189-lp4 perf]# ./perf stat --metric-only -M
Total_Memory_BW,Memory_WR_BW_Chip,Memory_RD_BW_Chip -I 1000 -C 0
# time MB Total_Memory_BW MB Memory_RD_BW_Chip_1 MB
Memory_WR_BW_Chip_1 MB Memory_WR_BW_Chip_0 MB Memory_RD_BW_Chip_0
1.67388 36.4 0.2
36.3 65.0 72.1
2.000374276 36.2 0.3
35.9 65.4 77.9
3.000543202 36.3 0.3
36.0 68.7 81.2
4.000702855 36.3 0.3
36.0 70.9 93.3
5.000856837 36.0 0.2
35.8 67.4 81.5
^C 5.367865273 13.2 0.1
13.1 23.5 28.3
Performance counter stats for 'CPU(s) 0':
194.4 1.3193.1
361.0434.3
5.368039176 seconds time elapsed
We can only get single chip data's sum in Total_Memory_BW. Please let me know
if I am missing something.
Thanks,
Kajol Jain
>
> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 6f179b9903a0..03aa4bd4a38b 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -820,11 +820,11 @@ static int add_metric(struct list_head *metric_list,
> struct expr_id *parent,
> struct expr_ids *ids);
>
> -static int
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
> perf: util/evsel.c:1452: get_group_fd: Assertion `!(!leader->core.fd)' > failed. > Aborted (core dumped) > ``` This usually happens when you run out of file descriptors -Andi
RE: [PATCHv3 00/19] perf metric: Add support to reuse metric
On Wed, Jul 22, 2020 at 08:11:58PM +0200, Jiri Olsa wrote:
> On Tue, Jul 21, 2020 at 09:48:48AM -0500, Paul A. Clarke wrote:
> > On Sun, Jul 19, 2020 at 08:13:01PM +0200, Jiri Olsa wrote:
> > > hi,
> > > this patchset is adding the support to reused metric in
> > > another metric.
> > >
> > > For example, to define IPC by using CPI with change like:
> > >
> > > {
> > > "BriefDescription": "Instructions Per Cycle (per Logical
> > > Processor)",
> > > -"MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> > > +"MetricExpr": "1/CPI",
> > > "MetricGroup": "TopDownL1",
> > > "MetricName": "IPC"
> > > },
> > >
> > > I won't be able to find all the possible places we could
> > > use this at, so I wonder you guys (who was asking for this)
> > > would try it and come up with comments if there's something
> > > missing or we could already use it at some places.
> > >
> > > It's based on Arnaldo's tmp.perf/core.
> > >
> > > v3 changes:
> > > - added some acks
> > > - some patches got merged
> > > - added missing zalloc include [John Garry]
> > > - added ids array outside the egroup object [Ian]
> > > - removed wrong m->has_constraint assignment [Ian]
> > > - renamed 'list' to 'metric_list' [Ian]
> > > - fixed group metric and added test for it [Paul A. Clarke]
> > > - fixed memory leak [Arnaldo]
> > > - using lowercase keys for metrics in hashmap, because jevents
> > > converts metric_expr to lowercase
> > >
> > > Also available in here:
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > > perf/metric
> >
> > These changes seem to be mostly working for me.
> >
> > I attempted to exploit the new capability in the metrics definitions in
> > tools/perf/pmu-events/arch/powerpc/power9/metrics.json. Those changes
> > are included below.
> >
> > The one problem I found is with the "cpi_breakdown" metric group, as it
> > no longer works:
> > ```
> > # perf stat --metrics cpi_breakdown ./command
> > Cannot find metric or group `cpi_breakdown'
> > ```
> >
> > "cpi_breakdown" does show up in `perf list --metricgroup`, and all of the
> > (95!) metrics listed in that group are usable, so it's not obvious whether
> > my changes have a problem, or merely provoke one.
>
> I underestimated the recursion depth setup for groups,
> your change is working for me with following change:
>
> -#define RECURSION_ID_MAX 100
> +#define RECURSION_ID_MAX 1000
I just saw some odd behavior:
```
# perf stat --metrics cpi_breakdown --metrics cpi_breakdown --metric-only ./cmd
WARNING: grouped events cpus do not match, disabling group:
anon group { raw 0x2d018 }
anon group { raw 0x2d018 }
anon group { raw 0x2d018 }
anon group { raw 0x2d018 }
anon group { raw 0x1006a }
anon group { raw 0x400fa }
anon group { raw 0x400fa }
anon group { raw 0x400fa }
anon group { raw 0x400fa }
perf: util/evsel.c:1452: get_group_fd: Assertion `!(!leader->core.fd)' failed.
Aborted (core dumped)
```
It happened both with and without my changes on binaries which were built
yesterday. Three or four times in a row.
Oddly, it went away without any action on my part. Same commands a minute or so
later. I haven't seen this before. I can't reproduce it now. :-/
PC
RE: [PATCHv3 00/19] perf metric: Add support to reuse metric
On Wed, Jul 22, 2020 at 08:11:58PM +0200, Jiri Olsa wrote:
> On Tue, Jul 21, 2020 at 09:48:48AM -0500, Paul A. Clarke wrote:
> > On Sun, Jul 19, 2020 at 08:13:01PM +0200, Jiri Olsa wrote:
> > > hi,
> > > this patchset is adding the support to reused metric in
> > > another metric.
> > >
> > > For example, to define IPC by using CPI with change like:
> > >
> > > {
> > > "BriefDescription": "Instructions Per Cycle (per Logical
> > > Processor)",
> > > -"MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> > > +"MetricExpr": "1/CPI",
> > > "MetricGroup": "TopDownL1",
> > > "MetricName": "IPC"
> > > },
> > >
> > > I won't be able to find all the possible places we could
> > > use this at, so I wonder you guys (who was asking for this)
> > > would try it and come up with comments if there's something
> > > missing or we could already use it at some places.
> > >
> > > It's based on Arnaldo's tmp.perf/core.
> > >
> > > v3 changes:
> > > - added some acks
> > > - some patches got merged
> > > - added missing zalloc include [John Garry]
> > > - added ids array outside the egroup object [Ian]
> > > - removed wrong m->has_constraint assignment [Ian]
> > > - renamed 'list' to 'metric_list' [Ian]
> > > - fixed group metric and added test for it [Paul A. Clarke]
> > > - fixed memory leak [Arnaldo]
> > > - using lowercase keys for metrics in hashmap, because jevents
> > > converts metric_expr to lowercase
> > >
> > > Also available in here:
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > > perf/metric
> >
> > These changes seem to be mostly working for me.
> >
> > I attempted to exploit the new capability in the metrics definitions in
> > tools/perf/pmu-events/arch/powerpc/power9/metrics.json. Those changes
> > are included below.
> >
> > The one problem I found is with the "cpi_breakdown" metric group, as it
> > no longer works:
> > ```
> > # perf stat --metrics cpi_breakdown ./command
> > Cannot find metric or group `cpi_breakdown'
> > ```
> >
> > "cpi_breakdown" does show up in `perf list --metricgroup`, and all of the
> > (95!) metrics listed in that group are usable, so it's not obvious whether
> > my changes have a problem, or merely provoke one.
>
> I underestimated the recursion depth setup for groups,
> your change is working for me with following change:
>
> -#define RECURSION_ID_MAX 100
> +#define RECURSION_ID_MAX 1000
That indeed addressed the issue.
Is there a point where that limit was being hit and the code silently fails?
If so, should that failure be less silent?
PC
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
On Tue, Jul 21, 2020 at 09:48:48AM -0500, Paul A. Clarke wrote:
> On Sun, Jul 19, 2020 at 08:13:01PM +0200, Jiri Olsa wrote:
> > hi,
> > this patchset is adding the support to reused metric in
> > another metric.
> >
> > For example, to define IPC by using CPI with change like:
> >
> > {
> > "BriefDescription": "Instructions Per Cycle (per Logical
> > Processor)",
> > -"MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> > +"MetricExpr": "1/CPI",
> > "MetricGroup": "TopDownL1",
> > "MetricName": "IPC"
> > },
> >
> > I won't be able to find all the possible places we could
> > use this at, so I wonder you guys (who was asking for this)
> > would try it and come up with comments if there's something
> > missing or we could already use it at some places.
> >
> > It's based on Arnaldo's tmp.perf/core.
> >
> > v3 changes:
> > - added some acks
> > - some patches got merged
> > - added missing zalloc include [John Garry]
> > - added ids array outside the egroup object [Ian]
> > - removed wrong m->has_constraint assignment [Ian]
> > - renamed 'list' to 'metric_list' [Ian]
> > - fixed group metric and added test for it [Paul A. Clarke]
> > - fixed memory leak [Arnaldo]
> > - using lowercase keys for metrics in hashmap, because jevents
> > converts metric_expr to lowercase
> >
> > Also available in here:
> > git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > perf/metric
>
> These changes seem to be mostly working for me.
>
> I attempted to exploit the new capability in the metrics definitions in
> tools/perf/pmu-events/arch/powerpc/power9/metrics.json. Those changes
> are included below.
>
> The one problem I found is with the "cpi_breakdown" metric group, as it
> no longer works:
> ```
> # perf stat --metrics cpi_breakdown ./command
> Cannot find metric or group `cpi_breakdown'
> ```
>
> "cpi_breakdown" does show up in `perf list --metricgroup`, and all of the
> (95!) metrics listed in that group are usable, so it's not obvious whether
> my changes have a problem, or merely provoke one.
I underestimated the recursion depth setup for groups,
your change is working for me with following change:
-#define RECURSION_ID_MAX 100
+#define RECURSION_ID_MAX 1000
jirka
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
On Sun, Jul 19, 2020 at 08:13:01PM +0200, Jiri Olsa wrote:
> hi,
> this patchset is adding the support to reused metric in
> another metric.
>
> For example, to define IPC by using CPI with change like:
>
> {
> "BriefDescription": "Instructions Per Cycle (per Logical Processor)",
> -"MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> +"MetricExpr": "1/CPI",
> "MetricGroup": "TopDownL1",
> "MetricName": "IPC"
> },
>
> I won't be able to find all the possible places we could
> use this at, so I wonder you guys (who was asking for this)
> would try it and come up with comments if there's something
> missing or we could already use it at some places.
>
> It's based on Arnaldo's tmp.perf/core.
>
> v3 changes:
> - added some acks
> - some patches got merged
> - added missing zalloc include [John Garry]
> - added ids array outside the egroup object [Ian]
> - removed wrong m->has_constraint assignment [Ian]
> - renamed 'list' to 'metric_list' [Ian]
> - fixed group metric and added test for it [Paul A. Clarke]
> - fixed memory leak [Arnaldo]
> - using lowercase keys for metrics in hashmap, because jevents
> converts metric_expr to lowercase
>
> Also available in here:
> git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> perf/metric
These changes seem to be mostly working for me.
I attempted to exploit the new capability in the metrics definitions in
tools/perf/pmu-events/arch/powerpc/power9/metrics.json. Those changes
are included below.
The one problem I found is with the "cpi_breakdown" metric group, as it
no longer works:
```
# perf stat --metrics cpi_breakdown ./command
Cannot find metric or group `cpi_breakdown'
```
"cpi_breakdown" does show up in `perf list --metricgroup`, and all of the
(95!) metrics listed in that group are usable, so it's not obvious whether
my changes have a problem, or merely provoke one.
diff --git a/tools/perf/pmu-events/arch/powerpc/power9/metrics.json
b/tools/perf/pmu-events/arch/powerpc/power9/metrics.json
index 80816d6402e9..f8784c608479 100644
--- a/tools/perf/pmu-events/arch/powerpc/power9/metrics.json
+++ b/tools/perf/pmu-events/arch/powerpc/power9/metrics.json
@@ -60,7 +60,7 @@
},
{
"BriefDescription": "Stalls due to short latency decimal floating
ops.",
-"MetricExpr": "(PM_CMPLU_STALL_DFU -
PM_CMPLU_STALL_DFLONG)/PM_RUN_INST_CMPL",
+"MetricExpr": "dfu_stall_cpi - dflong_stall_cpi",
"MetricGroup": "cpi_breakdown",
"MetricName": "dfu_other_stall_cpi"
},
@@ -72,7 +72,7 @@
},
{
"BriefDescription": "Completion stall by Dcache miss which resolved
off node memory/cache",
-"MetricExpr": "(PM_CMPLU_STALL_DMISS_L3MISS -
PM_CMPLU_STALL_DMISS_L21_L31 - PM_CMPLU_STALL_DMISS_LMEM -
PM_CMPLU_STALL_DMISS_REMOTE)/PM_RUN_INST_CMPL",
+"MetricExpr": "dmiss_non_local_stall_cpi - dmiss_remote_stall_cpi",
"MetricGroup": "cpi_breakdown",
"MetricName": "dmiss_distant_stall_cpi"
},
@@ -90,7 +90,7 @@
},
{
"BriefDescription": "Completion stall due to cache miss that resolves
in the L2 or L3 without conflict",
-"MetricExpr": "(PM_CMPLU_STALL_DMISS_L2L3 -
PM_CMPLU_STALL_DMISS_L2L3_CONFLICT)/PM_RUN_INST_CMPL",
+"MetricExpr": "dmiss_l2l3_stall_cpi - dmiss_l2l3_conflict_stall_cpi",
"MetricGroup": "cpi_breakdown",
"MetricName": "dmiss_l2l3_noconflict_stall_cpi"
},
@@ -114,7 +114,7 @@
},
{
"BriefDescription": "Completion stall by Dcache miss which resolved
outside of local memory",
-"MetricExpr": "(PM_CMPLU_STALL_DMISS_L3MISS -
PM_CMPLU_STALL_DMISS_L21_L31 - PM_CMPLU_STALL_DMISS_LMEM)/PM_RUN_INST_CMPL",
+"MetricExpr": "dmiss_l3miss_stall_cpi - dmiss_l21_l31_stall_cpi -
dmiss_lmem_stall_cpi",
"MetricGroup": "cpi_breakdown",
"MetricName": "dmiss_non_local_stall_cpi"
},
@@ -126,7 +126,7 @@
},
{
"BriefDescription": "Stalls due to short latency double precision
ops.",
-"MetricExpr": "(PM_CMPLU_STALL_DP -
PM_CMPLU_STALL_DPLONG)/PM_RUN_INST_CMPL",
+"MetricExpr": "dp_stall_cpi - dplong_stall_cpi",
"MetricGroup": "cpi_breakdown",
"MetricName": "dp_other_stall_cpi"
},
@@ -155,7 +155,7 @@
"MetricName": "emq_full_stall_cpi"
},
{
-"MetricExpr": "(PM_CMPLU_STALL_ERAT_MISS +
PM_CMPLU_STALL_EMQ_FULL)/PM_RUN_INST_CMPL",
+"MetricExpr": "erat_miss_stall_cpi + emq_full_stall_cpi",
"MetricGroup": "cpi_breakdown",
"MetricName": "emq_stall_cpi"
},
@@ -173,7 +173,7 @@
},
{
"BriefDescription": "Completion stall due to execution units for other
reasons.",
-"MetricExpr": "(PM_CMPLU_STALL_EXEC_UNIT - PM_CMPLU_STALL_FXU -
PM_CMPLU_STALL_DP - PM_CMPLU_STALL_DFU - PM_CMPLU_STALL_PM -
PM_CMPLU_STALL_CRYPTO - PM_CMPLU_STALL_VFXU -
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
On Mon, Jul 20, 2020 at 02:32:40PM +0530, kajoljain wrote:
>
>
> On 7/20/20 1:49 PM, Jiri Olsa wrote:
> > On Mon, Jul 20, 2020 at 01:39:24PM +0530, kajoljain wrote:
> >
> > SNIP
> >
> >> This is with your perf/metric branch:
> >> command# ./perf stat -M PowerBUS_Frequency -C 0 -I 1000
> >> assertion failed at util/metricgroup.c:709
> >> # time counts unit events
> >> 1.54545 7,807,505 hv_24x7/pm_pb_cyc,chip=0/ #
> >> 2.0 GHz PowerBUS_Frequency_0
> >> 1.54545 7,807,485 hv_24x7/pm_pb_cyc,chip=1/
> >>
> >> 2.000232761 7,807,500 hv_24x7/pm_pb_cyc,chip=0/ #
> >> 2.0 GHz PowerBUS_Frequency_0
> >> 2.000232761 7,807,478 hv_24x7/pm_pb_cyc,chip=1/
> >>
> >> 3.000363762 7,799,665 hv_24x7/pm_pb_cyc,chip=0/ #
> >> 1.9 GHz PowerBUS_Frequency_0
> >> 3.000363762 7,807,502 hv_24x7/pm_pb_cyc,chip=1/
> >>
> >> ^C 3.259418599 2,022,150 hv_24x7/pm_pb_cyc,chip=0/ #
> >> 0.5 GHz PowerBUS_Frequency_0
> >> 3.259418599 2,022,164 hv_24x7/pm_pb_cyc,chip=1/
> >>
> >>
> >> Performance counter stats for 'CPU(s) 0':
> >>
> >> 25,436,820 hv_24x7/pm_pb_cyc,chip=0/ # 6.4 GHz
> >> PowerBUS_Frequency_0
> >> 25,444,629 hv_24x7/pm_pb_cyc,chip=1/
> >>
> >>
> >>3.259505529 seconds time elapsed
> >
> > I found the bug, we are not adding runtime metrics as standalone ones,
> > but as referenced metrics.. will fix and try to add test for that
> >
> > as for testing.. do I need some special ppc server to have support for
> > this?
>
> Hi jiri,
> We need power9 lpar machine and need to make sure `CONFIG_HV_PERF_CTRS` is
> enabled.
could you please try with following patch on top?
thanks,
jirka
---
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 6f179b9903a0..03aa4bd4a38b 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -820,11 +820,11 @@ static int add_metric(struct list_head *metric_list,
struct expr_id *parent,
struct expr_ids *ids);
-static int resolve_metric(struct metric *m,
- bool metric_no_group,
- struct list_head *metric_list,
- struct pmu_events_map *map,
- struct expr_ids *ids)
+static int __resolve_metric(struct metric *m,
+ bool metric_no_group,
+ struct list_head *metric_list,
+ struct pmu_events_map *map,
+ struct expr_ids *ids)
{
struct hashmap_entry *cur;
size_t bkt;
@@ -869,6 +869,23 @@ static int resolve_metric(struct metric *m,
return 0;
}
+static int resolve_metric(bool metric_no_group,
+ struct list_head *metric_list,
+ struct pmu_events_map *map,
+ struct expr_ids *ids)
+{
+ struct metric *m;
+ int err;
+
+ list_for_each_entry(m, metric_list, nd) {
+ err = __resolve_metric(m, metric_no_group, metric_list, map,
ids);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
static int add_metric(struct list_head *metric_list,
struct pmu_event *pe,
bool metric_no_group,
@@ -876,6 +893,7 @@ static int add_metric(struct list_head *metric_list,
struct expr_id *parent,
struct expr_ids *ids)
{
+ struct metric *orig = *m;
int ret = 0;
pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
@@ -892,7 +910,7 @@ static int add_metric(struct list_head *metric_list,
* those events to metric_list.
*/
- for (j = 0; j < count && !ret; j++) {
+ for (j = 0; j < count && !ret; j++, *m = orig) {
ret = __add_metric(metric_list, pe, metric_no_group, j,
m, parent, ids);
}
}
@@ -907,8 +925,8 @@ static int metricgroup__add_metric(const char *metric, bool
metric_no_group,
{
struct expr_ids ids = { 0 };
+ struct metric *m = NULL;
struct pmu_event *pe;
- struct metric *m;
LIST_HEAD(list);
int i, ret;
bool has_match = false;
@@ -925,7 +943,7 @@ static int metricgroup__add_metric(const char *metric, bool
metric_no_group,
* Process any possible referenced metrics
* included in the expression.
*/
- ret = resolve_metric(m, metric_no_group,
+ ret =
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
On 7/20/20 1:49 PM, Jiri Olsa wrote: > On Mon, Jul 20, 2020 at 01:39:24PM +0530, kajoljain wrote: > > SNIP > >> This is with your perf/metric branch: >> command# ./perf stat -M PowerBUS_Frequency -C 0 -I 1000 >> assertion failed at util/metricgroup.c:709 >> # time counts unit events >> 1.54545 7,807,505 hv_24x7/pm_pb_cyc,chip=0/ # >> 2.0 GHz PowerBUS_Frequency_0 >> 1.54545 7,807,485 hv_24x7/pm_pb_cyc,chip=1/ >> >> 2.000232761 7,807,500 hv_24x7/pm_pb_cyc,chip=0/ # >> 2.0 GHz PowerBUS_Frequency_0 >> 2.000232761 7,807,478 hv_24x7/pm_pb_cyc,chip=1/ >> >> 3.000363762 7,799,665 hv_24x7/pm_pb_cyc,chip=0/ # >> 1.9 GHz PowerBUS_Frequency_0 >> 3.000363762 7,807,502 hv_24x7/pm_pb_cyc,chip=1/ >> >> ^C 3.259418599 2,022,150 hv_24x7/pm_pb_cyc,chip=0/ # >> 0.5 GHz PowerBUS_Frequency_0 >> 3.259418599 2,022,164 hv_24x7/pm_pb_cyc,chip=1/ >> >> >> Performance counter stats for 'CPU(s) 0': >> >> 25,436,820 hv_24x7/pm_pb_cyc,chip=0/ # 6.4 GHz >> PowerBUS_Frequency_0 >> 25,444,629 hv_24x7/pm_pb_cyc,chip=1/ >> >> >>3.259505529 seconds time elapsed > > I found the bug, we are not adding runtime metrics as standalone ones, > but as referenced metrics.. will fix and try to add test for that > > as for testing.. do I need some special ppc server to have support for this? Hi jiri, We need power9 lpar machine and need to make sure `CONFIG_HV_PERF_CTRS` is enabled. Thanks, Kajol Jain > > thanks, > jirka >
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
On Mon, Jul 20, 2020 at 01:39:24PM +0530, kajoljain wrote: SNIP > This is with your perf/metric branch: > command# ./perf stat -M PowerBUS_Frequency -C 0 -I 1000 > assertion failed at util/metricgroup.c:709 > # time counts unit events > 1.54545 7,807,505 hv_24x7/pm_pb_cyc,chip=0/ # 2.0 > GHz PowerBUS_Frequency_0 > 1.54545 7,807,485 hv_24x7/pm_pb_cyc,chip=1/ > > 2.000232761 7,807,500 hv_24x7/pm_pb_cyc,chip=0/ # 2.0 > GHz PowerBUS_Frequency_0 > 2.000232761 7,807,478 hv_24x7/pm_pb_cyc,chip=1/ > > 3.000363762 7,799,665 hv_24x7/pm_pb_cyc,chip=0/ # 1.9 > GHz PowerBUS_Frequency_0 > 3.000363762 7,807,502 hv_24x7/pm_pb_cyc,chip=1/ > > ^C 3.259418599 2,022,150 hv_24x7/pm_pb_cyc,chip=0/ # > 0.5 GHz PowerBUS_Frequency_0 > 3.259418599 2,022,164 hv_24x7/pm_pb_cyc,chip=1/ > > > Performance counter stats for 'CPU(s) 0': > > 25,436,820 hv_24x7/pm_pb_cyc,chip=0/ # 6.4 GHz > PowerBUS_Frequency_0 > 25,444,629 hv_24x7/pm_pb_cyc,chip=1/ > > >3.259505529 seconds time elapsed I found the bug, we are not adding runtime metrics as standalone ones, but as referenced metrics.. will fix and try to add test for that as for testing.. do I need some special ppc server to have support for this? thanks, jirka
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
On 7/20/20 12:52 PM, Jiri Olsa wrote:
> On Mon, Jul 20, 2020 at 12:14:00PM +0530, kajoljain wrote:
>>
>>
>> On 7/19/20 11:43 PM, Jiri Olsa wrote:
>>> hi,
>>> this patchset is adding the support to reused metric in
>>> another metric.
>>>
>>> For example, to define IPC by using CPI with change like:
>>>
>>> {
>>> "BriefDescription": "Instructions Per Cycle (per Logical
>>> Processor)",
>>> -"MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
>>> +"MetricExpr": "1/CPI",
>>> "MetricGroup": "TopDownL1",
>>> "MetricName": "IPC"
>>> },
>>>
>>> I won't be able to find all the possible places we could
>>> use this at, so I wonder you guys (who was asking for this)
>>> would try it and come up with comments if there's something
>>> missing or we could already use it at some places.
>>>
>>> It's based on Arnaldo's tmp.perf/core.
>>>
>>> v3 changes:
>>> - added some acks
>>> - some patches got merged
>>> - added missing zalloc include [John Garry]
>>> - added ids array outside the egroup object [Ian]
>>> - removed wrong m->has_constraint assignment [Ian]
>>> - renamed 'list' to 'metric_list' [Ian]
>>> - fixed group metric and added test for it [Paul A. Clarke]
>>> - fixed memory leak [Arnaldo]
>>> - using lowercase keys for metrics in hashmap, because jevents
>>> converts metric_expr to lowercase
>>>
>>> Also available in here:
>>> git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
>>> perf/metric
>>>
>>
>> Hi Jiri,
>>I am trying to review these patches and also test it in power box. I am
>> testing your `perf/metric` branch.
>> With your current patches,some of hv-24x7 events not giving appropriate
>> result
>> while doing "--metric-only" command. I can't see corresponding output for
>> all chips.
>
> hi,
> is that just for --metric-only option?
Hi Jiri,
So basically, this issue is for both with/without metric-only option.
Without metric-only option,
I am not able to see aggregate result for other chips.
This is with upstream kernel:
command# ./perf stat -M PowerBUS_Frequency -C 0 -I 1000
# time counts unit events
1.76370 7,807,494 hv_24x7/pm_pb_cyc,chip=0/ # 2.0
GHz PowerBUS_Frequency_0
1.76370 7,807,456 hv_24x7/pm_pb_cyc,chip=1/ # 2.0
GHz PowerBUS_Frequency_1
2.000259226 7,807,490 hv_24x7/pm_pb_cyc,chip=0/ # 2.0
GHz PowerBUS_Frequency_0
2.000259226 7,799,691 hv_24x7/pm_pb_cyc,chip=1/ # 1.9
GHz PowerBUS_Frequency_1
^C 2.745238246 5,816,562 hv_24x7/pm_pb_cyc,chip=0/ # 1.5
GHz PowerBUS_Frequency_0
2.745238246 5,816,580 hv_24x7/pm_pb_cyc,chip=1/ # 1.5
GHz PowerBUS_Frequency_1
Performance counter stats for 'CPU(s) 0':
21,431,546 hv_24x7/pm_pb_cyc,chip=0/ # 5.4 GHz
PowerBUS_Frequency_0
21,423,727 hv_24x7/pm_pb_cyc,chip=1/ # 5.4 GHz
PowerBUS_Frequency_1
This is with your perf/metric branch:
command# ./perf stat -M PowerBUS_Frequency -C 0 -I 1000
assertion failed at util/metricgroup.c:709
# time counts unit events
1.54545 7,807,505 hv_24x7/pm_pb_cyc,chip=0/ # 2.0
GHz PowerBUS_Frequency_0
1.54545 7,807,485 hv_24x7/pm_pb_cyc,chip=1/
2.000232761 7,807,500 hv_24x7/pm_pb_cyc,chip=0/ # 2.0
GHz PowerBUS_Frequency_0
2.000232761 7,807,478 hv_24x7/pm_pb_cyc,chip=1/
3.000363762 7,799,665 hv_24x7/pm_pb_cyc,chip=0/ # 1.9
GHz PowerBUS_Frequency_0
3.000363762 7,807,502 hv_24x7/pm_pb_cyc,chip=1/
^C 3.259418599 2,022,150 hv_24x7/pm_pb_cyc,chip=0/ # 0.5
GHz PowerBUS_Frequency_0
3.259418599 2,022,164 hv_24x7/pm_pb_cyc,chip=1/
Performance counter stats for 'CPU(s) 0':
25,436,820 hv_24x7/pm_pb_cyc,chip=0/ # 6.4 GHz
PowerBUS_Frequency_0
25,444,629 hv_24x7/pm_pb_cyc,chip=1/
3.259505529 seconds time elapsed
Thanks,
Kajol Jain
>
>>
>> This is output on power9 machine:
>>
>> Without your patches on upstream kernel:
>>
>> command# ./perf stat --metric-only -M PowerBUS_Frequency -I 1000 -C 0
>> # time GHz PowerBUS_Frequency_0 GHz PowerBUS_Frequency_1
>> 1.738772.02.0
>> 2.0002405512.01.9
>> ^C 2.4525905320.90.9
>>
>> Performance counter stats for 'CPU(s) 0':
>>
>>4.84.8
>>
>>2.452654834 seconds time elapsed
>>
>> With your patches on perf/metric branch:
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
On Mon, Jul 20, 2020 at 12:14:00PM +0530, kajoljain wrote:
>
>
> On 7/19/20 11:43 PM, Jiri Olsa wrote:
> > hi,
> > this patchset is adding the support to reused metric in
> > another metric.
> >
> > For example, to define IPC by using CPI with change like:
> >
> > {
> > "BriefDescription": "Instructions Per Cycle (per Logical
> > Processor)",
> > -"MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> > +"MetricExpr": "1/CPI",
> > "MetricGroup": "TopDownL1",
> > "MetricName": "IPC"
> > },
> >
> > I won't be able to find all the possible places we could
> > use this at, so I wonder you guys (who was asking for this)
> > would try it and come up with comments if there's something
> > missing or we could already use it at some places.
> >
> > It's based on Arnaldo's tmp.perf/core.
> >
> > v3 changes:
> > - added some acks
> > - some patches got merged
> > - added missing zalloc include [John Garry]
> > - added ids array outside the egroup object [Ian]
> > - removed wrong m->has_constraint assignment [Ian]
> > - renamed 'list' to 'metric_list' [Ian]
> > - fixed group metric and added test for it [Paul A. Clarke]
> > - fixed memory leak [Arnaldo]
> > - using lowercase keys for metrics in hashmap, because jevents
> > converts metric_expr to lowercase
> >
> > Also available in here:
> > git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > perf/metric
> >
>
> Hi Jiri,
>I am trying to review these patches and also test it in power box. I am
> testing your `perf/metric` branch.
> With your current patches,some of hv-24x7 events not giving appropriate result
> while doing "--metric-only" command. I can't see corresponding output for all
> chips.
hi,
is that just for --metric-only option?
>
> This is output on power9 machine:
>
> Without your patches on upstream kernel:
>
> command# ./perf stat --metric-only -M PowerBUS_Frequency -I 1000 -C 0
> # time GHz PowerBUS_Frequency_0 GHz PowerBUS_Frequency_1
> 1.738772.02.0
> 2.0002405512.01.9
> ^C 2.4525905320.90.9
>
> Performance counter stats for 'CPU(s) 0':
>
>4.84.8
>
>2.452654834 seconds time elapsed
>
> With your patches on perf/metric branch:
>
> command# ./perf stat --metric-only -M PowerBUS_Frequency -I 1000 -C 0
> assertion failed at util/metricgroup.c:709
> # time GHz PowerBUS_Frequency_0
> 1.738752.0
> 2.0003807062.0
> ^C 2.6589621821.3
>
> Performance counter stats for 'CPU(s) 0':
>
>5.2
>
> Please let me know, if I am missing something.
hum, I'll need to add test for metric with hv-24x7 events
thanks,
jirka
Re: [PATCHv3 00/19] perf metric: Add support to reuse metric
On 7/19/20 11:43 PM, Jiri Olsa wrote:
> hi,
> this patchset is adding the support to reused metric in
> another metric.
>
> For example, to define IPC by using CPI with change like:
>
> {
> "BriefDescription": "Instructions Per Cycle (per Logical Processor)",
> -"MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
> +"MetricExpr": "1/CPI",
> "MetricGroup": "TopDownL1",
> "MetricName": "IPC"
> },
>
> I won't be able to find all the possible places we could
> use this at, so I wonder you guys (who was asking for this)
> would try it and come up with comments if there's something
> missing or we could already use it at some places.
>
> It's based on Arnaldo's tmp.perf/core.
>
> v3 changes:
> - added some acks
> - some patches got merged
> - added missing zalloc include [John Garry]
> - added ids array outside the egroup object [Ian]
> - removed wrong m->has_constraint assignment [Ian]
> - renamed 'list' to 'metric_list' [Ian]
> - fixed group metric and added test for it [Paul A. Clarke]
> - fixed memory leak [Arnaldo]
> - using lowercase keys for metrics in hashmap, because jevents
> converts metric_expr to lowercase
>
> Also available in here:
> git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> perf/metric
>
Hi Jiri,
I am trying to review these patches and also test it in power box. I am
testing your `perf/metric` branch.
With your current patches,some of hv-24x7 events not giving appropriate result
while doing "--metric-only" command. I can't see corresponding output for all
chips.
This is output on power9 machine:
Without your patches on upstream kernel:
command# ./perf stat --metric-only -M PowerBUS_Frequency -I 1000 -C 0
# time GHz PowerBUS_Frequency_0 GHz PowerBUS_Frequency_1
1.738772.02.0
2.0002405512.01.9
^C 2.4525905320.90.9
Performance counter stats for 'CPU(s) 0':
4.84.8
2.452654834 seconds time elapsed
With your patches on perf/metric branch:
command# ./perf stat --metric-only -M PowerBUS_Frequency -I 1000 -C 0
assertion failed at util/metricgroup.c:709
# time GHz PowerBUS_Frequency_0
1.738752.0
2.0003807062.0
^C 2.6589621821.3
Performance counter stats for 'CPU(s) 0':
5.2
Please let me know, if I am missing something.
Thanks,
Kajol Jain
> thanks,
> jirka
>
>
> ---
> Jiri Olsa (19):
> perf metric: Fix memory leak in expr__add_id function
> perf metric: Add expr__add_id function
> perf metric: Change expr__get_id to return struct expr_id_data
> perf metric: Add expr__del_id function
> perf metric: Add macros for iterating map events
> perf metric: Add add_metric function
> perf metric: Rename __metricgroup__add_metric to __add_metric
> perf metric: Collect referenced metrics in struct metric_ref_node
> perf metric: Collect referenced metrics in struct metric_expr
> perf metric: Add referenced metrics to hash data
> perf metric: Compute referenced metrics
> perf metric: Add events for the current list
> perf metric: Add cache_miss_cycles to metric parse test
> perf metric: Add DCache_L2 to metric parse test
> perf metric: Add recursion check when processing nested metrics
> perf metric: Make compute_single function more precise
> perf metric: Add metric group test
> perf metric: Rename struct egroup to metric
> perf metric: Rename group_list to metric_list
>
> tools/perf/tests/parse-metric.c | 206
> +
> tools/perf/util/expr.c | 143 -
> tools/perf/util/expr.h | 30 +++-
> tools/perf/util/expr.y | 16 +++--
> tools/perf/util/metricgroup.c | 466
> +++
> tools/perf/util/metricgroup.h | 6 ++
> tools/perf/util/stat-shadow.c | 20 --
> 7 files changed, 751 insertions(+), 136 deletions(-)
>
[PATCHv3 00/19] perf metric: Add support to reuse metric
hi,
this patchset is adding the support to reused metric in
another metric.
For example, to define IPC by using CPI with change like:
{
"BriefDescription": "Instructions Per Cycle (per Logical Processor)",
-"MetricExpr": "INST_RETIRED.ANY / CPU_CLK_UNHALTED.THREAD",
+"MetricExpr": "1/CPI",
"MetricGroup": "TopDownL1",
"MetricName": "IPC"
},
I won't be able to find all the possible places we could
use this at, so I wonder you guys (who was asking for this)
would try it and come up with comments if there's something
missing or we could already use it at some places.
It's based on Arnaldo's tmp.perf/core.
v3 changes:
- added some acks
- some patches got merged
- added missing zalloc include [John Garry]
- added ids array outside the egroup object [Ian]
- removed wrong m->has_constraint assignment [Ian]
- renamed 'list' to 'metric_list' [Ian]
- fixed group metric and added test for it [Paul A. Clarke]
- fixed memory leak [Arnaldo]
- using lowercase keys for metrics in hashmap, because jevents
converts metric_expr to lowercase
Also available in here:
git://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
perf/metric
thanks,
jirka
---
Jiri Olsa (19):
perf metric: Fix memory leak in expr__add_id function
perf metric: Add expr__add_id function
perf metric: Change expr__get_id to return struct expr_id_data
perf metric: Add expr__del_id function
perf metric: Add macros for iterating map events
perf metric: Add add_metric function
perf metric: Rename __metricgroup__add_metric to __add_metric
perf metric: Collect referenced metrics in struct metric_ref_node
perf metric: Collect referenced metrics in struct metric_expr
perf metric: Add referenced metrics to hash data
perf metric: Compute referenced metrics
perf metric: Add events for the current list
perf metric: Add cache_miss_cycles to metric parse test
perf metric: Add DCache_L2 to metric parse test
perf metric: Add recursion check when processing nested metrics
perf metric: Make compute_single function more precise
perf metric: Add metric group test
perf metric: Rename struct egroup to metric
perf metric: Rename group_list to metric_list
tools/perf/tests/parse-metric.c | 206
+
tools/perf/util/expr.c | 143 -
tools/perf/util/expr.h | 30 +++-
tools/perf/util/expr.y | 16 +++--
tools/perf/util/metricgroup.c | 466
+++
tools/perf/util/metricgroup.h | 6 ++
tools/perf/util/stat-shadow.c | 20 --
7 files changed, 751 insertions(+), 136 deletions(-)
