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 <jo...@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
