lkml.org 
[lkml]   [2020]   [Jul]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 13/18] perf metric: Add events for the current group
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

\
 
 \ /
  Last update: 2020-07-15 23:20    [W:0.997 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site