[PATCH 1/2] perf annotate-data: Show offset and size in hex

[PATCH 1/2] perf annotate-data: Show offset and size in hex

[Namhyung Kim]

It'd be better to have them in hex to check cacheline alignment.

 Percent     offset       size  field
  100.00          0      0x1c0  struct cfs_rq    {
    0.00          0       0x10      struct load_weight  load {
    0.00          0        0x8          long unsigned int       weight;
    0.00        0x8        0x4          u32     inv_weight;
                                    };
    0.00       0x10        0x4      unsigned int        nr_running;
   14.56       0x14        0x4      unsigned int        h_nr_running;
    0.00       0x18        0x4      unsigned int        idle_nr_running;
    0.00       0x1c        0x4      unsigned int        idle_h_nr_running;
  ...

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/annotate-data.c | 4 ++--
 tools/perf/util/annotate-data.c        | 2 +-
 tools/perf/util/sort.c                 | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate-data.c b/tools/perf/ui/browsers/annotate-data.c
index f563a3bb072c..cd562a8822b7 100644
--- a/tools/perf/ui/browsers/annotate-data.c
+++ b/tools/perf/ui/browsers/annotate-data.c
@@ -427,12 +427,12 @@ static void browser__write(struct ui_browser *uib, void *entry, int row)
 
 	/* print type info */
 	if (be->indent == 0 && !member->var_name) {
-		ui_browser__printf(uib, " %10d %10d  %s%s",
+		ui_browser__printf(uib, " %#10x %#10x  %s%s",
 				   member->offset, member->size,
 				   member->type_name,
 				   list_empty(&member->children) || be->folded? ";" : " {");
 	} else {
-		ui_browser__printf(uib, " %10d %10d  %*s%s\t%s%s",
+		ui_browser__printf(uib, " %#10x %#10x  %*s%s\t%s%s",
 				   member->offset, member->size,
 				   be->indent * 4, "", member->type_name,
 				   member->var_name ?: "",
diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
index aa330c7d8edd..25105b3b9a13 100644
--- a/tools/perf/util/annotate-data.c
+++ b/tools/perf/util/annotate-data.c
@@ -1660,7 +1660,7 @@ static void print_annotated_data_type(struct annotated_data_type *mem_type,
 		nr_events++;
 	}
 
-	printf(" %10d %10d  %*s%s\t%s",
+	printf(" %#10x %#10x  %*s%s\t%s",
 	       member->offset, member->size, indent, "", member->type_name,
 	       member->var_name ?: "");
 
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index c4046d5d1749..d315308f9170 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -2312,7 +2312,7 @@ static int hist_entry__typeoff_snprintf(struct hist_entry *he, char *bf,
 				 he->mem_type_off, true);
 	buf[4095] = '\0';
 
-	return repsep_snprintf(bf, size, "%s %+d (%s)", he_type->self.type_name,
+	return repsep_snprintf(bf, size, "%s +%#x (%s)", he_type->self.type_name,
 			       he->mem_type_off, buf);
 }
 
-- 
2.46.0.184.g6999bdac58-goog

[PATCH 2/2] perf annotate-data: Add 'typecln' sort key

[Namhyung Kim]

Sometimes it's useful to organize member fields in cache-line boundary.
The 'typecln' sort key is short for type-cacheline and to show samples
in each cacheline.  The cacheline size is fixed to 64 for now, but it
can read the actual size once it saves the value from sysfs.

For example, you maybe want to which cacheline in a target is hot or
cold.  The following shows members in the cfs_rq's first cache line.

  $ perf report -s type,typecln,typeoff -H
  ...
  -    2.67%        struct cfs_rq
     +    1.23%        struct cfs_rq: cache-line 2
     +    0.57%        struct cfs_rq: cache-line 4
     +    0.46%        struct cfs_rq: cache-line 6
     -    0.41%        struct cfs_rq: cache-line 0
             0.39%        struct cfs_rq +0x14 (h_nr_running)
             0.02%        struct cfs_rq +0x38 (tasks_timeline.rb_leftmost)
  ...

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.h |  1 +
 tools/perf/util/sort.c | 52 ++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/sort.h |  1 +
 3 files changed, 54 insertions(+)

diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 30c13fc8cbe4..deb1087c5948 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -86,6 +86,7 @@ enum hist_column {
 	HISTC_TYPE,
 	HISTC_TYPE_OFFSET,
 	HISTC_SYMBOL_OFFSET,
+	HISTC_TYPE_CACHELINE,
 	HISTC_NR_COLS, /* Last entry */
 };
 
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index d315308f9170..013020f33ece 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -2326,6 +2326,57 @@ struct sort_entry sort_type_offset = {
 	.se_width_idx	= HISTC_TYPE_OFFSET,
 };
 
+/* --sort typecln */
+
+/* TODO: use actual value in the system */
+#define TYPE_CACHELINE_SIZE  64
+
+static int64_t
+sort__typecln_sort(struct hist_entry *left, struct hist_entry *right)
+{
+	struct annotated_data_type *left_type = left->mem_type;
+	struct annotated_data_type *right_type = right->mem_type;
+	int64_t left_cln, right_cln;
+	int64_t ret;
+
+	if (!left_type) {
+		sort__type_init(left);
+		left_type = left->mem_type;
+	}
+
+	if (!right_type) {
+		sort__type_init(right);
+		right_type = right->mem_type;
+	}
+
+	ret = strcmp(left_type->self.type_name, right_type->self.type_name);
+	if (ret)
+		return ret;
+
+	left_cln = left->mem_type_off / TYPE_CACHELINE_SIZE;
+	right_cln = right->mem_type_off / TYPE_CACHELINE_SIZE;
+	return left_cln - right_cln;
+}
+
+static int hist_entry__typecln_snprintf(struct hist_entry *he, char *bf,
+				     size_t size, unsigned int width __maybe_unused)
+{
+	struct annotated_data_type *he_type = he->mem_type;
+
+	return repsep_snprintf(bf, size, "%s: cache-line %d", he_type->self.type_name,
+			       he->mem_type_off / TYPE_CACHELINE_SIZE);
+}
+
+struct sort_entry sort_type_cacheline = {
+	.se_header	= "Data Type Cacheline",
+	.se_cmp		= sort__type_cmp,
+	.se_collapse	= sort__typecln_sort,
+	.se_sort	= sort__typecln_sort,
+	.se_init	= sort__type_init,
+	.se_snprintf	= hist_entry__typecln_snprintf,
+	.se_width_idx	= HISTC_TYPE_CACHELINE,
+};
+
 
 struct sort_dimension {
 	const char		*name;
@@ -2384,6 +2435,7 @@ static struct sort_dimension common_sort_dimensions[] = {
 	DIM(SORT_ANNOTATE_DATA_TYPE, "type", sort_type),
 	DIM(SORT_ANNOTATE_DATA_TYPE_OFFSET, "typeoff", sort_type_offset),
 	DIM(SORT_SYM_OFFSET, "symoff", sort_sym_offset),
+	DIM(SORT_ANNOTATE_DATA_TYPE_CACHELINE, "typecln", sort_type_cacheline),
 };
 
 #undef DIM
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 6357bc32c5ca..9ff68c6786e7 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -71,6 +71,7 @@ enum sort_type {
 	SORT_ANNOTATE_DATA_TYPE,
 	SORT_ANNOTATE_DATA_TYPE_OFFSET,
 	SORT_SYM_OFFSET,
+	SORT_ANNOTATE_DATA_TYPE_CACHELINE,
 
 	/* branch stack specific sort keys */
 	__SORT_BRANCH_STACK,
-- 
2.46.0.184.g6999bdac58-goog

Re: [PATCH 1/2] perf annotate-data: Show offset and size in hex

[Namhyung Kim]

On Mon, Aug 19, 2024 at 5:12 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
>
>
> On Mon, Aug 19, 2024, 8:36 PM Namhyung Kim <namhyung@kernel.org> wrote:
>>
>> It'd be better to have them in hex to check cacheline alignment.
>>
>>
> Why is it better? To compare with the usual output of some other tool?
>
> Spelling out reasons helps reviewers/users.

Cache line sizes are power of 2 so it'd be natural to use hex and
check whether an offset is in the same boundary.  Also perf annotate
shows instruction offsets in hex.

>
> Maybe this should be selectable?

I can add an option and/or a config if you want.

Thanks,
Namhyung

>>
>>
>
>>  Percent     offset       size  field
>>   100.00          0      0x1c0  struct cfs_rq    {
>>     0.00          0       0x10      struct load_weight  load {
>>     0.00          0        0x8          long unsigned int       weight;
>>     0.00        0x8        0x4          u32     inv_weight;
>>                                     };
>>     0.00       0x10        0x4      unsigned int        nr_running;
>>    14.56       0x14        0x4      unsigned int        h_nr_running;
>>     0.00       0x18        0x4      unsigned int        idle_nr_running;
>>     0.00       0x1c        0x4      unsigned int        idle_h_nr_running;
>>   ...
>>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  tools/perf/ui/browsers/annotate-data.c | 4 ++--
>>  tools/perf/util/annotate-data.c        | 2 +-
>>  tools/perf/util/sort.c                 | 2 +-
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/perf/ui/browsers/annotate-data.c b/tools/perf/ui/browsers/annotate-data.c
>> index f563a3bb072c..cd562a8822b7 100644
>> --- a/tools/perf/ui/browsers/annotate-data.c
>> +++ b/tools/perf/ui/browsers/annotate-data.c
>> @@ -427,12 +427,12 @@ static void browser__write(struct ui_browser *uib, void *entry, int row)
>>
>>         /* print type info */
>>         if (be->indent == 0 && !member->var_name) {
>> -               ui_browser__printf(uib, " %10d %10d  %s%s",
>> +               ui_browser__printf(uib, " %#10x %#10x  %s%s",
>>                                    member->offset, member->size,
>>                                    member->type_name,
>>                                    list_empty(&member->children) || be->folded? ";" : " {");
>>         } else {
>> -               ui_browser__printf(uib, " %10d %10d  %*s%s\t%s%s",
>> +               ui_browser__printf(uib, " %#10x %#10x  %*s%s\t%s%s",
>>                                    member->offset, member->size,
>>                                    be->indent * 4, "", member->type_name,
>>                                    member->var_name ?: "",
>> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
>> index aa330c7d8edd..25105b3b9a13 100644
>> --- a/tools/perf/util/annotate-data.c
>> +++ b/tools/perf/util/annotate-data.c
>> @@ -1660,7 +1660,7 @@ static void print_annotated_data_type(struct annotated_data_type *mem_type,
>>                 nr_events++;
>>         }
>>
>> -       printf(" %10d %10d  %*s%s\t%s",
>> +       printf(" %#10x %#10x  %*s%s\t%s",
>>                member->offset, member->size, indent, "", member->type_name,
>>                member->var_name ?: "");
>>
>> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
>> index c4046d5d1749..d315308f9170 100644
>> --- a/tools/perf/util/sort.c
>> +++ b/tools/perf/util/sort.c
>> @@ -2312,7 +2312,7 @@ static int hist_entry__typeoff_snprintf(struct hist_entry *he, char *bf,
>>                                  he->mem_type_off, true);
>>         buf[4095] = '\0';
>>
>> -       return repsep_snprintf(bf, size, "%s %+d (%s)", he_type->self.type_name,
>> +       return repsep_snprintf(bf, size, "%s +%#x (%s)", he_type->self.type_name,
>>                                he->mem_type_off, buf);
>>  }
>>
>> --
>> 2.46.0.184.g6999bdac58-goog
>>
>>

Re: [PATCH 1/2] perf annotate-data: Show offset and size in hex

[Arnaldo Carvalho de Melo]

On Mon, Aug 19, 2024 at 07:21:34PM -0700, Namhyung Kim wrote:
> On Mon, Aug 19, 2024 at 5:12 PM Arnaldo Carvalho de Melo
> <arnaldo.melo@gmail.com> wrote:
> >
> >
> >
> > On Mon, Aug 19, 2024, 8:36 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >>
> >> It'd be better to have them in hex to check cacheline alignment.
> >>
> >>
> > Why is it better? To compare with the usual output of some other tool?
> >
> > Spelling out reasons helps reviewers/users.
> 
> Cache line sizes are power of 2 so it'd be natural to use hex and
> check whether an offset is in the same boundary.  Also perf annotate
> shows instruction offsets in hex.
> 
> >
> > Maybe this should be selectable?
> 
> I can add an option and/or a config if you want.

It would be interesting to have, but I'm merging the patches to make
progress...

- Arnaldo