[PATCH v2 1/3] perf stat: Remove dead code

[PATCH v2 1/3] perf stat: Remove dead code

[Ian Rogers]

No need to set os.evsel twice.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/stat-display.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 493ec372fdec..4b57c0c07632 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -946,7 +946,6 @@ static void print_metric_headers(struct perf_stat_config *config,
 		out.print_metric = print_metric_header;
 		out.new_line = new_line_metric;
 		out.force_header = true;
-		os.evsel = counter;
 		perf_stat__print_shadow_stats(config, counter, 0,
 					      0,
 					      &out,
-- 
2.28.0.526.ge36021eeef-goog

[PATCH v2 2/3] perf metricgroup: Fix typo in comment.

[Ian Rogers]

Add missing character.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/metricgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 8831b964288f..662f4e8777d5 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -150,7 +150,7 @@ static void expr_ids__exit(struct expr_ids *ids)
 }
 
 /**
- * Find a group of events in perf_evlist that correpond to those from a parsed
+ * Find a group of events in perf_evlist that correspond to those from a parsed
  * metric expression. Note, as find_evsel_group is called in the same order as
  * perf_evlist was constructed, metric_no_merge doesn't need to test for
  * underfilling a group.
-- 
2.28.0.526.ge36021eeef-goog

Re: [PATCH v2 2/3] perf metricgroup: Fix typo in comment.

[Namhyung Kim]

On Thu, Sep 10, 2020 at 12:26 PM Ian Rogers <irogers@google.com> wrote:
>
> Add missing character.
>
> Signed-off-by: Ian Rogers <irogers@google.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks
Namhyung

> ---
>  tools/perf/util/metricgroup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 8831b964288f..662f4e8777d5 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -150,7 +150,7 @@ static void expr_ids__exit(struct expr_ids *ids)
>  }
>
>  /**
> - * Find a group of events in perf_evlist that correpond to those from a parsed
> + * Find a group of events in perf_evlist that correspond to those from a parsed
>   * metric expression. Note, as find_evsel_group is called in the same order as
>   * perf_evlist was constructed, metric_no_merge doesn't need to test for
>   * underfilling a group.
> --
> 2.28.0.526.ge36021eeef-goog
>

Re: [PATCH v2 2/3] perf metricgroup: Fix typo in comment.

[Arnaldo Carvalho de Melo]

Em Thu, Sep 10, 2020 at 02:38:58PM +0900, Namhyung Kim escreveu:
> On Thu, Sep 10, 2020 at 12:26 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Add missing character.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> Acked-by: Namhyung Kim <namhyung@kernel.org>

Applied this and the first one, continuing...

- Arnaldo
 
> Thanks
> Namhyung
> 
> > ---
> >  tools/perf/util/metricgroup.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 8831b964288f..662f4e8777d5 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -150,7 +150,7 @@ static void expr_ids__exit(struct expr_ids *ids)
> >  }
> >
> >  /**
> > - * Find a group of events in perf_evlist that correpond to those from a parsed
> > + * Find a group of events in perf_evlist that correspond to those from a parsed
> >   * metric expression. Note, as find_evsel_group is called in the same order as
> >   * perf_evlist was constructed, metric_no_merge doesn't need to test for
> >   * underfilling a group.
> > --
> > 2.28.0.526.ge36021eeef-goog
> >

-- 

- Arnaldo

[PATCH v2 3/3] perf metricgroup: Fix uncore metric expressions

[Ian Rogers]

A metric like DRAM_BW_Use has on SkylakeX events uncore_imc/cas_count_read/
and uncore_imc/case_count_write/. These events open 6 events per socket
with pmu names of uncore_imc_[0-5]. The current metric setup code in
find_evsel_group assumes one ID will map to 1 event to be recorded in
metric_events. For events with multiple matches, the first event is
recorded in metric_events (avoiding matching >1 event with the same
name) and the evlist_used updated so that duplicate events aren't
removed when the evlist has unused events removed.

Before this change:
$ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1

 Performance counter stats for 'system wide':

             41.14 MiB  uncore_imc/cas_count_read/
     1,002,614,251 ns   duration_time

       1.002614251 seconds time elapsed

After this change:
$ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1

 Performance counter stats for 'system wide':

            157.47 MiB  uncore_imc/cas_count_read/ #     0.00 DRAM_BW_Use
            126.97 MiB  uncore_imc/cas_count_write/
     1,003,019,728 ns   duration_time

v2. avoids iterating over the whole evlist as suggested by
    namhyung@kernel.org. It also fixes the metric_leader computation
    that was broken in the same commits.

Erroneous duplication introduced in:
commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events").

Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap").
Reported-by: Jin Yao <yao.jin@linux.intel.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/metricgroup.c | 45 ++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 662f4e8777d5..79080de9217d 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -206,6 +206,18 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 				sizeof(struct evsel *) * idnum);
 			current_leader = ev->leader;
 		}
+		/*
+		 * Check for duplicate events with the same name. For example,
+		 * uncore_imc/cas_count_read/ will turn into 6 events per socket
+		 * on skylakex. Only the first such event is placed in
+		 * metric_events.
+		 */
+		for (i = 0; i < matched_events; i++) {
+			if (!strcmp(metric_events[i]->name, ev->name))
+				break;
+		}
+		if (i != matched_events)
+			continue;
 		if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
 			if (has_constraint) {
 				/*
@@ -245,9 +257,36 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
 	metric_events[idnum] = NULL;
 
 	for (i = 0; i < idnum; i++) {
-		ev = metric_events[i];
-		ev->metric_leader = ev;
-		set_bit(ev->idx, evlist_used);
+		/* Don't free used events. */
+		set_bit(metric_events[i]->idx, evlist_used);
+		/*
+		 * The metric leader points to the identically named event in
+		 * metric_events.
+		 */
+		metric_events[i]->metric_leader = metric_events[i];
+		/*
+		 * Mark two events with identical names in the same group (or
+		 * globally) as being in use as uncore events may be duplicated
+		 * for each pmu. Set the metric leader to be the event that
+		 * appears in metric_events.
+		 */
+		if (!has_constraint) {
+			for_each_group_evsel(ev, metric_events[i]->leader) {
+				if (ev != metric_events[i] &&
+				    !strcmp(metric_events[i]->name, ev->name)) {
+					set_bit(ev->idx, evlist_used);
+					ev->metric_leader = metric_events[i];
+				}
+			}
+		} else {
+			evlist__for_each_entry(perf_evlist, ev) {
+				if (ev != metric_events[i] &&
+				    !strcmp(metric_events[i]->name, ev->name)) {
+					set_bit(ev->idx, evlist_used);
+					ev->metric_leader = metric_events[i];
+				}
+			}
+		}
 	}
 
 	return metric_events[0];
-- 
2.28.0.526.ge36021eeef-goog

Re: [PATCH v2 3/3] perf metricgroup: Fix uncore metric expressions

[Namhyung Kim]

On Thu, Sep 10, 2020 at 12:26 PM Ian Rogers <irogers@google.com> wrote:
>
> A metric like DRAM_BW_Use has on SkylakeX events uncore_imc/cas_count_read/
> and uncore_imc/case_count_write/. These events open 6 events per socket
> with pmu names of uncore_imc_[0-5]. The current metric setup code in
> find_evsel_group assumes one ID will map to 1 event to be recorded in
> metric_events. For events with multiple matches, the first event is
> recorded in metric_events (avoiding matching >1 event with the same
> name) and the evlist_used updated so that duplicate events aren't
> removed when the evlist has unused events removed.
>
> Before this change:
> $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
>
>  Performance counter stats for 'system wide':
>
>              41.14 MiB  uncore_imc/cas_count_read/
>      1,002,614,251 ns   duration_time
>
>        1.002614251 seconds time elapsed
>
> After this change:
> $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
>
>  Performance counter stats for 'system wide':
>
>             157.47 MiB  uncore_imc/cas_count_read/ #     0.00 DRAM_BW_Use
>             126.97 MiB  uncore_imc/cas_count_write/
>      1,003,019,728 ns   duration_time
>
> v2. avoids iterating over the whole evlist as suggested by
>     namhyung@kernel.org. It also fixes the metric_leader computation
>     that was broken in the same commits.
>
> Erroneous duplication introduced in:
> commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events").
>
> Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap").
> Reported-by: Jin Yao <yao.jin@linux.intel.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/metricgroup.c | 45 ++++++++++++++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 662f4e8777d5..79080de9217d 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -206,6 +206,18 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>                                 sizeof(struct evsel *) * idnum);
>                         current_leader = ev->leader;
>                 }
> +               /*
> +                * Check for duplicate events with the same name. For example,
> +                * uncore_imc/cas_count_read/ will turn into 6 events per socket
> +                * on skylakex. Only the first such event is placed in
> +                * metric_events.
> +                */
> +               for (i = 0; i < matched_events; i++) {
> +                       if (!strcmp(metric_events[i]->name, ev->name))
> +                               break;
> +               }
> +               if (i != matched_events)
> +                       continue;

