[PATCH] Fix setting of app_started in ocount

Will:

We had a user report orphaned processes left behind by ocount when
it failed to start correctly.  Specifically, they noted the error
message was:

  ocount ./test64
  perf_event_open failed with Permission denied
  Caught runtime error while setting up counters
  Internal Error.  Perf event setup failed.
  Error running ocount

I was able to reproduce the error by forcing the op_perf_even_open() to
fail with the following patch.

---
 pe_counting/ocount_counter.cpp | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/pe_counting/ocount_counter.cpp b/pe_counting/ocount_counter.cpp
index 712c229..3926428 100644
--- a/pe_counting/ocount_counter.cpp
+++ b/pe_counting/ocount_counter.cpp
@@ -98,6 +98,9 @@ ocount_counter::~ocount_counter() {
 int ocount_counter::perf_event_open(pid_t _pid, int _cpu)
 {
 	fd = op_perf_event_open(&attr, _pid, _cpu, -1, 0);
+   fd = -99;   // carll force error
+   cerr << "carll, forced error\n";
+
 	if (fd < 0) {
 		int ret = -1;
 		cverb << vdebug << "perf_event_open failed: " << strerror(errno) << endl;
-- 

The orphaned process seems to be the result of a commit that changed the order
of starting the application and perf events.  The issue is described in the
patch below that fixes the issue.  Would you please review the patch and see if
you agree with my analysis of the issue and if this patch is a correct fix.  

Thank you.

                   Carl Love

----------------------------------------------------------------------
[PATCH] Fix setting of app_started in ocount

I believe the following commit introduced an error that causes
ocount to leave orphaned processes running.

The commit
  commit 249fe0a4bb69e5bd2e9ee0a0667d925a86d4337c
    Author: William Cohen <[email protected]>
    Date:   Tue Aug 9 22:25:52 2016 -0400

        Only start the application if the perf events setup was successful

The patch changes the order of starting the application and performance
events. Given this change we have a new issue.  The issue is the
routine start_counting() calls fork, creating app_PID process. The parent
then tries to setup the performance events, then if the performance events
were setup correctly, app_PID is then told to start before exiting.  If the
performance counter setup fails, the app_PID is left running.  The app_PID
is never told to start the workload, which is correct, but we don't record
the fact that app_PID is running.  The error path then fails to kill app_PID
in routine main(), in file oprofile-git/pe_counting, at about line 909
because the if statement:

 if (startApp && app_started && (run_result != APP_ABNORMAL_END)) {

is false because app_started is False.

The fix, I believe, is to set app_started to True in the parent code if
the fork was successful.  With this fix, there is no orphan processes
left after ocount exits.  Currently, app_started is only set to True if
the performance counter events were successfully setup.
---
 pe_counting/ocount.cpp | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/pe_counting/ocount.cpp b/pe_counting/ocount.cpp
index 7717717..3e59120 100644
--- a/pe_counting/ocount.cpp
+++ b/pe_counting/ocount.cpp
@@ -242,6 +242,11 @@ bool start_counting(void)
 
 	// parent
 	int startup;
+
+	if ( app_PID != -1)
+		// app_PID child process created successfully
+		app_started = true;
+
 	if (startApp) {
 		if (read(app_ready_pipe[0], &startup, sizeof(startup)) == -1) {
 			perror("Internal error on app_ready_pipe");
@@ -297,7 +302,6 @@ bool start_counting(void)
 			perror("Internal error on start_app_pipe");
 			return false;
 		}
-		app_started = true;
 	}
 
 	return ret;
-- 
1.8.3.1



_______________________________________________
oprofile-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/oprofile-list