* [PATCH] perf c2c: Fix sorting.
@ 2020-01-09 4:30 Andres Freund
2020-01-09 8:48 ` Jiri Olsa
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Andres Freund @ 2020-01-09 4:30 UTC (permalink / raw)
To: Jiri Olsa
Cc: linux-kernel, Andres Freund, Jiri Olsa, Andi Kleen,
Arnaldo Carvalho de Melo, Alexander Shishkin, Michael Petlan,
Namhyung Kim, Peter Zijlstra, stable
Commit 722ddfde366f ("perf tools: Fix time sorting") changed -
correctly so - hist_entry__sort to return int64. Unfortunately several
of the builtin-c2c.c comparison routines only happened to work due the
cast caused by the wrong return type.
This causes meaningless ordering of both the cacheline list, and the
cacheline details page. E.g a simple
perf c2c record -a sleep 3
perf c2c report
will result in cacheline table like
=================================================
Shared Data Cache Line Table
=================================================
#
# ----------- Cacheline ---------- Total Tot ----- LLC Load Hitm ----- ---- Store Reference ---- --- Load Dram ---- LLC Total ----- Core Load Hit ----- -- LLC Load Hit --
# Index Address Node PA cnt records Hitm Total Lcl Rmt Total L1Hit L1Miss Lcl Rmt Ld Miss Loads FB L1 L2 Llc Rmt
# ..... .................. .... ...... ....... ....... ....... ....... ....... ....... ....... ....... ........ ........ ....... ....... ....... ....... ....... ........ ........
#
0 0x7f0d27ffba00 N/A 0 52 0.12% 13 6 7 12 12 0 0 7 14 40 4 16 0 0 0
1 0x7f0d27ff61c0 N/A 0 6353 14.04% 1475 801 674 779 779 0 0 718 1392 5574 1299 1967 0 115 0
2 0x7f0d26d3ec80 N/A 0 71 0.15% 16 4 12 13 13 0 0 12 24 58 1 20 0 9 0
3 0x7f0d26d3ec00 N/A 0 98 0.22% 23 17 6 19 19 0 0 6 12 79 0 40 0 10 0
i.e. with the list not being ordered by Total Hitm.
Fixes: 722ddfde366f ("perf tools: Fix time sorting")
Signed-off-by: Andres Freund <andres@anarazel.de>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Michael Petlan <mpetlan@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: stable@vger.kernel.org # v3.16+
---
tools/perf/builtin-c2c.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index e69f44941aad..f2e9d2b1b913 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -595,8 +595,8 @@ tot_hitm_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
{
struct c2c_hist_entry *c2c_left;
struct c2c_hist_entry *c2c_right;
- unsigned int tot_hitm_left;
- unsigned int tot_hitm_right;
+ uint64_t tot_hitm_left;
+ uint64_t tot_hitm_right;
c2c_left = container_of(left, struct c2c_hist_entry, he);
c2c_right = container_of(right, struct c2c_hist_entry, he);
@@ -629,7 +629,8 @@ __f ## _cmp(struct perf_hpp_fmt *fmt __maybe_unused, \
\
c2c_left = container_of(left, struct c2c_hist_entry, he); \
c2c_right = container_of(right, struct c2c_hist_entry, he); \
- return c2c_left->stats.__f - c2c_right->stats.__f; \
+ return (uint64_t) c2c_left->stats.__f - \
+ (uint64_t) c2c_right->stats.__f; \
}
#define STAT_FN(__f) \
@@ -682,7 +683,8 @@ ld_llcmiss_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
c2c_left = container_of(left, struct c2c_hist_entry, he);
c2c_right = container_of(right, struct c2c_hist_entry, he);
- return llc_miss(&c2c_left->stats) - llc_miss(&c2c_right->stats);
+ return (uint64_t) llc_miss(&c2c_left->stats) -
+ (uint64_t) llc_miss(&c2c_right->stats);
}
static uint64_t total_records(struct c2c_stats *stats)
--
2.25.0.rc1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] perf c2c: Fix sorting.
2020-01-09 4:30 [PATCH] perf c2c: Fix sorting Andres Freund
@ 2020-01-09 8:48 ` Jiri Olsa
2020-01-09 17:00 ` Andres Freund
2020-01-09 9:58 ` Michael Petlan
2020-01-20 8:27 ` [tip: perf/core] perf c2c: Fix return type for histogram sorting comparision functions tip-bot2 for Andres Freund
2 siblings, 1 reply; 9+ messages in thread
From: Jiri Olsa @ 2020-01-09 8:48 UTC (permalink / raw)
To: Andres Freund
Cc: linux-kernel, Jiri Olsa, Andi Kleen, Arnaldo Carvalho de Melo,
Alexander Shishkin, Michael Petlan, Namhyung Kim, Peter Zijlstra,
stable
On Wed, Jan 08, 2020 at 08:30:30PM -0800, Andres Freund wrote:
> Commit 722ddfde366f ("perf tools: Fix time sorting") changed -
> correctly so - hist_entry__sort to return int64. Unfortunately several
> of the builtin-c2c.c comparison routines only happened to work due the
> cast caused by the wrong return type.
>
> This causes meaningless ordering of both the cacheline list, and the
> cacheline details page. E.g a simple
> perf c2c record -a sleep 3
> perf c2c report
> will result in cacheline table like
> =================================================
> Shared Data Cache Line Table
> =================================================
> #
> # ----------- Cacheline ---------- Total Tot ----- LLC Load Hitm ----- ---- Store Reference ---- --- Load Dram ---- LLC Total ----- Core Load Hit ----- -- LLC Load Hit --
> # Index Address Node PA cnt records Hitm Total Lcl Rmt Total L1Hit L1Miss Lcl Rmt Ld Miss Loads FB L1 L2 Llc Rmt
> # ..... .................. .... ...... ....... ....... ....... ....... ....... ....... ....... ....... ........ ........ ....... ....... ....... ....... ....... ........ ........
> #
> 0 0x7f0d27ffba00 N/A 0 52 0.12% 13 6 7 12 12 0 0 7 14 40 4 16 0 0 0
> 1 0x7f0d27ff61c0 N/A 0 6353 14.04% 1475 801 674 779 779 0 0 718 1392 5574 1299 1967 0 115 0
> 2 0x7f0d26d3ec80 N/A 0 71 0.15% 16 4 12 13 13 0 0 12 24 58 1 20 0 9 0
> 3 0x7f0d26d3ec00 N/A 0 98 0.22% 23 17 6 19 19 0 0 6 12 79 0 40 0 10 0
> i.e. with the list not being ordered by Total Hitm.
>
> Fixes: 722ddfde366f ("perf tools: Fix time sorting")
> Signed-off-by: Andres Freund <andres@anarazel.de>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Michael Petlan <mpetlan@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: stable@vger.kernel.org # v3.16+
> ---
> tools/perf/builtin-c2c.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index e69f44941aad..f2e9d2b1b913 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -595,8 +595,8 @@ tot_hitm_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> {
> struct c2c_hist_entry *c2c_left;
> struct c2c_hist_entry *c2c_right;
> - unsigned int tot_hitm_left;
> - unsigned int tot_hitm_right;
> + uint64_t tot_hitm_left;
> + uint64_t tot_hitm_right;
that change looks right, but I can't see how that could
happened because of change in Fixes: tag
was the return statement of this function:
return tot_hitm_left - tot_hitm_right;
considered to be 'unsigned int' and then converted to int64_t,
which would treat negative 'unsigned int' as big positive 'int64_t'?
thanks,
jirka
>
> c2c_left = container_of(left, struct c2c_hist_entry, he);
> c2c_right = container_of(right, struct c2c_hist_entry, he);
> @@ -629,7 +629,8 @@ __f ## _cmp(struct perf_hpp_fmt *fmt __maybe_unused, \
> \
> c2c_left = container_of(left, struct c2c_hist_entry, he); \
> c2c_right = container_of(right, struct c2c_hist_entry, he); \
> - return c2c_left->stats.__f - c2c_right->stats.__f; \
> + return (uint64_t) c2c_left->stats.__f - \
> + (uint64_t) c2c_right->stats.__f; \
> }
>
> #define STAT_FN(__f) \
> @@ -682,7 +683,8 @@ ld_llcmiss_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> c2c_left = container_of(left, struct c2c_hist_entry, he);
> c2c_right = container_of(right, struct c2c_hist_entry, he);
>
> - return llc_miss(&c2c_left->stats) - llc_miss(&c2c_right->stats);
> + return (uint64_t) llc_miss(&c2c_left->stats) -
> + (uint64_t) llc_miss(&c2c_right->stats);
> }
>
> static uint64_t total_records(struct c2c_stats *stats)
> --
> 2.25.0.rc1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf c2c: Fix sorting.
2020-01-09 4:30 [PATCH] perf c2c: Fix sorting Andres Freund
2020-01-09 8:48 ` Jiri Olsa
@ 2020-01-09 9:58 ` Michael Petlan
2020-01-09 13:18 ` Arnaldo Carvalho de Melo
2020-01-20 8:27 ` [tip: perf/core] perf c2c: Fix return type for histogram sorting comparision functions tip-bot2 for Andres Freund
2 siblings, 1 reply; 9+ messages in thread
From: Michael Petlan @ 2020-01-09 9:58 UTC (permalink / raw)
To: Andres Freund
Cc: Jiri Olsa, linux-kernel, Jiri Olsa, Andi Kleen,
Arnaldo Carvalho de Melo, Alexander Shishkin, Namhyung Kim,
Peter Zijlstra, stable
On Wed, 8 Jan 2020, Andres Freund wrote:
> Commit 722ddfde366f ("perf tools: Fix time sorting") changed -
> correctly so - hist_entry__sort to return int64. Unfortunately several
> of the builtin-c2c.c comparison routines only happened to work due the
> cast caused by the wrong return type.
>
> This causes meaningless ordering of both the cacheline list, and the
> cacheline details page. E.g a simple
> perf c2c record -a sleep 3
> perf c2c report
> will result in cacheline table like
> =================================================
> Shared Data Cache Line Table
> =================================================
> #
> # ----------- Cacheline ---------- Total Tot ----- LLC Load Hitm ----- ---- Store Reference ---- --- Load Dram ---- LLC Total ----- Core Load Hit ----- -- LLC Load Hit --
> # Index Address Node PA cnt records Hitm Total Lcl Rmt Total L1Hit L1Miss Lcl Rmt Ld Miss Loads FB L1 L2 Llc Rmt
> # ..... .................. .... ...... ....... ....... ....... ....... ....... ....... ....... ....... ........ ........ ....... ....... ....... ....... ....... ........ ........
> #
> 0 0x7f0d27ffba00 N/A 0 52 0.12% 13 6 7 12 12 0 0 7 14 40 4 16 0 0 0
> 1 0x7f0d27ff61c0 N/A 0 6353 14.04% 1475 801 674 779 779 0 0 718 1392 5574 1299 1967 0 115 0
> 2 0x7f0d26d3ec80 N/A 0 71 0.15% 16 4 12 13 13 0 0 12 24 58 1 20 0 9 0
> 3 0x7f0d26d3ec00 N/A 0 98 0.22% 23 17 6 19 19 0 0 6 12 79 0 40 0 10 0
> i.e. with the list not being ordered by Total Hitm.
>
> Fixes: 722ddfde366f ("perf tools: Fix time sorting")
> Signed-off-by: Andres Freund <andres@anarazel.de>
Tested on top of Arnaldo's perf/core branch. After the patch, the rows
are ordered by Tot Hitm.
Tested-by: Michael Petlan <mpetlan@redhat.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Michael Petlan <mpetlan@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: stable@vger.kernel.org # v3.16+
> ---
> tools/perf/builtin-c2c.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index e69f44941aad..f2e9d2b1b913 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -595,8 +595,8 @@ tot_hitm_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> {
> struct c2c_hist_entry *c2c_left;
> struct c2c_hist_entry *c2c_right;
> - unsigned int tot_hitm_left;
> - unsigned int tot_hitm_right;
> + uint64_t tot_hitm_left;
> + uint64_t tot_hitm_right;
>
> c2c_left = container_of(left, struct c2c_hist_entry, he);
> c2c_right = container_of(right, struct c2c_hist_entry, he);
> @@ -629,7 +629,8 @@ __f ## _cmp(struct perf_hpp_fmt *fmt __maybe_unused, \
> \
> c2c_left = container_of(left, struct c2c_hist_entry, he); \
> c2c_right = container_of(right, struct c2c_hist_entry, he); \
> - return c2c_left->stats.__f - c2c_right->stats.__f; \
> + return (uint64_t) c2c_left->stats.__f - \
> + (uint64_t) c2c_right->stats.__f; \
> }
>
> #define STAT_FN(__f) \
> @@ -682,7 +683,8 @@ ld_llcmiss_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> c2c_left = container_of(left, struct c2c_hist_entry, he);
> c2c_right = container_of(right, struct c2c_hist_entry, he);
>
> - return llc_miss(&c2c_left->stats) - llc_miss(&c2c_right->stats);
> + return (uint64_t) llc_miss(&c2c_left->stats) -
> + (uint64_t) llc_miss(&c2c_right->stats);
> }
>
> static uint64_t total_records(struct c2c_stats *stats)
> --
> 2.25.0.rc1
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf c2c: Fix sorting.
2020-01-09 9:58 ` Michael Petlan
@ 2020-01-09 13:18 ` Arnaldo Carvalho de Melo
2020-01-09 13:22 ` Jiri Olsa
0 siblings, 1 reply; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-01-09 13:18 UTC (permalink / raw)
To: Michael Petlan
Cc: Andres Freund, Jiri Olsa, linux-kernel, Jiri Olsa, Andi Kleen,
Alexander Shishkin, Namhyung Kim, Peter Zijlstra, stable
Em Thu, Jan 09, 2020 at 10:58:43AM +0100, Michael Petlan escreveu:
> On Wed, 8 Jan 2020, Andres Freund wrote:
> > Commit 722ddfde366f ("perf tools: Fix time sorting") changed -
> > correctly so - hist_entry__sort to return int64. Unfortunately several
> > of the builtin-c2c.c comparison routines only happened to work due the
> > cast caused by the wrong return type.
> >
> > This causes meaningless ordering of both the cacheline list, and the
> > cacheline details page. E.g a simple
> > perf c2c record -a sleep 3
> > perf c2c report
> > will result in cacheline table like
> > =================================================
> > Shared Data Cache Line Table
> > =================================================
> > #
> > # ----------- Cacheline ---------- Total Tot ----- LLC Load Hitm ----- ---- Store Reference ---- --- Load Dram ---- LLC Total ----- Core Load Hit ----- -- LLC Load Hit --
> > # Index Address Node PA cnt records Hitm Total Lcl Rmt Total L1Hit L1Miss Lcl Rmt Ld Miss Loads FB L1 L2 Llc Rmt
> > # ..... .................. .... ...... ....... ....... ....... ....... ....... ....... ....... ....... ........ ........ ....... ....... ....... ....... ....... ........ ........
> > #
> > 0 0x7f0d27ffba00 N/A 0 52 0.12% 13 6 7 12 12 0 0 7 14 40 4 16 0 0 0
> > 1 0x7f0d27ff61c0 N/A 0 6353 14.04% 1475 801 674 779 779 0 0 718 1392 5574 1299 1967 0 115 0
> > 2 0x7f0d26d3ec80 N/A 0 71 0.15% 16 4 12 13 13 0 0 12 24 58 1 20 0 9 0
> > 3 0x7f0d26d3ec00 N/A 0 98 0.22% 23 17 6 19 19 0 0 6 12 79 0 40 0 10 0
> > i.e. with the list not being ordered by Total Hitm.
> >
> > Fixes: 722ddfde366f ("perf tools: Fix time sorting")
> > Signed-off-by: Andres Freund <andres@anarazel.de>
>
> Tested on top of Arnaldo's perf/core branch. After the patch, the rows
> are ordered by Tot Hitm.
>
> Tested-by: Michael Petlan <mpetlan@redhat.com>
Jiri, so you think we should use a different Fixes: cset? Or plain
remove it? I haven't checked it, just trying to figure out if you guys
came up with a conclusion so that I can review/apply.
- Arnaldo
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Andi Kleen <ak@linux.intel.com>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Cc: Michael Petlan <mpetlan@redhat.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: stable@vger.kernel.org # v3.16+
> > ---
> > tools/perf/builtin-c2c.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> > index e69f44941aad..f2e9d2b1b913 100644
> > --- a/tools/perf/builtin-c2c.c
> > +++ b/tools/perf/builtin-c2c.c
> > @@ -595,8 +595,8 @@ tot_hitm_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> > {
> > struct c2c_hist_entry *c2c_left;
> > struct c2c_hist_entry *c2c_right;
> > - unsigned int tot_hitm_left;
> > - unsigned int tot_hitm_right;
> > + uint64_t tot_hitm_left;
> > + uint64_t tot_hitm_right;
> >
> > c2c_left = container_of(left, struct c2c_hist_entry, he);
> > c2c_right = container_of(right, struct c2c_hist_entry, he);
> > @@ -629,7 +629,8 @@ __f ## _cmp(struct perf_hpp_fmt *fmt __maybe_unused, \
> > \
> > c2c_left = container_of(left, struct c2c_hist_entry, he); \
> > c2c_right = container_of(right, struct c2c_hist_entry, he); \
> > - return c2c_left->stats.__f - c2c_right->stats.__f; \
> > + return (uint64_t) c2c_left->stats.__f - \
> > + (uint64_t) c2c_right->stats.__f; \
> > }
> >
> > #define STAT_FN(__f) \
> > @@ -682,7 +683,8 @@ ld_llcmiss_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> > c2c_left = container_of(left, struct c2c_hist_entry, he);
> > c2c_right = container_of(right, struct c2c_hist_entry, he);
> >
> > - return llc_miss(&c2c_left->stats) - llc_miss(&c2c_right->stats);
> > + return (uint64_t) llc_miss(&c2c_left->stats) -
> > + (uint64_t) llc_miss(&c2c_right->stats);
> > }
> >
> > static uint64_t total_records(struct c2c_stats *stats)
> > --
> > 2.25.0.rc1
> >
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf c2c: Fix sorting.
2020-01-09 13:18 ` Arnaldo Carvalho de Melo
@ 2020-01-09 13:22 ` Jiri Olsa
0 siblings, 0 replies; 9+ messages in thread
From: Jiri Olsa @ 2020-01-09 13:22 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Michael Petlan, Andres Freund, linux-kernel, Jiri Olsa,
Andi Kleen, Alexander Shishkin, Namhyung Kim, Peter Zijlstra,
stable
On Thu, Jan 09, 2020 at 10:18:34AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 09, 2020 at 10:58:43AM +0100, Michael Petlan escreveu:
> > On Wed, 8 Jan 2020, Andres Freund wrote:
> > > Commit 722ddfde366f ("perf tools: Fix time sorting") changed -
> > > correctly so - hist_entry__sort to return int64. Unfortunately several
> > > of the builtin-c2c.c comparison routines only happened to work due the
> > > cast caused by the wrong return type.
> > >
> > > This causes meaningless ordering of both the cacheline list, and the
> > > cacheline details page. E.g a simple
> > > perf c2c record -a sleep 3
> > > perf c2c report
> > > will result in cacheline table like
> > > =================================================
> > > Shared Data Cache Line Table
> > > =================================================
> > > #
> > > # ----------- Cacheline ---------- Total Tot ----- LLC Load Hitm ----- ---- Store Reference ---- --- Load Dram ---- LLC Total ----- Core Load Hit ----- -- LLC Load Hit --
> > > # Index Address Node PA cnt records Hitm Total Lcl Rmt Total L1Hit L1Miss Lcl Rmt Ld Miss Loads FB L1 L2 Llc Rmt
> > > # ..... .................. .... ...... ....... ....... ....... ....... ....... ....... ....... ....... ........ ........ ....... ....... ....... ....... ....... ........ ........
> > > #
> > > 0 0x7f0d27ffba00 N/A 0 52 0.12% 13 6 7 12 12 0 0 7 14 40 4 16 0 0 0
> > > 1 0x7f0d27ff61c0 N/A 0 6353 14.04% 1475 801 674 779 779 0 0 718 1392 5574 1299 1967 0 115 0
> > > 2 0x7f0d26d3ec80 N/A 0 71 0.15% 16 4 12 13 13 0 0 12 24 58 1 20 0 9 0
> > > 3 0x7f0d26d3ec00 N/A 0 98 0.22% 23 17 6 19 19 0 0 6 12 79 0 40 0 10 0
> > > i.e. with the list not being ordered by Total Hitm.
> > >
> > > Fixes: 722ddfde366f ("perf tools: Fix time sorting")
> > > Signed-off-by: Andres Freund <andres@anarazel.de>
> >
> > Tested on top of Arnaldo's perf/core branch. After the patch, the rows
> > are ordered by Tot Hitm.
> >
> > Tested-by: Michael Petlan <mpetlan@redhat.com>
>
> Jiri, so you think we should use a different Fixes: cset? Or plain
> remove it? I haven't checked it, just trying to figure out if you guys
> came up with a conclusion so that I can review/apply.
waiting for Andres's answer.. making sure I understand the issue ;-)
jirka
>
> - Arnaldo
>
> > > Cc: Jiri Olsa <jolsa@kernel.org>
> > > Cc: Andi Kleen <ak@linux.intel.com>
> > > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > > Cc: Michael Petlan <mpetlan@redhat.com>
> > > Cc: Namhyung Kim <namhyung@kernel.org>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: stable@vger.kernel.org # v3.16+
> > > ---
> > > tools/perf/builtin-c2c.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> > > index e69f44941aad..f2e9d2b1b913 100644
> > > --- a/tools/perf/builtin-c2c.c
> > > +++ b/tools/perf/builtin-c2c.c
> > > @@ -595,8 +595,8 @@ tot_hitm_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> > > {
> > > struct c2c_hist_entry *c2c_left;
> > > struct c2c_hist_entry *c2c_right;
> > > - unsigned int tot_hitm_left;
> > > - unsigned int tot_hitm_right;
> > > + uint64_t tot_hitm_left;
> > > + uint64_t tot_hitm_right;
> > >
> > > c2c_left = container_of(left, struct c2c_hist_entry, he);
> > > c2c_right = container_of(right, struct c2c_hist_entry, he);
> > > @@ -629,7 +629,8 @@ __f ## _cmp(struct perf_hpp_fmt *fmt __maybe_unused, \
> > > \
> > > c2c_left = container_of(left, struct c2c_hist_entry, he); \
> > > c2c_right = container_of(right, struct c2c_hist_entry, he); \
> > > - return c2c_left->stats.__f - c2c_right->stats.__f; \
> > > + return (uint64_t) c2c_left->stats.__f - \
> > > + (uint64_t) c2c_right->stats.__f; \
> > > }
> > >
> > > #define STAT_FN(__f) \
> > > @@ -682,7 +683,8 @@ ld_llcmiss_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> > > c2c_left = container_of(left, struct c2c_hist_entry, he);
> > > c2c_right = container_of(right, struct c2c_hist_entry, he);
> > >
> > > - return llc_miss(&c2c_left->stats) - llc_miss(&c2c_right->stats);
> > > + return (uint64_t) llc_miss(&c2c_left->stats) -
> > > + (uint64_t) llc_miss(&c2c_right->stats);
> > > }
> > >
> > > static uint64_t total_records(struct c2c_stats *stats)
> > > --
> > > 2.25.0.rc1
> > >
> > >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf c2c: Fix sorting.
2020-01-09 8:48 ` Jiri Olsa
@ 2020-01-09 17:00 ` Andres Freund
2020-01-09 21:46 ` Jiri Olsa
0 siblings, 1 reply; 9+ messages in thread
From: Andres Freund @ 2020-01-09 17:00 UTC (permalink / raw)
To: Jiri Olsa
Cc: linux-kernel, Jiri Olsa, Andi Kleen, Arnaldo Carvalho de Melo,
Alexander Shishkin, Michael Petlan, Namhyung Kim, Peter Zijlstra,
stable
Hi,
On 2020-01-09 09:48:22 +0100, Jiri Olsa wrote:
> On Wed, Jan 08, 2020 at 08:30:30PM -0800, Andres Freund wrote:
> > Commit 722ddfde366f ("perf tools: Fix time sorting") changed -
> > correctly so - hist_entry__sort to return int64. Unfortunately several
> > of the builtin-c2c.c comparison routines only happened to work due the
> > cast caused by the wrong return type.
> >
> > This causes meaningless ordering of both the cacheline list, and the
> > cacheline details page. E.g a simple
> > perf c2c record -a sleep 3
> > perf c2c report
> > will result in cacheline table like
> > =================================================
> > Shared Data Cache Line Table
> > =================================================
> > #
> > # ----------- Cacheline ---------- Total Tot ----- LLC Load Hitm ----- ---- Store Reference ---- --- Load Dram ---- LLC Total ----- Core Load Hit ----- -- LLC Load Hit --
> > # Index Address Node PA cnt records Hitm Total Lcl Rmt Total L1Hit L1Miss Lcl Rmt Ld Miss Loads FB L1 L2 Llc Rmt
> > # ..... .................. .... ...... ....... ....... ....... ....... ....... ....... ....... ....... ........ ........ ....... ....... ....... ....... ....... ........ ........
> > #
> > 0 0x7f0d27ffba00 N/A 0 52 0.12% 13 6 7 12 12 0 0 7 14 40 4 16 0 0 0
> > 1 0x7f0d27ff61c0 N/A 0 6353 14.04% 1475 801 674 779 779 0 0 718 1392 5574 1299 1967 0 115 0
> > 2 0x7f0d26d3ec80 N/A 0 71 0.15% 16 4 12 13 13 0 0 12 24 58 1 20 0 9 0
> > 3 0x7f0d26d3ec00 N/A 0 98 0.22% 23 17 6 19 19 0 0 6 12 79 0 40 0 10 0
> > i.e. with the list not being ordered by Total Hitm.
> >
> > Fixes: 722ddfde366f ("perf tools: Fix time sorting")
> > Signed-off-by: Andres Freund <andres@anarazel.de>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Andi Kleen <ak@linux.intel.com>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Cc: Michael Petlan <mpetlan@redhat.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: stable@vger.kernel.org # v3.16+
> > ---
> > tools/perf/builtin-c2c.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> > index e69f44941aad..f2e9d2b1b913 100644
> > --- a/tools/perf/builtin-c2c.c
> > +++ b/tools/perf/builtin-c2c.c
> > @@ -595,8 +595,8 @@ tot_hitm_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> > {
> > struct c2c_hist_entry *c2c_left;
> > struct c2c_hist_entry *c2c_right;
> > - unsigned int tot_hitm_left;
> > - unsigned int tot_hitm_right;
> > + uint64_t tot_hitm_left;
> > + uint64_t tot_hitm_right;
>
> that change looks right, but I can't see how that could
> happened because of change in Fixes: tag
>
> was the return statement of this function:
>
> return tot_hitm_left - tot_hitm_right;
>
> considered to be 'unsigned int' and then converted to int64_t,
> which would treat negative 'unsigned int' as big positive 'int64_t'?
Correct. So e.g. when comparing 1 and 2 tot_hitm, we'd get (int64_t)
UINT_MAX as a result, which is obviously wrong. However, due to
hist_entry__sort() returning int at the time, this was masked, as the
int64_t was cast to int. Thereby again yielding a negative number for
the comparisons of hist_entry__sort()'s result. After
hist_entry__sort() was fixed however, there never could be negative
return values (but 0's are possible) of hist_entry__sort() for c2c.
I briefly looked for places outside of c2c for similar issues in
hist_entry comparison routines, but didn't find any.
Greetings,
Andres Freund
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf c2c: Fix sorting.
2020-01-09 17:00 ` Andres Freund
@ 2020-01-09 21:46 ` Jiri Olsa
2020-01-14 16:28 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 9+ messages in thread
From: Jiri Olsa @ 2020-01-09 21:46 UTC (permalink / raw)
To: Andres Freund
Cc: linux-kernel, Jiri Olsa, Andi Kleen, Arnaldo Carvalho de Melo,
Alexander Shishkin, Michael Petlan, Namhyung Kim, Peter Zijlstra,
stable
On Thu, Jan 09, 2020 at 09:00:41AM -0800, Andres Freund wrote:
SNIP
> > > tools/perf/builtin-c2c.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> > > index e69f44941aad..f2e9d2b1b913 100644
> > > --- a/tools/perf/builtin-c2c.c
> > > +++ b/tools/perf/builtin-c2c.c
> > > @@ -595,8 +595,8 @@ tot_hitm_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> > > {
> > > struct c2c_hist_entry *c2c_left;
> > > struct c2c_hist_entry *c2c_right;
> > > - unsigned int tot_hitm_left;
> > > - unsigned int tot_hitm_right;
> > > + uint64_t tot_hitm_left;
> > > + uint64_t tot_hitm_right;
> >
> > that change looks right, but I can't see how that could
> > happened because of change in Fixes: tag
> >
> > was the return statement of this function:
> >
> > return tot_hitm_left - tot_hitm_right;
> >
> > considered to be 'unsigned int' and then converted to int64_t,
> > which would treat negative 'unsigned int' as big positive 'int64_t'?
>
> Correct. So e.g. when comparing 1 and 2 tot_hitm, we'd get (int64_t)
> UINT_MAX as a result, which is obviously wrong. However, due to
> hist_entry__sort() returning int at the time, this was masked, as the
> int64_t was cast to int. Thereby again yielding a negative number for
> the comparisons of hist_entry__sort()'s result. After
> hist_entry__sort() was fixed however, there never could be negative
> return values (but 0's are possible) of hist_entry__sort() for c2c.
I see.. ok
Acked-by: Jiri Olsa <jolsa@redhat.com>
thanks,
jirka
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf c2c: Fix sorting.
2020-01-09 21:46 ` Jiri Olsa
@ 2020-01-14 16:28 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-01-14 16:28 UTC (permalink / raw)
To: Jiri Olsa
Cc: Andres Freund, linux-kernel, Jiri Olsa, Andi Kleen,
Alexander Shishkin, Michael Petlan, Namhyung Kim, Peter Zijlstra,
stable
Em Thu, Jan 09, 2020 at 10:46:11PM +0100, Jiri Olsa escreveu:
> On Thu, Jan 09, 2020 at 09:00:41AM -0800, Andres Freund wrote:
>
> SNIP
>
> > > > tools/perf/builtin-c2c.c | 10 ++++++----
> > > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> > > > index e69f44941aad..f2e9d2b1b913 100644
> > > > --- a/tools/perf/builtin-c2c.c
> > > > +++ b/tools/perf/builtin-c2c.c
> > > > @@ -595,8 +595,8 @@ tot_hitm_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> > > > {
> > > > struct c2c_hist_entry *c2c_left;
> > > > struct c2c_hist_entry *c2c_right;
> > > > - unsigned int tot_hitm_left;
> > > > - unsigned int tot_hitm_right;
> > > > + uint64_t tot_hitm_left;
> > > > + uint64_t tot_hitm_right;
> > >
> > > that change looks right, but I can't see how that could
> > > happened because of change in Fixes: tag
> > >
> > > was the return statement of this function:
> > >
> > > return tot_hitm_left - tot_hitm_right;
> > >
> > > considered to be 'unsigned int' and then converted to int64_t,
> > > which would treat negative 'unsigned int' as big positive 'int64_t'?
> >
> > Correct. So e.g. when comparing 1 and 2 tot_hitm, we'd get (int64_t)
> > UINT_MAX as a result, which is obviously wrong. However, due to
> > hist_entry__sort() returning int at the time, this was masked, as the
> > int64_t was cast to int. Thereby again yielding a negative number for
> > the comparisons of hist_entry__sort()'s result. After
> > hist_entry__sort() was fixed however, there never could be negative
> > return values (but 0's are possible) of hist_entry__sort() for c2c.
>
> I see.. ok
>
> Acked-by: Jiri Olsa <jolsa@redhat.com>
Thanks, applied.
- Arnaldo
^ permalink raw reply [flat|nested] 9+ messages in thread
* [tip: perf/core] perf c2c: Fix return type for histogram sorting comparision functions
2020-01-09 4:30 [PATCH] perf c2c: Fix sorting Andres Freund
2020-01-09 8:48 ` Jiri Olsa
2020-01-09 9:58 ` Michael Petlan
@ 2020-01-20 8:27 ` tip-bot2 for Andres Freund
2 siblings, 0 replies; 9+ messages in thread
From: tip-bot2 for Andres Freund @ 2020-01-20 8:27 UTC (permalink / raw)
To: linux-tip-commits
Cc: Andres Freund, Michael Petlan, Jiri Olsa, Alexander Shishkin,
Andi Kleen, Namhyung Kim, Peter Zijlstra, stable, #, v3.16+,
Arnaldo Carvalho de Melo, x86, LKML
The following commit has been merged into the perf/core branch of tip:
Commit-ID: c1c8013ec34d7163431d18367808ea40b2e305f8
Gitweb: https://git.kernel.org/tip/c1c8013ec34d7163431d18367808ea40b2e305f8
Author: Andres Freund <andres@anarazel.de>
AuthorDate: Wed, 08 Jan 2020 20:30:30 -08:00
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 14 Jan 2020 13:29:21 -03:00
perf c2c: Fix return type for histogram sorting comparision functions
Commit 722ddfde366f ("perf tools: Fix time sorting") changed - correctly
so - hist_entry__sort to return int64. Unfortunately several of the
builtin-c2c.c comparison routines only happened to work due the cast
caused by the wrong return type.
This causes meaningless ordering of both the cacheline list, and the
cacheline details page. E.g a simple:
perf c2c record -a sleep 3
perf c2c report
will result in cacheline table like
=================================================
Shared Data Cache Line Table
=================================================
#
# ------- Cacheline ---------- Total Tot - LLC Load Hitm - - Store Reference - - Load Dram - LLC Total - Core Load Hit - - LLC Load Hit -
# Index Address Node PA cnt records Hitm Total Lcl Rmt Total L1Hit L1Miss Lcl Rmt Ld Miss Loads FB L1 L2 Llc Rmt
# ..... .............. .... ...... ....... ...... ..... ..... ... .... ..... ...... ...... .... ...... ..... ..... ..... ... .... .......
0 0x7f0d27ffba00 N/A 0 52 0.12% 13 6 7 12 12 0 0 7 14 40 4 16 0 0 0
1 0x7f0d27ff61c0 N/A 0 6353 14.04% 1475 801 674 779 779 0 0 718 1392 5574 1299 1967 0 115 0
2 0x7f0d26d3ec80 N/A 0 71 0.15% 16 4 12 13 13 0 0 12 24 58 1 20 0 9 0
3 0x7f0d26d3ec00 N/A 0 98 0.22% 23 17 6 19 19 0 0 6 12 79 0 40 0 10 0
i.e. with the list not being ordered by Total Hitm.
Fixes: 722ddfde366f ("perf tools: Fix time sorting")
Signed-off-by: Andres Freund <andres@anarazel.de>
Tested-by: Michael Petlan <mpetlan@redhat.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: stable@vger.kernel.org # v3.16+
Link: http://lore.kernel.org/lkml/20200109043030.233746-1-andres@anarazel.de
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/builtin-c2c.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 3463512..246ac0b 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -595,8 +595,8 @@ tot_hitm_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
{
struct c2c_hist_entry *c2c_left;
struct c2c_hist_entry *c2c_right;
- unsigned int tot_hitm_left;
- unsigned int tot_hitm_right;
+ uint64_t tot_hitm_left;
+ uint64_t tot_hitm_right;
c2c_left = container_of(left, struct c2c_hist_entry, he);
c2c_right = container_of(right, struct c2c_hist_entry, he);
@@ -629,7 +629,8 @@ __f ## _cmp(struct perf_hpp_fmt *fmt __maybe_unused, \
\
c2c_left = container_of(left, struct c2c_hist_entry, he); \
c2c_right = container_of(right, struct c2c_hist_entry, he); \
- return c2c_left->stats.__f - c2c_right->stats.__f; \
+ return (uint64_t) c2c_left->stats.__f - \
+ (uint64_t) c2c_right->stats.__f; \
}
#define STAT_FN(__f) \
@@ -682,7 +683,8 @@ ld_llcmiss_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
c2c_left = container_of(left, struct c2c_hist_entry, he);
c2c_right = container_of(right, struct c2c_hist_entry, he);
- return llc_miss(&c2c_left->stats) - llc_miss(&c2c_right->stats);
+ return (uint64_t) llc_miss(&c2c_left->stats) -
+ (uint64_t) llc_miss(&c2c_right->stats);
}
static uint64_t total_records(struct c2c_stats *stats)
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-01-20 8:27 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-09 4:30 [PATCH] perf c2c: Fix sorting Andres Freund
2020-01-09 8:48 ` Jiri Olsa
2020-01-09 17:00 ` Andres Freund
2020-01-09 21:46 ` Jiri Olsa
2020-01-14 16:28 ` Arnaldo Carvalho de Melo
2020-01-09 9:58 ` Michael Petlan
2020-01-09 13:18 ` Arnaldo Carvalho de Melo
2020-01-09 13:22 ` Jiri Olsa
2020-01-20 8:27 ` [tip: perf/core] perf c2c: Fix return type for histogram sorting comparision functions tip-bot2 for Andres Freund
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.