Re: [PATCH v7 4/6] perf/tools: Enhance JSON/metric infrastructure to handle "?"

2020-03-30 Thread Jiri Olsa
On Fri, Mar 27, 2020 at 03:55:26PM +0530, Kajol Jain wrote:

SNIP

>  
>  %%
>   struct expr_scanner_ctx *sctx = expr_get_extra(yyscanner);
> @@ -93,7 +106,7 @@ if { return IF; }
>  else { return ELSE; }
>  #smt_on  { return SMT_ON; }
>  {number} { return value(yyscanner, 10); }
> -{symbol} { return str(yyscanner, ID); }
> +{symbol} { return str(yyscanner, ID, sctx->runtime_param); }
>  "|"  { return '|'; }
>  "^"  { return '^'; }
>  "&"  { return '&'; }
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index b905eaa907a7..66695b4a358d 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -90,6 +90,7 @@ struct egroup {
>   const char *metric_name;
>   const char *metric_expr;
>   const char *metric_unit;
> + int param;

could you please use runtime_param everywhere? ... s/param/runtime_param/
or maybe just 'runtime' to keep it short..? it's param in any case ;-)

other than that it looks ok

jirka



Re: [PATCH v7 4/6] perf/tools: Enhance JSON/metric infrastructure to handle "?"

2020-03-30 Thread Jiri Olsa
On Fri, Mar 27, 2020 at 03:55:26PM +0530, Kajol Jain wrote:

SNIP

> 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"));

could we add test case for runtime param > 1 in here?
expr_parse should return value that would depend on this value..

jirka



[PATCH v7 4/6] perf/tools: Enhance JSON/metric infrastructure to handle "?"

2020-03-27 Thread Kajol Jain
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.

To understand which data count belongs to which parameter value,
we also printing param value in generic_metric function.

For example,
command:# ./perf stat  -M PowerBUS_Frequency -C 0 -I 1000
 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.

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 | 28 ---
 tools/perf/util/metricgroup.h |  2 ++
 tools/perf/util/stat-shadow.c | 17 ++--
 8 files changed, 79 insertions(+), 27 deletions(-)

diff --git a/tools/perf/arch/powerpc/util/header.c 
b/tools/perf/arch/powerpc/util/header.c
index 3b4cdfc5efd6..d4870074f14c 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 ? 1 : 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 a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index c3382d58cf40..9d56e5fb3873 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -27,10 +27,11 @@ void expr__ctx_init(struct expr_parse_ctx *ctx)
 
 static int
 __expr__parse(double *val, struct expr_parse_ctx *ctx, const char *expr,
- int start)
+ int start, int param)
 {
struct