Re: [PATCH v3 1/3] perf evlist: Create a new function perf_evlist_forced_leader

Re: [PATCH v3 1/3] perf evlist: Create a new function perf_evlist_forced_leader

[Arnaldo Carvalho de Melo]

Em Sat, May 19, 2018 at 12:00:32AM +0800, Jin Yao escreveu:
> For non-explicit group, perf report supports a option '--group'
> which can enable group output. We also need to support perf annotate
> with the same '--group'.
> 
> Create a new function perf_evlist_forced_leader which contains
> common code to force setting the group leader.
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/util/evlist.c | 15 +++++++++++++++
>  tools/perf/util/evlist.h |  3 +++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index a59281d..6983f6a 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1795,3 +1795,18 @@ bool perf_evlist__exclude_kernel(struct perf_evlist *evlist)
>  
>  	return true;
>  }
> +
> +/*
> + * Events in data file are not collect in groups, but we still want
> + * the group display. Set the artificial group and set the leader's
> + * forced_leader flag to notify the display code.
> + */
> +void perf_evlist_forced_leader(struct perf_evlist *evlist)

This should've been:

void perf_evlist__forced_leader(struct perf_evlist *evlist)

But then, by the verb conjugation, I thought that this would be a query,
i.e. "is this a forced leader?", but then, it returning void didn't
match that expectation, which leads me having to actually _read_ that
code to understand what is that this function does, which I would, but
just to check that it matches what it seems to be, now I'm trying to do
it to figure it out what is it that this thing does :-\


I see, its a 'd' to many, this should be:

void perf_evlist__force_leader(struct perf_evlist *evlist)

Please confirm that this is what you intend by posting a v4, so far,
just for this specific patch and any other in the series that uses this
function, ok?

- Arnaldo

> +{
> +	if (!evlist->nr_groups) {
> +		struct perf_evsel *leader = perf_evlist__first(evlist);
> +
> +		perf_evlist__set_leader(evlist);
> +		leader->forced_leader = true;
> +	}
> +}
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index 6c41b2f..d77d514 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -309,4 +309,7 @@ struct perf_evsel *perf_evlist__event2evsel(struct perf_evlist *evlist,
>  					    union perf_event *event);
>  
>  bool perf_evlist__exclude_kernel(struct perf_evlist *evlist);
> +
> +void perf_evlist_forced_leader(struct perf_evlist *evlist);
> +
>  #endif /* __PERF_EVLIST_H */
> -- 
> 2.7.4

[PATCH v3 0/3] perf annotate: Support '--group' option

[Jin Yao]

For non-explicit group, perf report has already supported a option
'--group' which can enable group output.

This patch-set will support perf annotate with the same '--group'.

For example,

    perf record -e cycles,branches ./div
    perf annotate main --stdio --group

                     :            Disassembly of section .text:
                     :
                     :            00000000004004b0 <main>:
                     :            main():
                     :
                     :                    return i;
                     :            }
                     :
                     :            int main(void)
                     :            {
        0.00    0.00 :   4004b0:       push   %rbx
                     :                    int i;
                     :                    int flag;
                     :                    volatile double x = 1212121212, y = 121212;
                     :
                     :                    s_randseed = time(0);
        0.00    0.00 :   4004b1:       xor    %edi,%edi
                     :                    srand(s_randseed);
        0.00    0.00 :   4004b3:       mov    $0x77359400,%ebx
                     :
                     :                    return i;
                     :            }
                     :

v3:
---------
Move the comment which was on setup_forced_leader() to the
new function perf_evlist_forced_leader().

Impact
------
  perf evlist: Create a new function perf_evlist_forced_leader
  perf report: Use perf_evlist_forced_leader to support '--group'

 
v2:
---------
Arnaldo points out that it should be done the way it is for
perf report --group. v2 refers to this way and the patch is
totally rewritten.


Init post:
----------
Post the patch 'perf annotate: Support multiple events without group'

Jin Yao (3):
  perf evlist: Create a new function perf_evlist_forced_leader
  perf report: Use perf_evlist_forced_leader to support '--group'
  perf annotate: Support '--group' option

 tools/perf/builtin-annotate.c |  7 +++++++
 tools/perf/builtin-report.c   | 13 ++-----------
 tools/perf/util/evlist.c      | 15 +++++++++++++++
 tools/perf/util/evlist.h      |  3 +++
 4 files changed, 27 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH v3 1/3] perf evlist: Create a new function perf_evlist_forced_leader

