| Message ID | 20200309062552.29911-7-kjain@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/perf: Add json file metric support for the hv_24x7 socket/chip level events | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/merge (ab326587bb5fb91cc97df9b9f48e9e1469f04621) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/next (247257b03b04398ca07da4bce3d17bee25d623cb) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linus/master (2c523b344dfa65a3738e7039832044aa133c75fb) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/fixes (59bee45b9712c759ea4d3dcc4eff1752f3a66558) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linux-next (b86a6a241b7c60ca7a6ca4fb3c0d2aedbbf2c1b6) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
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 <kjain@linux.ibm.com> > --- > 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 <string.h> > #include <linux/stringify.h> > #include "header.h" > +#include "metricgroup.h" > +#include "evlist.h" > +#include <dirent.h> > +#include "pmu.h" > +#include <api/fs/fs.h> > > #define mfspr(rn) ({unsigned long rval; \ > asm volatile("mfspr %0," __stringify(rn) \ > @@ -16,6 +21,8 @@ > #define PVR_VER(pvr) (((pvr) >> 16) & 0xFFFF) /* Version field */ > #define PVR_REV(pvr) (((pvr) >> 0) & 0xFFFF) /* 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 *)&count) < 0) > + return 1; > + else > + return count; Why this cast dance? We have sysfs__read_int(path, &count). Also this is more compact: return sysfs__read_int(path, &count) < 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(¶mval, "%d", expr__runtimeparam); if (size < 0) *dst++ = '0'; else { while (i < size) *dst++ = paramval[i++]; free(paramval); } > + } > else > *dst++ = *str; > str++; > @@ -72,8 +87,8 @@ number [0-9]+ > > sch [-,=] > spec \\{sch} > -sym [0-9a-zA-Z_\.:@]+ > -symbol {spec}*{sym}*{spec}*{sym}* > +sym [0-9a-zA-Z_\.:@?]+ > +symbol {spec}*{sym}*{spec}*{sym}*{spec}*{sym} > > %% > { > 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; > + > + if (expr__find_other(pe->metric_expr, > + NULL, &ids, &idnum) < 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)); Shorter form that works even if you change that type: + eg = malloc(sizeof(*eg)); > + 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); Please check strdup() return just like you checked the malloc(sizeof(struct egroup)). > + eg->metric_expr = pe->metric_expr; > + eg->metric_unit = pe->unit; > + list_add_tail(&eg->nd, group_list); > + ret = 0; > + > + if (ret != 0) > + break; > + } > + 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, &ids, &idnum) < 0) Why break the above in two lines? > + 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)); Ditto > + 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(&eg->nd, group_list); > + ret = 0; > + > + return ret; > +} > + > static int metricgroup__add_metric(const char *metric, struct strbuf *events, > struct list_head *group_list) > { > @@ -493,35 +585,17 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events, > continue; > if (match_metric(pe->metric_group, metric) || > match_metric(pe->metric_name, metric)) { > - const char **ids; > - int idnum; > - struct egroup *eg; > > pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name); > > - if (expr__find_other(pe->metric_expr, > - NULL, &ids, &idnum) < 0) > - continue; > - if (events->len > 0) > - strbuf_addf(events, ","); > - > - if (metricgroup__has_constraint(pe)) > - metricgroup__add_metric_non_group(events, ids, idnum); > + if (strstr(pe->metric_expr, "?")) > + ret = metricgroup__add_metric_runtime_param(events, > + group_list, pe); > else > - metricgroup__add_metric_weak_group(events, ids, idnum); > - > - eg = malloc(sizeof(struct egroup)); *eg > - if (!eg) { > - ret = -ENOMEM; > - break; > - } > - 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(&eg->nd, group_list); > - ret = 0; > + ret = metricgroup__add_metric_param(events, > + group_list, pe); > + if (ret == -EINVAL) > + continue; > } > } > return ret; > diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h > index 475c7f912864..81224ba1270d 100644 > --- a/tools/perf/util/metricgroup.h > +++ b/tools/perf/util/metricgroup.h > @@ -34,4 +34,5 @@ int metricgroup__parse_groups(const struct option *opt, > void metricgroup__print(bool metrics, bool groups, char *filter, > bool raw, bool details); > bool metricgroup__has_metric(const char *metric); > +int arch_get_runtimeparam(void); > #endif > diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c > index 0fd713d3674f..92c4c9abbaa0 100644 > --- a/tools/perf/util/stat-shadow.c > +++ b/tools/perf/util/stat-shadow.c > @@ -777,6 +777,14 @@ static void generic_metric(struct perf_stat_config *config, > } > > if (!metric_events[i]) { > + > + if (strstr(metric_expr, "?")) { > + char *tmp = strrchr(metric_name, '_'); > + So at this point a metric name is guaranteed to have a _? > + tmp++; > + expr__runtimeparam = strtol(tmp, &tmp, 10); > + } > + > if (expr__parse(&ratio, &pctx, metric_expr) == 0) { > char *unit; > char metric_bf[64]; > -- > 2.18.1 >
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 <kjain@linux.ibm.com> >> --- >> 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 <string.h> >> #include <linux/stringify.h> >> #include "header.h" >> +#include "metricgroup.h" >> +#include "evlist.h" >> +#include <dirent.h> >> +#include "pmu.h" >> +#include <api/fs/fs.h> >> >> #define mfspr(rn) ({unsigned long rval; \ >> asm volatile("mfspr %0," __stringify(rn) \ >> @@ -16,6 +21,8 @@ >> #define PVR_VER(pvr) (((pvr) >> 16) & 0xFFFF) /* Version field */ >> #define PVR_REV(pvr) (((pvr) >> 0) & 0xFFFF) /* 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 *)&count) < 0) >> + return 1; >> + else >> + return count; > > Why this cast dance? We have sysfs__read_int(path, &count). > > Also this is more compact: > > return sysfs__read_int(path, &count) < 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; > int size = asprintf(¶mval, "%d", expr__runtimeparam); > > if (size < 0) > *dst++ = '0'; > else { > while (i < size) > *dst++ = paramval[i++]; > free(paramval); > } > > >> + } >> else >> *dst++ = *str; >> str++; >> @@ -72,8 +87,8 @@ number [0-9]+ >> >> sch [-,=] >> spec \\{sch} >> -sym [0-9a-zA-Z_\.:@]+ >> -symbol {spec}*{sym}*{spec}*{sym}* >> +sym [0-9a-zA-Z_\.:@?]+ >> +symbol {spec}*{sym}*{spec}*{sym}*{spec}*{sym} >> >> %% >> { >> 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; >> + >> + if (expr__find_other(pe->metric_expr, >> + NULL, &ids, &idnum) < 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)); > > Shorter form that works even if you change that type: > > + eg = malloc(sizeof(*eg)); > >> + 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); > > Please check strdup() return just like you checked the malloc(sizeof(struct egroup)). Right, I missed that part, will update it. > >> + eg->metric_expr = pe->metric_expr; >> + eg->metric_unit = pe->unit; >> + list_add_tail(&eg->nd, group_list); >> + ret = 0; >> + >> + if (ret != 0) >> + break; >> + } >> + 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, &ids, &idnum) < 0) > > Why break the above in two lines? > >> + 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)); > > Ditto > >> + 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(&eg->nd, group_list); >> + ret = 0; >> + >> + return ret; >> +} >> + >> static int metricgroup__add_metric(const char *metric, struct strbuf *events, >> struct list_head *group_list) >> { >> @@ -493,35 +585,17 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events, >> continue; >> if (match_metric(pe->metric_group, metric) || >> match_metric(pe->metric_name, metric)) { >> - const char **ids; >> - int idnum; >> - struct egroup *eg; >> >> pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name); >> >> - if (expr__find_other(pe->metric_expr, >> - NULL, &ids, &idnum) < 0) >> - continue; >> - if (events->len > 0) >> - strbuf_addf(events, ","); >> - >> - if (metricgroup__has_constraint(pe)) >> - metricgroup__add_metric_non_group(events, ids, idnum); >> + if (strstr(pe->metric_expr, "?")) >> + ret = metricgroup__add_metric_runtime_param(events, >> + group_list, pe); >> else >> - metricgroup__add_metric_weak_group(events, ids, idnum); >> - >> - eg = malloc(sizeof(struct egroup)); > > *eg >> - if (!eg) { >> - ret = -ENOMEM; >> - break; >> - } >> - 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(&eg->nd, group_list); >> - ret = 0; >> + ret = metricgroup__add_metric_param(events, >> + group_list, pe); >> + if (ret == -EINVAL) >> + continue; >> } >> } >> return ret; >> diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h >> index 475c7f912864..81224ba1270d 100644 >> --- a/tools/perf/util/metricgroup.h >> +++ b/tools/perf/util/metricgroup.h >> @@ -34,4 +34,5 @@ int metricgroup__parse_groups(const struct option *opt, >> void metricgroup__print(bool metrics, bool groups, char *filter, >> bool raw, bool details); >> bool metricgroup__has_metric(const char *metric); >> +int arch_get_runtimeparam(void); >> #endif >> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c >> index 0fd713d3674f..92c4c9abbaa0 100644 >> --- a/tools/perf/util/stat-shadow.c >> +++ b/tools/perf/util/stat-shadow.c >> @@ -777,6 +777,14 @@ static void generic_metric(struct perf_stat_config *config, >> } >> >> if (!metric_events[i]) { >> + >> + if (strstr(metric_expr, "?")) { >> + char *tmp = strrchr(metric_name, '_'); >> + > > So at this point a metric name is guaranteed to have a _? Yes, because we are appending '_' with parameter value in metric name when we adding these events in 'group_list'. So, I added it, because we can have multiple values of that runtime_parameter, so by appending it in metric name, we can understand to which event the counter data belongs. Thanks for reviewing the patch, I will send updated patch. Thanks, Kajol > >> + tmp++; >> + expr__runtimeparam = strtol(tmp, &tmp, 10); >> + } >> + >> if (expr__parse(&ratio, &pctx, metric_expr) == 0) { >> char *unit; >> char metric_bf[64]; >> -- >> 2.18.1 >> >
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
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
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 <string.h> > > #include <linux/stringify.h> > > #include "header.h" > > +#include "metricgroup.h" > > +#include "evlist.h" > > +#include <dirent.h> > > +#include "pmu.h" > > +#include <api/fs/fs.h> > > > > #define mfspr(rn) ({unsigned long rval; \ > > asm volatile("mfspr %0," __stringify(rn) \ > > @@ -16,6 +21,8 @@ > > #define PVR_VER(pvr) (((pvr) >> 16) & 0xFFFF) /* Version field */ > > #define PVR_REV(pvr) (((pvr) >> 0) & 0xFFFF) /* 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
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, &ids, &idnum) < 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(&eg->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, &ids, &idnum) < 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(&eg->nd, group_list); > + ret = 0; > + > + return ret; > +} SNIP
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
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, &ids, &idnum) < 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(&eg->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, &ids, &idnum) < 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(&eg->nd, group_list); >> + ret = 0; >> + >> + return ret; >> +} > > 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 <string.h> #include <linux/stringify.h> #include "header.h" +#include "metricgroup.h" +#include "evlist.h" +#include <dirent.h> +#include "pmu.h" +#include <api/fs/fs.h> #define mfspr(rn) ({unsigned long rval; \ asm volatile("mfspr %0," __stringify(rn) \ @@ -16,6 +21,8 @@ #define PVR_VER(pvr) (((pvr) >> 16) & 0xFFFF) /* Version field */ #define PVR_REV(pvr) (((pvr) >> 0) & 0xFFFF) /* 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 *)&count) < 0) + return 1; + else + return 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); + char * paramval = (char *)malloc(size); + int i = 0; + + if(!paramval) + *dst++ = '0'; + else { + sprintf(paramval, "%d", expr__runtimeparam); + while(i < size) + *dst++ = paramval[i++]; + free(paramval); + } + } else *dst++ = *str; str++; @@ -72,8 +87,8 @@ number [0-9]+ sch [-,=] spec \\{sch} -sym [0-9a-zA-Z_\.:@]+ -symbol {spec}*{sym}*{spec}*{sym}* +sym [0-9a-zA-Z_\.:@?]+ +symbol {spec}*{sym}*{spec}*{sym}*{spec}*{sym} %% { 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; + + if (expr__find_other(pe->metric_expr, + NULL, &ids, &idnum) < 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(&eg->nd, group_list); + ret = 0; + + if (ret != 0) + break; + } + 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, &ids, &idnum) < 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(&eg->nd, group_list); + ret = 0; + + return ret; +} + static int metricgroup__add_metric(const char *metric, struct strbuf *events, struct list_head *group_list) { @@ -493,35 +585,17 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events, continue; if (match_metric(pe->metric_group, metric) || match_metric(pe->metric_name, metric)) { - const char **ids; - int idnum; - struct egroup *eg; pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name); - if (expr__find_other(pe->metric_expr, - NULL, &ids, &idnum) < 0) - continue; - if (events->len > 0) - strbuf_addf(events, ","); - - if (metricgroup__has_constraint(pe)) - metricgroup__add_metric_non_group(events, ids, idnum); + if (strstr(pe->metric_expr, "?")) + ret = metricgroup__add_metric_runtime_param(events, + group_list, pe); else - metricgroup__add_metric_weak_group(events, ids, idnum); - - eg = malloc(sizeof(struct egroup)); - if (!eg) { - ret = -ENOMEM; - break; - } - 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(&eg->nd, group_list); - ret = 0; + ret = metricgroup__add_metric_param(events, + group_list, pe); + if (ret == -EINVAL) + continue; } } return ret; diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h index 475c7f912864..81224ba1270d 100644 --- a/tools/perf/util/metricgroup.h +++ b/tools/perf/util/metricgroup.h @@ -34,4 +34,5 @@ int metricgroup__parse_groups(const struct option *opt, void metricgroup__print(bool metrics, bool groups, char *filter, bool raw, bool details); bool metricgroup__has_metric(const char *metric); +int arch_get_runtimeparam(void); #endif diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c index 0fd713d3674f..92c4c9abbaa0 100644 --- a/tools/perf/util/stat-shadow.c +++ b/tools/perf/util/stat-shadow.c @@ -777,6 +777,14 @@ static void generic_metric(struct perf_stat_config *config, } if (!metric_events[i]) { + + if (strstr(metric_expr, "?")) { + char *tmp = strrchr(metric_name, '_'); + + tmp++; + expr__runtimeparam = strtol(tmp, &tmp, 10); + } + if (expr__parse(&ratio, &pctx, metric_expr) == 0) { char *unit; char metric_bf[64];
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 <kjain@linux.ibm.com> --- 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(-)