[v3,3/9] perf stat: factor out event handling loop into a function
diff mbox series

Message ID dcb0b0f5-4c92-3027-a508-c3946c51f3d1@linux.intel.com
State New
Headers show
Series
  • perf: support enable and disable commands in stat and record modes
Related show

Commit Message

Alexey Budankov May 13, 2020, 8 a.m. UTC
Factor out event handling loop into handle_events() function.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/perf/builtin-stat.c | 85 +++++++++++++++++++++++----------------
 1 file changed, 50 insertions(+), 35 deletions(-)

Comments

Jiri Olsa May 20, 2020, 12:38 p.m. UTC | #1
On Wed, May 13, 2020 at 11:00:47AM +0300, Alexey Budankov wrote:
> 
> Factor out event handling loop into handle_events() function.
> 
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> ---
>  tools/perf/builtin-stat.c | 85 +++++++++++++++++++++++----------------
>  1 file changed, 50 insertions(+), 35 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index e0c1ad23c768..9775b0905146 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -371,6 +371,16 @@ static void process_interval(void)
>  	print_counters(&rs, 0, NULL);
>  }
>  
> +static bool print_interval_and_stop(struct perf_stat_config *config, int *times)
> +{
> +	if (config->interval) {
> +		process_interval();
> +		if (interval_count && !(--(*times)))
> +			return true;
> +	}
> +	return false;
> +}
> +
>  static void enable_counters(void)
>  {
>  	if (stat_config.initial_delay)
> @@ -436,6 +446,42 @@ static bool is_target_alive(struct target *_target,
>  	return false;
>  }
>  
> +static int handle_events(pid_t pid, struct perf_stat_config *config)
> +{
> +	pid_t child = 0;
> +	bool res, stop = false;
> +	struct timespec time_to_sleep;
> +	int sleep_time, status = 0, times = config->times;
> +
> +	if (config->interval)
> +		sleep_time = config->interval;
> +	else if (config->timeout)
> +		sleep_time = config->timeout;
> +	else
> +		sleep_time = 1000;
> +
> +	time_to_sleep.tv_sec  = sleep_time / MSEC_PER_SEC;
> +	time_to_sleep.tv_nsec = (sleep_time % MSEC_PER_SEC) * NSEC_PER_MSEC;
> +
> +	do {
> +		if (pid != -1)
> +			child = waitpid(pid, &status, WNOHANG);
> +		if (child || stop || done)
> +			break;
> +		nanosleep(&time_to_sleep, NULL);
> +		if (pid == -1)
> +			stop = !is_target_alive(&target, evsel_list->core.threads);
> +		if (config->timeout) {
> +			stop = !stop ? true : stop;
> +		} else {
> +			res = print_interval_and_stop(config, &times);
> +			stop = !stop ? res : stop;
> +		}
> +	} while (1);
> +
> +	return status;
> +}
> +
>  enum counter_recovery {
>  	COUNTER_SKIP,
>  	COUNTER_RETRY,
> @@ -494,12 +540,10 @@ static enum counter_recovery stat_handle_error(struct evsel *counter)
>  static int __run_perf_stat(int argc, const char **argv, int run_idx)
>  {
>  	int interval = stat_config.interval;
> -	int times = stat_config.times;
>  	int timeout = stat_config.timeout;
>  	char msg[BUFSIZ];
>  	unsigned long long t0, t1;
>  	struct evsel *counter;
> -	struct timespec ts;
>  	size_t l;
>  	int status = 0;
>  	const bool forks = (argc > 0);
> @@ -508,17 +552,6 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>  	int i, cpu;
>  	bool second_pass = false;
>  
> -	if (interval) {
> -		ts.tv_sec  = interval / USEC_PER_MSEC;
> -		ts.tv_nsec = (interval % USEC_PER_MSEC) * NSEC_PER_MSEC;
> -	} else if (timeout) {
> -		ts.tv_sec  = timeout / USEC_PER_MSEC;
> -		ts.tv_nsec = (timeout % USEC_PER_MSEC) * NSEC_PER_MSEC;
> -	} else {
> -		ts.tv_sec  = 1;
> -		ts.tv_nsec = 0;
> -	}
> -
>  	if (forks) {
>  		if (perf_evlist__prepare_workload(evsel_list, &target, argv, is_pipe,
>  						  workload_exec_failed_signal) < 0) {
> @@ -675,16 +708,9 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>  		perf_evlist__start_workload(evsel_list);
>  		enable_counters();
>  
> -		if (interval || timeout) {
> -			while (!waitpid(child_pid, &status, WNOHANG)) {
> -				nanosleep(&ts, NULL);
> -				if (timeout)
> -					break;
> -				process_interval();
> -				if (interval_count && !(--times))
> -					break;
> -			}
> -		}
> +		if (interval || timeout)
> +			handle_events(child_pid, &stat_config);
> +
>  		if (child_pid != -1) {
>  			if (timeout)
>  				kill(child_pid, SIGTERM);
> @@ -701,18 +727,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>  			psignal(WTERMSIG(status), argv[0]);
>  	} else {
>  		enable_counters();
> -		while (!done) {
> -			nanosleep(&ts, NULL);
> -			if (!is_target_alive(&target, evsel_list->core.threads))
> -				break;
> -			if (timeout)
> -				break;
> -			if (interval) {
> -				process_interval();
> -				if (interval_count && !(--times))
> -					break;
> -			}
> -		}
> +		handle_events(-1, &stat_config);

this makes me worried.. I'm not sure if it's good idea
to squash these 2 looops into one, because they are already
complex as they are.. and one of you following patches is
making it even more complex

wouldn't it be better if you just add single call into
each of them.. that would poll on your fd and process the
commands if needed?

jirka
Alexey Budankov May 20, 2020, 3:17 p.m. UTC | #2
On 20.05.2020 15:38, Jiri Olsa wrote:
> On Wed, May 13, 2020 at 11:00:47AM +0300, Alexey Budankov wrote:
>>
>> Factor out event handling loop into handle_events() function.
>>
>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>> ---
>>  tools/perf/builtin-stat.c | 85 +++++++++++++++++++++++----------------
>>  1 file changed, 50 insertions(+), 35 deletions(-)
>>
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index e0c1ad23c768..9775b0905146 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -371,6 +371,16 @@ static void process_interval(void)
>>  	print_counters(&rs, 0, NULL);
>>  }
>>  
>> +static bool print_interval_and_stop(struct perf_stat_config *config, int *times)
>> +{
>> +	if (config->interval) {
>> +		process_interval();
>> +		if (interval_count && !(--(*times)))
>> +			return true;
>> +	}
>> +	return false;
>> +}
>> +
>>  static void enable_counters(void)
>>  {
>>  	if (stat_config.initial_delay)
>> @@ -436,6 +446,42 @@ static bool is_target_alive(struct target *_target,
>>  	return false;
>>  }
>>  
>> +static int handle_events(pid_t pid, struct perf_stat_config *config)
>> +{
>> +	pid_t child = 0;
>> +	bool res, stop = false;
>> +	struct timespec time_to_sleep;
>> +	int sleep_time, status = 0, times = config->times;
>> +
>> +	if (config->interval)
>> +		sleep_time = config->interval;
>> +	else if (config->timeout)
>> +		sleep_time = config->timeout;
>> +	else
>> +		sleep_time = 1000;
>> +
>> +	time_to_sleep.tv_sec  = sleep_time / MSEC_PER_SEC;
>> +	time_to_sleep.tv_nsec = (sleep_time % MSEC_PER_SEC) * NSEC_PER_MSEC;
>> +
>> +	do {
>> +		if (pid != -1)
>> +			child = waitpid(pid, &status, WNOHANG);
>> +		if (child || stop || done)
>> +			break;
>> +		nanosleep(&time_to_sleep, NULL);
>> +		if (pid == -1)
>> +			stop = !is_target_alive(&target, evsel_list->core.threads);
>> +		if (config->timeout) {
>> +			stop = !stop ? true : stop;
>> +		} else {
>> +			res = print_interval_and_stop(config, &times);
>> +			stop = !stop ? res : stop;
>> +		}
>> +	} while (1);
>> +
>> +	return status;
>> +}
>> +
>>  enum counter_recovery {
>>  	COUNTER_SKIP,
>>  	COUNTER_RETRY,
>> @@ -494,12 +540,10 @@ static enum counter_recovery stat_handle_error(struct evsel *counter)
>>  static int __run_perf_stat(int argc, const char **argv, int run_idx)
>>  {
>>  	int interval = stat_config.interval;
>> -	int times = stat_config.times;
>>  	int timeout = stat_config.timeout;
>>  	char msg[BUFSIZ];
>>  	unsigned long long t0, t1;
>>  	struct evsel *counter;
>> -	struct timespec ts;
>>  	size_t l;
>>  	int status = 0;
>>  	const bool forks = (argc > 0);
>> @@ -508,17 +552,6 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>>  	int i, cpu;
>>  	bool second_pass = false;
>>  
>> -	if (interval) {
>> -		ts.tv_sec  = interval / USEC_PER_MSEC;
>> -		ts.tv_nsec = (interval % USEC_PER_MSEC) * NSEC_PER_MSEC;
>> -	} else if (timeout) {
>> -		ts.tv_sec  = timeout / USEC_PER_MSEC;
>> -		ts.tv_nsec = (timeout % USEC_PER_MSEC) * NSEC_PER_MSEC;
>> -	} else {
>> -		ts.tv_sec  = 1;
>> -		ts.tv_nsec = 0;
>> -	}
>> -
>>  	if (forks) {
>>  		if (perf_evlist__prepare_workload(evsel_list, &target, argv, is_pipe,
>>  						  workload_exec_failed_signal) < 0) {
>> @@ -675,16 +708,9 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>>  		perf_evlist__start_workload(evsel_list);
>>  		enable_counters();
>>  
>> -		if (interval || timeout) {
>> -			while (!waitpid(child_pid, &status, WNOHANG)) {
>> -				nanosleep(&ts, NULL);
>> -				if (timeout)
>> -					break;
>> -				process_interval();
>> -				if (interval_count && !(--times))
>> -					break;
>> -			}
>> -		}
>> +		if (interval || timeout)
>> +			handle_events(child_pid, &stat_config);
>> +
>>  		if (child_pid != -1) {
>>  			if (timeout)
>>  				kill(child_pid, SIGTERM);
>> @@ -701,18 +727,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>>  			psignal(WTERMSIG(status), argv[0]);
>>  	} else {
>>  		enable_counters();
>> -		while (!done) {
>> -			nanosleep(&ts, NULL);
>> -			if (!is_target_alive(&target, evsel_list->core.threads))
>> -				break;
>> -			if (timeout)
>> -				break;
>> -			if (interval) {
>> -				process_interval();
>> -				if (interval_count && !(--times))
>> -					break;
>> -			}
>> -		}
>> +		handle_events(-1, &stat_config);
> 
> this makes me worried.. I'm not sure if it's good idea
> to squash these 2 looops into one, because they are already
> complex as they are.. and one of you following patches is
> making it even more complex

Loops bodies are mostly identical. The only difference is in events
they wait for and API used for that. Adding of more events will
complicate further. The code is duplicated, thus needs refactoring.
If the following patch complicates lets organize the patch it into
several smaller functions.

> 
> wouldn't it be better if you just add single call into
> each of them.. that would poll on your fd and process the
> commands if needed?

That's of course possible, but doesn't manage existing complexity
at the first place - __run_perf_stat().

Let's still have handle_events() as a general dispatcher and implement
handlers for different events as separate functions?

~Alexey
Jiri Olsa May 21, 2020, 11:48 a.m. UTC | #3
On Wed, May 20, 2020 at 06:17:40PM +0300, Alexey Budankov wrote:

SNIP

> >> @@ -675,16 +708,9 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> >>  		perf_evlist__start_workload(evsel_list);
> >>  		enable_counters();
> >>  
> >> -		if (interval || timeout) {
> >> -			while (!waitpid(child_pid, &status, WNOHANG)) {
> >> -				nanosleep(&ts, NULL);
> >> -				if (timeout)
> >> -					break;
> >> -				process_interval();
> >> -				if (interval_count && !(--times))
> >> -					break;
> >> -			}
> >> -		}
> >> +		if (interval || timeout)
> >> +			handle_events(child_pid, &stat_config);
> >> +
> >>  		if (child_pid != -1) {
> >>  			if (timeout)
> >>  				kill(child_pid, SIGTERM);
> >> @@ -701,18 +727,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> >>  			psignal(WTERMSIG(status), argv[0]);
> >>  	} else {
> >>  		enable_counters();
> >> -		while (!done) {
> >> -			nanosleep(&ts, NULL);
> >> -			if (!is_target_alive(&target, evsel_list->core.threads))
> >> -				break;
> >> -			if (timeout)
> >> -				break;
> >> -			if (interval) {
> >> -				process_interval();
> >> -				if (interval_count && !(--times))
> >> -					break;
> >> -			}
> >> -		}
> >> +		handle_events(-1, &stat_config);
> > 
> > this makes me worried.. I'm not sure if it's good idea
> > to squash these 2 looops into one, because they are already
> > complex as they are.. and one of you following patches is
> > making it even more complex
> 
> Loops bodies are mostly identical. The only difference is in events
> they wait for and API used for that. Adding of more events will
> complicate further. The code is duplicated, thus needs refactoring.
> If the following patch complicates lets organize the patch it into
> several smaller functions.

yea, that might help

jirka

> 
> > 
> > wouldn't it be better if you just add single call into
> > each of them.. that would poll on your fd and process the
> > commands if needed?
> 
> That's of course possible, but doesn't manage existing complexity
> at the first place - __run_perf_stat().
> 
> Let's still have handle_events() as a general dispatcher and implement
> handlers for different events as separate functions?
> 
> ~Alexey
>

Patch
diff mbox series

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index e0c1ad23c768..9775b0905146 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -371,6 +371,16 @@  static void process_interval(void)
 	print_counters(&rs, 0, NULL);
 }
 
+static bool print_interval_and_stop(struct perf_stat_config *config, int *times)
+{
+	if (config->interval) {
+		process_interval();
+		if (interval_count && !(--(*times)))
+			return true;
+	}
+	return false;
+}
+
 static void enable_counters(void)
 {
 	if (stat_config.initial_delay)
@@ -436,6 +446,42 @@  static bool is_target_alive(struct target *_target,
 	return false;
 }
 
+static int handle_events(pid_t pid, struct perf_stat_config *config)
+{
+	pid_t child = 0;
+	bool res, stop = false;
+	struct timespec time_to_sleep;
+	int sleep_time, status = 0, times = config->times;
+
+	if (config->interval)
+		sleep_time = config->interval;
+	else if (config->timeout)
+		sleep_time = config->timeout;
+	else
+		sleep_time = 1000;
+
+	time_to_sleep.tv_sec  = sleep_time / MSEC_PER_SEC;
+	time_to_sleep.tv_nsec = (sleep_time % MSEC_PER_SEC) * NSEC_PER_MSEC;
+
+	do {
+		if (pid != -1)
+			child = waitpid(pid, &status, WNOHANG);
+		if (child || stop || done)
+			break;
+		nanosleep(&time_to_sleep, NULL);
+		if (pid == -1)
+			stop = !is_target_alive(&target, evsel_list->core.threads);
+		if (config->timeout) {
+			stop = !stop ? true : stop;
+		} else {
+			res = print_interval_and_stop(config, &times);
+			stop = !stop ? res : stop;
+		}
+	} while (1);
+
+	return status;
+}
+
 enum counter_recovery {
 	COUNTER_SKIP,
 	COUNTER_RETRY,
@@ -494,12 +540,10 @@  static enum counter_recovery stat_handle_error(struct evsel *counter)
 static int __run_perf_stat(int argc, const char **argv, int run_idx)
 {
 	int interval = stat_config.interval;
-	int times = stat_config.times;
 	int timeout = stat_config.timeout;
 	char msg[BUFSIZ];
 	unsigned long long t0, t1;
 	struct evsel *counter;
-	struct timespec ts;
 	size_t l;
 	int status = 0;
 	const bool forks = (argc > 0);
@@ -508,17 +552,6 @@  static int __run_perf_stat(int argc, const char **argv, int run_idx)
 	int i, cpu;
 	bool second_pass = false;
 
-	if (interval) {
-		ts.tv_sec  = interval / USEC_PER_MSEC;
-		ts.tv_nsec = (interval % USEC_PER_MSEC) * NSEC_PER_MSEC;
-	} else if (timeout) {
-		ts.tv_sec  = timeout / USEC_PER_MSEC;
-		ts.tv_nsec = (timeout % USEC_PER_MSEC) * NSEC_PER_MSEC;
-	} else {
-		ts.tv_sec  = 1;
-		ts.tv_nsec = 0;
-	}
-
 	if (forks) {
 		if (perf_evlist__prepare_workload(evsel_list, &target, argv, is_pipe,
 						  workload_exec_failed_signal) < 0) {
@@ -675,16 +708,9 @@  static int __run_perf_stat(int argc, const char **argv, int run_idx)
 		perf_evlist__start_workload(evsel_list);
 		enable_counters();
 
-		if (interval || timeout) {
-			while (!waitpid(child_pid, &status, WNOHANG)) {
-				nanosleep(&ts, NULL);
-				if (timeout)
-					break;
-				process_interval();
-				if (interval_count && !(--times))
-					break;
-			}
-		}
+		if (interval || timeout)
+			handle_events(child_pid, &stat_config);
+
 		if (child_pid != -1) {
 			if (timeout)
 				kill(child_pid, SIGTERM);
@@ -701,18 +727,7 @@  static int __run_perf_stat(int argc, const char **argv, int run_idx)
 			psignal(WTERMSIG(status), argv[0]);
 	} else {
 		enable_counters();
-		while (!done) {
-			nanosleep(&ts, NULL);
-			if (!is_target_alive(&target, evsel_list->core.threads))
-				break;
-			if (timeout)
-				break;
-			if (interval) {
-				process_interval();
-				if (interval_count && !(--times))
-					break;
-			}
-		}
+		handle_events(-1, &stat_config);
 	}
 
 	disable_counters();