We have the same logic in the below.  Maybe it'd better to factor out..


>                 if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
>                         if (has_constraint) {
>                                 /*
> @@ -245,9 +257,36 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>         metric_events[idnum] = NULL;
>
>         for (i = 0; i < idnum; i++) {
> -               ev = metric_events[i];
> -               ev->metric_leader = ev;
> -               set_bit(ev->idx, evlist_used);
> +               /* Don't free used events. */
> +               set_bit(metric_events[i]->idx, evlist_used);
> +               /*
> +                * The metric leader points to the identically named event in
> +                * metric_events.
> +                */
> +               metric_events[i]->metric_leader = metric_events[i];
> +               /*
> +                * Mark two events with identical names in the same group (or
> +                * globally) as being in use as uncore events may be duplicated
> +                * for each pmu. Set the metric leader to be the event that
> +                * appears in metric_events.
> +                */

I thought this again, and it's not guaranteed that the metric leader is
a group leader so below won't work IMHO.  Instead we should iterate
evlist always, but started from the metric leader with the
evlist__for_each_entry_continue.

Thanks
Namhyung


> +               if (!has_constraint) {
> +                       for_each_group_evsel(ev, metric_events[i]->leader) {
> +                               if (ev != metric_events[i] &&
> +                                   !strcmp(metric_events[i]->name, ev->name)) {
> +                                       set_bit(ev->idx, evlist_used);
> +                                       ev->metric_leader = metric_events[i];
> +                               }
> +                       }
> +               } else {
> +                       evlist__for_each_entry(perf_evlist, ev) {
> +                               if (ev != metric_events[i] &&
> +                                   !strcmp(metric_events[i]->name, ev->name)) {
> +                                       set_bit(ev->idx, evlist_used);
> +                                       ev->metric_leader = metric_events[i];
> +                               }
> +                       }
> +               }
>         }
>
>         return metric_events[0];
> --
> 2.28.0.526.ge36021eeef-goog
>

