[BUG] perf script segfault

[BUG] perf script segfault

[David Ahern]

Surprised Stephane has not hit this one yet:

$ perf record -e <any-event> -a | perf script
Segmentation fault (core dumped)

It's the second one that core dumps.

$ gdb perf core.16704
...
[New LWP 16704]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
Core was generated by `/tmp/perf/perf script'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  perf_tool__fill_defaults (tool=tool@entry=0x0) at util/session.c:259
259		if (tool->sample == NULL)
...
(gdb) bt
#0  perf_tool__fill_defaults (tool=tool@entry=0x0) at util/session.c:259
#1  0x00000000004a5baa in __perf_session__process_pipe_events 
(session=0x2178b80) at util/session.c:1178
#2  perf_session__process_events (session=0x2178b80) at util/session.c:1416
#3  0x000000000043c5dc in __cmd_script (script=0x7fffc7a4d840) at 
builtin-script.c:803
#4  cmd_script (argc=<optimized out>, argv=<optimized out>, 
prefix=<optimized out>) at builtin-script.c:1840
...

tool was moved to ordered_events and is not initialized for pipe mode. I 
don't have time to look into it more than that before PTO on Wednesday.

David

Re: [BUG] perf script segfault

[Arnaldo Carvalho de Melo]

Em Mon, Mar 30, 2015 at 04:51:34PM -0600, David Ahern escreveu:
> Surprised Stephane has not hit this one yet:
> 
> $ perf record -e <any-event> -a | perf script
> Segmentation fault (core dumped)
> 
> It's the second one that core dumps.
> 
> $ gdb perf core.16704
> ...
> [New LWP 16704]
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib64/libthread_db.so.1".
> Core was generated by `/tmp/perf/perf script'.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0  perf_tool__fill_defaults (tool=tool@entry=0x0) at util/session.c:259
> 259		if (tool->sample == NULL)
> ...
> (gdb) bt
> #0  perf_tool__fill_defaults (tool=tool@entry=0x0) at util/session.c:259
> #1  0x00000000004a5baa in __perf_session__process_pipe_events
> (session=0x2178b80) at util/session.c:1178
> #2  perf_session__process_events (session=0x2178b80) at util/session.c:1416
> #3  0x000000000043c5dc in __cmd_script (script=0x7fffc7a4d840) at
> builtin-script.c:803
> #4  cmd_script (argc=<optimized out>, argv=<optimized out>,
> prefix=<optimized out>) at builtin-script.c:1840
> ...
> 
> tool was moved to ordered_events and is not initialized for pipe mode. I
> don't have time to look into it more than that before PTO on Wednesday.

I guess this one is enough, no? Checking with your example...


diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index adf0740c563b..f0c8dc519db0 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -139,6 +139,7 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 	    tool->ordered_events && !perf_evlist__sample_id_all(session->evlist)) {
 		dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
 		tool->ordered_events = false;
+		session->ordered_events.tool = tool;
 	} else {
 		ordered_events__init(&session->ordered_events, &session->machines,
 				     session->evlist, tool, ordered_events__deliver_event);

Re: [BUG] perf script segfault

[Arnaldo Carvalho de Melo]

Em Mon, Mar 30, 2015 at 08:45:33PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Mar 30, 2015 at 04:51:34PM -0600, David Ahern escreveu:
> > tool was moved to ordered_events and is not initialized for pipe mode. I
> > don't have time to look into it more than that before PTO on Wednesday.
 
> I guess this one is enough, no? Checking with your example...

So the following is better, can you give it a try, please?

- Arnaldo


>From fbd7d154f01c47db71a3d8b0546911872aa1de54 Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Tue, 31 Mar 2015 09:53:50 -0300
Subject: [PATCH 1/1] perf session: Always initialize ordered_events

Even when it is not used to actually reorder events, some of its fields
are used, like session->ordered_events->tool, to shorten function
signatures where tool, for instance, was being passed, as the tool is
needed for the ordered_events code, we need it there and might as well
use it for other perf_session needs.

This fixes a problem where 'perf script' had some condition that made
session->ordered_events not to be initialized even with its
script->tool ordered_events related flags asking for it to be, which
looks like another bug and needs to be investigated further.

Always initializing session->ordered_events at least leaves the current
assumptions in place, so do it now.

Reported-by: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Don Zickus <dzickus@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/n/tip-b1xxk0rwkz2a0gip1uufmjqg@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/session.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index adf0740c563b..89c66797abe4 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -110,6 +110,8 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 
 	session->repipe = repipe;
 	machines__init(&session->machines);
+	ordered_events__init(&session->ordered_events, &session->machines,
+			     session->evlist, tool, ordered_events__deliver_event);
 
 	if (file) {
 		if (perf_data_file__open(file))
@@ -139,9 +141,6 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 	    tool->ordered_events && !perf_evlist__sample_id_all(session->evlist)) {
 		dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
 		tool->ordered_events = false;
-	} else {
-		ordered_events__init(&session->ordered_events, &session->machines,
-				     session->evlist, tool, ordered_events__deliver_event);
 	}
 
 	return session;
-- 
1.8.3.1

Re: [BUG] perf script segfault

[David Ahern]

On 3/31/15 6:59 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Mar 30, 2015 at 08:45:33PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Mon, Mar 30, 2015 at 04:51:34PM -0600, David Ahern escreveu:
>>> tool was moved to ordered_events and is not initialized for pipe mode. I
>>> don't have time to look into it more than that before PTO on Wednesday.
>
>> I guess this one is enough, no? Checking with your example...
>
> So the following is better, can you give it a try, please?
>
> - Arnaldo
>
>
>  From fbd7d154f01c47db71a3d8b0546911872aa1de54 Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Tue, 31 Mar 2015 09:53:50 -0300
> Subject: [PATCH 1/1] perf session: Always initialize ordered_events
>
> Even when it is not used to actually reorder events, some of its fields
> are used, like session->ordered_events->tool, to shorten function
> signatures where tool, for instance, was being passed, as the tool is
> needed for the ordered_events code, we need it there and might as well
> use it for other perf_session needs.
>
> This fixes a problem where 'perf script' had some condition that made
> session->ordered_events not to be initialized even with its
> script->tool ordered_events related flags asking for it to be, which
> looks like another bug and needs to be investigated further.
>
> Always initializing session->ordered_events at least leaves the current
> assumptions in place, so do it now.
>
> Reported-by: David Ahern <dsahern@gmail.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Don Zickus <dzickus@redhat.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Stephane Eranian <eranian@google.com>
> Link: http://lkml.kernel.org/n/tip-b1xxk0rwkz2a0gip1uufmjqg@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>   tools/perf/util/session.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index adf0740c563b..89c66797abe4 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -110,6 +110,8 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
>
>   	session->repipe = repipe;
>   	machines__init(&session->machines);
> +	ordered_events__init(&session->ordered_events, &session->machines,
> +			     session->evlist, tool, ordered_events__deliver_event);
>
>   	if (file) {
>   		if (perf_data_file__open(file))
> @@ -139,9 +141,6 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
>   	    tool->ordered_events && !perf_evlist__sample_id_all(session->evlist)) {
>   		dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
>   		tool->ordered_events = false;
> -	} else {
> -		ordered_events__init(&session->ordered_events, &session->machines,
> -				     session->evlist, tool, ordered_events__deliver_event);
>   	}
>
>   	return session;
>

Works for me. Reviewed-and-Tested-by: David Ahern <dsahern@gmail.com>

Re: [BUG] perf script segfault

[Jiri Olsa]

On Tue, Mar 31, 2015 at 09:59:20AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Mar 30, 2015 at 08:45:33PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Mar 30, 2015 at 04:51:34PM -0600, David Ahern escreveu:
> > > tool was moved to ordered_events and is not initialized for pipe mode. I
> > > don't have time to look into it more than that before PTO on Wednesday.
>  
> > I guess this one is enough, no? Checking with your example...
> 
> So the following is better, can you give it a try, please?
> 
> - Arnaldo
> 
> 
> From fbd7d154f01c47db71a3d8b0546911872aa1de54 Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Tue, 31 Mar 2015 09:53:50 -0300
> Subject: [PATCH 1/1] perf session: Always initialize ordered_events
> 
> Even when it is not used to actually reorder events, some of its fields
> are used, like session->ordered_events->tool, to shorten function

hum, when did this happen? I somehow missed those changes.. :-\

I think we should have ordered_events struct free of perf_tool,
machines and perf_evlist structs

why dont we store it into perf_session? like in attached patch

plus I think the deliver function could take all the info from
session, because it's all already there...

---
static int ordered_events__deliver_event(struct ordered_events *oe,
                                         struct ordered_event *event,
                                         struct perf_sample *sample)
{
        struct perf_session *session = container_of(oe, struct perf_session,
                                                    ordered_events);

        return machines__deliver_event(&session->machines,
				       session->evlist, event->event,
                                       sample,
				       session->tool,
				       event->file_offset);
}
---

jirka


---
diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c
index 6002fa3fcf77..1c464eff4d62 100644
--- a/tools/perf/util/ordered-events.c
+++ b/tools/perf/util/ordered-events.c
@@ -293,7 +293,7 @@ int ordered_events__flush(struct ordered_events *oe, enum oe_flush how)
 }
 
 void ordered_events__init(struct ordered_events *oe, struct machines *machines,
-			  struct perf_evlist *evlist, struct perf_tool *tool,
+			  struct perf_evlist *evlist,
 			  ordered_events__deliver_t deliver)
 {
 	INIT_LIST_HEAD(&oe->events);
@@ -303,7 +303,6 @@ void ordered_events__init(struct ordered_events *oe, struct machines *machines,
 	oe->cur_alloc_size = 0;
 	oe->evlist	   = evlist;
 	oe->machines	   = machines;
-	oe->tool	   = tool;
 	oe->deliver	   = deliver;
 }
 
diff --git a/tools/perf/util/ordered-events.h b/tools/perf/util/ordered-events.h
index 173e13f28c08..b5008b07b5cc 100644
--- a/tools/perf/util/ordered-events.h
+++ b/tools/perf/util/ordered-events.h
@@ -41,7 +41,6 @@ struct ordered_events {
 	struct ordered_event	*last;
 	struct machines		*machines;
 	struct perf_evlist	*evlist;
-	struct perf_tool	*tool;
 	ordered_events__deliver_t deliver;
 	int			buffer_idx;
 	unsigned int		nr_events;
@@ -54,7 +53,7 @@ int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
 void ordered_events__delete(struct ordered_events *oe, struct ordered_event *event);
 int ordered_events__flush(struct ordered_events *oe, enum oe_flush how);
 void ordered_events__init(struct ordered_events *oe, struct machines *machines,
-			  struct perf_evlist *evlsit, struct perf_tool *tool,
+			  struct perf_evlist *evlsit,
 			  ordered_events__deliver_t deliver);
 void ordered_events__free(struct ordered_events *oe);
 
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index adf0740c563b..04795d8f83a1 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -96,8 +96,11 @@ static int ordered_events__deliver_event(struct ordered_events *oe,
 					 struct ordered_event *event,
 					 struct perf_sample *sample)
 {
+	struct perf_session *session = container_of(oe, struct perf_session,
+						    ordered_events);
+
 	return machines__deliver_event(oe->machines, oe->evlist, event->event,
-				       sample, oe->tool, event->file_offset);
+				       sample, session->tool, event->file_offset);
 }
 
 struct perf_session *perf_session__new(struct perf_data_file *file,
@@ -109,6 +112,7 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 		goto out;
 
 	session->repipe = repipe;
+	session->tool   = tool;
 	machines__init(&session->machines);
 
 	if (file) {
@@ -141,7 +145,7 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 		tool->ordered_events = false;
 	} else {
 		ordered_events__init(&session->ordered_events, &session->machines,
-				     session->evlist, tool, ordered_events__deliver_event);
+				     session->evlist, ordered_events__deliver_event);
 	}
 
 	return session;
@@ -941,7 +945,7 @@ static s64 perf_session__process_user_event(struct perf_session *session,
 					    u64 file_offset)
 {
 	struct ordered_events *oe = &session->ordered_events;
-	struct perf_tool *tool = oe->tool;
+	struct perf_tool *tool = session->tool;
 	int fd = perf_data_file__fd(session->file);
 	int err;
 
@@ -982,7 +986,7 @@ int perf_session__deliver_synth_event(struct perf_session *session,
 				      struct perf_sample *sample)
 {
 	struct perf_evlist *evlist = session->evlist;
-	struct perf_tool *tool = session->ordered_events.tool;
+	struct perf_tool *tool = session->tool;
 
 	events_stats__inc(&evlist->stats, event->header.type);
 
@@ -1060,7 +1064,7 @@ static s64 perf_session__process_event(struct perf_session *session,
 				       union perf_event *event, u64 file_offset)
 {
 	struct perf_evlist *evlist = session->evlist;
-	struct perf_tool *tool = session->ordered_events.tool;
+	struct perf_tool *tool = session->tool;
 	struct perf_sample sample;
 	int ret;
 
@@ -1165,7 +1169,7 @@ volatile int session_done;
 static int __perf_session__process_pipe_events(struct perf_session *session)
 {
 	struct ordered_events *oe = &session->ordered_events;
-	struct perf_tool *tool = oe->tool;
+	struct perf_tool *tool = session->tool;
 	int fd = perf_data_file__fd(session->file);
 	union perf_event *event;
 	uint32_t size, cur_size = 0;
@@ -1298,7 +1302,7 @@ static int __perf_session__process_events(struct perf_session *session,
 					  u64 file_size)
 {
 	struct ordered_events *oe = &session->ordered_events;
-	struct perf_tool *tool = oe->tool;
+	struct perf_tool *tool = session->tool;
 	int fd = perf_data_file__fd(session->file);
 	u64 head, page_offset, file_offset, file_pos, size;
 	int err, mmap_prot, mmap_flags, map_idx = 0;
diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
index 1310998f8318..d5fa7b7916ef 100644
--- a/tools/perf/util/session.h
+++ b/tools/perf/util/session.h
@@ -26,6 +26,7 @@ struct perf_session {
 	u64			one_mmap_offset;
 	struct ordered_events	ordered_events;
 	struct perf_data_file	*file;
+	struct perf_tool	*tool;
 };
 
 #define PRINT_IP_OPT_IP		(1<<0)

Re: [BUG] perf script segfault

[Arnaldo Carvalho de Melo]

Em Tue, Mar 31, 2015 at 03:58:01PM +0200, Jiri Olsa escreveu:
> On Tue, Mar 31, 2015 at 09:59:20AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Mar 30, 2015 at 08:45:33PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Mon, Mar 30, 2015 at 04:51:34PM -0600, David Ahern escreveu:
> > > > tool was moved to ordered_events and is not initialized for pipe mode. I
> > > > don't have time to look into it more than that before PTO on Wednesday.
> >  
> > > I guess this one is enough, no? Checking with your example...
> > 
> > So the following is better, can you give it a try, please?
> > 
> > - Arnaldo
> > 
> > 
> > From fbd7d154f01c47db71a3d8b0546911872aa1de54 Mon Sep 17 00:00:00 2001
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Date: Tue, 31 Mar 2015 09:53:50 -0300
> > Subject: [PATCH 1/1] perf session: Always initialize ordered_events
> > 
> > Even when it is not used to actually reorder events, some of its fields
> > are used, like session->ordered_events->tool, to shorten function
> 
> hum, when did this happen? I somehow missed those changes.. :-\
> 
> I think we should have ordered_events struct free of perf_tool,
> machines and perf_evlist structs

If possible, yes, lemme see, yeah, thanks for the patch, testing it,
good that we can remove that from ordered_events!

- Arnaldo
 
> why dont we store it into perf_session? like in attached patch
> 
> plus I think the deliver function could take all the info from
> session, because it's all already there...
> 
> ---
> static int ordered_events__deliver_event(struct ordered_events *oe,
>                                          struct ordered_event *event,
>                                          struct perf_sample *sample)
> {
>         struct perf_session *session = container_of(oe, struct perf_session,
>                                                     ordered_events);
> 
>         return machines__deliver_event(&session->machines,
> 				       session->evlist, event->event,
>                                        sample,
> 				       session->tool,
> 				       event->file_offset);
> }
> ---
> 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c
> index 6002fa3fcf77..1c464eff4d62 100644
> --- a/tools/perf/util/ordered-events.c
> +++ b/tools/perf/util/ordered-events.c
> @@ -293,7 +293,7 @@ int ordered_events__flush(struct ordered_events *oe, enum oe_flush how)
>  }
>  
>  void ordered_events__init(struct ordered_events *oe, struct machines *machines,
> -			  struct perf_evlist *evlist, struct perf_tool *tool,
> +			  struct perf_evlist *evlist,
>  			  ordered_events__deliver_t deliver)
>  {
>  	INIT_LIST_HEAD(&oe->events);
> @@ -303,7 +303,6 @@ void ordered_events__init(struct ordered_events *oe, struct machines *machines,
>  	oe->cur_alloc_size = 0;
>  	oe->evlist	   = evlist;
>  	oe->machines	   = machines;
> -	oe->tool	   = tool;
>  	oe->deliver	   = deliver;
>  }
>  
> diff --git a/tools/perf/util/ordered-events.h b/tools/perf/util/ordered-events.h
> index 173e13f28c08..b5008b07b5cc 100644
> --- a/tools/perf/util/ordered-events.h
> +++ b/tools/perf/util/ordered-events.h
> @@ -41,7 +41,6 @@ struct ordered_events {
>  	struct ordered_event	*last;
>  	struct machines		*machines;
>  	struct perf_evlist	*evlist;
> -	struct perf_tool	*tool;
>  	ordered_events__deliver_t deliver;
>  	int			buffer_idx;
>  	unsigned int		nr_events;
> @@ -54,7 +53,7 @@ int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
>  void ordered_events__delete(struct ordered_events *oe, struct ordered_event *event);
>  int ordered_events__flush(struct ordered_events *oe, enum oe_flush how);
>  void ordered_events__init(struct ordered_events *oe, struct machines *machines,
> -			  struct perf_evlist *evlsit, struct perf_tool *tool,
> +			  struct perf_evlist *evlsit,
>  			  ordered_events__deliver_t deliver);
>  void ordered_events__free(struct ordered_events *oe);
>  
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index adf0740c563b..04795d8f83a1 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -96,8 +96,11 @@ static int ordered_events__deliver_event(struct ordered_events *oe,
>  					 struct ordered_event *event,
>  					 struct perf_sample *sample)
>  {
> +	struct perf_session *session = container_of(oe, struct perf_session,
> +						    ordered_events);
> +
>  	return machines__deliver_event(oe->machines, oe->evlist, event->event,
> -				       sample, oe->tool, event->file_offset);
> +				       sample, session->tool, event->file_offset);
>  }
>  
>  struct perf_session *perf_session__new(struct perf_data_file *file,
> @@ -109,6 +112,7 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
>  		goto out;
>  
>  	session->repipe = repipe;
> +	session->tool   = tool;
>  	machines__init(&session->machines);
>  
>  	if (file) {
> @@ -141,7 +145,7 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
>  		tool->ordered_events = false;
>  	} else {
>  		ordered_events__init(&session->ordered_events, &session->machines,
> -				     session->evlist, tool, ordered_events__deliver_event);
> +				     session->evlist, ordered_events__deliver_event);
>  	}
>  
>  	return session;
> @@ -941,7 +945,7 @@ static s64 perf_session__process_user_event(struct perf_session *session,
>  					    u64 file_offset)
>  {
>  	struct ordered_events *oe = &session->ordered_events;
> -	struct perf_tool *tool = oe->tool;
> +	struct perf_tool *tool = session->tool;
>  	int fd = perf_data_file__fd(session->file);
>  	int err;
>  
> @@ -982,7 +986,7 @@ int perf_session__deliver_synth_event(struct perf_session *session,
>  				      struct perf_sample *sample)
>  {
>  	struct perf_evlist *evlist = session->evlist;
> -	struct perf_tool *tool = session->ordered_events.tool;
> +	struct perf_tool *tool = session->tool;
>  
>  	events_stats__inc(&evlist->stats, event->header.type);
>  
> @@ -1060,7 +1064,7 @@ static s64 perf_session__process_event(struct perf_session *session,
>  				       union perf_event *event, u64 file_offset)
>  {
>  	struct perf_evlist *evlist = session->evlist;
> -	struct perf_tool *tool = session->ordered_events.tool;
> +	struct perf_tool *tool = session->tool;
>  	struct perf_sample sample;
>  	int ret;
>  
> @@ -1165,7 +1169,7 @@ volatile int session_done;
>  static int __perf_session__process_pipe_events(struct perf_session *session)
>  {
>  	struct ordered_events *oe = &session->ordered_events;
> -	struct perf_tool *tool = oe->tool;
> +	struct perf_tool *tool = session->tool;
>  	int fd = perf_data_file__fd(session->file);
>  	union perf_event *event;
>  	uint32_t size, cur_size = 0;
> @@ -1298,7 +1302,7 @@ static int __perf_session__process_events(struct perf_session *session,
>  					  u64 file_size)
>  {
>  	struct ordered_events *oe = &session->ordered_events;
> -	struct perf_tool *tool = oe->tool;
> +	struct perf_tool *tool = session->tool;
>  	int fd = perf_data_file__fd(session->file);
>  	u64 head, page_offset, file_offset, file_pos, size;
>  	int err, mmap_prot, mmap_flags, map_idx = 0;
> diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
> index 1310998f8318..d5fa7b7916ef 100644
> --- a/tools/perf/util/session.h
> +++ b/tools/perf/util/session.h
> @@ -26,6 +26,7 @@ struct perf_session {
>  	u64			one_mmap_offset;
>  	struct ordered_events	ordered_events;
>  	struct perf_data_file	*file;
> +	struct perf_tool	*tool;
>  };
>  
>  #define PRINT_IP_OPT_IP		(1<<0)

Re: [BUG] perf script segfault

[Arnaldo Carvalho de Melo]

Em Tue, Mar 31, 2015 at 11:02:16AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Mar 31, 2015 at 03:58:01PM +0200, Jiri Olsa escreveu:
> > On Tue, Mar 31, 2015 at 09:59:20AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Mon, Mar 30, 2015 at 08:45:33PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Mon, Mar 30, 2015 at 04:51:34PM -0600, David Ahern escreveu:
> > > > > tool was moved to ordered_events and is not initialized for pipe mode. I
> > > > > don't have time to look into it more than that before PTO on Wednesday.
> > > So the following is better, can you give it a try, please?

> > > Subject: [PATCH 1/1] perf session: Always initialize ordered_events
> > > Even when it is not used to actually reorder events, some of its fields
> > > are used, like session->ordered_events->tool, to shorten function

> > hum, when did this happen? I somehow missed those changes.. :-\

> > I think we should have ordered_events struct free of perf_tool,
> > machines and perf_evlist structs
 
> If possible, yes, lemme see, yeah, thanks for the patch, testing it,
> good that we can remove that from ordered_events!

Ok, if I just apply your patch I stumble again on the bug I alluded to
in my patch changeset, i.e.:


Program terminated with signal 11, Segmentation fault.
#0  0x00000000004d11c0 in list_del (entry=0x0) at /home/acme/git/linux/tools/perf/util/include/../../../../include/linux/list.h:107
107		__list_del(entry->prev, entry->next);
Missing separate debuginfos, use: debuginfo-install audit-libs-2.4.1-5.el7.x86_64 bzip2-libs-1.0.6-12.el7.x86_64 elfutils-libelf-0.160-1.el7.x86_64 elfutils-libs-0.160-1.el7.x86_64 glibc-2.17-78.el7.x86_64 libgcc-4.8.3-9.el7.x86_64 nss-softokn-freebl-3.16.2.3-9.el7.x86_64 numactl-libs-2.0.9-4.el7.x86_64 perl-libs-5.16.3-285.el7.x86_64 python-libs-2.7.5-16.el7.x86_64 slang-2.2.4-11.el7.x86_64 xz-libs-5.1.2-9alpha.el7.x86_64 zlib-1.2.7-13.el7.x86_64
(gdb) bt
#0  0x00000000004d11c0 in list_del (entry=0x0) at /home/acme/git/linux/tools/perf/util/include/../../../../include/linux/list.h:107
#1  0x00000000004d20c4 in ordered_events__free (oe=0x1fe7e50) at util/ordered-events.c:315
#2  0x00000000004cf936 in __perf_session__process_pipe_events (session=0x1fe7c60) at util/session.c:1256
#3  0x00000000004cff53 in perf_session__process_events (session=0x1fe7c60) at util/session.c:1420
#4  0x0000000000440649 in __cmd_script (script=0x7fff7d2371c0) at builtin-script.c:803
#5  0x0000000000443b64 in cmd_script (argc=0, argv=0x7fff7d237f10, prefix=0x0) at builtin-script.c:1840
#6  0x0000000000488388 in run_builtin (p=0x871150 <commands+336>, argc=3, argv=0x7fff7d237f10) at perf.c:370
#7  0x00000000004885e7 in handle_internal_command (argc=3, argv=0x7fff7d237f10) at perf.c:429
#8  0x0000000000488733 in run_argv (argcp=0x7fff7d237d6c, argv=0x7fff7d237d60) at perf.c:473
#9  0x0000000000488a94 in main (argc=3, argv=0x7fff7d237f10) at perf.c:588
(gdb)

I.e. it tries to use uninitialized ordered_events stuff on the way out of
processing pipe events.

Which is related to the fact that builtin-script.c has:

                .tool = {
<SNIP>
                        .ordered_events  = true,
                        .ordering_requires_timestamps = true,
                },

Indicating it wants the ordered_events to be used, but then, when we get to
perf_session__new() we end up failing this test:

        if (tool && tool->ordering_requires_timestamps &&
            tool->ordered_events && !perf_evlist__sample_id_all(session->evlist)) {
                dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
                tool->ordered_events = false;
        } else {
                ordered_events__init(&session->ordered_events, &session->machines,
                                     session->evlist, ordered_events__deliver_event);
        }

And setting tool->ordered_events to false;

What fails is perf_evlist__sample_id_all(session->evlist), I went as far as
looking at the perf_evlist__sample_id_all call that will find a first evsel,
with all its fields zeroed, i.e. at some point it finds out it is a pipe (perf
script < perf.data) and seems to not read the perf_event_attr attributes, etc,
but I need to dig deeper here to figure out this and why is it that in that
case we end with two 'perf script' processes when I think it should be just one, etc.

So, to keep the assumptions in place I'll keep my patch, the one that David
tested, and redo yours on top, looking as well at ways to use what is in
perf_session, should provide a patch soon.

- Arnaldo

Re: [BUG] perf script segfault

[Jiri Olsa]

On Tue, Mar 31, 2015 at 11:13:26AM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> And setting tool->ordered_events to false;
> 
> What fails is perf_evlist__sample_id_all(session->evlist), I went as far as
> looking at the perf_evlist__sample_id_all call that will find a first evsel,
> with all its fields zeroed, i.e. at some point it finds out it is a pipe (perf
> script < perf.data) and seems to not read the perf_event_attr attributes, etc,
> but I need to dig deeper here to figure out this and why is it that in that
> case we end with two 'perf script' processes when I think it should be just one, etc.
> 
> So, to keep the assumptions in place I'll keep my patch, the one that David
> tested, and redo yours on top, looking as well at ways to use what is in
> perf_session, should provide a patch soon.

I was just replying to the poluted ordered_events .. I haven't tracked
the segfault totaly.. we can change that after your fix of course ;-)

jirka

Re: [BUG] perf script segfault

[Arnaldo Carvalho de Melo]

Em Tue, Mar 31, 2015 at 04:25:57PM +0200, Jiri Olsa escreveu:
> On Tue, Mar 31, 2015 at 11:13:26AM -0300, Arnaldo Carvalho de Melo wrote:
> 
> SNIP
> 
> > And setting tool->ordered_events to false;
> > 
> > What fails is perf_evlist__sample_id_all(session->evlist), I went as far as
> > looking at the perf_evlist__sample_id_all call that will find a first evsel,
> > with all its fields zeroed, i.e. at some point it finds out it is a pipe (perf
> > script < perf.data) and seems to not read the perf_event_attr attributes, etc,
> > but I need to dig deeper here to figure out this and why is it that in that
> > case we end with two 'perf script' processes when I think it should be just one, etc.
> > 
> > So, to keep the assumptions in place I'll keep my patch, the one that David
> > tested, and redo yours on top, looking as well at ways to use what is in
> > perf_session, should provide a patch soon.
> 
> I was just replying to the poluted ordered_events .. I haven't tracked

:-) Yeah, that was kinda a layering violation, its good that we can
remove it from there, as for evlist... I did a quick check and yeah, we
can remove it from ordered_events and move the  perf_evlist__parse to
the deliver event function, where it will be obtained from perf_session
that was obtained from container_of (oe), its just one statistic that
remains, but yeah, probably with some work that can as well be removed.

We could then think of this as needed while isolating those
ordered_events code out of perf_session, to make it useable by other
tools (my initial intent as for trace to use it), but in the end result,
after the deliver_event was introduced, it is not needed anymore.

Yeah, machines can as well be obtained from perf_session.

> the segfault totaly.. we can change that after your fix of course ;-)

:-)

- Arnaldo

Re: [BUG] perf script segfault

[Arnaldo Carvalho de Melo]

Em Tue, Mar 31, 2015 at 11:37:44AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Mar 31, 2015 at 04:25:57PM +0200, Jiri Olsa escreveu:
> > I was just replying to the poluted ordered_events .. I haven't tracked
 
> :-) Yeah, that was kinda a layering violation, its good that we can
> remove it from there, as for evlist... I did a quick check and yeah, we
> can remove it from ordered_events and move the  perf_evlist__parse to
> the deliver event function, where it will be obtained from perf_session
> that was obtained from container_of (oe), its just one statistic that
> remains, but yeah, probably with some work that can as well be removed.
 
> We could then think of this as needed while isolating those
> ordered_events code out of perf_session, to make it useable by other
> tools (my initial intent as for trace to use it), but in the end result,
> after the deliver_event was introduced, it is not needed anymore.
 
> Yeah, machines can as well be obtained from perf_session.
 
> > the segfault totaly.. we can change that after your fix of course ;-)
 
> :-)

So, can you take a look at this, that is your patch, rebased on top of
the fix, plus removing the pollution in ordered_events? Compile tested
only, testing now...

- Arnaldo

diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
index c4ffe2bd0738..09b9e8d3fcf7 100644
--- a/tools/perf/util/event.h
+++ b/tools/perf/util/event.h
@@ -242,7 +242,6 @@ struct events_stats {
 	u32 nr_invalid_chains;
 	u32 nr_unknown_id;
 	u32 nr_unprocessable_samples;
-	u32 nr_unordered_events;
 };
 
 struct attr_event {
diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c
index 6002fa3fcf77..52be201b9b25 100644
--- a/tools/perf/util/ordered-events.c
+++ b/tools/perf/util/ordered-events.c
@@ -2,7 +2,6 @@
 #include <linux/compiler.h>
 #include <linux/string.h>
 #include "ordered-events.h"
-#include "evlist.h"
 #include "session.h"
 #include "asm/bug.h"
 #include "debug.h"
@@ -167,7 +166,7 @@ int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
 		pr_oe_time(oe->last_flush, "last flush, last_flush_type %d\n",
 			   oe->last_flush_type);
 
-		oe->evlist->stats.nr_unordered_events++;
+		oe->nr_unordered_events++;
 	}
 
 	oevent = ordered_events__new_event(oe, timestamp, event);
@@ -187,7 +186,6 @@ static int __ordered_events__flush(struct ordered_events *oe)
 {
 	struct list_head *head = &oe->events;
 	struct ordered_event *tmp, *iter;
-	struct perf_sample sample;
 	u64 limit = oe->next_flush;
 	u64 last_ts = oe->last ? oe->last->timestamp : 0ULL;
 	bool show_progress = limit == ULLONG_MAX;
@@ -206,15 +204,9 @@ static int __ordered_events__flush(struct ordered_events *oe)
 
 		if (iter->timestamp > limit)
 			break;
-
-		ret = perf_evlist__parse_sample(oe->evlist, iter->event, &sample);
+		ret = oe->deliver(oe, iter);
 		if (ret)
-			pr_err("Can't parse sample, err = %d\n", ret);
-		else {
-			ret = oe->deliver(oe, iter, &sample);
-			if (ret)
-				return ret;
-		}
+			return ret;
 
 		ordered_events__delete(oe, iter);
 		oe->last_flush = iter->timestamp;
@@ -292,18 +284,13 @@ int ordered_events__flush(struct ordered_events *oe, enum oe_flush how)
 	return err;
 }
 
-void ordered_events__init(struct ordered_events *oe, struct machines *machines,
-			  struct perf_evlist *evlist, struct perf_tool *tool,
-			  ordered_events__deliver_t deliver)
+void ordered_events__init(struct ordered_events *oe, ordered_events__deliver_t deliver)
 {
 	INIT_LIST_HEAD(&oe->events);
 	INIT_LIST_HEAD(&oe->cache);
 	INIT_LIST_HEAD(&oe->to_free);
 	oe->max_alloc_size = (u64) -1;
 	oe->cur_alloc_size = 0;
-	oe->evlist	   = evlist;
-	oe->machines	   = machines;
-	oe->tool	   = tool;
 	oe->deliver	   = deliver;
 }
 
diff --git a/tools/perf/util/ordered-events.h b/tools/perf/util/ordered-events.h
index 173e13f28c08..f403991e3bfd 100644
--- a/tools/perf/util/ordered-events.h
+++ b/tools/perf/util/ordered-events.h
@@ -3,10 +3,7 @@
 
 #include <linux/types.h>
 
-struct perf_tool;
-struct perf_evlist;
 struct perf_sample;
-struct machines;
 
 struct ordered_event {
 	u64			timestamp;
@@ -25,8 +22,7 @@ enum oe_flush {
 struct ordered_events;
 
 typedef int (*ordered_events__deliver_t)(struct ordered_events *oe,
-					 struct ordered_event *event,
-					 struct perf_sample *sample);
+					 struct ordered_event *event);
 
 struct ordered_events {
 	u64			last_flush;
@@ -39,13 +35,11 @@ struct ordered_events {
 	struct list_head	to_free;
 	struct ordered_event	*buffer;
 	struct ordered_event	*last;
-	struct machines		*machines;
-	struct perf_evlist	*evlist;
-	struct perf_tool	*tool;
 	ordered_events__deliver_t deliver;
 	int			buffer_idx;
 	unsigned int		nr_events;
 	enum oe_flush		last_flush_type;
+	u32			nr_unordered_events;
 	bool                    copy_on_queue;
 };
 
@@ -53,9 +47,7 @@ int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
 			  struct perf_sample *sample, u64 file_offset);
 void ordered_events__delete(struct ordered_events *oe, struct ordered_event *event);
 int ordered_events__flush(struct ordered_events *oe, enum oe_flush how);
-void ordered_events__init(struct ordered_events *oe, struct machines *machines,
-			  struct perf_evlist *evlsit, struct perf_tool *tool,
-			  ordered_events__deliver_t deliver);
+void ordered_events__init(struct ordered_events *oe, ordered_events__deliver_t deliver);
 void ordered_events__free(struct ordered_events *oe);
 
 static inline
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 89c66797abe4..dcdd63bd967b 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -93,11 +93,20 @@ static void perf_session__set_comm_exec(struct perf_session *session)
 }
 
 static int ordered_events__deliver_event(struct ordered_events *oe,
-					 struct ordered_event *event,
-					 struct perf_sample *sample)
+					 struct ordered_event *event)
 {
-	return machines__deliver_event(oe->machines, oe->evlist, event->event,
-				       sample, oe->tool, event->file_offset);
+	struct perf_sample sample;
+	struct perf_session *session = container_of(oe, struct perf_session,
+						    ordered_events);
+	int ret = perf_evlist__parse_sample(session->evlist, event->event, &sample);
+
+	if (ret) {
+		pr_err("Can't parse sample, err = %d\n", ret);
+		return ret;
+	}
+
+	return machines__deliver_event(&session->machines, session->evlist, event->event,
+				       &sample, session->tool, event->file_offset);
 }
 
 struct perf_session *perf_session__new(struct perf_data_file *file,
@@ -109,9 +118,9 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 		goto out;
 
 	session->repipe = repipe;
+	session->tool   = tool;
 	machines__init(&session->machines);
-	ordered_events__init(&session->ordered_events, &session->machines,
-			     session->evlist, tool, ordered_events__deliver_event);
+	ordered_events__init(&session->ordered_events, ordered_events__deliver_event);
 
 	if (file) {
 		if (perf_data_file__open(file))
@@ -940,7 +949,7 @@ static s64 perf_session__process_user_event(struct perf_session *session,
 					    u64 file_offset)
 {
 	struct ordered_events *oe = &session->ordered_events;
-	struct perf_tool *tool = oe->tool;
+	struct perf_tool *tool = session->tool;
 	int fd = perf_data_file__fd(session->file);
 	int err;
 
@@ -981,7 +990,7 @@ int perf_session__deliver_synth_event(struct perf_session *session,
 				      struct perf_sample *sample)
 {
 	struct perf_evlist *evlist = session->evlist;
-	struct perf_tool *tool = session->ordered_events.tool;
+	struct perf_tool *tool = session->tool;
 
 	events_stats__inc(&evlist->stats, event->header.type);
 
@@ -1059,7 +1068,7 @@ static s64 perf_session__process_event(struct perf_session *session,
 				       union perf_event *event, u64 file_offset)
 {
 	struct perf_evlist *evlist = session->evlist;
-	struct perf_tool *tool = session->ordered_events.tool;
+	struct perf_tool *tool = session->tool;
 	struct perf_sample sample;
 	int ret;
 
@@ -1154,9 +1163,12 @@ static void perf_tool__warn_about_errors(const struct perf_tool *tool,
 			    "Do you have a KVM guest running and not using 'perf kvm'?\n",
 			    stats->nr_unprocessable_samples);
 	}
+}
 
-	if (stats->nr_unordered_events != 0)
-		ui__warning("%u out of order events recorded.\n", stats->nr_unordered_events);
+static void ordered_events__warn_about_errors(struct ordered_events *oe)
+{
+	if (oe->nr_unordered_events != 0)
+		ui__warning("%u out of order events recorded.\n", oe->nr_unordered_events);
 }
 
 volatile int session_done;
@@ -1164,7 +1176,7 @@ volatile int session_done;
 static int __perf_session__process_pipe_events(struct perf_session *session)
 {
 	struct ordered_events *oe = &session->ordered_events;
-	struct perf_tool *tool = oe->tool;
+	struct perf_tool *tool = session->tool;
 	int fd = perf_data_file__fd(session->file);
 	union perf_event *event;
 	uint32_t size, cur_size = 0;
@@ -1248,6 +1260,7 @@ done:
 out_err:
 	free(buf);
 	perf_tool__warn_about_errors(tool, &session->evlist->stats);
+	ordered_events__warn_about_errors(&session->ordered_events);
 	ordered_events__free(&session->ordered_events);
 	return err;
 }
@@ -1297,7 +1310,7 @@ static int __perf_session__process_events(struct perf_session *session,
 					  u64 file_size)
 {
 	struct ordered_events *oe = &session->ordered_events;
-	struct perf_tool *tool = oe->tool;
+	struct perf_tool *tool = session->tool;
 	int fd = perf_data_file__fd(session->file);
 	u64 head, page_offset, file_offset, file_pos, size;
 	int err, mmap_prot, mmap_flags, map_idx = 0;
@@ -1394,6 +1407,7 @@ out:
 out_err:
 	ui_progress__finish();
 	perf_tool__warn_about_errors(tool, &session->evlist->stats);
+	ordered_events__warn_about_errors(&session->ordered_events);
 	ordered_events__free(&session->ordered_events);
 	session->one_mmap = false;
 	return err;
diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
index 1310998f8318..d5fa7b7916ef 100644
--- a/tools/perf/util/session.h
+++ b/tools/perf/util/session.h
@@ -26,6 +26,7 @@ struct perf_session {
 	u64			one_mmap_offset;
 	struct ordered_events	ordered_events;
 	struct perf_data_file	*file;
+	struct perf_tool	*tool;
 };
 
 #define PRINT_IP_OPT_IP		(1<<0)

Re: [BUG] perf script segfault

[Jiri Olsa]

On Tue, Mar 31, 2015 at 11:57:39AM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

>  	struct ordered_events *oe = &session->ordered_events;
> -	struct perf_tool *tool = oe->tool;
> +	struct perf_tool *tool = session->tool;
>  	int fd = perf_data_file__fd(session->file);
>  	u64 head, page_offset, file_offset, file_pos, size;
>  	int err, mmap_prot, mmap_flags, map_idx = 0;
> @@ -1394,6 +1407,7 @@ out:
>  out_err:
>  	ui_progress__finish();
>  	perf_tool__warn_about_errors(tool, &session->evlist->stats);
> +	ordered_events__warn_about_errors(&session->ordered_events);

how about just single warning function ?

  perf_session__warn_about_errors(struct perf_session *session)

session has now both tool and ordered_events

jirka

Re: [BUG] perf script segfault

[Jiri Olsa]

On Tue, Mar 31, 2015 at 05:48:44PM +0200, Jiri Olsa wrote:
> On Tue, Mar 31, 2015 at 11:57:39AM -0300, Arnaldo Carvalho de Melo wrote:
> 
> SNIP
> 
> >  	struct ordered_events *oe = &session->ordered_events;
> > -	struct perf_tool *tool = oe->tool;
> > +	struct perf_tool *tool = session->tool;
> >  	int fd = perf_data_file__fd(session->file);
> >  	u64 head, page_offset, file_offset, file_pos, size;
> >  	int err, mmap_prot, mmap_flags, map_idx = 0;
> > @@ -1394,6 +1407,7 @@ out:
> >  out_err:
> >  	ui_progress__finish();
> >  	perf_tool__warn_about_errors(tool, &session->evlist->stats);
> > +	ordered_events__warn_about_errors(&session->ordered_events);
> 
> how about just single warning function ?
> 
>   perf_session__warn_about_errors(struct perf_session *session)
> 
> session has now both tool and ordered_events

other than this it looks ok to me

thanks,
jirka

Re: [BUG] perf script segfault

[Arnaldo Carvalho de Melo]

Em Tue, Mar 31, 2015 at 05:50:48PM +0200, Jiri Olsa escreveu:
> On Tue, Mar 31, 2015 at 05:48:44PM +0200, Jiri Olsa wrote:
> > On Tue, Mar 31, 2015 at 11:57:39AM -0300, Arnaldo Carvalho de Melo wrote:
> > 
> > SNIP
> > 
> > >  	struct ordered_events *oe = &session->ordered_events;
> > > -	struct perf_tool *tool = oe->tool;
> > > +	struct perf_tool *tool = session->tool;
> > >  	int fd = perf_data_file__fd(session->file);
> > >  	u64 head, page_offset, file_offset, file_pos, size;
> > >  	int err, mmap_prot, mmap_flags, map_idx = 0;
> > > @@ -1394,6 +1407,7 @@ out:
> > >  out_err:
> > >  	ui_progress__finish();
> > >  	perf_tool__warn_about_errors(tool, &session->evlist->stats);
> > > +	ordered_events__warn_about_errors(&session->ordered_events);
> > 
> > how about just single warning function ?
> > 
> >   perf_session__warn_about_errors(struct perf_session *session)
> > 
> > session has now both tool and ordered_events
> 
> other than this it looks ok to me

Like this? If so, can I have your Acked-by? :-)

- Arnaldo

>From a6ee71dd2814dff89670b3764ac9fbb897366312 Mon Sep 17 00:00:00 2001
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Tue, 31 Mar 2015 12:48:16 -0300
Subject: [PATCH 1/1] perf ordered_samples: Remove references to
 perf_{evlist,tool} and machines

As these can be obtained from the ordered_events pointer, via
container_of, reducing the cross section of ordered_samples.

These were added to ordered_samples in:

 commit b7b61cbebd789a3dbca522e3fdb727fe5c95593f
 Author: Arnaldo Carvalho de Melo <acme@redhat.com>
 Date:   Tue Mar 3 11:58:45 2015 -0300

    perf ordered_events: Shorten function signatures

    By keeping pointers to machines, evlist and tool in ordered_events.

But that was more a transitional patch while moving stuff out from
perf_session.c to ordered_events.c and possibly not even needed by then,
as we could use the container_of() method and instead of having the
nr_unordered_samples stats in events_stats, we can have it in
ordered_samples.

Based-on-a-patch-by: Jiri Olsa <jolsa@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: David Ahern <dsahern@gmail.com>
Cc: Don Zickus <dzickus@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/n/tip-4lk0t9js82g0tfc0x1onpkjt@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/event.h          |  1 -
 tools/perf/util/ordered-events.c | 21 ++++--------------
 tools/perf/util/ordered-events.h | 14 +++---------
 tools/perf/util/session.c        | 47 +++++++++++++++++++++++++---------------
 tools/perf/util/session.h        |  1 +
 5 files changed, 37 insertions(+), 47 deletions(-)

diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
index c4ffe2bd0738..09b9e8d3fcf7 100644
--- a/tools/perf/util/event.h
+++ b/tools/perf/util/event.h
@@ -242,7 +242,6 @@ struct events_stats {
 	u32 nr_invalid_chains;
 	u32 nr_unknown_id;
 	u32 nr_unprocessable_samples;
-	u32 nr_unordered_events;
 };
 
 struct attr_event {
diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c
index 6002fa3fcf77..52be201b9b25 100644
--- a/tools/perf/util/ordered-events.c
+++ b/tools/perf/util/ordered-events.c
@@ -2,7 +2,6 @@
 #include <linux/compiler.h>
 #include <linux/string.h>
 #include "ordered-events.h"
-#include "evlist.h"
 #include "session.h"
 #include "asm/bug.h"
 #include "debug.h"
@@ -167,7 +166,7 @@ int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
 		pr_oe_time(oe->last_flush, "last flush, last_flush_type %d\n",
 			   oe->last_flush_type);
 
-		oe->evlist->stats.nr_unordered_events++;
+		oe->nr_unordered_events++;
 	}
 
 	oevent = ordered_events__new_event(oe, timestamp, event);
@@ -187,7 +186,6 @@ static int __ordered_events__flush(struct ordered_events *oe)
 {
 	struct list_head *head = &oe->events;
 	struct ordered_event *tmp, *iter;
-	struct perf_sample sample;
 	u64 limit = oe->next_flush;
 	u64 last_ts = oe->last ? oe->last->timestamp : 0ULL;
 	bool show_progress = limit == ULLONG_MAX;
@@ -206,15 +204,9 @@ static int __ordered_events__flush(struct ordered_events *oe)
 
 		if (iter->timestamp > limit)
 			break;
-
-		ret = perf_evlist__parse_sample(oe->evlist, iter->event, &sample);
+		ret = oe->deliver(oe, iter);
 		if (ret)
-			pr_err("Can't parse sample, err = %d\n", ret);
-		else {
-			ret = oe->deliver(oe, iter, &sample);
-			if (ret)
-				return ret;
-		}
+			return ret;
 
 		ordered_events__delete(oe, iter);
 		oe->last_flush = iter->timestamp;
@@ -292,18 +284,13 @@ int ordered_events__flush(struct ordered_events *oe, enum oe_flush how)
 	return err;
 }
 
-void ordered_events__init(struct ordered_events *oe, struct machines *machines,
-			  struct perf_evlist *evlist, struct perf_tool *tool,
-			  ordered_events__deliver_t deliver)
+void ordered_events__init(struct ordered_events *oe, ordered_events__deliver_t deliver)
 {
 	INIT_LIST_HEAD(&oe->events);
 	INIT_LIST_HEAD(&oe->cache);
 	INIT_LIST_HEAD(&oe->to_free);
 	oe->max_alloc_size = (u64) -1;
 	oe->cur_alloc_size = 0;
-	oe->evlist	   = evlist;
-	oe->machines	   = machines;
-	oe->tool	   = tool;
 	oe->deliver	   = deliver;
 }
 
diff --git a/tools/perf/util/ordered-events.h b/tools/perf/util/ordered-events.h
index 173e13f28c08..f403991e3bfd 100644
--- a/tools/perf/util/ordered-events.h
+++ b/tools/perf/util/ordered-events.h
@@ -3,10 +3,7 @@
 
 #include <linux/types.h>
 
-struct perf_tool;
-struct perf_evlist;
 struct perf_sample;
-struct machines;
 
 struct ordered_event {
 	u64			timestamp;
@@ -25,8 +22,7 @@ enum oe_flush {
 struct ordered_events;
 
 typedef int (*ordered_events__deliver_t)(struct ordered_events *oe,
-					 struct ordered_event *event,
-					 struct perf_sample *sample);
+					 struct ordered_event *event);
 
 struct ordered_events {
 	u64			last_flush;
@@ -39,13 +35,11 @@ struct ordered_events {
 	struct list_head	to_free;
 	struct ordered_event	*buffer;
 	struct ordered_event	*last;
-	struct machines		*machines;
-	struct perf_evlist	*evlist;
-	struct perf_tool	*tool;
 	ordered_events__deliver_t deliver;
 	int			buffer_idx;
 	unsigned int		nr_events;
 	enum oe_flush		last_flush_type;
+	u32			nr_unordered_events;
 	bool                    copy_on_queue;
 };
 
@@ -53,9 +47,7 @@ int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
 			  struct perf_sample *sample, u64 file_offset);
 void ordered_events__delete(struct ordered_events *oe, struct ordered_event *event);
 int ordered_events__flush(struct ordered_events *oe, enum oe_flush how);
-void ordered_events__init(struct ordered_events *oe, struct machines *machines,
-			  struct perf_evlist *evlsit, struct perf_tool *tool,
-			  ordered_events__deliver_t deliver);
+void ordered_events__init(struct ordered_events *oe, ordered_events__deliver_t deliver);
 void ordered_events__free(struct ordered_events *oe);
 
 static inline
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 89c66797abe4..dfacf1d50162 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -93,11 +93,20 @@ static void perf_session__set_comm_exec(struct perf_session *session)
 }
 
 static int ordered_events__deliver_event(struct ordered_events *oe,
-					 struct ordered_event *event,
-					 struct perf_sample *sample)
+					 struct ordered_event *event)
 {
-	return machines__deliver_event(oe->machines, oe->evlist, event->event,
-				       sample, oe->tool, event->file_offset);
+	struct perf_sample sample;
+	struct perf_session *session = container_of(oe, struct perf_session,
+						    ordered_events);
+	int ret = perf_evlist__parse_sample(session->evlist, event->event, &sample);
+
+	if (ret) {
+		pr_err("Can't parse sample, err = %d\n", ret);
+		return ret;
+	}
+
+	return machines__deliver_event(&session->machines, session->evlist, event->event,
+				       &sample, session->tool, event->file_offset);
 }
 
 struct perf_session *perf_session__new(struct perf_data_file *file,
@@ -109,9 +118,9 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
 		goto out;
 
 	session->repipe = repipe;
+	session->tool   = tool;
 	machines__init(&session->machines);
-	ordered_events__init(&session->ordered_events, &session->machines,
-			     session->evlist, tool, ordered_events__deliver_event);
+	ordered_events__init(&session->ordered_events, ordered_events__deliver_event);
 
 	if (file) {
 		if (perf_data_file__open(file))
@@ -940,7 +949,7 @@ static s64 perf_session__process_user_event(struct perf_session *session,
 					    u64 file_offset)
 {
 	struct ordered_events *oe = &session->ordered_events;
-	struct perf_tool *tool = oe->tool;
+	struct perf_tool *tool = session->tool;
 	int fd = perf_data_file__fd(session->file);
 	int err;
 
@@ -981,7 +990,7 @@ int perf_session__deliver_synth_event(struct perf_session *session,
 				      struct perf_sample *sample)
 {
 	struct perf_evlist *evlist = session->evlist;
-	struct perf_tool *tool = session->ordered_events.tool;
+	struct perf_tool *tool = session->tool;
 
 	events_stats__inc(&evlist->stats, event->header.type);
 
@@ -1059,7 +1068,7 @@ static s64 perf_session__process_event(struct perf_session *session,
 				       union perf_event *event, u64 file_offset)
 {
 	struct perf_evlist *evlist = session->evlist;
-	struct perf_tool *tool = session->ordered_events.tool;
+	struct perf_tool *tool = session->tool;
 	struct perf_sample sample;
 	int ret;
 
@@ -1116,10 +1125,12 @@ static struct thread *perf_session__register_idle_thread(struct perf_session *se
 	return thread;
 }
 
-static void perf_tool__warn_about_errors(const struct perf_tool *tool,
-					 const struct events_stats *stats)
+static void perf_session__warn_about_errors(const struct perf_session *session)
 {
-	if (tool->lost == perf_event__process_lost &&
+	const struct events_stats *stats = &session->evlist->stats;
+	const struct ordered_events *oe = &session->ordered_events;
+
+	if (session->tool->lost == perf_event__process_lost &&
 	    stats->nr_events[PERF_RECORD_LOST] != 0) {
 		ui__warning("Processed %d events and lost %d chunks!\n\n"
 			    "Check IO/CPU overload!\n\n",
@@ -1155,8 +1166,8 @@ static void perf_tool__warn_about_errors(const struct perf_tool *tool,
 			    stats->nr_unprocessable_samples);
 	}
 
-	if (stats->nr_unordered_events != 0)
-		ui__warning("%u out of order events recorded.\n", stats->nr_unordered_events);
+	if (oe->nr_unordered_events != 0)
+		ui__warning("%u out of order events recorded.\n", oe->nr_unordered_events);
 }
 
 volatile int session_done;
@@ -1164,7 +1175,7 @@ volatile int session_done;
 static int __perf_session__process_pipe_events(struct perf_session *session)
 {
 	struct ordered_events *oe = &session->ordered_events;
-	struct perf_tool *tool = oe->tool;
+	struct perf_tool *tool = session->tool;
 	int fd = perf_data_file__fd(session->file);
 	union perf_event *event;
 	uint32_t size, cur_size = 0;
@@ -1247,7 +1258,7 @@ done:
 	err = ordered_events__flush(oe, OE_FLUSH__FINAL);
 out_err:
 	free(buf);
-	perf_tool__warn_about_errors(tool, &session->evlist->stats);
+	perf_session__warn_about_errors(session);
 	ordered_events__free(&session->ordered_events);
 	return err;
 }
@@ -1297,7 +1308,7 @@ static int __perf_session__process_events(struct perf_session *session,
 					  u64 file_size)
 {
 	struct ordered_events *oe = &session->ordered_events;
-	struct perf_tool *tool = oe->tool;
+	struct perf_tool *tool = session->tool;
 	int fd = perf_data_file__fd(session->file);
 	u64 head, page_offset, file_offset, file_pos, size;
 	int err, mmap_prot, mmap_flags, map_idx = 0;
@@ -1393,7 +1404,7 @@ out:
 	err = ordered_events__flush(oe, OE_FLUSH__FINAL);
 out_err:
 	ui_progress__finish();
-	perf_tool__warn_about_errors(tool, &session->evlist->stats);
+	perf_session__warn_about_errors(session);
 	ordered_events__free(&session->ordered_events);
 	session->one_mmap = false;
 	return err;
diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
index 1310998f8318..d5fa7b7916ef 100644
--- a/tools/perf/util/session.h
+++ b/tools/perf/util/session.h
@@ -26,6 +26,7 @@ struct perf_session {
 	u64			one_mmap_offset;
 	struct ordered_events	ordered_events;
 	struct perf_data_file	*file;
+	struct perf_tool	*tool;
 };
 
 #define PRINT_IP_OPT_IP		(1<<0)
-- 
1.8.3.1

Re: [BUG] perf script segfault

[Arnaldo Carvalho de Melo]

Em Tue, Mar 31, 2015 at 01:14:20PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Mar 31, 2015 at 05:50:48PM +0200, Jiri Olsa escreveu:
> > On Tue, Mar 31, 2015 at 05:48:44PM +0200, Jiri Olsa wrote:
> > > On Tue, Mar 31, 2015 at 11:57:39AM -0300, Arnaldo Carvalho de Melo wrote:
> > > 
> > > SNIP
> > > 
> > > >  	struct ordered_events *oe = &session->ordered_events;
> > > > -	struct perf_tool *tool = oe->tool;
> > > > +	struct perf_tool *tool = session->tool;
> > > >  	int fd = perf_data_file__fd(session->file);
> > > >  	u64 head, page_offset, file_offset, file_pos, size;
> > > >  	int err, mmap_prot, mmap_flags, map_idx = 0;
> > > > @@ -1394,6 +1407,7 @@ out:
> > > >  out_err:
> > > >  	ui_progress__finish();
> > > >  	perf_tool__warn_about_errors(tool, &session->evlist->stats);
> > > > +	ordered_events__warn_about_errors(&session->ordered_events);
> > > 
> > > how about just single warning function ?
> > > 
> > >   perf_session__warn_about_errors(struct perf_session *session)
> > > 
> > > session has now both tool and ordered_events
> > 
> > other than this it looks ok to me
> 
> Like this? If so, can I have your Acked-by? :-)

Out for lunch, put it on the tmp.perf/core branch in my repo,

bbl,
 
> - Arnaldo
> 
> From a6ee71dd2814dff89670b3764ac9fbb897366312 Mon Sep 17 00:00:00 2001
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date: Tue, 31 Mar 2015 12:48:16 -0300
> Subject: [PATCH 1/1] perf ordered_samples: Remove references to
>  perf_{evlist,tool} and machines
> 
> As these can be obtained from the ordered_events pointer, via
> container_of, reducing the cross section of ordered_samples.
> 
> These were added to ordered_samples in:
> 
>  commit b7b61cbebd789a3dbca522e3fdb727fe5c95593f
>  Author: Arnaldo Carvalho de Melo <acme@redhat.com>
>  Date:   Tue Mar 3 11:58:45 2015 -0300
> 
>     perf ordered_events: Shorten function signatures
> 
>     By keeping pointers to machines, evlist and tool in ordered_events.
> 
> But that was more a transitional patch while moving stuff out from
> perf_session.c to ordered_events.c and possibly not even needed by then,
> as we could use the container_of() method and instead of having the
> nr_unordered_samples stats in events_stats, we can have it in
> ordered_samples.
> 
> Based-on-a-patch-by: Jiri Olsa <jolsa@kernel.org>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Don Zickus <dzickus@redhat.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Stephane Eranian <eranian@google.com>
> Link: http://lkml.kernel.org/n/tip-4lk0t9js82g0tfc0x1onpkjt@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/util/event.h          |  1 -
>  tools/perf/util/ordered-events.c | 21 ++++--------------
>  tools/perf/util/ordered-events.h | 14 +++---------
>  tools/perf/util/session.c        | 47 +++++++++++++++++++++++++---------------
>  tools/perf/util/session.h        |  1 +
>  5 files changed, 37 insertions(+), 47 deletions(-)
> 
> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
> index c4ffe2bd0738..09b9e8d3fcf7 100644
> --- a/tools/perf/util/event.h
> +++ b/tools/perf/util/event.h
> @@ -242,7 +242,6 @@ struct events_stats {
>  	u32 nr_invalid_chains;
>  	u32 nr_unknown_id;
>  	u32 nr_unprocessable_samples;
> -	u32 nr_unordered_events;
>  };
>  
>  struct attr_event {
> diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c
> index 6002fa3fcf77..52be201b9b25 100644
> --- a/tools/perf/util/ordered-events.c
> +++ b/tools/perf/util/ordered-events.c
> @@ -2,7 +2,6 @@
>  #include <linux/compiler.h>
>  #include <linux/string.h>
>  #include "ordered-events.h"
> -#include "evlist.h"
>  #include "session.h"
>  #include "asm/bug.h"
>  #include "debug.h"
> @@ -167,7 +166,7 @@ int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
>  		pr_oe_time(oe->last_flush, "last flush, last_flush_type %d\n",
>  			   oe->last_flush_type);
>  
> -		oe->evlist->stats.nr_unordered_events++;
> +		oe->nr_unordered_events++;
>  	}
>  
>  	oevent = ordered_events__new_event(oe, timestamp, event);
> @@ -187,7 +186,6 @@ static int __ordered_events__flush(struct ordered_events *oe)
>  {
>  	struct list_head *head = &oe->events;
>  	struct ordered_event *tmp, *iter;
> -	struct perf_sample sample;
>  	u64 limit = oe->next_flush;
>  	u64 last_ts = oe->last ? oe->last->timestamp : 0ULL;
>  	bool show_progress = limit == ULLONG_MAX;
> @@ -206,15 +204,9 @@ static int __ordered_events__flush(struct ordered_events *oe)
>  
>  		if (iter->timestamp > limit)
>  			break;
> -
> -		ret = perf_evlist__parse_sample(oe->evlist, iter->event, &sample);
> +		ret = oe->deliver(oe, iter);
>  		if (ret)
> -			pr_err("Can't parse sample, err = %d\n", ret);
> -		else {
> -			ret = oe->deliver(oe, iter, &sample);
> -			if (ret)
> -				return ret;
> -		}
> +			return ret;
>  
>  		ordered_events__delete(oe, iter);
>  		oe->last_flush = iter->timestamp;
> @@ -292,18 +284,13 @@ int ordered_events__flush(struct ordered_events *oe, enum oe_flush how)
>  	return err;
>  }
>  
> -void ordered_events__init(struct ordered_events *oe, struct machines *machines,
> -			  struct perf_evlist *evlist, struct perf_tool *tool,
> -			  ordered_events__deliver_t deliver)
> +void ordered_events__init(struct ordered_events *oe, ordered_events__deliver_t deliver)
>  {
>  	INIT_LIST_HEAD(&oe->events);
>  	INIT_LIST_HEAD(&oe->cache);
>  	INIT_LIST_HEAD(&oe->to_free);
>  	oe->max_alloc_size = (u64) -1;
>  	oe->cur_alloc_size = 0;
> -	oe->evlist	   = evlist;
> -	oe->machines	   = machines;
> -	oe->tool	   = tool;
>  	oe->deliver	   = deliver;
>  }
>  
> diff --git a/tools/perf/util/ordered-events.h b/tools/perf/util/ordered-events.h
> index 173e13f28c08..f403991e3bfd 100644
> --- a/tools/perf/util/ordered-events.h
> +++ b/tools/perf/util/ordered-events.h
> @@ -3,10 +3,7 @@
>  
>  #include <linux/types.h>
>  
> -struct perf_tool;
> -struct perf_evlist;
>  struct perf_sample;
> -struct machines;
>  
>  struct ordered_event {
>  	u64			timestamp;
> @@ -25,8 +22,7 @@ enum oe_flush {
>  struct ordered_events;
>  
>  typedef int (*ordered_events__deliver_t)(struct ordered_events *oe,
> -					 struct ordered_event *event,
> -					 struct perf_sample *sample);
> +					 struct ordered_event *event);
>  
>  struct ordered_events {
>  	u64			last_flush;
> @@ -39,13 +35,11 @@ struct ordered_events {
>  	struct list_head	to_free;
>  	struct ordered_event	*buffer;
>  	struct ordered_event	*last;
> -	struct machines		*machines;
> -	struct perf_evlist	*evlist;
> -	struct perf_tool	*tool;
>  	ordered_events__deliver_t deliver;
>  	int			buffer_idx;
>  	unsigned int		nr_events;
>  	enum oe_flush		last_flush_type;
> +	u32			nr_unordered_events;
>  	bool                    copy_on_queue;
>  };
>  
> @@ -53,9 +47,7 @@ int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
>  			  struct perf_sample *sample, u64 file_offset);
>  void ordered_events__delete(struct ordered_events *oe, struct ordered_event *event);
>  int ordered_events__flush(struct ordered_events *oe, enum oe_flush how);
> -void ordered_events__init(struct ordered_events *oe, struct machines *machines,
> -			  struct perf_evlist *evlsit, struct perf_tool *tool,
> -			  ordered_events__deliver_t deliver);
> +void ordered_events__init(struct ordered_events *oe, ordered_events__deliver_t deliver);
>  void ordered_events__free(struct ordered_events *oe);
>  
>  static inline
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 89c66797abe4..dfacf1d50162 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -93,11 +93,20 @@ static void perf_session__set_comm_exec(struct perf_session *session)
>  }
>  
>  static int ordered_events__deliver_event(struct ordered_events *oe,
> -					 struct ordered_event *event,
> -					 struct perf_sample *sample)
> +					 struct ordered_event *event)
>  {
> -	return machines__deliver_event(oe->machines, oe->evlist, event->event,
> -				       sample, oe->tool, event->file_offset);
> +	struct perf_sample sample;
> +	struct perf_session *session = container_of(oe, struct perf_session,
> +						    ordered_events);
> +	int ret = perf_evlist__parse_sample(session->evlist, event->event, &sample);
> +
> +	if (ret) {
> +		pr_err("Can't parse sample, err = %d\n", ret);
> +		return ret;
> +	}
> +
> +	return machines__deliver_event(&session->machines, session->evlist, event->event,
> +				       &sample, session->tool, event->file_offset);
>  }
>  
>  struct perf_session *perf_session__new(struct perf_data_file *file,
> @@ -109,9 +118,9 @@ struct perf_session *perf_session__new(struct perf_data_file *file,
>  		goto out;
>  
>  	session->repipe = repipe;
> +	session->tool   = tool;
>  	machines__init(&session->machines);
> -	ordered_events__init(&session->ordered_events, &session->machines,
> -			     session->evlist, tool, ordered_events__deliver_event);
> +	ordered_events__init(&session->ordered_events, ordered_events__deliver_event);
>  
>  	if (file) {
>  		if (perf_data_file__open(file))
> @@ -940,7 +949,7 @@ static s64 perf_session__process_user_event(struct perf_session *session,
>  					    u64 file_offset)
>  {
>  	struct ordered_events *oe = &session->ordered_events;
> -	struct perf_tool *tool = oe->tool;
> +	struct perf_tool *tool = session->tool;
>  	int fd = perf_data_file__fd(session->file);
>  	int err;
>  
> @@ -981,7 +990,7 @@ int perf_session__deliver_synth_event(struct perf_session *session,
>  				      struct perf_sample *sample)
>  {
>  	struct perf_evlist *evlist = session->evlist;
> -	struct perf_tool *tool = session->ordered_events.tool;
> +	struct perf_tool *tool = session->tool;
>  
>  	events_stats__inc(&evlist->stats, event->header.type);
>  
> @@ -1059,7 +1068,7 @@ static s64 perf_session__process_event(struct perf_session *session,
>  				       union perf_event *event, u64 file_offset)
>  {
>  	struct perf_evlist *evlist = session->evlist;
> -	struct perf_tool *tool = session->ordered_events.tool;
> +	struct perf_tool *tool = session->tool;
>  	struct perf_sample sample;
>  	int ret;
>  
> @@ -1116,10 +1125,12 @@ static struct thread *perf_session__register_idle_thread(struct perf_session *se
>  	return thread;
>  }
>  
> -static void perf_tool__warn_about_errors(const struct perf_tool *tool,
> -					 const struct events_stats *stats)
> +static void perf_session__warn_about_errors(const struct perf_session *session)
>  {
> -	if (tool->lost == perf_event__process_lost &&
> +	const struct events_stats *stats = &session->evlist->stats;
> +	const struct ordered_events *oe = &session->ordered_events;
> +
> +	if (session->tool->lost == perf_event__process_lost &&
>  	    stats->nr_events[PERF_RECORD_LOST] != 0) {
>  		ui__warning("Processed %d events and lost %d chunks!\n\n"
>  			    "Check IO/CPU overload!\n\n",
> @@ -1155,8 +1166,8 @@ static void perf_tool__warn_about_errors(const struct perf_tool *tool,
>  			    stats->nr_unprocessable_samples);
>  	}
>  
> -	if (stats->nr_unordered_events != 0)
> -		ui__warning("%u out of order events recorded.\n", stats->nr_unordered_events);
> +	if (oe->nr_unordered_events != 0)
> +		ui__warning("%u out of order events recorded.\n", oe->nr_unordered_events);
>  }
>  
>  volatile int session_done;
> @@ -1164,7 +1175,7 @@ volatile int session_done;
>  static int __perf_session__process_pipe_events(struct perf_session *session)
>  {
>  	struct ordered_events *oe = &session->ordered_events;
> -	struct perf_tool *tool = oe->tool;
> +	struct perf_tool *tool = session->tool;
>  	int fd = perf_data_file__fd(session->file);
>  	union perf_event *event;
>  	uint32_t size, cur_size = 0;
> @@ -1247,7 +1258,7 @@ done:
>  	err = ordered_events__flush(oe, OE_FLUSH__FINAL);
>  out_err:
>  	free(buf);
> -	perf_tool__warn_about_errors(tool, &session->evlist->stats);
> +	perf_session__warn_about_errors(session);
>  	ordered_events__free(&session->ordered_events);
>  	return err;
>  }
> @@ -1297,7 +1308,7 @@ static int __perf_session__process_events(struct perf_session *session,
>  					  u64 file_size)
>  {
>  	struct ordered_events *oe = &session->ordered_events;
> -	struct perf_tool *tool = oe->tool;
> +	struct perf_tool *tool = session->tool;
>  	int fd = perf_data_file__fd(session->file);
>  	u64 head, page_offset, file_offset, file_pos, size;
>  	int err, mmap_prot, mmap_flags, map_idx = 0;
> @@ -1393,7 +1404,7 @@ out:
>  	err = ordered_events__flush(oe, OE_FLUSH__FINAL);
>  out_err:
>  	ui_progress__finish();
> -	perf_tool__warn_about_errors(tool, &session->evlist->stats);
> +	perf_session__warn_about_errors(session);
>  	ordered_events__free(&session->ordered_events);
>  	session->one_mmap = false;
>  	return err;
> diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
> index 1310998f8318..d5fa7b7916ef 100644
> --- a/tools/perf/util/session.h
> +++ b/tools/perf/util/session.h
> @@ -26,6 +26,7 @@ struct perf_session {
>  	u64			one_mmap_offset;
>  	struct ordered_events	ordered_events;
>  	struct perf_data_file	*file;
> +	struct perf_tool	*tool;
>  };
>  
>  #define PRINT_IP_OPT_IP		(1<<0)
> -- 
> 1.8.3.1
> 

Re: [BUG] perf script segfault

[Jiri Olsa]

On Tue, Mar 31, 2015 at 01:14:20PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Mar 31, 2015 at 05:50:48PM +0200, Jiri Olsa escreveu:
> > On Tue, Mar 31, 2015 at 05:48:44PM +0200, Jiri Olsa wrote:
> > > On Tue, Mar 31, 2015 at 11:57:39AM -0300, Arnaldo Carvalho de Melo wrote:
> > > 
> > > SNIP
> > > 
> > > >  	struct ordered_events *oe = &session->ordered_events;
> > > > -	struct perf_tool *tool = oe->tool;
> > > > +	struct perf_tool *tool = session->tool;
> > > >  	int fd = perf_data_file__fd(session->file);
> > > >  	u64 head, page_offset, file_offset, file_pos, size;
> > > >  	int err, mmap_prot, mmap_flags, map_idx = 0;
> > > > @@ -1394,6 +1407,7 @@ out:
> > > >  out_err:
> > > >  	ui_progress__finish();
> > > >  	perf_tool__warn_about_errors(tool, &session->evlist->stats);
> > > > +	ordered_events__warn_about_errors(&session->ordered_events);
> > > 
> > > how about just single warning function ?
> > > 
> > >   perf_session__warn_about_errors(struct perf_session *session)
> > > 
> > > session has now both tool and ordered_events
> > 
> > other than this it looks ok to me
> 
> Like this? If so, can I have your Acked-by? :-)

I luv it! ;-) thanks

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

Re: [BUG] perf script segfault

[David Ahern]

what happened to this patch? not in your core or urgent branches and 
perf-script is still bombing

On 3/31/15 7:36 AM, David Ahern wrote:
> On 3/31/15 6:59 AM, Arnaldo Carvalho de Melo wrote:
>> Em Mon, Mar 30, 2015 at 08:45:33PM -0300, Arnaldo Carvalho de Melo
>> escreveu:
>>> Em Mon, Mar 30, 2015 at 04:51:34PM -0600, David Ahern escreveu:
>>>> tool was moved to ordered_events and is not initialized for pipe
>>>> mode. I
>>>> don't have time to look into it more than that before PTO on Wednesday.
>>
>>> I guess this one is enough, no? Checking with your example...
>>
>> So the following is better, can you give it a try, please?
>>
>> - Arnaldo
>>
>>
>>  From fbd7d154f01c47db71a3d8b0546911872aa1de54 Mon Sep 17 00:00:00 2001
>> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Date: Tue, 31 Mar 2015 09:53:50 -0300
>> Subject: [PATCH 1/1] perf session: Always initialize ordered_events
>>
>> Even when it is not used to actually reorder events, some of its fields
>> are used, like session->ordered_events->tool, to shorten function
>> signatures where tool, for instance, was being passed, as the tool is
>> needed for the ordered_events code, we need it there and might as well
>> use it for other perf_session needs.
>>
>> This fixes a problem where 'perf script' had some condition that made
>> session->ordered_events not to be initialized even with its
>> script->tool ordered_events related flags asking for it to be, which
>> looks like another bug and needs to be investigated further.
>>
>> Always initializing session->ordered_events at least leaves the current
>> assumptions in place, so do it now.
>>
>> Reported-by: David Ahern <dsahern@gmail.com>
>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>> Cc: Borislav Petkov <bp@suse.de>
>> Cc: Don Zickus <dzickus@redhat.com>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Stephane Eranian <eranian@google.com>
>> Link:
>> http://lkml.kernel.org/n/tip-b1xxk0rwkz2a0gip1uufmjqg@git.kernel.org
>> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>> ---
>>   tools/perf/util/session.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
>> index adf0740c563b..89c66797abe4 100644
>> --- a/tools/perf/util/session.c
>> +++ b/tools/perf/util/session.c
>> @@ -110,6 +110,8 @@ struct perf_session *perf_session__new(struct
>> perf_data_file *file,
>>
>>       session->repipe = repipe;
>>       machines__init(&session->machines);
>> +    ordered_events__init(&session->ordered_events, &session->machines,
>> +                 session->evlist, tool, ordered_events__deliver_event);
>>
>>       if (file) {
>>           if (perf_data_file__open(file))
>> @@ -139,9 +141,6 @@ struct perf_session *perf_session__new(struct
>> perf_data_file *file,
>>           tool->ordered_events &&
>> !perf_evlist__sample_id_all(session->evlist)) {
>>           dump_printf("WARNING: No sample_id_all support, falling back
>> to unordered processing\n");
>>           tool->ordered_events = false;
>> -    } else {
>> -        ordered_events__init(&session->ordered_events,
>> &session->machines,
>> -                     session->evlist, tool,
>> ordered_events__deliver_event);
>>       }
>>
>>       return session;
>>
>
> Works for me. Reviewed-and-Tested-by: David Ahern <dsahern@gmail.com>

Re: [BUG] perf script segfault

[David Ahern]

On 4/7/15 5:41 PM, David Ahern wrote:
> what happened to this patch? not in your core or urgent branches and
> perf-script is still bombing

d'oh. never mind; user error.