Re: [PATCH V2 2/5] perf metricgroup: Factor out metricgroup__add_metric_weak_group()

Arnaldo Carvalho de Melo
2020-03-10T17:45:46Z

Em Mon, Feb 24, 2020 at 01:59:21PM -0800, kan.liang@linux.intel.com escreveu:
> From: Kan Liang <kan.liang@linux.intel.com>
>
> Factor out metricgroup__add_metric_weak_group() which add metrics into a
> weak group. The change can improve code readability. Because following
> patch will introduce a function which add standalone metrics.
>
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> ---
> tools/perf/util/metricgroup.c | 57 +++++++++++++++++++++++++------------------
> 1 file changed, 33 insertions(+), 24 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 02aee94..1cd042c 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -399,13 +399,42 @@ void metricgroup__print(bool metrics, bool metricgroups, char *filter,
> strlist__delete(metriclist);
> }
>
> +static void metricgroup__add_metric_weak_group(struct strbuf *events,
> + const char **ids,
> + int idnum)
> +{
> + bool no_group = false;
> + int i;
> +
> + for (i = 0; i < idnum; i++) {
> + pr_debug("found event %s\n", ids[i]);
> + /*
> + * Duration time maps to a software event and can make
> + * groups not count. Always use it outside a
> + * group.
> + */
> + if (!strcmp(ids[i], "duration_time")) {
> + if (i > 0)
> + strbuf_addf(events, "}:W,");
> + strbuf_addf(events, "duration_time");

At some point we need to do some error checking on these strbuf calls,
but since this was the state in this file, lets not block this patchkit
on such grounds,

- Arnaldo

> + no_group = true;
> + continue;
> + }
> + strbuf_addf(events, "%s%s",
> + i == 0 || no_group ? "{" : ",",
> + ids[i]);
> + no_group = false;
> + }
> + if (!no_group)
> + strbuf_addf(events, "}:W");
> +}
> +
> static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> struct list_head *group_list)
> {
> struct pmu_events_map *map = perf_pmu__find_map(NULL);
> struct pmu_event *pe;
> - int ret = -EINVAL;
> - int i, j;
> + int i, ret = -EINVAL;
>
> if (!map)
> return 0;
> @@ -422,7 +451,6 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> const char **ids;
> int idnum;
> struct egroup *eg;
> - bool no_group = false;
>
> pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>
> @@ -431,27 +459,8 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> 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");
> +
> + metricgroup__add_metric_weak_group(events, ids, idnum);
>
> eg = malloc(sizeof(struct egroup));
> if (!eg) {
> --
> 2.7.4
>

--

- Arnaldo