Re: [PATCH v2 3/3] perf metricgroup: Fix uncore metric expressions

[Ian Rogers]

On Wed, Sep 9, 2020 at 10:51 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Sep 10, 2020 at 12:26 PM Ian Rogers <irogers@google.com> wrote:
> >
> > A metric like DRAM_BW_Use has on SkylakeX events uncore_imc/cas_count_read/
> > and uncore_imc/case_count_write/. These events open 6 events per socket
> > with pmu names of uncore_imc_[0-5]. The current metric setup code in
> > find_evsel_group assumes one ID will map to 1 event to be recorded in
> > metric_events. For events with multiple matches, the first event is
> > recorded in metric_events (avoiding matching >1 event with the same
> > name) and the evlist_used updated so that duplicate events aren't
> > removed when the evlist has unused events removed.
> >
> > Before this change:
> > $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
> >
> >  Performance counter stats for 'system wide':
> >
> >              41.14 MiB  uncore_imc/cas_count_read/
> >      1,002,614,251 ns   duration_time
> >
> >        1.002614251 seconds time elapsed
> >
> > After this change:
> > $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
> >
> >  Performance counter stats for 'system wide':
> >
> >             157.47 MiB  uncore_imc/cas_count_read/ #     0.00 DRAM_BW_Use
> >             126.97 MiB  uncore_imc/cas_count_write/
> >      1,003,019,728 ns   duration_time
> >
> > v2. avoids iterating over the whole evlist as suggested by
> >     namhyung@kernel.org. It also fixes the metric_leader computation
> >     that was broken in the same commits.
> >
> > Erroneous duplication introduced in:
> > commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events").
> >
> > Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap").
> > Reported-by: Jin Yao <yao.jin@linux.intel.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/metricgroup.c | 45 ++++++++++++++++++++++++++++++++---
> >  1 file changed, 42 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 662f4e8777d5..79080de9217d 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -206,6 +206,18 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> >                                 sizeof(struct evsel *) * idnum);
> >                         current_leader = ev->leader;
> >                 }
> > +               /*
> > +                * Check for duplicate events with the same name. For example,
> > +                * uncore_imc/cas_count_read/ will turn into 6 events per socket
> > +                * on skylakex. Only the first such event is placed in
> > +                * metric_events.
> > +                */
> > +               for (i = 0; i < matched_events; i++) {
> > +                       if (!strcmp(metric_events[i]->name, ev->name))
> > +                               break;
> > +               }
> > +               if (i != matched_events)
> > +                       continue;
>
> We have the same logic in the below.  Maybe it'd better to factor out..