[Jin Yao]

For non-explicit group, perf report supports a option '--group'
which can enable group output. We also need to support perf annotate
with the same '--group'.

Create a new function perf_evlist_forced_leader which contains
common code to force setting the group leader.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/util/evlist.c | 15 +++++++++++++++
 tools/perf/util/evlist.h |  3 +++
 2 files changed, 18 insertions(+)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index a59281d..6983f6a 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1795,3 +1795,18 @@ bool perf_evlist__exclude_kernel(struct perf_evlist *evlist)
 
 	return true;
 }
+
+/*
+ * Events in data file are not collect in groups, but we still want
+ * the group display. Set the artificial group and set the leader's
+ * forced_leader flag to notify the display code.
+ */
+void perf_evlist_forced_leader(struct perf_evlist *evlist)
+{
+	if (!evlist->nr_groups) {
+		struct perf_evsel *leader = perf_evlist__first(evlist);
+
+		perf_evlist__set_leader(evlist);
+		leader->forced_leader = true;
+	}
+}
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 6c41b2f..d77d514 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -309,4 +309,7 @@ struct perf_evsel *perf_evlist__event2evsel(struct perf_evlist *evlist,
 					    union perf_event *event);
 
 bool perf_evlist__exclude_kernel(struct perf_evlist *evlist);
+
+void perf_evlist_forced_leader(struct perf_evlist *evlist);
+
 #endif /* __PERF_EVLIST_H */
-- 
2.7.4

[PATCH v3 2/3] perf report: Use perf_evlist_forced_leader to support '--group'

[Jin Yao]

Since we have created a new function perf_evlist_forced_leader,
so now remove the old code and use perf_evlist_forced_leader
instead.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/builtin-report.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 4c931af..d41b208 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -194,20 +194,11 @@ static int hist_iter__branch_callback(struct hist_entry_iter *iter,
 	return err;
 }
 
-/*
- * Events in data file are not collect in groups, but we still want
- * the group display. Set the artificial group and set the leader's
- * forced_leader flag to notify the display code.
- */
 static void setup_forced_leader(struct report *report,
 				struct perf_evlist *evlist)
 {
-	if (report->group_set && !evlist->nr_groups) {
-		struct perf_evsel *leader = perf_evlist__first(evlist);
-
-		perf_evlist__set_leader(evlist);
-		leader->forced_leader = true;
-	}
+	if (report->group_set)
+		perf_evlist_forced_leader(evlist);
 }
 
 static int process_feature_event(struct perf_tool *tool,
-- 
2.7.4

[PATCH v3 3/3] perf annotate: Support '--group' option

[Jin Yao]

With the '--group' option, even for non-explicit group, perf annotate
will enable the group output.

For example,

perf record -e cycles,branches ./div
perf annotate main --stdio --group

                 :            Disassembly of section .text:
                 :
                 :            00000000004004b0 <main>:
                 :            main():
                 :
                 :                    return i;
                 :            }
                 :
                 :            int main(void)
                 :            {
    0.00    0.00 :   4004b0:       push   %rbx
                 :                    int i;
                 :                    int flag;
                 :                    volatile double x = 1212121212, y = 121212;
                 :
                 :                    s_randseed = time(0);
    0.00    0.00 :   4004b1:       xor    %edi,%edi
                 :                    srand(s_randseed);
    0.00    0.00 :   4004b3:       mov    $0x77359400,%ebx
                 :
                 :                    return i;
                 :            }
                 :

But if without --group, there is only one event reported.

perf annotate main --stdio

         :            Disassembly of section .text:
         :
         :            00000000004004b0 <main>:
         :            main():
         :
         :                    return i;
         :            }
         :
         :            int main(void)
         :            {
    0.00 :   4004b0:       push   %rbx
         :                    int i;
         :                    int flag;
         :                    volatile double x = 1212121212, y = 121212;
         :
         :                    s_randseed = time(0);
    0.00 :   4004b1:       xor    %edi,%edi
         :                    srand(s_randseed);
    0.00 :   4004b3:       mov    $0x77359400,%ebx
         :
         :                    return i;
         :            }

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/builtin-annotate.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 6e5d9f7..5272d48 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -45,6 +45,7 @@ struct perf_annotate {
 	bool	   print_line;
 	bool	   skip_missing;
 	bool	   has_br_stack;
+	bool	   group_set;
 	const char *sym_hist_filter;
 	const char *cpu_list;
 	DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
@@ -508,6 +509,9 @@ int cmd_annotate(int argc, const char **argv)
 		    "Don't shorten the displayed pathnames"),
 	OPT_BOOLEAN(0, "skip-missing", &annotate.skip_missing,
 		    "Skip symbols that cannot be annotated"),
