| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 2 Mar 2020 16:08:47 +0100
From: Jiri Olsa <jolsa@...hat.com>
To: Kajol Jain <kjain@...ux.ibm.com>
Cc: acme@...nel.org, linuxppc-dev@...ts.ozlabs.org, mpe@...erman.id.au,
sukadev@...ux.vnet.ibm.com, linux-kernel@...r.kernel.org,
linux-perf-users@...r.kernel.org, anju@...ux.vnet.ibm.com,
maddy@...ux.vnet.ibm.com, ravi.bangoria@...ux.ibm.com,
peterz@...radead.org, yao.jin@...ux.intel.com, ak@...ux.intel.com,
jolsa@...nel.org, kan.liang@...ux.intel.com, jmario@...hat.com,
alexander.shishkin@...ux.intel.com, mingo@...nel.org,
paulus@...abs.org, namhyung@...nel.org, mpetlan@...hat.com,
gregkh@...uxfoundation.org, benh@...nel.crashing.org,
mamatha4@...ux.vnet.ibm.com, mark.rutland@....com,
tglx@...utronix.de
Subject: Re: [PATCH v3 6/8] perf/tools: Enhance JSON/metric infrastructure to
handle "?"
On Sat, Feb 29, 2020 at 03:11:57PM +0530, Kajol Jain wrote:
SNIP
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 02aee946b6c1..f629828cc0de 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -399,6 +399,11 @@ void metricgroup__print(bool metrics, bool metricgroups, char *filter,
> strlist__delete(metriclist);
> }
>
> +int __weak arch_get_runtimeparam(void)
> +{
> + return 1;
> +}
> +
> static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> struct list_head *group_list)
> {
> @@ -419,52 +424,77 @@ 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;
> - bool no_group = false;
> + int k, count;
two things in here.. there's already ack-ed patchset from Kan Liang:
Support metric group constraint
>[PATCH V2 2/5] perf metricgroup: Factor out metricgroup__add_metric_weak_group()
that's changing this place, so you might want to synchronize with that
> +
> + if (strstr(pe->metric_expr, "?"))
> + count = arch_get_runtimeparam();
> + else
> + count = 1;
> +
> + /* This loop is added to create multiple
> + * events depend on count value and add
> + * those events to group_list.
> + */
> + for (k = 0; k < count; k++) {
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + bool no_group = false;
> + char value[PATH_MAX];
> +
> + pr_debug("metric expr %s for %s\n",
> + pe->metric_expr, pe->metric_name);
> + expr__runtimeparam = k;
the other thing is that I don't really follow what's going on in here
you're setting expr__runtimeparam to the loop index,
which you get from some arch related file
we should do this in arch-specific way.. I think that Kan's change is
already moving some bits into separate function and that should make
all this more readable, but perhaps we might need more, so all the
'repeating' code will be in a function
please either separate this to arch code, or make it understandable
for people from other archs ;-)
jirka
> + if (expr__find_other(pe->metric_expr, NULL,
> + &ids, &idnum) < 0)
> + continue;
> + if (events->len > 0)
> + strbuf_addf(events, ",");
> + for (j = 0; j < idnum; j++) {
> + pr_debug("found event %s\n", ids[j]);
> + /*
> + * Duration time maps to a software
> + * event and can make groups not count.
> + * Always use it outside a group.
> + */
> + if (!strcmp(ids[j], "duration_time")) {
> + if (j > 0)
> + strbuf_addf(events,
> + "}:W,");
> + strbuf_addf(events,
> + "duration_time");
> + no_group = true;
> + continue;
> + }
> + strbuf_addf(events, "%s%s",
> + j == 0 || no_group ? "{" :
> + ",", ids[j]);
> + no_group = false;
> + }
> + if (!no_group)
> + strbuf_addf(events, "}:W");
>
> - pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
> + eg = malloc(sizeof(struct egroup));
> + if (!eg) {
> + ret = -ENOMEM;
> + break;
> + }
> + eg->ids = ids;
> + eg->idnum = idnum;
>
> - if (expr__find_other(pe->metric_expr,
> - NULL, &ids, &idnum) < 0)
> - continue;
> - if (events->len > 0)
> - strbuf_addf(events, ",");
> - for (j = 0; j < idnum; j++) {
> - pr_debug("found event %s\n", ids[j]);
> - /*
> - * Duration time maps to a software event and can make
> - * groups not count. Always use it outside a
> - * group.
> - */
> - if (!strcmp(ids[j], "duration_time")) {
> - if (j > 0)
> - strbuf_addf(events, "}:W,");
> - strbuf_addf(events, "duration_time");
> - no_group = true;
> - continue;
> + if (strstr(pe->metric_expr, "?")) {
> + sprintf(value, "%s%c%d",
> + pe->metric_name, '_', k);
> + } else {
> + strcpy(value, pe->metric_name);
> }
> - strbuf_addf(events, "%s%s",
> - j == 0 || no_group ? "{" : ",",
> - ids[j]);
> - no_group = false;
> - }
> - if (!no_group)
> - strbuf_addf(events, "}:W");
>
> - eg = malloc(sizeof(struct egroup));
> - if (!eg) {
> - ret = -ENOMEM;
> - break;
> + 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;
> }
> - 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
Powered by blists - more mailing lists