You're right, actually with this loop the below is redundant.

>
> >                 if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
> >                         if (has_constraint) {
> >                                 /*
> > @@ -245,9 +257,36 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> >         metric_events[idnum] = NULL;
> >
> >         for (i = 0; i < idnum; i++) {
> > -               ev = metric_events[i];
> > -               ev->metric_leader = ev;
> > -               set_bit(ev->idx, evlist_used);
> > +               /* Don't free used events. */
> > +               set_bit(metric_events[i]->idx, evlist_used);
> > +               /*
> > +                * The metric leader points to the identically named event in
> > +                * metric_events.
> > +                */
> > +               metric_events[i]->metric_leader = metric_events[i];
> > +               /*
> > +                * Mark two events with identical names in the same group (or
> > +                * globally) as being in use as uncore events may be duplicated
> > +                * for each pmu. Set the metric leader to be the event that
> > +                * appears in metric_events.
> > +                */
>
> I thought this again, and it's not guaranteed that the metric leader is
> a group leader so below won't work IMHO.  Instead we should iterate
> evlist always, but started from the metric leader with the
> evlist__for_each_entry_continue.

Thanks for pointing out evlist__for_each_entry_continue! For the
constraint case it avoids iterating a bunch of list elements. For the
sibling group case we do know that all the metric's events are
siblings with the exception of duration time that is pulled out
specially and not at risk of being aliased.

Thanks,
Ian

> Thanks
> Namhyung
>
>
> > +               if (!has_constraint) {
> > +                       for_each_group_evsel(ev, metric_events[i]->leader) {
> > +                               if (ev != metric_events[i] &&
> > +                                   !strcmp(metric_events[i]->name, ev->name)) {
> > +                                       set_bit(ev->idx, evlist_used);
> > +                                       ev->metric_leader = metric_events[i];
> > +                               }
> > +                       }
> > +               } else {
> > +                       evlist__for_each_entry(perf_evlist, ev) {
> > +                               if (ev != metric_events[i] &&
> > +                                   !strcmp(metric_events[i]->name, ev->name)) {
> > +                                       set_bit(ev->idx, evlist_used);
> > +                                       ev->metric_leader = metric_events[i];
> > +                               }
> > +                       }
> > +               }
> >         }
> >
> >         return metric_events[0];
> > --
> > 2.28.0.526.ge36021eeef-goog
> >

Re: [PATCH v2 1/3] perf stat: Remove dead code

[Namhyung Kim]

Hi Ian,

On Thu, Sep 10, 2020 at 12:26 PM Ian Rogers <irogers@google.com> wrote:
>
> No need to set os.evsel twice.
>
> Signed-off-by: Ian Rogers <irogers@google.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Btw, I think setting the 'out' variable can be moved out of the loop.

Thanks
Namhyung

> ---
>  tools/perf/util/stat-display.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 493ec372fdec..4b57c0c07632 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -946,7 +946,6 @@ static void print_metric_headers(struct perf_stat_config *config,
>                 out.print_metric = print_metric_header;
>                 out.new_line = new_line_metric;
>                 out.force_header = true;
> -               os.evsel = counter;
>                 perf_stat__print_shadow_stats(config, counter, 0,
>                                               0,
>                                               &out,
> --
> 2.28.0.526.ge36021eeef-goog
>