Re: [PATCH 4/4] tools/perf: Support pipeline stage cycles for powerpc

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

> On 12-Mar-2021, at 6:26 PM, Jiri Olsa <jolsa@redhat.com> wrote:
> 
> On Tue, Mar 09, 2021 at 09:04:00AM -0500, Athira Rajeev wrote:
>> The pipeline stage cycles details can be recorded on powerpc from
>> the contents of Performance Monitor Unit (PMU) registers. On
>> ISA v3.1 platform, sampling registers exposes the cycles spent in
>> different pipeline stages. Patch adds perf tools support to present
>> two of the cycle counter information along with memory latency (weight).
>> 
>> Re-use the field 'ins_lat' for storing the first pipeline stage cycle.
>> This is stored in 'var2_w' field of 'perf_sample_weight'.
>> 
>> Add a new field 'p_stage_cyc' to store the second pipeline stage cycle
>> which is stored in 'var3_w' field of perf_sample_weight.
>> 
>> Add new sort function 'Pipeline Stage Cycle' and include this in
>> default_mem_sort_order[]. This new sort function may be used to denote
>> some other pipeline stage in another architecture. So add this to
>> list of sort entries that can have dynamic header string.
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> tools/perf/Documentation/perf-report.txt |  1 +
>> tools/perf/arch/powerpc/util/event.c     | 18 ++++++++++++++++--
>> tools/perf/util/event.h                  |  1 +
>> tools/perf/util/hist.c                   | 11 ++++++++---
>> tools/perf/util/hist.h                   |  1 +
>> tools/perf/util/session.c                |  4 +++-
>> tools/perf/util/sort.c                   | 24 ++++++++++++++++++++++--
>> tools/perf/util/sort.h                   |  2 ++
>> 8 files changed, 54 insertions(+), 8 deletions(-)
>> 
>> diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
>> index f546b5e9db05..9691d9c227ba 100644
>> --- a/tools/perf/Documentation/perf-report.txt
>> +++ b/tools/perf/Documentation/perf-report.txt
>> @@ -112,6 +112,7 @@ OPTIONS
>> 	- ins_lat: Instruction latency in core cycles. This is the global instruction
>> 	  latency
>> 	- local_ins_lat: Local instruction latency version
>> +	- p_stage_cyc: Number of cycles spent in a pipeline stage.
> 
> please specify in here that it's ppc only

Ok Sure,

> 
> SNIP
> 
>> +struct sort_entry sort_p_stage_cyc = {
>> +	.se_header      = "Pipeline Stage Cycle",
>> +	.se_cmp         = sort__global_p_stage_cyc_cmp,
>> +	.se_snprintf	= hist_entry__p_stage_cyc_snprintf,
>> +	.se_width_idx	= HISTC_P_STAGE_CYC,
>> +};
>> +
>> struct sort_entry sort_mem_daddr_sym = {
>> 	.se_header	= "Data Symbol",
>> 	.se_cmp		= sort__daddr_cmp,
>> @@ -1853,6 +1872,7 @@ static void sort_dimension_add_dynamic_header(struct sort_dimension *sd)
>> 	DIM(SORT_CODE_PAGE_SIZE, "code_page_size", sort_code_page_size),
>> 	DIM(SORT_LOCAL_INS_LAT, "local_ins_lat", sort_local_ins_lat),
>> 	DIM(SORT_GLOBAL_INS_LAT, "ins_lat", sort_global_ins_lat),
>> +	DIM(SORT_P_STAGE_CYC, "p_stage_cyc", sort_p_stage_cyc),
> 
> this might be out of scope for this patch, but would it make sense
> to add arch specific sort dimension? so the specific column is
> not even visible on arch that it's not supported on
> 

Hi Jiri,

Thanks for the suggestions.

Below is an approach I came up with for adding dynamic sort key based on architecture support.
With this patch, perf report for mem mode will display new sort key only in supported archs. 
Please help to review if this approach looks good. I have created this on top of my current set. If this looks fine, 
I can include this in version2 patch set.

From 8ebbe6ae802d895103335899e4e60dde5e562f33 Mon Sep 17 00:00:00 2001
From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Date: Mon, 15 Mar 2021 02:33:28 +0000
Subject: [PATCH] tools/perf: Add dynamic sort dimensions for mem mode

Add dynamic sort dimensions for mem mode.

Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
 tools/perf/arch/powerpc/util/event.c |  7 +++++
 tools/perf/util/event.h              |  1 +
 tools/perf/util/sort.c               | 43 +++++++++++++++++++++++++++-
 3 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/powerpc/util/event.c b/tools/perf/arch/powerpc/util/event.c
index b80fbee83b6e..fddfc288c415 100644
--- a/tools/perf/arch/powerpc/util/event.c
+++ b/tools/perf/arch/powerpc/util/event.c
@@ -44,3 +44,10 @@ const char *arch_perf_header_entry__add(const char *se_header)
 		return "Dispatch Cyc";
 	return se_header;
 }
