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

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

[Jiri Olsa]

On Fri, May 18, 2018 at 08:57:39PM +0800, Jin Yao wrote:
> 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 | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 4c931af..63fe776 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -202,12 +202,8 @@ static int hist_iter__branch_callback(struct hist_entry_iter *iter,

there's comment in this place, which no longer applies in here:


/*
 * 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.
 */

could you please move/change/apply it to perf_evlist_forced_leader

thanks,
jirka

>  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
> 

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

[Jin, Yao]

On 5/18/2018 3:04 PM, Jiri Olsa wrote:
> On Fri, May 18, 2018 at 08:57:39PM +0800, Jin Yao wrote:
>> 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 | 8 ++------
>>   1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
>> index 4c931af..63fe776 100644
>> --- a/tools/perf/builtin-report.c
>> +++ b/tools/perf/builtin-report.c
>> @@ -202,12 +202,8 @@ static int hist_iter__branch_callback(struct hist_entry_iter *iter,
> 
> there's comment in this place, which no longer applies in here:
> 
> 
> /*
>   * 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.
>   */
> 
> could you please move/change/apply it to perf_evlist_forced_leader
> 
> thanks,
> jirka
> 

Oh, yes, I should move the comment to perf_evlist_forced_leader.

I will do it in next version.

Thanks
Jin Yao

>>   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 v2 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;
                     :            }
                     :

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   |  8 ++------
 tools/perf/util/evlist.c      | 10 ++++++++++
 tools/perf/util/evlist.h      |  3 +++
 4 files changed, 22 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH v2 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 | 10 ++++++++++
 tools/perf/util/evlist.h |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index a59281d..ed8a9d5 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1795,3 +1795,13 @@ bool perf_evlist__exclude_kernel(struct perf_evlist *evlist)
 
 	return true;
 }
+
+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 v2 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 | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 4c931af..63fe776 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -202,12 +202,8 @@ static int hist_iter__branch_callback(struct hist_entry_iter *iter,
 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 v2 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