From: Jiri Olsa <jolsa@redhat.com>
To: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>,
Madhavan Srinivasan <maddy@linux.ibm.com>,
Peter Zijlstra <peterz@infradead.org>,
Kajol Jain <kjain@linux.ibm.com>,
linux-kernel@vger.kernel.org, acme@kernel.org,
linux-perf-users@vger.kernel.org, jolsa@kernel.org,
linuxppc-dev@lists.ozlabs.org, kan.liang@linux.intel.com
Subject: Re: [PATCH 4/4] tools/perf: Support pipeline stage cycles for powerpc
Date: Wed, 17 Mar 2021 13:16:01 +0100 [thread overview]
Message-ID: <YFHzAZjE1f8nq0l8@krava> (raw)
In-Reply-To: <CA827A39-FA2A-4B0C-BF8F-9DB428CD58B8@linux.vnet.ibm.com>
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 <jolsa@redhat.com> 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 < 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> * 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 < 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) &&<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 < 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 = &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
prev parent reply other threads:[~2021-03-17 12:17 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-09 14:03 [PATCH 0/4] powerpc/perf: Export processor pipeline stage cycles information Athira Rajeev
2021-03-09 14:03 ` [PATCH 1/4] powerpc/perf: Expose processor pipeline stage cycles using PERF_SAMPLE_WEIGHT_STRUCT Athira Rajeev
2021-03-09 14:03 ` [PATCH 2/4] tools/perf: Add dynamic headers for perf report columns Athira Rajeev
2021-03-12 12:57 ` Jiri Olsa
2021-03-15 7:41 ` Athira Rajeev
2021-03-09 14:03 ` [PATCH 3/4] tools/perf: Add powerpc support for PERF_SAMPLE_WEIGHT_STRUCT Athira Rajeev
2021-03-09 14:04 ` [PATCH 4/4] tools/perf: Support pipeline stage cycles for powerpc Athira Rajeev
2021-03-12 12:56 ` Jiri Olsa
2021-03-15 7:52 ` Athira Rajeev
2021-03-15 23:18 ` Jiri Olsa
[not found] ` <CA827A39-FA2A-4B0C-BF8F-9DB428CD58B8@linux.vnet.ibm.com>
2021-03-17 12:16 ` Jiri Olsa [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YFHzAZjE1f8nq0l8@krava \
--to=jolsa@redhat.com \
--cc=acme@kernel.org \
--cc=atrajeev@linux.vnet.ibm.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=kjain@linux.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=maddy@linux.ibm.com \
--cc=peterz@infradead.org \
--cc=ravi.bangoria@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).