Re: [PATCH] Fix setting of app_started in ocount

Will:

I opened OProfile bug 290 for this issue so it can be tracked.

                Carl Love
----------------------------------------------------

On Wed, 2019-07-10 at 10:46 -0700, Carl Love wrote:
> 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;



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