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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <YFHzAZjE1f8nq0l8@krava>
Date:   Wed, 17 Mar 2021 13:16:01 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     Athira Rajeev <atrajeev@...ux.vnet.ibm.com>
Cc:     Ravi Bangoria <ravi.bangoria@...ux.ibm.com>,
        Madhavan Srinivasan <maddy@...ux.ibm.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Kajol Jain <kjain@...ux.ibm.com>, linux-kernel@...r.kernel.org,
        acme@...nel.org, linux-perf-users@...r.kernel.org,
        jolsa@...nel.org, linuxppc-dev@...ts.ozlabs.org,
        kan.liang@...ux.intel.com
Subject: Re: [PATCH 4/4] tools/perf: Support pipeline stage cycles for powerpc

On Wed, Mar 17, 2021 at 05:01:27PM +0530, Athira Rajeev wrote:
> <html><head></head><body dir="auto" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="ApplePlainTextBody"><div class="ApplePlainTextBody"><br><br><blockquote type="cite">On 16-Mar-2021, at 4:48 AM, Jiri Olsa &lt;jolsa@...hat.com&gt; wrote:<br><br>On Mon, Mar 15, 2021 at 01:22:09PM +0530, Athira Rajeev wrote:<br><br>SNIP<br><br><blockquote type="cite">+<br>+static char *setup_dynamic_sort_keys(char *str)<br>+{<br>+<span class="Apple-tab-span" style="white-space:pre">	</span>unsigned int j;<br>+<br>+<span class="Apple-tab-span" style="white-space:pre">	</span>if (sort__mode == SORT_MODE__MEMORY)<br>+<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>for (j = 0; j &lt; ARRAY_SIZE(dynamic_sort_keys_mem); j++)<br>+<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>if (arch_support_dynamic_key(dynamic_sort_keys_mem[j])) {<br>+<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>str = suffix_if_not_in(dynamic_sort_keys_mem[j], str);<br>+<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>if (str == NULL)<br>+<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>return str;<br>+<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>}<br>+<br>+<span class="Apple-tab-span" style="white-space:pre">	</span>return str;<br>+}<br>+<br>static int __setup_sorting(struct evlist *evlist)<br>{<br><span class="Apple-tab-span" style="white-space:pre">	</span>char *str;<br>@@ -3050,6 +3085,12 @@ static int __setup_sorting(struct evlist *evlist)<br><span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>}<br><span class="Apple-tab-span" style="white-space:pre">	</span>}<br><br>+<span class="Apple-tab-span" style="white-space:pre">	</span>str = setup_dynamic_sort_keys(str);<br>+<span class="Apple-tab-span" style="white-space:pre">	</span>if (str == NULL) {<br>+<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>pr_err("Not enough memory to setup dynamic sort keys");<br>+<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>return -ENOMEM;<br>+<span class="Apple-tab-span" style="white-space:pre">	</span>}<br></blockquote><br>hum, so this is basicaly overloading the default_mem_sort_order for<br>architecture, right?<br><br>then I think it'd be easier just overload default_mem_sort_order directly<br><br>I was thinking more about adding extra (arch specific) loop to<br>sort_dimension__add or somehow add arch's specific stuff to<br>memory_sort_dimensions<br></blockquote><br>Hi Jiri,<br><br>Above patch was to append additional sort keys to sort order based on<br>sort mode and architecture support. I had initially thought of defining two<br>orders ( say default_mem_sort_order plus mem_sort_order_pstage ). But if<br>new sort keys gets added for mem mode in future, we will need to keep<br>updating both orders. So preferred the approach of "appending" supported sort<br>keys to default order.<br><br>Following your thought on using "sort_dimension__add", I tried below approach<br>which is easier. The new sort dimension "p_stage_cyc" is presently only supported<br>on powerpc. For unsupported platforms, we don't want to display it<br>in the perf report output columns. Hence added check in sort_dimension__add()<br>and skip the sort key incase its not applicable for particular arch.<br><br>Please help to check if below approach looks fine.<br><br><br>diff --git a/tools/perf/arch/powerpc/util/event.c b/tools/perf/arch/powerpc/util/event.c<br>index b80fbee83b6e..7205767d75eb 100644<br>--- a/tools/perf/arch/powerpc/util/event.c<br>+++ b/tools/perf/arch/powerpc/util/event.c<br>@@ -44,3 +44,10 @@ const char *arch_perf_header_entry__add(const char *se_header)<br> <span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>return "Dispatch Cyc";<br> <span class="Apple-tab-span" style="white-space:pre">	</span>return se_header;<br> }<br>+<br>+int arch_support_sort_key(const char *sort_key)<br>+{<br>+<span class="Apple-tab-span" style="white-space:pre">	</span>if (!strcmp(sort_key, "p_stage_cyc"))<br>+<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>return 1;<br>+<span class="Apple-tab-span" style="white-space:pre">	</span>return 0;<br>+}<br>diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h<br>index 65f89e80916f..612a92aaaefb 100644<br>--- a/tools/perf/util/event.h<br>+++ b/tools/perf/util/event.h<br>@@ -429,5 +429,6 @@ char *get_page_size_name(u64 size, char *str);<br> void arch_perf_parse_sample_weight(struct perf_sample *data, const __u64 *array, u64 type);<br> void arch_perf_synthesize_sample_weight(const struct perf_sample *data, __u64 *array, u64 type);<br> const char *arch_perf_header_entry__add(const char *se_header);<br>+int arch_support_sort_key(const char *sort_key);<br><br> #endif /* __PERF_RECORD_H */<br>diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c<br>index cbb3899e7eca..d8b0b0b43a81 100644<br>--- a/tools/perf/util/sort.c<br>+++ b/tools/perf/util/sort.c<br>@@ -47,6 +47,7 @@ regex_t<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>ignore_callees_regex;<br> int<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>have_ignore_callees = 0;<br> enum sort_mode<span class="Apple-tab-span" style="white-space:pre">	</span>sort__mode = SORT_MODE__NORMAL;<br> const char<span class="Apple-tab-span" style="white-space:pre">	</span>*dynamic_headers[] = {"local_ins_lat", "p_stage_cyc"};<br>+const char<span class="Apple-tab-span" style="white-space:pre">	</span>*arch_specific_sort_keys[] = {"p_stage_cyc"};<br><br> /*<br> &nbsp;* Replaces all occurrences of a char used with the:<br>@@ -1837,6 +1838,11 @@ struct sort_dimension {<br> <span class="Apple-tab-span" style="white-space:pre">	</span>int<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>taken;<br> };<br><br>+int __weak arch_support_sort_key(const char *sort_key __maybe_unused)<br>+{<br>+<span class="Apple-tab-span" style="white-space:pre">	</span>return 0;<br>+}<br>+<br> const char * __weak arch_perf_header_entry__add(const char *se_header)<br> {<br> <span class="Apple-tab-span" style="white-space:pre">	</span>return se_header;<br>@@ -2773,6 +2779,18 @@ int sort_dimension__add(struct perf_hpp_list *list, const char *tok,<br> {<br> <span class="Apple-tab-span" style="white-space:pre">	</span>unsigned int i, j;<br><br>+<span class="Apple-tab-span" style="white-space:pre">	</span>/* Check to see if there are any arch specific<br>+<span class="Apple-tab-span" style="white-space:pre">	</span> * sort dimensions not applicable for the current<br>+<span class="Apple-tab-span" style="white-space:pre">	</span> * architecture. If so, Skip that sort key since<br>+<span class="Apple-tab-span" style="white-space:pre">	</span> * we don't want to display it in the output fields.<br>+<span class="Apple-tab-span" style="white-space:pre">	</span> */<br>+<span class="Apple-tab-span" style="white-space:pre">	</span>for (j = 0; j &lt; ARRAY_SIZE(arch_specific_sort_keys); j++) {<br>+<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>if (!strcmp(arch_specific_sort_keys[j], tok) &amp;&amp;<br>+<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>!arch_support_sort_key(tok)) {<br>+<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>return 0;<br>+<span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>}<br>+<span class="Apple-tab-span" style="white-space:pre">	</span>}<br>+<br> <span class="Apple-tab-span" style="white-space:pre">	</span>for (i = 0; i &lt; ARRAY_SIZE(common_sort_dimensions); i++) {<br> <span class="Apple-tab-span" style="white-space:pre">	</span><span class="Apple-tab-span" style="white-space:pre">	</span>struct sort_dimension *sd = &amp;common_sort_dimensions[i];<br><br>— <br>2.26.2<br><br>Thanks<br>Athira<br><br><blockquote type="cite"><br>jirka<br><br></blockquote><br></div></body></html>
> 

apart from the html content, looks ok ;-)

jirka