Re: [PATCH v6 09/11] perf/tools: Enhance JSON/metric infrastructure to handle "?"
On 3/24/20 6:41 PM, Jiri Olsa wrote: > On Fri, Mar 20, 2020 at 06:24:04PM +0530, Kajol Jain wrote: >> Patch enhances current metric infrastructure to handle "?" in the metric >> expression. The "?" can be use for parameters whose value not known while >> creating metric events and which can be replace later at runtime to >> the proper value. It also add flexibility to create multiple events out >> of single metric event added in json file. >> >> Patch adds function 'arch_get_runtimeparam' which is a arch specific >> function, returns the count of metric events need to be created. >> By default it return 1. >> >> This infrastructure needed for hv_24x7 socket/chip level events. >> "hv_24x7" chip level events needs specific chip-id to which the >> data is requested. Function 'arch_get_runtimeparam' implemented >> in header.c which extract number of sockets from sysfs file >> "sockets" under "/sys/devices/hv_24x7/interface/". >> >> >> With this patch basically we are trying to create as many metric events >> as define by runtime_param. >> >> For that one loop is added in function 'metricgroup__add_metric', >> which create multiple events at run time depend on return value of >> 'arch_get_runtimeparam' and merge that event in 'group_list'. >> >> To achieve that we are actually passing this parameter value as part of >> `expr__find_other` function and changing "?" present in metric expression >> with this value. >> >> As in our json file, there gonna be single metric event, and out of >> which we are creating multiple events, I am also merging this value >> to the original metric name to specify parameter value. >> >> For example, >> command:# ./perf stat -M PowerBUS_Frequency -C 0 -I 1000 >> # time counts unit events >> 1.000101867 9,356,933 hv_24x7/pm_pb_cyc,chip=0/ # >> 2.3 GHz PowerBUS_Frequency_0 >> 1.000101867 9,366,134 hv_24x7/pm_pb_cyc,chip=1/ # >> 2.3 GHz PowerBUS_Frequency_1 >> 2.000314878 9,365,868 hv_24x7/pm_pb_cyc,chip=0/ # >> 2.3 GHz PowerBUS_Frequency_0 >> 2.000314878 9,366,092 hv_24x7/pm_pb_cyc,chip=1/ # >> 2.3 GHz PowerBUS_Frequency_1 >> >> So, here _0 and _1 after PowerBUS_Frequency specify parameter value. >> >> As after adding this to group_list, again we call expr__parse >> in 'generic_metric' function present in util/stat-display.c. >> By this time again we need to pass this parameter value. So, now to get this >> value >> actually I am trying to extract it from metric name itself. Because >> otherwise it gonna point to last updated value present in runtime_param. >> And gonna match for that value only. > > so why can't we pass that param as integer value through the metric objects? > > it get's created in metricgroup__add_metric_param: > - as struct egroup *eg > - we can add egroup::param and store the param value there > > then in metricgroup__setup_events it moves to: > - struct metric_expr *expr > - we can add metric_expr::param to keep the param > > then in perf_stat__print_shadow_stats there's: > - struct metric_expr *mexp loop > - calling generic_metric metric - we could call it with mexp::param > - and pass the param to expr__parse > Hi jiri, Thanks for the suggestion, Yes it make more sense to use like that. Will update. Thanks, Kajol > jirka >
Re: [PATCH v6 09/11] perf/tools: Enhance JSON/metric infrastructure to handle "?"
On Fri, Mar 20, 2020 at 06:24:04PM +0530, Kajol Jain wrote: > Patch enhances current metric infrastructure to handle "?" in the metric > expression. The "?" can be use for parameters whose value not known while > creating metric events and which can be replace later at runtime to > the proper value. It also add flexibility to create multiple events out > of single metric event added in json file. > > Patch adds function 'arch_get_runtimeparam' which is a arch specific > function, returns the count of metric events need to be created. > By default it return 1. > > This infrastructure needed for hv_24x7 socket/chip level events. > "hv_24x7" chip level events needs specific chip-id to which the > data is requested. Function 'arch_get_runtimeparam' implemented > in header.c which extract number of sockets from sysfs file > "sockets" under "/sys/devices/hv_24x7/interface/". > > > With this patch basically we are trying to create as many metric events > as define by runtime_param. > > For that one loop is added in function 'metricgroup__add_metric', > which create multiple events at run time depend on return value of > 'arch_get_runtimeparam' and merge that event in 'group_list'. > > To achieve that we are actually passing this parameter value as part of > `expr__find_other` function and changing "?" present in metric expression > with this value. > > As in our json file, there gonna be single metric event, and out of > which we are creating multiple events, I am also merging this value > to the original metric name to specify parameter value. > > For example, > command:# ./perf stat -M PowerBUS_Frequency -C 0 -I 1000 > # time counts unit events > 1.000101867 9,356,933 hv_24x7/pm_pb_cyc,chip=0/ # 2.3 > GHz PowerBUS_Frequency_0 > 1.000101867 9,366,134 hv_24x7/pm_pb_cyc,chip=1/ # 2.3 > GHz PowerBUS_Frequency_1 > 2.000314878 9,365,868 hv_24x7/pm_pb_cyc,chip=0/ # 2.3 > GHz PowerBUS_Frequency_0 > 2.000314878 9,366,092 hv_24x7/pm_pb_cyc,chip=1/ # 2.3 > GHz PowerBUS_Frequency_1 > > So, here _0 and _1 after PowerBUS_Frequency specify parameter value. > > As after adding this to group_list, again we call expr__parse > in 'generic_metric' function present in util/stat-display.c. > By this time again we need to pass this parameter value. So, now to get this > value > actually I am trying to extract it from metric name itself. Because > otherwise it gonna point to last updated value present in runtime_param. > And gonna match for that value only. so why can't we pass that param as integer value through the metric objects? it get's created in metricgroup__add_metric_param: - as struct egroup *eg - we can add egroup::param and store the param value there then in metricgroup__setup_events it moves to: - struct metric_expr *expr - we can add metric_expr::param to keep the param then in perf_stat__print_shadow_stats there's: - struct metric_expr *mexp loop - calling generic_metric metric - we could call it with mexp::param - and pass the param to expr__parse jirka
Re: [PATCH v6 09/11] perf/tools: Enhance JSON/metric infrastructure to handle "?"
On Fri, Mar 20, 2020 at 06:24:04PM +0530, Kajol Jain wrote:
SNIP
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 52fb119d25c8..b4b91d8ad5be 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -474,8 +474,13 @@ static bool metricgroup__has_constraint(struct pmu_event
> *pe)
> return false;
> }
>
> +int __weak arch_get_runtimeparam(void)
> +{
> + return 1;
> +}
> +
> static int metricgroup__add_metric_param(struct strbuf *events,
> - struct list_head *group_list, struct pmu_event *pe)
> + struct list_head *group_list, struct pmu_event *pe, int param)
> {
>
> const char **ids;
> @@ -483,7 +488,7 @@ static int metricgroup__add_metric_param(struct strbuf
> *events,
could you please call this function __metricgroup__add_metric instead?
> struct egroup *eg;
> int ret = -EINVAL;
>
> - if (expr__find_other(pe->metric_expr, NULL, , ) < 0)
> + if (expr__find_other(pe->metric_expr, NULL, , , param) < 0)
> return ret;
>
> if (events->len > 0)
> @@ -502,11 +507,21 @@ static int metricgroup__add_metric_param(struct strbuf
> *events,
>
> eg->ids = ids;
> eg->idnum = idnum;
> - eg->metric_name = pe->metric_name;
> + if (strstr(pe->metric_expr, "?")) {
> + char value[PATH_MAX];
> +
> + sprintf(value, "%s%c%d", pe->metric_name, '_', param);
> + eg->metric_name = strdup(value);
how is eg->metric_name getting released?
> + if (!eg->metric_name) {
> + ret = -ENOMEM;
> + return ret;
return -ENOMEM; ??
> + }
> + }
> + else
> + eg->metric_name = pe->metric_name;
> eg->metric_expr = pe->metric_expr;
> eg->metric_unit = pe->unit;
> list_add_tail(>nd, group_list);
> -
> return 0;
> }
>
SNIP
thanks,
jirka
[PATCH v6 09/11] perf/tools: Enhance JSON/metric infrastructure to handle "?"
Patch enhances current metric infrastructure to handle "?" in the metric
expression. The "?" can be use for parameters whose value not known while
creating metric events and which can be replace later at runtime to
the proper value. It also add flexibility to create multiple events out
of single metric event added in json file.
Patch adds function 'arch_get_runtimeparam' which is a arch specific
function, returns the count of metric events need to be created.
By default it return 1.
This infrastructure needed for hv_24x7 socket/chip level events.
"hv_24x7" chip level events needs specific chip-id to which the
data is requested. Function 'arch_get_runtimeparam' implemented
in header.c which extract number of sockets from sysfs file
"sockets" under "/sys/devices/hv_24x7/interface/".
With this patch basically we are trying to create as many metric events
as define by runtime_param.
For that one loop is added in function 'metricgroup__add_metric',
which create multiple events at run time depend on return value of
'arch_get_runtimeparam' and merge that event in 'group_list'.
To achieve that we are actually passing this parameter value as part of
`expr__find_other` function and changing "?" present in metric expression
with this value.
As in our json file, there gonna be single metric event, and out of
which we are creating multiple events, I am also merging this value
to the original metric name to specify parameter value.
For example,
command:# ./perf stat -M PowerBUS_Frequency -C 0 -I 1000
# time counts unit events
1.000101867 9,356,933 hv_24x7/pm_pb_cyc,chip=0/ # 2.3
GHz PowerBUS_Frequency_0
1.000101867 9,366,134 hv_24x7/pm_pb_cyc,chip=1/ # 2.3
GHz PowerBUS_Frequency_1
2.000314878 9,365,868 hv_24x7/pm_pb_cyc,chip=0/ # 2.3
GHz PowerBUS_Frequency_0
2.000314878 9,366,092 hv_24x7/pm_pb_cyc,chip=1/ # 2.3
GHz PowerBUS_Frequency_1
So, here _0 and _1 after PowerBUS_Frequency specify parameter value.
As after adding this to group_list, again we call expr__parse
in 'generic_metric' function present in util/stat-display.c.
By this time again we need to pass this parameter value. So, now to get this
value
actually I am trying to extract it from metric name itself. Because
otherwise it gonna point to last updated value present in runtime_param.
And gonna match for that value only.
Signed-off-by: Kajol Jain
---
tools/perf/arch/powerpc/util/header.c | 8 ++
tools/perf/tests/expr.c | 8 +++---
tools/perf/util/expr.c| 11
tools/perf/util/expr.h| 5 ++--
tools/perf/util/expr.l| 27 +-
tools/perf/util/metricgroup.c | 40 +++
tools/perf/util/metricgroup.h | 1 +
tools/perf/util/stat-shadow.c | 12 ++--
8 files changed, 86 insertions(+), 26 deletions(-)
diff --git a/tools/perf/arch/powerpc/util/header.c
b/tools/perf/arch/powerpc/util/header.c
index 3b4cdfc5efd6..c0a86afe63fb 100644
--- a/tools/perf/arch/powerpc/util/header.c
+++ b/tools/perf/arch/powerpc/util/header.c
@@ -7,6 +7,8 @@
#include
#include
#include "header.h"
+#include "metricgroup.h"
+#include
#define mfspr(rn) ({unsigned long rval; \
asm volatile("mfspr %0," __stringify(rn) \
@@ -44,3 +46,9 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
return bufp;
}
+
+int arch_get_runtimeparam(void)
+{
+ int count;
+ return sysfs__read_int("/devices/hv_24x7/interface/sockets", ) <
0 ? 3 : count;
+}
diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index ea10fc4412c4..516504cf0ea5 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -10,7 +10,7 @@ static int test(struct expr_parse_ctx *ctx, const char *e,
double val2)
{
double val;
- if (expr__parse(, ctx, e))
+ if (expr__parse(, ctx, e, 1))
TEST_ASSERT_VAL("parse test failed", 0);
TEST_ASSERT_VAL("unexpected value", val == val2);
return 0;
@@ -44,15 +44,15 @@ int test__expr(struct test *t __maybe_unused, int subtest
__maybe_unused)
return ret;
p = "FOO/0";
- ret = expr__parse(, , p);
+ ret = expr__parse(, , p, 1);
TEST_ASSERT_VAL("division by zero", ret == -1);
p = "BAR/";
- ret = expr__parse(, , p);
+ ret = expr__parse(, , p, 1);
TEST_ASSERT_VAL("missing operand", ret == -1);
TEST_ASSERT_VAL("find other",
- expr__find_other("FOO + BAR + BAZ + BOZO", "FOO",
, _other) == 0);
+ expr__find_other("FOO + BAR + BAZ + BOZO", "FOO",
, _other, 1) == 0);
TEST_ASSERT_VAL("find other", num_other == 3);
TEST_ASSERT_VAL("find other", !strcmp(other[0], "BAR"));
TEST_ASSERT_VAL("find other", !strcmp(other[1], "BAZ"));
diff --git