+	OPT_BOOLEAN_SET(0, "group", &symbol_conf.event_group,
+			&annotate.group_set,
+			"Show event group information together"),
 	OPT_STRING('C', "cpu", &annotate.cpu_list, "cpu", "list of cpus to profile"),
 	OPT_CALLBACK(0, "symfs", NULL, "directory",
 		     "Look for files with symbols relative to this directory",
@@ -570,6 +574,9 @@ int cmd_annotate(int argc, const char **argv)
 	annotate.has_br_stack = perf_header__has_feat(&annotate.session->header,
 						      HEADER_BRANCH_STACK);
 
+	if (annotate.group_set)
+		perf_evlist_forced_leader(annotate.session->evlist);
+
 	ret = symbol__annotation_init();
 	if (ret < 0)
 		goto out_delete;
-- 
2.7.4

Re: [PATCH v3 1/3] perf evlist: Create a new function perf_evlist_forced_leader

[Jin, Yao]

On 5/18/2018 10:18 PM, Arnaldo Carvalho de Melo wrote:
> Em Sat, May 19, 2018 at 12:00:32AM +0800, Jin Yao escreveu:
>> For non-explicit group, perf report supports a option '--group'
>> which can enable group output. We also need to support perf annotate
>> with the same '--group'.
>>
>> Create a new function perf_evlist_forced_leader which contains
>> common code to force setting the group leader.
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>>   tools/perf/util/evlist.c | 15 +++++++++++++++
>>   tools/perf/util/evlist.h |  3 +++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index a59281d..6983f6a 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -1795,3 +1795,18 @@ bool perf_evlist__exclude_kernel(struct perf_evlist *evlist)
>>   
>>   	return true;
>>   }
>> +
>> +/*
>> + * Events in data file are not collect in groups, but we still want
>> + * the group display. Set the artificial group and set the leader's
>> + * forced_leader flag to notify the display code.
>> + */
>> +void perf_evlist_forced_leader(struct perf_evlist *evlist)
> 
> This should've been:
> 
> void perf_evlist__forced_leader(struct perf_evlist *evlist)
> 
> But then, by the verb conjugation, I thought that this would be a query,
> i.e. "is this a forced leader?", but then, it returning void didn't
> match that expectation, which leads me having to actually _read_ that
> code to understand what is that this function does, which I would, but
> just to check that it matches what it seems to be, now I'm trying to do
> it to figure it out what is it that this thing does :-\
> 
> 
> I see, its a 'd' to many, this should be:
> 
> void perf_evlist__force_leader(struct perf_evlist *evlist)
> 
> Please confirm that this is what you intend by posting a v4, so far,
> just for this specific patch and any other in the series that uses this
> function, ok?
> 
> - Arnaldo
> 

Yes, the function is not a query, it's just to setup the forced_leader.

Maybe we can use the name "perf_evlist__setup_forced_leader". But it 
looks the name is long so the name "perf_evlist__force_leader" is also good.

I will use the name "perf_evlist__force_leader" in v4.

Thanks
Jin Yao



>> +{
>> +	if (!evlist->nr_groups) {
>> +		struct perf_evsel *leader = perf_evlist__first(evlist);
>> +
>> +		perf_evlist__set_leader(evlist);
>> +		leader->forced_leader = true;
>> +	}
>> +}
>> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
>> index 6c41b2f..d77d514 100644
>> --- a/tools/perf/util/evlist.h
>> +++ b/tools/perf/util/evlist.h
>> @@ -309,4 +309,7 @@ struct perf_evsel *perf_evlist__event2evsel(struct perf_evlist *evlist,
>>   					    union perf_event *event);
>>   
>>   bool perf_evlist__exclude_kernel(struct perf_evlist *evlist);
>> +
>> +void perf_evlist_forced_leader(struct perf_evlist *evlist);
>> +
>>   #endif /* __PERF_EVLIST_H */
>> -- 
>> 2.7.4