[PATCH 0/3] perf tools: Fix buildid processing

[PATCH 0/3] perf tools: Fix buildid processing

[Adrian Hunter]

Hi

Here are 3 patches related to the "getting stuck at end" problem.

The first patch dodges the deadlock.

The second patch stops fork from messing up i.e.
prevents the deadlock condition from arising in
the first place.

The third patch processes the data in order i.e.
prevents fork from having to deal with weird data
anyway.


Adrian Hunter (3):
      perf tools: Avoid deadlock when map_groups are broken
      perf tools: Make fork event processing more resilient
      perf tools: Fix buildid processing

 tools/perf/builtin-record.c | 11 +++++++++++
 tools/perf/util/machine.c   | 20 ++++++++++++++++++--
 tools/perf/util/thread.c    |  6 ++++++
 3 files changed, 35 insertions(+), 2 deletions(-)


Regards
Adrian

[PATCH 1/3] perf tools: Avoid deadlock when map_groups are broken

[Adrian Hunter]

Attempting to clone map groups onto themselves will deadlock.
It only happens because of other bugs, but the code should
protect itself anyway.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/thread.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 28c4b746baa1..471102554a78 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -191,6 +191,12 @@ static int thread__clone_map_groups(struct thread *thread,
 	if (thread->pid_ == parent->pid_)
 		return 0;
 
+	if (thread->mg == parent->mg) {
+		dump_printf("broken map groups on thread %d/%d parent %d/%d\n",
+			    thread->pid_, thread->tid, parent->pid_, parent->tid);
+		return 0;
+	}
+
 	/* But this one is new process, copy maps. */
 	for (i = 0; i < MAP__NR_TYPES; ++i)
 		if (map_groups__clone(thread->mg, parent->mg, i) < 0)
-- 
1.9.1

[tip:perf/core] perf tools: Avoid deadlock when map_groups are broken

[tip-bot for Adrian Hunter]

Commit-ID:  0d7e7acc47133e82911e24758178815cfee309e9
Gitweb:     http://git.kernel.org/tip/0d7e7acc47133e82911e24758178815cfee309e9
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Wed, 19 Aug 2015 17:29:19 +0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 19 Aug 2015 14:15:09 -0300

perf tools: Avoid deadlock when map_groups are broken

Attempting to clone map groups onto themselves will deadlock.

It only happens because of other bugs, but the code should protect
itself anyway.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Tested-by: Jiri Olsa <jolsa@kernel.org>
Link: http://lkml.kernel.org/r/1439994561-27436-2-git-send-email-adrian.hunter@intel.com
[ Use pr_debug() instead of dump_fprintf() ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/thread.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 28c4b74..0a9ae80 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -191,6 +191,12 @@ static int thread__clone_map_groups(struct thread *thread,
 	if (thread->pid_ == parent->pid_)
 		return 0;
 
+	if (thread->mg == parent->mg) {
+		pr_debug("broken map groups on thread %d/%d parent %d/%d\n",
+			 thread->pid_, thread->tid, parent->pid_, parent->tid);
+		return 0;
+	}
+
 	/* But this one is new process, copy maps. */
 	for (i = 0; i < MAP__NR_TYPES; ++i)
 		if (map_groups__clone(thread->mg, parent->mg, i) < 0)

[PATCH 2/3] perf tools: Make fork event processing more resilient

[Adrian Hunter]

When processing a fork event, the tools lookup the parent
thread by its tid.  In a couple of cases, it is possible
for that thread to have the wrong pid.  That can happen
if the data is being processed out of order, or if the
(fork) event that would have removed the erroneous
thread was lost.  Assume the latter case, print a dump
message, remove the erroneous thread, create a new one
with the correct pid, and keep going.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/machine.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 7ff682770fdb..f1a4c833121e 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1387,6 +1387,24 @@ int machine__process_fork_event(struct machine *machine, union perf_event *event
 							event->fork.ptid);
 	int err = 0;
 
+	if (dump_trace)
+		perf_event__fprintf_task(event, stdout);
+
+	/*
+	 * There may be an existing thread that is not actually the parent,
+	 * either because we are processing events out of order, or because the
+	 * (fork) event that would have removed the thread was lost. Assume the
+	 * latter case and continue on as best we can.
+	 */
+	if (parent->pid_ != (pid_t)event->fork.ppid) {
+		dump_printf("removing erroneous parent thread %d/%d\n",
+			    parent->pid_, parent->tid);
+		machine__remove_thread(machine, parent);
+		thread__put(parent);
+		parent = machine__findnew_thread(machine, event->fork.ppid,
+						 event->fork.ptid);
+	}
+
 	/* if a thread currently exists for the thread id remove it */
 	if (thread != NULL) {
 		machine__remove_thread(machine, thread);
@@ -1395,8 +1413,6 @@ int machine__process_fork_event(struct machine *machine, union perf_event *event
 
 	thread = machine__findnew_thread(machine, event->fork.pid,
 					 event->fork.tid);
-	if (dump_trace)
-		perf_event__fprintf_task(event, stdout);
 
 	if (thread == NULL || parent == NULL ||
 	    thread__fork(thread, parent, sample->time) < 0) {
-- 
1.9.1

[tip:perf/core] perf tools: Make fork event processing more resilient

[tip-bot for Adrian Hunter]

Commit-ID:  5cb73340d92a716fd2776700742c3558206ae298
Gitweb:     http://git.kernel.org/tip/5cb73340d92a716fd2776700742c3558206ae298
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Wed, 19 Aug 2015 17:29:20 +0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 19 Aug 2015 14:15:25 -0300

perf tools: Make fork event processing more resilient

When processing a fork event, the tools lookup the parent thread by its
tid.  In a couple of cases, it is possible for that thread to have the
wrong pid.

That can happen if the data is being processed out of order, or if the
(fork) event that would have removed the erroneous thread was lost.

Assume the latter case, print a dump message, remove the erroneous
thread, create a new one with the correct pid, and keep going.

Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Tested-by: Jiri Olsa <jolsa@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/1439994561-27436-3-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 7ff6827..f1a4c83 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1387,6 +1387,24 @@ int machine__process_fork_event(struct machine *machine, union perf_event *event
 							event->fork.ptid);
 	int err = 0;
 
+	if (dump_trace)
+		perf_event__fprintf_task(event, stdout);
+
+	/*
+	 * There may be an existing thread that is not actually the parent,
+	 * either because we are processing events out of order, or because the
+	 * (fork) event that would have removed the thread was lost. Assume the
+	 * latter case and continue on as best we can.
+	 */
+	if (parent->pid_ != (pid_t)event->fork.ppid) {
+		dump_printf("removing erroneous parent thread %d/%d\n",
+			    parent->pid_, parent->tid);
+		machine__remove_thread(machine, parent);
+		thread__put(parent);
+		parent = machine__findnew_thread(machine, event->fork.ppid,
+						 event->fork.ptid);
+	}
+
 	/* if a thread currently exists for the thread id remove it */
 	if (thread != NULL) {
 		machine__remove_thread(machine, thread);
@@ -1395,8 +1413,6 @@ int machine__process_fork_event(struct machine *machine, union perf_event *event
 
 	thread = machine__findnew_thread(machine, event->fork.pid,
 					 event->fork.tid);
-	if (dump_trace)
-		perf_event__fprintf_task(event, stdout);
 
 	if (thread == NULL || parent == NULL ||
 	    thread__fork(thread, parent, sample->time) < 0) {

[PATCH 3/3] perf tools: Fix buildid processing

[Adrian Hunter]

After recording, 'perf record' post-processes the data to
determine which buildids are needed. That processing must
process the data in time order, if possible, because
otherwise dependent events, like forks and mmaps, will
not make sense.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/builtin-record.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index de165a1b9240..f36b88938499 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -500,6 +500,15 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 		return -1;
 	}
 
+	/*
+	 * Normally perf_session__new would do this, but it doesn't have the
+	 * evlist.
+	 */
+	if (rec->tool.ordered_events && !perf_evlist__sample_id_all(rec->evlist)) {
+		dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
+		rec->tool.ordered_events = false;
+	}
+
 	fd = perf_data_file__fd(file);
 	rec->session = session;
 
@@ -965,9 +974,11 @@ static struct record record = {
 	.tool = {
 		.sample		= process_sample_event,
 		.fork		= perf_event__process_fork,
+		.exit		= perf_event__process_exit,
 		.comm		= perf_event__process_comm,
 		.mmap		= perf_event__process_mmap,
 		.mmap2		= perf_event__process_mmap2,
+		.ordered_events	= true,
 	},
 };
 
-- 
1.9.1

Re: [PATCH 3/3] perf tools: Fix buildid processing

[Adrian Hunter]

On 19/08/2015 5:29 p.m., Adrian Hunter wrote:
> After recording, 'perf record' post-processes the data to
> determine which buildids are needed. That processing must
> process the data in time order, if possible, because
> otherwise dependent events, like forks and mmaps, will
> not make sense.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>   tools/perf/builtin-record.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index de165a1b9240..f36b88938499 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -500,6 +500,15 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>   		return -1;
>   	}
>
> +	/*
> +	 * Normally perf_session__new would do this, but it doesn't have the
> +	 * evlist.
> +	 */
> +	if (rec->tool.ordered_events && !perf_evlist__sample_id_all(rec->evlist)) {
> +		dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
> +		rec->tool.ordered_events = false;
> +	}
> +

In fact this chunk is misplaced, it needs to be below the call to record__open()
after which the attributes are correctly configured.  And dump_printf() is no good
for 'perf record', pr_debug would be better. Don't have time to do that right now,
I'll do it tommorow unless Arnaldo fixes it up.

>   	fd = perf_data_file__fd(file);
>   	rec->session = session;
>
> @@ -965,9 +974,11 @@ static struct record record = {
>   	.tool = {
>   		.sample		= process_sample_event,
>   		.fork		= perf_event__process_fork,
> +		.exit		= perf_event__process_exit,
>   		.comm		= perf_event__process_comm,
>   		.mmap		= perf_event__process_mmap,
>   		.mmap2		= perf_event__process_mmap2,
> +		.ordered_events	= true,
>   	},
>   };
>
>

Re: [PATCH 3/3] perf tools: Fix buildid processing

[Arnaldo Carvalho de Melo]

Em Wed, Aug 19, 2015 at 06:23:51PM +0300, Adrian Hunter escreveu:
> On 19/08/2015 5:29 p.m., Adrian Hunter wrote:
> >After recording, 'perf record' post-processes the data to
> >determine which buildids are needed. That processing must
> >process the data in time order, if possible, because
> >otherwise dependent events, like forks and mmaps, will
> >not make sense.
> >
> >Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> >---
> >  tools/perf/builtin-record.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> >diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> >index de165a1b9240..f36b88938499 100644
> >--- a/tools/perf/builtin-record.c
> >+++ b/tools/perf/builtin-record.c
> >@@ -500,6 +500,15 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
> >  		return -1;
> >  	}
> >
> >+	/*
> >+	 * Normally perf_session__new would do this, but it doesn't have the
> >+	 * evlist.
> >+	 */
> >+	if (rec->tool.ordered_events && !perf_evlist__sample_id_all(rec->evlist)) {
> >+		dump_printf("WARNING: No sample_id_all support, falling back to unordered processing\n");
> >+		rec->tool.ordered_events = false;
> >+	}
> >+
> 
> In fact this chunk is misplaced, it needs to be below the call to record__open()
> after which the attributes are correctly configured.  And dump_printf() is no good
> for 'perf record', pr_debug would be better. Don't have time to do that right now,
> I'll do it tommorow unless Arnaldo fixes it up.

I'll fix it up, thanks!

- Arnaldo
 
> >  	fd = perf_data_file__fd(file);
> >  	rec->session = session;
> >
> >@@ -965,9 +974,11 @@ static struct record record = {
> >  	.tool = {
> >  		.sample		= process_sample_event,
> >  		.fork		= perf_event__process_fork,
> >+		.exit		= perf_event__process_exit,
> >  		.comm		= perf_event__process_comm,
> >  		.mmap		= perf_event__process_mmap,
> >  		.mmap2		= perf_event__process_mmap2,
> >+		.ordered_events	= true,
> >  	},
> >  };
> >
> >

[tip:perf/core] perf tools: Fix buildid processing

[tip-bot for Adrian Hunter]

Commit-ID:  cca8482c0651cea97aade58cc22109ce9fffbfa2
Gitweb:     http://git.kernel.org/tip/cca8482c0651cea97aade58cc22109ce9fffbfa2
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Wed, 19 Aug 2015 17:29:21 +0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 19 Aug 2015 14:15:26 -0300

perf tools: Fix buildid processing

After recording, 'perf record' post-processes the data to determine
which buildids are needed.

That processing must process the data in time order, if possible,
because otherwise dependent events, like forks and mmaps, will not make
sense.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Tested-by: Jiri Olsa <jolsa@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/1439994561-27436-4-git-send-email-adrian.hunter@intel.com
[ Moved the sample_id_add to after trying to open the events, use pr_warning ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index de165a1..20b56eb 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -521,6 +521,15 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 		goto out_child;
 	}
 
+	/*
+	 * Normally perf_session__new would do this, but it doesn't have the
+	 * evlist.
+	 */
+	if (rec->tool.ordered_events && !perf_evlist__sample_id_all(rec->evlist)) {
+		pr_warning("WARNING: No sample_id_all support, falling back to unordered processing\n");
+		rec->tool.ordered_events = false;
+	}
+
 	if (!rec->evlist->nr_groups)
 		perf_header__clear_feat(&session->header, HEADER_GROUP_DESC);
 
@@ -965,9 +974,11 @@ static struct record record = {
 	.tool = {
 		.sample		= process_sample_event,
 		.fork		= perf_event__process_fork,
+		.exit		= perf_event__process_exit,
 		.comm		= perf_event__process_comm,
 		.mmap		= perf_event__process_mmap,
 		.mmap2		= perf_event__process_mmap2,
+		.ordered_events	= true,
 	},
 };
 

Re: [PATCH 0/3] perf tools: Fix buildid processing

[Jiri Olsa]

On Wed, Aug 19, 2015 at 05:29:18PM +0300, Adrian Hunter wrote:
> Hi
> 
> Here are 3 patches related to the "getting stuck at end" problem.
> 
> The first patch dodges the deadlock.
> 
> The second patch stops fork from messing up i.e.
> prevents the deadlock condition from arising in
> the first place.
> 
> The third patch processes the data in order i.e.
> prevents fork from having to deal with weird data
> anyway.

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

thanks,
jirka