Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"
On 3/12/20 4:22 PM, Jiri Olsa wrote:
> On Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain wrote:
>
> SNIP
>
>> +static int metricgroup__add_metric_runtime_param(struct strbuf *events,
>> +struct list_head *group_list, struct pmu_event *pe)
>> +{
>> +int i, count;
>> +int ret = -EINVAL;
>> +
>> +count = arch_get_runtimeparam();
>> +
>> +/* This loop is added to create multiple
>> + * events depend on count value and add
>> + * those events to group_list.
>> + */
>> +
>> +for (i = 0; i < count; i++) {
>> +const char **ids;
>> +int idnum;
>> +struct egroup *eg;
>> +char value[PATH_MAX];
>> +
>> +expr__runtimeparam = i;
>> +
>> +if (expr__find_other(pe->metric_expr,
>> +NULL, , ) < 0)
>> +return ret;
>> +
>> +if (events->len > 0)
>> +strbuf_addf(events, ",");
>> +
>> +if (metricgroup__has_constraint(pe))
>> +metricgroup__add_metric_non_group(events, ids, idnum);
>> +else
>> +metricgroup__add_metric_weak_group(events, ids, idnum);
>> +
>> +eg = malloc(sizeof(struct egroup));
>> +if (!eg) {
>> +ret = -ENOMEM;
>> +return ret;
>> +}
>> +sprintf(value, "%s%c%d", pe->metric_name, '_', i);
>> +eg->ids = ids;
>> +eg->idnum = idnum;
>> +eg->metric_name = strdup(value);
>> +eg->metric_expr = pe->metric_expr;
>> +eg->metric_unit = pe->unit;
>> +list_add_tail(>nd, group_list);
>> +ret = 0;
>> +
>> +if (ret != 0)
>> +break;
>
> the inside loop is essentialy what you factor out to
> metricgroup__add_metric_param right? please nove
> addition of metricgroup__add_metric_param function
> into separate patch
>
> jirka
>
Ok will put it in seperate patch.
Thanks,
kajol
>
>> +}
>> +return ret;
>> +}
>> +static int metricgroup__add_metric_param(struct strbuf *events,
>> +struct list_head *group_list, struct pmu_event *pe)
>> +{
>> +
>> +const char **ids;
>> +int idnum;
>> +struct egroup *eg;
>> +int ret = -EINVAL;
>> +
>> +if (expr__find_other(pe->metric_expr,
>> + NULL, , ) < 0)
>> +return ret;
>> +if (events->len > 0)
>> +strbuf_addf(events, ",");
>> +
>> +if (metricgroup__has_constraint(pe))
>> +metricgroup__add_metric_non_group(events, ids, idnum);
>> +else
>> +metricgroup__add_metric_weak_group(events, ids, idnum);
>> +
>> +eg = malloc(sizeof(struct egroup));
>> +if (!eg)
>> +ret = -ENOMEM;
>> +
>> +eg->ids = ids;
>> +eg->idnum = idnum;
>> +eg->metric_name = pe->metric_name;
>> +eg->metric_expr = pe->metric_expr;
>> +eg->metric_unit = pe->unit;
>> +list_add_tail(>nd, group_list);
>> +ret = 0;
>> +
>> +return ret;
>> +}
>
> SNIP
>
Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"
On Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain wrote:
SNIP
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 9377538f4097..d17664e628db 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -15,6 +15,7 @@ struct parse_ctx {
> struct parse_id ids[MAX_PARSE_ID];
> };
>
> +int expr__runtimeparam;
> void expr__ctx_init(struct parse_ctx *ctx);
> void expr__add_id(struct parse_ctx *ctx, const char *id, double val);
> int expr__parse(double *final_val, struct parse_ctx *ctx, const char *expr);
> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
> index 1928f2a3dddc..ec4b00671f67 100644
> --- a/tools/perf/util/expr.l
> +++ b/tools/perf/util/expr.l
> @@ -45,6 +45,21 @@ static char *normalize(char *str)
> *dst++ = '/';
> else if (*str == '\\')
> *dst++ = *++str;
> +else if (*str == '?') {
> +
> + int size = snprintf(NULL, 0, "%d", expr__runtimeparam);
> + char * paramval = (char *)malloc(size);
can't we agree that any reasonable number in here
wouldn't cross 20 bytes in string or so and use
buffer for that instead of that malloc exercise?
thanks,
jirka
> + int i = 0;
> +
> + if(!paramval)
> + *dst++ = '0';
> + else {
> + sprintf(paramval, "%d", expr__runtimeparam);
> + while(i < size)
> + *dst++ = paramval[i++];
> + free(paramval);
> + }
> + }
SNIP
Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"
On Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain wrote:
SNIP
> +static int metricgroup__add_metric_runtime_param(struct strbuf *events,
> + struct list_head *group_list, struct pmu_event *pe)
> +{
> + int i, count;
> + int ret = -EINVAL;
> +
> + count = arch_get_runtimeparam();
> +
> + /* This loop is added to create multiple
> + * events depend on count value and add
> + * those events to group_list.
> + */
> +
> + for (i = 0; i < count; i++) {
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + char value[PATH_MAX];
> +
> + expr__runtimeparam = i;
> +
> + if (expr__find_other(pe->metric_expr,
> + NULL, , ) < 0)
> + return ret;
> +
> + if (events->len > 0)
> + strbuf_addf(events, ",");
> +
> + if (metricgroup__has_constraint(pe))
> + metricgroup__add_metric_non_group(events, ids, idnum);
> + else
> + metricgroup__add_metric_weak_group(events, ids, idnum);
> +
> + eg = malloc(sizeof(struct egroup));
> + if (!eg) {
> + ret = -ENOMEM;
> + return ret;
> + }
> + sprintf(value, "%s%c%d", pe->metric_name, '_', i);
> + eg->ids = ids;
> + eg->idnum = idnum;
> + eg->metric_name = strdup(value);
> + eg->metric_expr = pe->metric_expr;
> + eg->metric_unit = pe->unit;
> + list_add_tail(>nd, group_list);
> + ret = 0;
> +
> + if (ret != 0)
> + break;
the inside loop is essentialy what you factor out to
metricgroup__add_metric_param right? please nove
addition of metricgroup__add_metric_param function
into separate patch
jirka
> + }
> + return ret;
> +}
> +static int metricgroup__add_metric_param(struct strbuf *events,
> + struct list_head *group_list, struct pmu_event *pe)
> +{
> +
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + int ret = -EINVAL;
> +
> + if (expr__find_other(pe->metric_expr,
> + NULL, , ) < 0)
> + return ret;
> + if (events->len > 0)
> + strbuf_addf(events, ",");
> +
> + if (metricgroup__has_constraint(pe))
> + metricgroup__add_metric_non_group(events, ids, idnum);
> + else
> + metricgroup__add_metric_weak_group(events, ids, idnum);
> +
> + eg = malloc(sizeof(struct egroup));
> + if (!eg)
> + ret = -ENOMEM;
> +
> + eg->ids = ids;
> + eg->idnum = idnum;
> + eg->metric_name = pe->metric_name;
> + eg->metric_expr = pe->metric_expr;
> + eg->metric_unit = pe->unit;
> + list_add_tail(>nd, group_list);
> + ret = 0;
> +
> + return ret;
> +}
SNIP
Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"
On Tue, Mar 10, 2020 at 03:34:55PM -0300, Arnaldo Carvalho de Melo wrote:
SNIP
> > diff --git a/tools/perf/arch/powerpc/util/header.c
> > b/tools/perf/arch/powerpc/util/header.c
> > index 3b4cdfc5efd6..036f6b2ce202 100644
> > --- a/tools/perf/arch/powerpc/util/header.c
> > +++ b/tools/perf/arch/powerpc/util/header.c
> > @@ -7,6 +7,11 @@
> > #include
> > #include
> > #include "header.h"
> > +#include "metricgroup.h"
> > +#include "evlist.h"
> > +#include
> > +#include "pmu.h"
> > +#include
> >
> > #define mfspr(rn) ({unsigned long rval; \
> > asm volatile("mfspr %0," __stringify(rn) \
> > @@ -16,6 +21,8 @@
> > #define PVR_VER(pvr)(((pvr) >> 16) & 0x) /* Version field */
> > #define PVR_REV(pvr)(((pvr) >> 0) & 0x) /* Revison field */
> >
> > +#define SOCKETS_INFO_FILE_PATH "/devices/hv_24x7/interface/"
> > +
> > int
> > get_cpuid(char *buffer, size_t sz)
> > {
> > @@ -44,3 +51,18 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
> >
> > return bufp;
> > }
> > +
> > +int arch_get_runtimeparam(void)
> > +{
> > + int count;
> > + char path[PATH_MAX];
> > + char filename[] = "sockets";
> > +
> > + snprintf(path, PATH_MAX,
> > +SOCKETS_INFO_FILE_PATH "%s", filename);
also, what's the point of using snprintf in here?
jirka
Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"
On Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain wrote:
SNIP
> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
> index 1928f2a3dddc..ec4b00671f67 100644
> --- a/tools/perf/util/expr.l
> +++ b/tools/perf/util/expr.l
> @@ -45,6 +45,21 @@ static char *normalize(char *str)
> *dst++ = '/';
> else if (*str == '\\')
> *dst++ = *++str;
> +else if (*str == '?') {
> +
extra line ^^^
jirka
> + int size = snprintf(NULL, 0, "%d", expr__runtimeparam);
> + char * paramval = (char *)malloc(size);
> + int i = 0;
> +
> + if(!paramval)
SNIP
Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"
On Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain wrote:
SNIP
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index c3a8c701609a..11eeeb929b91 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -474,6 +474,98 @@ static bool metricgroup__has_constraint(struct pmu_event
> *pe)
> return false;
> }
>
> +int __weak arch_get_runtimeparam(void)
> +{
> + return 1;
> +}
> +
> +static int metricgroup__add_metric_runtime_param(struct strbuf *events,
> + struct list_head *group_list, struct pmu_event *pe)
> +{
> + int i, count;
> + int ret = -EINVAL;
> +
> + count = arch_get_runtimeparam();
> +
> + /* This loop is added to create multiple
> + * events depend on count value and add
> + * those events to group_list.
> + */
> +
> + for (i = 0; i < count; i++) {
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + char value[PATH_MAX];
> +
> + expr__runtimeparam = i;
so the expr__runtimeparam is always set before we call the
expr parsing function - wither expr__find_other or expr__parse,
and it's used inside the normalize flexer function, which has
access to the passed context.. so I don't see a reason why
expr__runtimeparam couldn't be added in struct parse_ctx
and used from there..
while in this, perhaps we should rename parse_ctx to expr_ctx,
to keep the namespace straight (in separate patch)
thanks,
jirka
Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"
On 3/11/20 12:04 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain escreveu:
>> 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.
>>
>> 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'.
>>
>> 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/".
>>
>> Signed-off-by: Kajol Jain
>> ---
>> tools/perf/arch/powerpc/util/header.c | 22 +
>> tools/perf/util/expr.h| 1 +
>> tools/perf/util/expr.l| 19 +++-
>> tools/perf/util/metricgroup.c | 124 --
>> tools/perf/util/metricgroup.h | 1 +
>> tools/perf/util/stat-shadow.c | 8 ++
>> 6 files changed, 148 insertions(+), 27 deletions(-)
>>
>> diff --git a/tools/perf/arch/powerpc/util/header.c
>> b/tools/perf/arch/powerpc/util/header.c
>> index 3b4cdfc5efd6..036f6b2ce202 100644
>> --- a/tools/perf/arch/powerpc/util/header.c
>> +++ b/tools/perf/arch/powerpc/util/header.c
>> @@ -7,6 +7,11 @@
>> #include
>> #include
>> #include "header.h"
>> +#include "metricgroup.h"
>> +#include "evlist.h"
>> +#include
>> +#include "pmu.h"
>> +#include
>>
>> #define mfspr(rn) ({unsigned long rval; \
>> asm volatile("mfspr %0," __stringify(rn) \
>> @@ -16,6 +21,8 @@
>> #define PVR_VER(pvr)(((pvr) >> 16) & 0x) /* Version field */
>> #define PVR_REV(pvr)(((pvr) >> 0) & 0x) /* Revison field */
>>
>> +#define SOCKETS_INFO_FILE_PATH "/devices/hv_24x7/interface/"
>> +
>> int
>> get_cpuid(char *buffer, size_t sz)
>> {
>> @@ -44,3 +51,18 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
>>
>> return bufp;
>> }
>> +
>> +int arch_get_runtimeparam(void)
>> +{
>> +int count;
>> +char path[PATH_MAX];
>> +char filename[] = "sockets";
>> +
>> +snprintf(path, PATH_MAX,
>> + SOCKETS_INFO_FILE_PATH "%s", filename);
>> +
>> +if (sysfs__read_ull(path, (unsigned long long *)) < 0)
>> +return 1;
>> +else
>> +return count;
>
> Why this cast dance? We have sysfs__read_int(path, ).
>
> Also this is more compact:
>
> return sysfs__read_int(path, ) < 0 ? 1 : count;
Hi Arnaldo,
Yes it will be better to use like that.
>
>> +}
>> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
>> index 9377538f4097..d17664e628db 100644
>> --- a/tools/perf/util/expr.h
>> +++ b/tools/perf/util/expr.h
>> @@ -15,6 +15,7 @@ struct parse_ctx {
>> struct parse_id ids[MAX_PARSE_ID];
>> };
>>
>> +int expr__runtimeparam;
>> void expr__ctx_init(struct parse_ctx *ctx);
>> void expr__add_id(struct parse_ctx *ctx, const char *id, double val);
>> int expr__parse(double *final_val, struct parse_ctx *ctx, const char *expr);
>> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
>> index 1928f2a3dddc..ec4b00671f67 100644
>> --- a/tools/perf/util/expr.l
>> +++ b/tools/perf/util/expr.l
>> @@ -45,6 +45,21 @@ static char *normalize(char *str)
>> *dst++ = '/';
>> else if (*str == '\\')
>> *dst++ = *++str;
>> +else if (*str == '?') {
>> +
>> +int size = snprintf(NULL, 0, "%d", expr__runtimeparam);
>
> TIL that C99 allows for a NULL str to format and return the number of
> bytes it would write if the string was large enough... wonders never
> cease :-)
>
>> +char * paramval = (char *)malloc(size);
>
> No need for the cast, malloc returns void *, or has that changed? 8-)
> and please no space before the variable name.
>
Okk, Will take care of that.
> Humm this is all complicated, why not use asprintf and have something
> like:
>
>> +int i = 0;
>> +
>> +if(!paramval)
>> +*dst++ = '0';
>> +else {
>> +sprintf(paramval, "%d", expr__runtimeparam);
>> +while(i < size)
>> +*dst++ = paramval[i++];
>> +free(paramval);
>> +}
>
> char *paramval;
>
Re: [PATCH v4 6/8] perf/tools: Enhance JSON/metric infrastructure to handle "?"
Em Mon, Mar 09, 2020 at 11:55:50AM +0530, Kajol Jain escreveu:
> 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.
>
> 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'.
>
> 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/".
>
> Signed-off-by: Kajol Jain
> ---
> tools/perf/arch/powerpc/util/header.c | 22 +
> tools/perf/util/expr.h| 1 +
> tools/perf/util/expr.l| 19 +++-
> tools/perf/util/metricgroup.c | 124 --
> tools/perf/util/metricgroup.h | 1 +
> tools/perf/util/stat-shadow.c | 8 ++
> 6 files changed, 148 insertions(+), 27 deletions(-)
>
> diff --git a/tools/perf/arch/powerpc/util/header.c
> b/tools/perf/arch/powerpc/util/header.c
> index 3b4cdfc5efd6..036f6b2ce202 100644
> --- a/tools/perf/arch/powerpc/util/header.c
> +++ b/tools/perf/arch/powerpc/util/header.c
> @@ -7,6 +7,11 @@
> #include
> #include
> #include "header.h"
> +#include "metricgroup.h"
> +#include "evlist.h"
> +#include
> +#include "pmu.h"
> +#include
>
> #define mfspr(rn) ({unsigned long rval; \
>asm volatile("mfspr %0," __stringify(rn) \
> @@ -16,6 +21,8 @@
> #define PVR_VER(pvr)(((pvr) >> 16) & 0x) /* Version field */
> #define PVR_REV(pvr)(((pvr) >> 0) & 0x) /* Revison field */
>
> +#define SOCKETS_INFO_FILE_PATH "/devices/hv_24x7/interface/"
> +
> int
> get_cpuid(char *buffer, size_t sz)
> {
> @@ -44,3 +51,18 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
>
> return bufp;
> }
> +
> +int arch_get_runtimeparam(void)
> +{
> + int count;
> + char path[PATH_MAX];
> + char filename[] = "sockets";
> +
> + snprintf(path, PATH_MAX,
> + SOCKETS_INFO_FILE_PATH "%s", filename);
> +
> + if (sysfs__read_ull(path, (unsigned long long *)) < 0)
> + return 1;
> + else
> + return count;
Why this cast dance? We have sysfs__read_int(path, ).
Also this is more compact:
return sysfs__read_int(path, ) < 0 ? 1 : count;
> +}
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 9377538f4097..d17664e628db 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -15,6 +15,7 @@ struct parse_ctx {
> struct parse_id ids[MAX_PARSE_ID];
> };
>
> +int expr__runtimeparam;
> void expr__ctx_init(struct parse_ctx *ctx);
> void expr__add_id(struct parse_ctx *ctx, const char *id, double val);
> int expr__parse(double *final_val, struct parse_ctx *ctx, const char *expr);
> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
> index 1928f2a3dddc..ec4b00671f67 100644
> --- a/tools/perf/util/expr.l
> +++ b/tools/perf/util/expr.l
> @@ -45,6 +45,21 @@ static char *normalize(char *str)
> *dst++ = '/';
> else if (*str == '\\')
> *dst++ = *++str;
> +else if (*str == '?') {
> +
> + int size = snprintf(NULL, 0, "%d", expr__runtimeparam);
TIL that C99 allows for a NULL str to format and return the number of
bytes it would write if the string was large enough... wonders never
cease :-)
> + char * paramval = (char *)malloc(size);
No need for the cast, malloc returns void *, or has that changed? 8-)
and please no space before the variable name.
Humm this is all complicated, why not use asprintf and have something
like:
> + int i = 0;
> +
> + if(!paramval)
> + *dst++ = '0';
> + else {
> + sprintf(paramval, "%d", expr__runtimeparam);
> + while(i < size)
> + *dst++ = paramval[i++];
> + free(paramval);
> + }
char *paramval;
int size = asprintf(, "%d",
expr__runtimeparam);
if (size < 0)
*dst++ = '0';
else {
while (i < size)
