[v8,09/13] perf stat: implement control commands handling
diff mbox series

Message ID 5ed69a1e-052a-9790-7642-cb9c9a53d786@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 June 17, 2020, 8:41 a.m. UTC
Implement handling of 'enable' and 'disable' control commands
coming from control file descriptor. process_evlist() function
checks for events on control fds and makes required operations.
If poll event splits initiated timeout interval then the reminder
is calculated and still waited in the following poll() syscall.

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

Comments

Jiri Olsa June 23, 2020, 2:54 p.m. UTC | #1
On Wed, Jun 17, 2020 at 11:41:30AM +0300, Alexey Budankov wrote:

SNIP

>  
>  	while (1) {
>  		if (forks)
> @@ -581,8 +617,17 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times, st
>  		if (done || stop || child)
>  			break;
>  
> -		nanosleep(ts, NULL);
> -		stop = process_timeout(timeout, interval, times);
> +		clock_gettime(CLOCK_MONOTONIC, &time_start);
> +		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
> +			stop = process_timeout(timeout, interval, times);
> +			time_to_sleep = sleep_time;
> +		} else { /* fd revent */
> +			stop = process_evlist(evsel_list, interval, times);
> +			clock_gettime(CLOCK_MONOTONIC, &time_stop);
> +			diff_timespec(&time_diff, &time_stop, &time_start);
> +			time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +
> +					time_diff.tv_nsec / NSEC_PER_MSEC;

should we check time_to_sleep > time_diff first?

jirka
Jiri Olsa June 23, 2020, 2:54 p.m. UTC | #2
On Wed, Jun 17, 2020 at 11:41:30AM +0300, Alexey Budankov wrote:
> 
> Implement handling of 'enable' and 'disable' control commands
> coming from control file descriptor. process_evlist() function
> checks for events on control fds and makes required operations.
> If poll event splits initiated timeout interval then the reminder
> is calculated and still waited in the following poll() syscall.
> 
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> ---
>  tools/perf/builtin-stat.c | 67 +++++++++++++++++++++++++++++----------
>  1 file changed, 50 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index f88d5ee55022..cc56d71a3ed5 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -492,6 +492,31 @@ static bool process_timeout(int timeout, unsigned int interval, int *times)
>  	return print_interval(interval, times);
>  }
>  
> +static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
> +{
> +	bool stop = false;
> +	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
> +
> +	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
> +		switch (cmd) {
> +		case EVLIST_CTL_CMD_ENABLE:
> +			pr_info(EVLIST_ENABLED_MSG);
> +			stop = print_interval(interval, times);

why is interval printed in here?

> +			break;
> +		case EVLIST_CTL_CMD_DISABLE:
> +			stop = print_interval(interval, times);

and here?

it should be called from the main loop when the interval time is elapsed no?

jirka
Alexey Budankov June 24, 2020, 1:39 p.m. UTC | #3
On 23.06.2020 17:54, Jiri Olsa wrote:
> On Wed, Jun 17, 2020 at 11:41:30AM +0300, Alexey Budankov wrote:
>>
>> Implement handling of 'enable' and 'disable' control commands
>> coming from control file descriptor. process_evlist() function
>> checks for events on control fds and makes required operations.
>> If poll event splits initiated timeout interval then the reminder
>> is calculated and still waited in the following poll() syscall.
>>
>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>> ---
>>  tools/perf/builtin-stat.c | 67 +++++++++++++++++++++++++++++----------
>>  1 file changed, 50 insertions(+), 17 deletions(-)
>>
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index f88d5ee55022..cc56d71a3ed5 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -492,6 +492,31 @@ static bool process_timeout(int timeout, unsigned int interval, int *times)
>>  	return print_interval(interval, times);
>>  }
>>  
>> +static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
>> +{
>> +	bool stop = false;
>> +	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
>> +
>> +	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
>> +		switch (cmd) {
>> +		case EVLIST_CTL_CMD_ENABLE:
>> +			pr_info(EVLIST_ENABLED_MSG);
>> +			stop = print_interval(interval, times);
> 
> why is interval printed in here?
> 
>> +			break;
>> +		case EVLIST_CTL_CMD_DISABLE:
>> +			stop = print_interval(interval, times);
> 
> and here?
> 
> it should be called from the main loop when the interval time is elapsed no?

It is called from the main loop too and it is also additionally called here
to provide indication and counter values on commands processing times.

~Alexey

> 
> jirka
>
Alexey Budankov June 24, 2020, 2:10 p.m. UTC | #4
On 23.06.2020 17:54, Jiri Olsa wrote:
> On Wed, Jun 17, 2020 at 11:41:30AM +0300, Alexey Budankov wrote:
> 
> SNIP
> 
>>  
>>  	while (1) {
>>  		if (forks)
>> @@ -581,8 +617,17 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times, st
>>  		if (done || stop || child)
>>  			break;
>>  
>> -		nanosleep(ts, NULL);
>> -		stop = process_timeout(timeout, interval, times);
>> +		clock_gettime(CLOCK_MONOTONIC, &time_start);
>> +		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
>> +			stop = process_timeout(timeout, interval, times);
>> +			time_to_sleep = sleep_time;
>> +		} else { /* fd revent */
>> +			stop = process_evlist(evsel_list, interval, times);
>> +			clock_gettime(CLOCK_MONOTONIC, &time_stop);
>> +			diff_timespec(&time_diff, &time_stop, &time_start);
>> +			time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +
>> +					time_diff.tv_nsec / NSEC_PER_MSEC;
> 
> should we check time_to_sleep > time_diff first?

Probably and if time_diff > time_to_sleep then time_to_sleep = 0 ?

~Alexey

> 
> jirka
>
Jiri Olsa June 25, 2020, 12:12 p.m. UTC | #5
On Wed, Jun 24, 2020 at 04:39:11PM +0300, Alexey Budankov wrote:
> 
> 
> On 23.06.2020 17:54, Jiri Olsa wrote:
> > On Wed, Jun 17, 2020 at 11:41:30AM +0300, Alexey Budankov wrote:
> >>
> >> Implement handling of 'enable' and 'disable' control commands
> >> coming from control file descriptor. process_evlist() function
> >> checks for events on control fds and makes required operations.
> >> If poll event splits initiated timeout interval then the reminder
> >> is calculated and still waited in the following poll() syscall.
> >>
> >> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> >> ---
> >>  tools/perf/builtin-stat.c | 67 +++++++++++++++++++++++++++++----------
> >>  1 file changed, 50 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> >> index f88d5ee55022..cc56d71a3ed5 100644
> >> --- a/tools/perf/builtin-stat.c
> >> +++ b/tools/perf/builtin-stat.c
> >> @@ -492,6 +492,31 @@ static bool process_timeout(int timeout, unsigned int interval, int *times)
> >>  	return print_interval(interval, times);
> >>  }
> >>  
> >> +static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
> >> +{
> >> +	bool stop = false;
> >> +	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
> >> +
> >> +	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
> >> +		switch (cmd) {
> >> +		case EVLIST_CTL_CMD_ENABLE:
> >> +			pr_info(EVLIST_ENABLED_MSG);
> >> +			stop = print_interval(interval, times);
> > 
> > why is interval printed in here?
> > 
> >> +			break;
> >> +		case EVLIST_CTL_CMD_DISABLE:
> >> +			stop = print_interval(interval, times);
> > 
> > and here?
> > 
> > it should be called from the main loop when the interval time is elapsed no?
> 
> It is called from the main loop too and it is also additionally called here
> to provide indication and counter values on commands processing times.

so it prints interval out of order?

jirka
Jiri Olsa June 25, 2020, 12:14 p.m. UTC | #6
On Wed, Jun 24, 2020 at 05:10:10PM +0300, Alexey Budankov wrote:
> 
> On 23.06.2020 17:54, Jiri Olsa wrote:
> > On Wed, Jun 17, 2020 at 11:41:30AM +0300, Alexey Budankov wrote:
> > 
> > SNIP
> > 
> >>  
> >>  	while (1) {
> >>  		if (forks)
> >> @@ -581,8 +617,17 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times, st
> >>  		if (done || stop || child)
> >>  			break;
> >>  
> >> -		nanosleep(ts, NULL);
> >> -		stop = process_timeout(timeout, interval, times);
> >> +		clock_gettime(CLOCK_MONOTONIC, &time_start);
> >> +		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
> >> +			stop = process_timeout(timeout, interval, times);
> >> +			time_to_sleep = sleep_time;
> >> +		} else { /* fd revent */
> >> +			stop = process_evlist(evsel_list, interval, times);
> >> +			clock_gettime(CLOCK_MONOTONIC, &time_stop);
> >> +			diff_timespec(&time_diff, &time_stop, &time_start);
> >> +			time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +
> >> +					time_diff.tv_nsec / NSEC_PER_MSEC;
> > 
> > should we check time_to_sleep > time_diff first?
> 
> Probably and if time_diff > time_to_sleep then time_to_sleep = 0 ?

or extra call to process_timeout? if we dont want to call evlist_poll
with 0 timeout

jirka

> 
> ~Alexey
> 
> > 
> > jirka
> > 
>
Alexey Budankov June 25, 2020, 2:52 p.m. UTC | #7
On 25.06.2020 15:12, Jiri Olsa wrote:
> On Wed, Jun 24, 2020 at 04:39:11PM +0300, Alexey Budankov wrote:
>>
>>
>> On 23.06.2020 17:54, Jiri Olsa wrote:
>>> On Wed, Jun 17, 2020 at 11:41:30AM +0300, Alexey Budankov wrote:
>>>>
>>>> Implement handling of 'enable' and 'disable' control commands
>>>> coming from control file descriptor. process_evlist() function
>>>> checks for events on control fds and makes required operations.
>>>> If poll event splits initiated timeout interval then the reminder
>>>> is calculated and still waited in the following poll() syscall.
>>>>
>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>> ---
>>>>  tools/perf/builtin-stat.c | 67 +++++++++++++++++++++++++++++----------
>>>>  1 file changed, 50 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>>>> index f88d5ee55022..cc56d71a3ed5 100644
>>>> --- a/tools/perf/builtin-stat.c
>>>> +++ b/tools/perf/builtin-stat.c
>>>> @@ -492,6 +492,31 @@ static bool process_timeout(int timeout, unsigned int interval, int *times)
>>>>  	return print_interval(interval, times);
>>>>  }
>>>>  
>>>> +static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
>>>> +{
>>>> +	bool stop = false;
>>>> +	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
>>>> +
>>>> +	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
>>>> +		switch (cmd) {
>>>> +		case EVLIST_CTL_CMD_ENABLE:
>>>> +			pr_info(EVLIST_ENABLED_MSG);
>>>> +			stop = print_interval(interval, times);
>>>
>>> why is interval printed in here?
>>>
>>>> +			break;
>>>> +		case EVLIST_CTL_CMD_DISABLE:
>>>> +			stop = print_interval(interval, times);
>>>
>>> and here?
>>>
>>> it should be called from the main loop when the interval time is elapsed no?
>>
>> It is called from the main loop too and it is also additionally called here
>> to provide indication and counter values on commands processing times.
> 
> so it prints interval out of order?

Looks like it does. The only issue with it I see is change in times value.

~Alexey

> 
> jirka
>
Alexey Budankov June 25, 2020, 2:58 p.m. UTC | #8
On 25.06.2020 15:14, Jiri Olsa wrote:
> On Wed, Jun 24, 2020 at 05:10:10PM +0300, Alexey Budankov wrote:
>>
>> On 23.06.2020 17:54, Jiri Olsa wrote:
>>> On Wed, Jun 17, 2020 at 11:41:30AM +0300, Alexey Budankov wrote:
>>>
>>> SNIP
>>>
>>>>  
>>>>  	while (1) {
>>>>  		if (forks)
>>>> @@ -581,8 +617,17 @@ static int dispatch_events(bool forks, int timeout, int interval, int *times, st
>>>>  		if (done || stop || child)
>>>>  			break;
>>>>  
>>>> -		nanosleep(ts, NULL);
>>>> -		stop = process_timeout(timeout, interval, times);
>>>> +		clock_gettime(CLOCK_MONOTONIC, &time_start);
>>>> +		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
>>>> +			stop = process_timeout(timeout, interval, times);
>>>> +			time_to_sleep = sleep_time;
>>>> +		} else { /* fd revent */
>>>> +			stop = process_evlist(evsel_list, interval, times);
>>>> +			clock_gettime(CLOCK_MONOTONIC, &time_stop);
>>>> +			diff_timespec(&time_diff, &time_stop, &time_start);
>>>> +			time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +
>>>> +					time_diff.tv_nsec / NSEC_PER_MSEC;
>>>
>>> should we check time_to_sleep > time_diff first?
>>
>> Probably and if time_diff > time_to_sleep then time_to_sleep = 0 ?
> 
> or extra call to process_timeout? if we dont want to call evlist_poll
> with 0 timeout

poll() man page says it is ok to call poll with 0 timeout so 
process_timeout() and initialization of time_to_sleep will be
done in common flow.

~Alexey

Patch
diff mbox series

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index f88d5ee55022..cc56d71a3ed5 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -492,6 +492,31 @@  static bool process_timeout(int timeout, unsigned int interval, int *times)
 	return print_interval(interval, times);
 }
 
+static bool process_evlist(struct evlist *evlist, unsigned int interval, int *times)
+{
+	bool stop = false;
+	enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
+
+	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
+		switch (cmd) {
+		case EVLIST_CTL_CMD_ENABLE:
+			pr_info(EVLIST_ENABLED_MSG);
+			stop = print_interval(interval, times);
+			break;
+		case EVLIST_CTL_CMD_DISABLE:
+			stop = print_interval(interval, times);
+			pr_info(EVLIST_DISABLED_MSG);
+			break;
+		case EVLIST_CTL_CMD_ACK:
+		case EVLIST_CTL_CMD_UNSUPPORTED:
+		default:
+			break;
+		}
+	}
+
+	return stop;
+}
+
 static void enable_counters(void)
 {
 	if (stat_config.initial_delay < 0) {
@@ -567,10 +592,21 @@  static bool is_target_alive(struct target *_target,
 	return false;
 }
 
-static int dispatch_events(bool forks, int timeout, int interval, int *times, struct timespec *ts)
+static int dispatch_events(bool forks, int timeout, int interval, int *times)
 {
 	bool stop = false;
 	int child = 0, status = 0;
+	int time_to_sleep, sleep_time;
+	struct timespec time_start, time_stop, time_diff;
+
+	if (interval)
+		sleep_time = interval;
+	else if (timeout)
+		sleep_time = timeout;
+	else
+		sleep_time = 1000;
+
+	time_to_sleep = sleep_time;
 
 	while (1) {
 		if (forks)
@@ -581,8 +617,17 @@  static int dispatch_events(bool forks, int timeout, int interval, int *times, st
 		if (done || stop || child)
 			break;
 
-		nanosleep(ts, NULL);
-		stop = process_timeout(timeout, interval, times);
+		clock_gettime(CLOCK_MONOTONIC, &time_start);
+		if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll timeout or EINTR */
+			stop = process_timeout(timeout, interval, times);
+			time_to_sleep = sleep_time;
+		} else { /* fd revent */
+			stop = process_evlist(evsel_list, interval, times);
+			clock_gettime(CLOCK_MONOTONIC, &time_stop);
+			diff_timespec(&time_diff, &time_stop, &time_start);
+			time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +
+					time_diff.tv_nsec / NSEC_PER_MSEC;
+		}
 	}
 
 	return status;
@@ -651,7 +696,6 @@  static int __run_perf_stat(int argc, const char **argv, int run_idx)
 	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);
@@ -660,17 +704,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) {
@@ -828,7 +861,7 @@  static int __run_perf_stat(int argc, const char **argv, int run_idx)
 		enable_counters();
 
 		if (interval || timeout)
-			status = dispatch_events(forks, timeout, interval, &times, &ts);
+			status = dispatch_events(forks, timeout, interval, &times);
 		if (child_pid != -1) {
 			if (timeout)
 				kill(child_pid, SIGTERM);
@@ -845,7 +878,7 @@  static int __run_perf_stat(int argc, const char **argv, int run_idx)
 			psignal(WTERMSIG(status), argv[0]);
 	} else {
 		enable_counters();
-		dispatch_events(forks, timeout, interval, &times, &ts);
+		dispatch_events(forks, timeout, interval, &times);
 	}
 
 	disable_counters();