From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4F971C433E0 for ; Wed, 20 May 2020 18:22:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 28BE120671 for ; Wed, 20 May 2020 18:22:55 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="dhdeCSjR" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726892AbgETSWv (ORCPT ); Wed, 20 May 2020 14:22:51 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36494 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726697AbgETSWu (ORCPT ); Wed, 20 May 2020 14:22:50 -0400 Received: from mail-il1-x144.google.com (mail-il1-x144.google.com [IPv6:2607:f8b0:4864:20::144]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 93095C08C5C0 for ; Wed, 20 May 2020 11:22:50 -0700 (PDT) Received: by mail-il1-x144.google.com with SMTP id l20so4164890ilj.10 for ; Wed, 20 May 2020 11:22:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=f/GVZd7ESIzgVNhsrJW5PyE298L4FuKCaYaDMiX7tBc=; b=dhdeCSjRiWfCTgZHaYc5lDpzJhgGmGznip0fO45gjhoi5zhucoXDkbfvGv2brtwixp KdEt3ZgZiavGZdGNv63+iCZwvib0MtVKkON4R8QPVSCxmF2CpJ7gI3ApbefB479r4AU4 lsFGV5VdbWc6PK4or0M3sSgcWMxjNGSGe6BUMagaZmhk9W9iJCSW09AWBpe/aaY+Sdql 2Y7i7vQ/oA60WvR+pr48dLv+7uLhlYHD6f4GFpfEinWSlo3wuFNbGluENEJGSa4h7oIy LFlkOyJnjYJuktEdan7rhgjrd9RY48nv+6gsYNyAKJSe57aUhbNFZKj22KLxDWmEwugm 73pg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=f/GVZd7ESIzgVNhsrJW5PyE298L4FuKCaYaDMiX7tBc=; b=Djm6eFs+oYBDH27Vphty9+wfqcjT7g3qcOQd5lOraxBRNwWiWvZTlHiYXjEStEXVJO rKxwvFDDTkGoZNlyN9exewzTNCECN2Az0n65PlsOYJFVCzSD638+RlHwsn1b/FjGUxTT zeMBsG/pVxLZyBz3Z9sfMh80V+ET/qs2PpVIlm5ZxTGk5MskahvmYKOQHwutH76oexB5 A61I9joo5GlZjrZLuRqwb6wUQVBVB2LQVBP7HzLiC5gT0Eg+fV3Ck/Yb8BIzxwj7lDRK 3/ubIOePVv45BE/yhVxzB17KfEiN3YnQ7PU5ZO6a9NbSu0Mdx3M5lbJHRQla9xmNabwm 1KZA== X-Gm-Message-State: AOAM532m9YshW84Er98EOgUVO2P+oy/u0+oLb4ioe2/VYbiKZpM2Awv1 iP1W5YtgS3wj3YgjSPcGX0YuTam8Sgvq6RENvT8+2g== X-Google-Smtp-Source: ABdhPJwJYzklFP1O+ry02hbJTZuRp3l0pIS9xKqKMX71WXe0sCoddPUAuAWbkZyBVf4WMXE/762PZsQB+WOQC/nzEYE= X-Received: by 2002:a92:1b17:: with SMTP id b23mr5057619ilb.199.1589998953923; Wed, 20 May 2020 11:22:33 -0700 (PDT) MIME-Version: 1.0 References: <20200520072814.128267-1-irogers@google.com> <20200520072814.128267-4-irogers@google.com> <20200520131412.GK157452@krava> In-Reply-To: <20200520131412.GK157452@krava> From: Ian Rogers Date: Wed, 20 May 2020 11:22:22 -0700 Message-ID: Subject: Re: [PATCH 3/7] perf metricgroup: Delay events string creation To: Jiri Olsa Cc: Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Namhyung Kim , Song Liu , Andrii Nakryiko , Kajol Jain , Andi Kleen , John Garry , Jin Yao , Kan Liang , Cong Wang , Kim Phillips , Paul Clarke , Srikar Dronamraju , LKML , Networking , bpf , linux-perf-users , Vince Weaver , Stephane Eranian Content-Type: text/plain; charset="UTF-8" Sender: bpf-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Wed, May 20, 2020 at 6:14 AM Jiri Olsa wrote: > > On Wed, May 20, 2020 at 12:28:10AM -0700, Ian Rogers wrote: > > Currently event groups are placed into groups_list at the same time as > > the events string containing the events is built. Separate these two > > operations and build the groups_list first, then the event string from > > the groups_list. This adds an ability to reorder the groups_list that > > will be used in a later patch. > > > > Signed-off-by: Ian Rogers > > --- > > tools/perf/util/metricgroup.c | 38 +++++++++++++++++++++++------------ > > 1 file changed, 25 insertions(+), 13 deletions(-) > > > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c > > index 7a43ee0a2e40..afd960d03a77 100644 > > --- a/tools/perf/util/metricgroup.c > > +++ b/tools/perf/util/metricgroup.c > > @@ -90,6 +90,7 @@ struct egroup { > > const char *metric_expr; > > const char *metric_unit; > > int runtime; > > + bool has_constraint; > > }; > > > > static struct evsel *find_evsel_group(struct evlist *perf_evlist, > > @@ -485,8 +486,8 @@ int __weak arch_get_runtimeparam(void) > > return 1; > > } > > > > -static int __metricgroup__add_metric(struct strbuf *events, > > - struct list_head *group_list, struct pmu_event *pe, int runtime) > > +static int __metricgroup__add_metric(struct list_head *group_list, > > + struct pmu_event *pe, int runtime) > > { > > struct egroup *eg; > > > > @@ -499,6 +500,7 @@ static int __metricgroup__add_metric(struct strbuf *events, > > eg->metric_expr = pe->metric_expr; > > eg->metric_unit = pe->unit; > > eg->runtime = runtime; > > + eg->has_constraint = metricgroup__has_constraint(pe); > > > > if (expr__find_other(pe->metric_expr, NULL, &eg->pctx, runtime) < 0) { > > expr__ctx_clear(&eg->pctx); > > @@ -506,14 +508,6 @@ static int __metricgroup__add_metric(struct strbuf *events, > > return -EINVAL; > > } > > > > - if (events->len > 0) > > - strbuf_addf(events, ","); > > - > > - if (metricgroup__has_constraint(pe)) > > - metricgroup__add_metric_non_group(events, &eg->pctx); > > - else > > - metricgroup__add_metric_weak_group(events, &eg->pctx); > > - > > list_add_tail(&eg->nd, group_list); > > > > return 0; > > @@ -524,6 +518,7 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events, > > { > > struct pmu_events_map *map = perf_pmu__find_map(NULL); > > struct pmu_event *pe; > > + struct egroup *eg; > > int i, ret = -EINVAL; > > > > if (!map) > > @@ -542,7 +537,8 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events, > > pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name); > > > > if (!strstr(pe->metric_expr, "?")) { > > - ret = __metricgroup__add_metric(events, group_list, pe, 1); > > + ret = __metricgroup__add_metric(group_list, > > + pe, 1); > > } else { > > int j, count; > > > > @@ -553,13 +549,29 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events, > > * those events to group_list. > > */ > > > > - for (j = 0; j < count; j++) > > - ret = __metricgroup__add_metric(events, group_list, pe, j); > > + for (j = 0; j < count; j++) { > > + ret = __metricgroup__add_metric( > > + group_list, pe, j); > > + } > > } > > if (ret == -ENOMEM) > > break; > > } > > } > > + if (!ret) { > > could you please do instead: > > if (ret) > return ret; > > so the code below cuts down one indent level and you > don't need to split up the lines Done, broken out as a separate patch in v2: https://lore.kernel.org/lkml/20200520182011.32236-3-irogers@google.com/ Thanks, Ian > thanks, > jirka > > > + list_for_each_entry(eg, group_list, nd) { > > + if (events->len > 0) > > + strbuf_addf(events, ","); > > + > > + if (eg->has_constraint) { > > + metricgroup__add_metric_non_group(events, > > + &eg->pctx); > > + } else { > > + metricgroup__add_metric_weak_group(events, > > + &eg->pctx); > > + } > > + } > > + } > > return ret; > > } > > > > -- > > 2.26.2.761.g0e0b3e54be-goog > > >