Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 15 Jul 2020 23:19:08 +0200
From:   Jiri Olsa <jolsa@redhat.com>
To:     Ian Rogers <irogers@google.com>
Cc:     Jiri Olsa <jolsa@kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Michael Petlan <mpetlan@redhat.com>,
        Andi Kleen <ak@linux.intel.com>,
        Kajol Jain <kjain@linux.ibm.com>,
        John Garry <john.garry@huawei.com>,
        "Paul A. Clarke" <pc@us.ibm.com>,
        Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH 13/18] perf metric: Add events for the current group
Message-ID: <20200715211908.GS183694@krava>
References: <20200712132634.138901-1-jolsa@kernel.org>
 <20200712132634.138901-14-jolsa@kernel.org>
 <CAP-5=fXHGGCFw_5zcXboFUYeNMMC5K0fbAXwPn0HRw91pt6A0A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAP-5=fXHGGCFw_5zcXboFUYeNMMC5K0fbAXwPn0HRw91pt6A0A@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jul 15, 2020 at 10:00:09AM -0700, Ian Rogers wrote:
> On Sun, Jul 12, 2020 at 6:27 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > There's no need to iterate the whole list of groups,
> > when adding new events. The currently created group
> > is the one we want to add.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/perf/util/metricgroup.c | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 8cbcc5e05fef..66f25362702d 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -811,17 +811,19 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> 
> Could we add a function comment to describe the arguments here?

ok

> Currently events is an empty list out argument that is built up by
> this code, now it will be incrementally updated. Except I don't think
> I'm capturing the current state correctly, it is confusing that there
> is the loop in the current code. It looks like events will be added
> multiple times redundantly.

oops, I meant to add the example of broken processing in here,
but forgot.. will update

> 
> >         if (ret)
> >                 return ret;
> >
> > -       list_for_each_entry(eg, group_list, nd) {
> > -               if (events->len > 0)
> > -                       strbuf_addf(events, ",");
> > +       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);
> > -               }
> > +       /*
> > +        * Even if we add multiple groups through the runtime
> > +        * param, they share same events.
> > +        */
> 
> I'm not clear what runtime param is here. Is it the \? arch runtime parameter?

yes, that's that ppc quirk.. adding extra same metrics based
on that runtime param.. for some reason ;-)

thanks,
jirka