+
+int arch_support_dynamic_key(const char *sort_key)
+{
+	if (!strcmp(sort_key, "p_stage_cyc"))
+		return 1;
+	return 0;
+}
diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
index 65f89e80916f..6cd4bf54dbdc 100644
--- a/tools/perf/util/event.h
+++ b/tools/perf/util/event.h
@@ -429,5 +429,6 @@ char *get_page_size_name(u64 size, char *str);
 void arch_perf_parse_sample_weight(struct perf_sample *data, const __u64 *array, u64 type);
 void arch_perf_synthesize_sample_weight(const struct perf_sample *data, __u64 *array, u64 type);
 const char *arch_perf_header_entry__add(const char *se_header);
+int arch_support_dynamic_key(const char *sort_key);
 
 #endif /* __PERF_RECORD_H */
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index cbb3899e7eca..e194b1187db8 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -37,7 +37,7 @@ const char	default_parent_pattern[] = "^sys_|^do_page_fault";
 const char	*parent_pattern = default_parent_pattern;
 const char	*default_sort_order = "comm,dso,symbol";
 const char	default_branch_sort_order[] = "comm,dso_from,symbol_from,symbol_to,cycles";
-const char	default_mem_sort_order[] = "local_weight,mem,sym,dso,symbol_daddr,dso_daddr,snoop,tlb,locked,blocked,local_ins_lat,p_stage_cyc";
+const char	default_mem_sort_order[] = "local_weight,mem,sym,dso,symbol_daddr,dso_daddr,snoop,tlb,locked,blocked,local_ins_lat";
 const char	default_top_sort_order[] = "dso,symbol";
 const char	default_diff_sort_order[] = "dso,symbol";
 const char	default_tracepoint_sort_order[] = "trace";
@@ -47,6 +47,7 @@ regex_t		ignore_callees_regex;
 int		have_ignore_callees = 0;
 enum sort_mode	sort__mode = SORT_MODE__NORMAL;
 const char	*dynamic_headers[] = {"local_ins_lat", "p_stage_cyc"};
+const char	*dynamic_sort_keys_mem[] = {"p_stage_cyc"};
 
 /*
  * Replaces all occurrences of a char used with the:
@@ -2997,6 +2998,20 @@ static char *prefix_if_not_in(const char *pre, char *str)
 	return n;
 }
 
+/*
+ * Adds 'suff,' suffix into 'str' if 'suff' is
+ * not already part of 'str'.
+ */
+static char *suffix_if_not_in(const char *suff, char *str)
+{
+	if (!str || strstr(str, suff))
+		return str;
+
+	if (asprintf(&str, "%s,%s", str, suff) < 0)
+		str = NULL;
+	return str;
+}
+
 static char *setup_overhead(char *keys)
 {
 	if (sort__mode == SORT_MODE__DIFF)
@@ -3010,6 +3025,26 @@ static char *setup_overhead(char *keys)
 	return keys;
 }
 
+int __weak arch_support_dynamic_key(const char *sort_key __maybe_unused)
+{
+	return 0;
+}
+
+static char *setup_dynamic_sort_keys(char *str)
+{
+	unsigned int j;
+
+	if (sort__mode == SORT_MODE__MEMORY)
+		for (j = 0; j < ARRAY_SIZE(dynamic_sort_keys_mem); j++)
+			if (arch_support_dynamic_key(dynamic_sort_keys_mem[j])) {
+				str = suffix_if_not_in(dynamic_sort_keys_mem[j], str);
+				if (str == NULL)
+					return str;
+			}
+
+	return str;
+}
+
 static int __setup_sorting(struct evlist *evlist)
 {
 	char *str;
@@ -3050,6 +3085,12 @@ static int __setup_sorting(struct evlist *evlist)
 		}
 	}
 
+	str = setup_dynamic_sort_keys(str);
+	if (str == NULL) {
+		pr_err("Not enough memory to setup dynamic sort keys");
+		return -ENOMEM;
+	}
+
 	ret = setup_sort_list(&perf_hpp_list, str, evlist);
 
 	free(str);
-- 
2.26.2


Thanks,
Athira

> 
>> };
>> 
>> #undef DIM
>> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
>> index 63f67a3f3630..23b20cbbc846 100644
>> --- a/tools/perf/util/sort.h
>> +++ b/tools/perf/util/sort.h
>> @@ -51,6 +51,7 @@ struct he_stat {
>> 	u64			period_guest_us;
>> 	u64			weight;
>> 	u64			ins_lat;
>> +	u64			p_stage_cyc;
>> 	u32			nr_events;
>> };
>> 
>> @@ -234,6 +235,7 @@ enum sort_type {
>> 	SORT_CODE_PAGE_SIZE,
>> 	SORT_LOCAL_INS_LAT,
>> 	SORT_GLOBAL_INS_LAT,
>> +	SORT_P_STAGE_CYC,
> 
> we could have the whole 'SORT_PEPELINE_STAGE_CYC',
> so it's more obvious

Ok.

> 
> thanks,
> jirka