Re: [PATCH 13/18] perf metric: Add events for the current group

[Jiri Olsa]

On Wed, Jul 15, 2020 at 03:14:37PM -0700, Ian Rogers wrote:
> On Wed, Jul 15, 2020 at 2:19 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> >
> > 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > >  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 ;-)
> 
> Do they share the same event? I thought the "?" was substituted in a
> loop for a value 0...arch_runtime_value and so you got an event for
> each of those values.

that's right.. I need to rethink this part, I think this change broke
the runtime param stuff, we need to keep some temp list for the ppc case..
but it was broken already with adding all the events from the group_list,
which could contain other metrics events

thanks,
jirka