Re: [PATCH v9 11/15] perf stat: implement control commands handling
On 07.07.2020 17:55, Alexey Budankov wrote:
>
> On 07.07.2020 17:23, Jiri Olsa wrote:
>> On Tue, Jul 07, 2020 at 04:24:28PM +0300, Alexey Budankov wrote:
>>>
>>> On 07.07.2020 16:14, Jiri Olsa wrote:
On Tue, Jul 07, 2020 at 04:07:42PM +0300, Alexey Budankov wrote:
>
> On 06.07.2020 22:34, Jiri Olsa wrote:
>> On Mon, Jul 06, 2020 at 05:47:54PM +0300, Alexey Budankov wrote:
>>>
>>> On 06.07.2020 15:34, Jiri Olsa wrote:
On Fri, Jul 03, 2020 at 10:47:22AM +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
> ---
> tools/perf/builtin-stat.c | 75
> ---
> 1 file changed, 55 insertions(+), 20 deletions(-)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 9e4288ecf2b8..5021f7286422 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int
> interval, int *times)
> return false;
> }
>
> +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, ) > 0) {
> + switch (cmd) {
> + case EVLIST_CTL_CMD_ENABLE:
> + pr_info(EVLIST_ENABLED_MSG);
> + stop = handle_interval(interval, times);
> + break;
> + case EVLIST_CTL_CMD_DISABLE:
> + stop = handle_interval(interval, times);
I still don't understand why you call handle_interval in here
I don't see it being necessary.. you enable events and handle_interval,
wil be called in the next iteration of dispatch_events, why complicate
this function with that?
>>>
>>> Printing event counts at the moment of command processing lets scripts
>>> built on top of stat output to provide more plain and accurate metrics.
>>> Otherwise it may get spikes in the beginning of the next time interval
>>> because not all counts lay inside [Events enabled, Events disable]
>>> If -I interval is large tail event count can be also large. Compare the
>>> output below with the output in the cover letter. Either way is possible
>>> but the latter one likely complicates the scripts I mentioned above.
>>>
>>> perf=tools/perf/perf
>>> ${perf} stat -D -1 -e cpu-cycles -a -I 1000 \
>>> --control fd:${ctl_fd},${ctl_fd_ack} \
>>> -- sleep 40 &
>>>
>>> Events disabled
>>> # time counts unit events
>>> 1.001100723cpu-cycles
>>>
>>> 2.003146566cpu-cycles
>>>
>>> 3.005073317cpu-cycles
>>>
>>> 4.006337062cpu-cycles
>>>
>>> Events enabled
>>> enable acked(ack)
>>> 5.011182000 54,128,692 cpu-cycles <===
>>>
>>> 6.012300167 3,648,804,827 cpu-cycles
>>>
>>> 7.013631689590,438,536 cpu-cycles
>>>
>>> 8.015558583406,935,663 cpu-cycles
>>>
>>> 9.017455505407,806,862 cpu-cycles
>>>
>>> 10.019300780399,351,824 cpu-cycles
>>>
>>> 11.021180025404,584,417 cpu-cycles
>>>
>>> 12.023033661537,787,981 cpu-cycles
>>>
>>> 13.024422354699,395,364 cpu-cycles
>>>
>>> 14.026325749397,871,324 cpu-cycles
>>>
>>> disable
Re: [PATCH v9 11/15] perf stat: implement control commands handling
On 07.07.2020 19:05, Jiri Olsa wrote:
> On Tue, Jul 07, 2020 at 05:55:14PM +0300, Alexey Budankov wrote:
>
> SNIP
>
>> process_evlist() now looks suboptimal since record mode code directly calls
>> evlist__ctlfd_process()
>> and then handles returned command specifically to the mode. So in v10 I
>> replaced process_evlist()
>> call with direct evlist__ctlfd_process() call and then handling the returned
>> command by printing
>> command msg tag and counter values in the required order. Like this:
>>
>> +clock_gettime(CLOCK_MONOTONIC, _start);
>> +if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll
>> timeout or EINTR */
>> +if (timeout)
>> +break;
>> +else
>> +stop = handle_interval(interval, times);
>> +time_to_sleep = sleep_time;
>> +} else { /* fd revent */
>> +if (evlist__ctlfd_process(evsel_list, ) > 0) {
>> +if (interval) {
>> +switch (cmd) {
>> +case EVLIST_CTL_CMD_ENABLE:
>> +pr_info(EVLIST_ENABLED_MSG);
>> +process_interval();
>> +break;
>> +case EVLIST_CTL_CMD_DISABLE:
>> +process_interval();
>> +pr_info(EVLIST_DISABLED_MSG);
>> +break;
>> +case EVLIST_CTL_CMD_ACK:
>> +case EVLIST_CTL_CMD_UNSUPPORTED:
>> +default:
>> +break;
>> +}
>> +}
>> +}
>> +clock_gettime(CLOCK_MONOTONIC, _stop);
>> +compute_tts(_start, _stop, _to_sleep);
>> +}
>
>
> hum, why not just get the bool from process_evlist like below?
Yes, also possible and works. However it checks twice to implement
parts of logically the same work and passes the result using extra
memory: switch/case at process_evlist(), 'if' at dispatch_events(),
dispatch_events() should also call process_interval() instead of
handle_interval() to avoid wasting of times counter for commands.
Alexey
>
> jirka
>
>
> ---
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 5021f7286422..32dd3de93f35 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -485,20 +485,20 @@ static bool handle_interval(unsigned int interval, int
> *times)
> return false;
> }
>
> -static bool process_evlist(struct evlist *evlist, unsigned int interval, int
> *times)
> +static bool process_evlist(struct evlist *evlist)
> {
> - bool stop = false;
> enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
> + bool display = false;
>
> if (evlist__ctlfd_process(evlist, ) > 0) {
> switch (cmd) {
> case EVLIST_CTL_CMD_ENABLE:
> pr_info(EVLIST_ENABLED_MSG);
> - stop = handle_interval(interval, times);
> + display = true;
> break;
> case EVLIST_CTL_CMD_DISABLE:
> - stop = handle_interval(interval, times);
> pr_info(EVLIST_DISABLED_MSG);
> + display = true;
> break;
> case EVLIST_CTL_CMD_ACK:
> case EVLIST_CTL_CMD_UNSUPPORTED:
> @@ -507,7 +507,7 @@ static bool process_evlist(struct evlist *evlist,
> unsigned int interval, int *ti
> }
> }
>
> - return stop;
> + return display;
> }
>
> static void enable_counters(void)
> @@ -618,7 +618,8 @@ static int dispatch_events(bool forks, int timeout, int
> interval, int *times)
> stop = handle_interval(interval, times);
> time_to_sleep = sleep_time;
> } else { /* fd revent */
> - stop = process_evlist(evsel_list, interval, times);
> + if (process_evlist(evsel_list))
> + stop = handle_interval(interval, times);
> clock_gettime(CLOCK_MONOTONIC, _stop);
> diff_timespec(_diff, _stop, _start);
> time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +
>
Re: [PATCH v9 11/15] perf stat: implement control commands handling
On Tue, Jul 07, 2020 at 05:55:14PM +0300, Alexey Budankov wrote:
SNIP
> process_evlist() now looks suboptimal since record mode code directly calls
> evlist__ctlfd_process()
> and then handles returned command specifically to the mode. So in v10 I
> replaced process_evlist()
> call with direct evlist__ctlfd_process() call and then handling the returned
> command by printing
> command msg tag and counter values in the required order. Like this:
>
> + clock_gettime(CLOCK_MONOTONIC, _start);
> + if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll
> timeout or EINTR */
> + if (timeout)
> + break;
> + else
> + stop = handle_interval(interval, times);
> + time_to_sleep = sleep_time;
> + } else { /* fd revent */
> + if (evlist__ctlfd_process(evsel_list, ) > 0) {
> + if (interval) {
> + switch (cmd) {
> + case EVLIST_CTL_CMD_ENABLE:
> + pr_info(EVLIST_ENABLED_MSG);
> + process_interval();
> + break;
> + case EVLIST_CTL_CMD_DISABLE:
> + process_interval();
> + pr_info(EVLIST_DISABLED_MSG);
> + break;
> + case EVLIST_CTL_CMD_ACK:
> + case EVLIST_CTL_CMD_UNSUPPORTED:
> + default:
> + break;
> + }
> + }
> + }
> + clock_gettime(CLOCK_MONOTONIC, _stop);
> + compute_tts(_start, _stop, _to_sleep);
> + }
hum, why not just get the bool from process_evlist like below?
jirka
---
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 5021f7286422..32dd3de93f35 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -485,20 +485,20 @@ static bool handle_interval(unsigned int interval, int
*times)
return false;
}
-static bool process_evlist(struct evlist *evlist, unsigned int interval, int
*times)
+static bool process_evlist(struct evlist *evlist)
{
- bool stop = false;
enum evlist_ctl_cmd cmd = EVLIST_CTL_CMD_UNSUPPORTED;
+ bool display = false;
if (evlist__ctlfd_process(evlist, ) > 0) {
switch (cmd) {
case EVLIST_CTL_CMD_ENABLE:
pr_info(EVLIST_ENABLED_MSG);
- stop = handle_interval(interval, times);
+ display = true;
break;
case EVLIST_CTL_CMD_DISABLE:
- stop = handle_interval(interval, times);
pr_info(EVLIST_DISABLED_MSG);
+ display = true;
break;
case EVLIST_CTL_CMD_ACK:
case EVLIST_CTL_CMD_UNSUPPORTED:
@@ -507,7 +507,7 @@ static bool process_evlist(struct evlist *evlist, unsigned
int interval, int *ti
}
}
- return stop;
+ return display;
}
static void enable_counters(void)
@@ -618,7 +618,8 @@ static int dispatch_events(bool forks, int timeout, int
interval, int *times)
stop = handle_interval(interval, times);
time_to_sleep = sleep_time;
} else { /* fd revent */
- stop = process_evlist(evsel_list, interval, times);
+ if (process_evlist(evsel_list))
+ stop = handle_interval(interval, times);
clock_gettime(CLOCK_MONOTONIC, _stop);
diff_timespec(_diff, _stop, _start);
time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +
Re: [PATCH v9 11/15] perf stat: implement control commands handling
On 07.07.2020 17:23, Jiri Olsa wrote:
> On Tue, Jul 07, 2020 at 04:24:28PM +0300, Alexey Budankov wrote:
>>
>> On 07.07.2020 16:14, Jiri Olsa wrote:
>>> On Tue, Jul 07, 2020 at 04:07:42PM +0300, Alexey Budankov wrote:
On 06.07.2020 22:34, Jiri Olsa wrote:
> On Mon, Jul 06, 2020 at 05:47:54PM +0300, Alexey Budankov wrote:
>>
>> On 06.07.2020 15:34, Jiri Olsa wrote:
>>> On Fri, Jul 03, 2020 at 10:47:22AM +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
---
tools/perf/builtin-stat.c | 75 ---
1 file changed, 55 insertions(+), 20 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 9e4288ecf2b8..5021f7286422 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -485,6 +485,31 @@ static bool handle_interval(unsigned int
interval, int *times)
return false;
}
+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, ) > 0) {
+ switch (cmd) {
+ case EVLIST_CTL_CMD_ENABLE:
+ pr_info(EVLIST_ENABLED_MSG);
+ stop = handle_interval(interval, times);
+ break;
+ case EVLIST_CTL_CMD_DISABLE:
+ stop = handle_interval(interval, times);
>>>
>>> I still don't understand why you call handle_interval in here
>>>
>>> I don't see it being necessary.. you enable events and handle_interval,
>>> wil be called in the next iteration of dispatch_events, why complicate
>>> this function with that?
>>
>> Printing event counts at the moment of command processing lets scripts
>> built on top of stat output to provide more plain and accurate metrics.
>> Otherwise it may get spikes in the beginning of the next time interval
>> because not all counts lay inside [Events enabled, Events disable]
>> If -I interval is large tail event count can be also large. Compare the
>> output below with the output in the cover letter. Either way is possible
>> but the latter one likely complicates the scripts I mentioned above.
>>
>> perf=tools/perf/perf
>> ${perf} stat -D -1 -e cpu-cycles -a -I 1000 \
>> --control fd:${ctl_fd},${ctl_fd_ack} \
>> -- sleep 40 &
>>
>> Events disabled
>> # time counts unit events
>> 1.001100723cpu-cycles
>>
>> 2.003146566cpu-cycles
>>
>> 3.005073317cpu-cycles
>>
>> 4.006337062cpu-cycles
>>
>> Events enabled
>> enable acked(ack)
>> 5.011182000 54,128,692 cpu-cycles <===
>>
>> 6.012300167 3,648,804,827 cpu-cycles
>>
>> 7.013631689590,438,536 cpu-cycles
>>
>> 8.015558583406,935,663 cpu-cycles
>>
>> 9.017455505407,806,862 cpu-cycles
>>
>> 10.019300780399,351,824 cpu-cycles
>>
>> 11.021180025404,584,417 cpu-cycles
>>
>> 12.023033661537,787,981 cpu-cycles
>>
>> 13.024422354699,395,364 cpu-cycles
>>
>> 14.026325749397,871,324 cpu-cycles
>>
>> disable acked()
>> Events disabled
>> 15.027857981396,956,159 cpu-cycles <===
>> 16.029279264cpu-cycles
Re: [PATCH v9 11/15] perf stat: implement control commands handling
On Tue, Jul 07, 2020 at 04:24:28PM +0300, Alexey Budankov wrote:
>
> On 07.07.2020 16:14, Jiri Olsa wrote:
> > On Tue, Jul 07, 2020 at 04:07:42PM +0300, Alexey Budankov wrote:
> >>
> >> On 06.07.2020 22:34, Jiri Olsa wrote:
> >>> On Mon, Jul 06, 2020 at 05:47:54PM +0300, Alexey Budankov wrote:
>
> On 06.07.2020 15:34, Jiri Olsa wrote:
> > On Fri, Jul 03, 2020 at 10:47:22AM +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
> >> ---
> >> tools/perf/builtin-stat.c | 75 ---
> >> 1 file changed, 55 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> >> index 9e4288ecf2b8..5021f7286422 100644
> >> --- a/tools/perf/builtin-stat.c
> >> +++ b/tools/perf/builtin-stat.c
> >> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int
> >> interval, int *times)
> >>return false;
> >> }
> >>
> >> +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, ) > 0) {
> >> + switch (cmd) {
> >> + case EVLIST_CTL_CMD_ENABLE:
> >> + pr_info(EVLIST_ENABLED_MSG);
> >> + stop = handle_interval(interval, times);
> >> + break;
> >> + case EVLIST_CTL_CMD_DISABLE:
> >> + stop = handle_interval(interval, times);
> >
> > I still don't understand why you call handle_interval in here
> >
> > I don't see it being necessary.. you enable events and handle_interval,
> > wil be called in the next iteration of dispatch_events, why complicate
> > this function with that?
>
> Printing event counts at the moment of command processing lets scripts
> built on top of stat output to provide more plain and accurate metrics.
> Otherwise it may get spikes in the beginning of the next time interval
> because not all counts lay inside [Events enabled, Events disable]
> If -I interval is large tail event count can be also large. Compare the
> output below with the output in the cover letter. Either way is possible
> but the latter one likely complicates the scripts I mentioned above.
>
> perf=tools/perf/perf
> ${perf} stat -D -1 -e cpu-cycles -a -I 1000 \
> --control fd:${ctl_fd},${ctl_fd_ack} \
> -- sleep 40 &
>
> Events disabled
> # time counts unit events
> 1.001100723cpu-cycles
>
> 2.003146566cpu-cycles
>
> 3.005073317cpu-cycles
>
> 4.006337062cpu-cycles
>
> Events enabled
> enable acked(ack)
> 5.011182000 54,128,692 cpu-cycles <===
>
> 6.012300167 3,648,804,827 cpu-cycles
>
> 7.013631689590,438,536 cpu-cycles
>
> 8.015558583406,935,663 cpu-cycles
>
> 9.017455505407,806,862 cpu-cycles
>
> 10.019300780399,351,824 cpu-cycles
>
> 11.021180025404,584,417 cpu-cycles
>
> 12.023033661537,787,981 cpu-cycles
>
> 13.024422354699,395,364 cpu-cycles
>
> 14.026325749397,871,324 cpu-cycles
>
> disable acked()
> Events disabled
> 15.027857981396,956,159 cpu-cycles <===
> 16.029279264cpu-cycles
>
Re: [PATCH v9 11/15] perf stat: implement control commands handling
On 07.07.2020 16:14, Jiri Olsa wrote:
> On Tue, Jul 07, 2020 at 04:07:42PM +0300, Alexey Budankov wrote:
>>
>> On 06.07.2020 22:34, Jiri Olsa wrote:
>>> On Mon, Jul 06, 2020 at 05:47:54PM +0300, Alexey Budankov wrote:
On 06.07.2020 15:34, Jiri Olsa wrote:
> On Fri, Jul 03, 2020 at 10:47:22AM +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
>> ---
>> tools/perf/builtin-stat.c | 75 ---
>> 1 file changed, 55 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index 9e4288ecf2b8..5021f7286422 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval,
>> int *times)
>> return false;
>> }
>>
>> +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, ) > 0) {
>> +switch (cmd) {
>> +case EVLIST_CTL_CMD_ENABLE:
>> +pr_info(EVLIST_ENABLED_MSG);
>> +stop = handle_interval(interval, times);
>> +break;
>> +case EVLIST_CTL_CMD_DISABLE:
>> +stop = handle_interval(interval, times);
>
> I still don't understand why you call handle_interval in here
>
> I don't see it being necessary.. you enable events and handle_interval,
> wil be called in the next iteration of dispatch_events, why complicate
> this function with that?
Printing event counts at the moment of command processing lets scripts
built on top of stat output to provide more plain and accurate metrics.
Otherwise it may get spikes in the beginning of the next time interval
because not all counts lay inside [Events enabled, Events disable]
If -I interval is large tail event count can be also large. Compare the
output below with the output in the cover letter. Either way is possible
but the latter one likely complicates the scripts I mentioned above.
perf=tools/perf/perf
${perf} stat -D -1 -e cpu-cycles -a -I 1000 \
--control fd:${ctl_fd},${ctl_fd_ack} \
-- sleep 40 &
Events disabled
# time counts unit events
1.001100723cpu-cycles
2.003146566cpu-cycles
3.005073317cpu-cycles
4.006337062cpu-cycles
Events enabled
enable acked(ack)
5.011182000 54,128,692 cpu-cycles <===
6.012300167 3,648,804,827 cpu-cycles
7.013631689590,438,536 cpu-cycles
8.015558583406,935,663 cpu-cycles
9.017455505407,806,862 cpu-cycles
10.019300780399,351,824 cpu-cycles
11.021180025404,584,417 cpu-cycles
12.023033661537,787,981 cpu-cycles
13.024422354699,395,364 cpu-cycles
14.026325749397,871,324 cpu-cycles
disable acked()
Events disabled
15.027857981396,956,159 cpu-cycles <===
16.029279264cpu-cycles
17.031131311cpu-cycles
18.033010580cpu-cycles
19.034918883
Re: [PATCH v9 11/15] perf stat: implement control commands handling
On Tue, Jul 07, 2020 at 04:07:42PM +0300, Alexey Budankov wrote:
>
> On 06.07.2020 22:34, Jiri Olsa wrote:
> > On Mon, Jul 06, 2020 at 05:47:54PM +0300, Alexey Budankov wrote:
> >>
> >> On 06.07.2020 15:34, Jiri Olsa wrote:
> >>> On Fri, Jul 03, 2020 at 10:47:22AM +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
> ---
> tools/perf/builtin-stat.c | 75 ---
> 1 file changed, 55 insertions(+), 20 deletions(-)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 9e4288ecf2b8..5021f7286422 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval,
> int *times)
> return false;
> }
>
> +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, ) > 0) {
> +switch (cmd) {
> +case EVLIST_CTL_CMD_ENABLE:
> +pr_info(EVLIST_ENABLED_MSG);
> +stop = handle_interval(interval, times);
> +break;
> +case EVLIST_CTL_CMD_DISABLE:
> +stop = handle_interval(interval, times);
> >>>
> >>> I still don't understand why you call handle_interval in here
> >>>
> >>> I don't see it being necessary.. you enable events and handle_interval,
> >>> wil be called in the next iteration of dispatch_events, why complicate
> >>> this function with that?
> >>
> >> Printing event counts at the moment of command processing lets scripts
> >> built on top of stat output to provide more plain and accurate metrics.
> >> Otherwise it may get spikes in the beginning of the next time interval
> >> because not all counts lay inside [Events enabled, Events disable]
> >> If -I interval is large tail event count can be also large. Compare the
> >> output below with the output in the cover letter. Either way is possible
> >> but the latter one likely complicates the scripts I mentioned above.
> >>
> >> perf=tools/perf/perf
> >> ${perf} stat -D -1 -e cpu-cycles -a -I 1000 \
> >> --control fd:${ctl_fd},${ctl_fd_ack} \
> >> -- sleep 40 &
> >>
> >> Events disabled
> >> # time counts unit events
> >> 1.001100723cpu-cycles
> >>
> >> 2.003146566cpu-cycles
> >>
> >> 3.005073317cpu-cycles
> >>
> >> 4.006337062cpu-cycles
> >>
> >> Events enabled
> >> enable acked(ack)
> >> 5.011182000 54,128,692 cpu-cycles <===
> >>
> >> 6.012300167 3,648,804,827 cpu-cycles
> >>
> >> 7.013631689590,438,536 cpu-cycles
> >>
> >> 8.015558583406,935,663 cpu-cycles
> >>
> >> 9.017455505407,806,862 cpu-cycles
> >>
> >> 10.019300780399,351,824 cpu-cycles
> >>
> >> 11.021180025404,584,417 cpu-cycles
> >>
> >> 12.023033661537,787,981 cpu-cycles
> >>
> >> 13.024422354699,395,364 cpu-cycles
> >>
> >> 14.026325749397,871,324 cpu-cycles
> >>
> >> disable acked()
> >> Events disabled
> >> 15.027857981396,956,159 cpu-cycles <===
> >> 16.029279264cpu-cycles
> >>
> >> 17.031131311cpu-cycles
> >>
> >> 18.033010580cpu-cycles
> >>
> >> 19.034918883cpu-cycles
> >>
Re: [PATCH v9 11/15] perf stat: implement control commands handling
On 06.07.2020 22:34, Jiri Olsa wrote:
> On Mon, Jul 06, 2020 at 05:47:54PM +0300, Alexey Budankov wrote:
>>
>> On 06.07.2020 15:34, Jiri Olsa wrote:
>>> On Fri, Jul 03, 2020 at 10:47:22AM +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
---
tools/perf/builtin-stat.c | 75 ---
1 file changed, 55 insertions(+), 20 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 9e4288ecf2b8..5021f7286422 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval,
int *times)
return false;
}
+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, ) > 0) {
+ switch (cmd) {
+ case EVLIST_CTL_CMD_ENABLE:
+ pr_info(EVLIST_ENABLED_MSG);
+ stop = handle_interval(interval, times);
+ break;
+ case EVLIST_CTL_CMD_DISABLE:
+ stop = handle_interval(interval, times);
>>>
>>> I still don't understand why you call handle_interval in here
>>>
>>> I don't see it being necessary.. you enable events and handle_interval,
>>> wil be called in the next iteration of dispatch_events, why complicate
>>> this function with that?
>>
>> Printing event counts at the moment of command processing lets scripts
>> built on top of stat output to provide more plain and accurate metrics.
>> Otherwise it may get spikes in the beginning of the next time interval
>> because not all counts lay inside [Events enabled, Events disable]
>> If -I interval is large tail event count can be also large. Compare the
>> output below with the output in the cover letter. Either way is possible
>> but the latter one likely complicates the scripts I mentioned above.
>>
>> perf=tools/perf/perf
>> ${perf} stat -D -1 -e cpu-cycles -a -I 1000 \
>> --control fd:${ctl_fd},${ctl_fd_ack} \
>> -- sleep 40 &
>>
>> Events disabled
>> # time counts unit events
>> 1.001100723cpu-cycles
>>
>> 2.003146566cpu-cycles
>>
>> 3.005073317cpu-cycles
>>
>> 4.006337062cpu-cycles
>>
>> Events enabled
>> enable acked(ack)
>> 5.011182000 54,128,692 cpu-cycles <===
>>
>> 6.012300167 3,648,804,827 cpu-cycles
>>
>> 7.013631689590,438,536 cpu-cycles
>>
>> 8.015558583406,935,663 cpu-cycles
>>
>> 9.017455505407,806,862 cpu-cycles
>>
>> 10.019300780399,351,824 cpu-cycles
>>
>> 11.021180025404,584,417 cpu-cycles
>>
>> 12.023033661537,787,981 cpu-cycles
>>
>> 13.024422354699,395,364 cpu-cycles
>>
>> 14.026325749397,871,324 cpu-cycles
>>
>> disable acked()
>> Events disabled
>> 15.027857981396,956,159 cpu-cycles <===
>> 16.029279264cpu-cycles
>>
>> 17.031131311cpu-cycles
>>
>> 18.033010580cpu-cycles
>>
>> 19.034918883cpu-cycles
>>
>> enable acked(ack)
>> Events enabled
>> 20.036758793183,544,975 cpu-cycles <===
>>
>> 21.038163289419,054,544 cpu-cycles
>>
>> 22.040108245413,993,309 cpu-cycles
Re: [PATCH v9 11/15] perf stat: implement control commands handling
On Mon, Jul 06, 2020 at 05:47:54PM +0300, Alexey Budankov wrote:
>
> On 06.07.2020 15:34, Jiri Olsa wrote:
> > On Fri, Jul 03, 2020 at 10:47:22AM +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
> >> ---
> >> tools/perf/builtin-stat.c | 75 ---
> >> 1 file changed, 55 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> >> index 9e4288ecf2b8..5021f7286422 100644
> >> --- a/tools/perf/builtin-stat.c
> >> +++ b/tools/perf/builtin-stat.c
> >> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval,
> >> int *times)
> >>return false;
> >> }
> >>
> >> +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, ) > 0) {
> >> + switch (cmd) {
> >> + case EVLIST_CTL_CMD_ENABLE:
> >> + pr_info(EVLIST_ENABLED_MSG);
> >> + stop = handle_interval(interval, times);
> >> + break;
> >> + case EVLIST_CTL_CMD_DISABLE:
> >> + stop = handle_interval(interval, times);
> >
> > I still don't understand why you call handle_interval in here
> >
> > I don't see it being necessary.. you enable events and handle_interval,
> > wil be called in the next iteration of dispatch_events, why complicate
> > this function with that?
>
> Printing event counts at the moment of command processing lets scripts
> built on top of stat output to provide more plain and accurate metrics.
> Otherwise it may get spikes in the beginning of the next time interval
> because not all counts lay inside [Events enabled, Events disable]
> If -I interval is large tail event count can be also large. Compare the
> output below with the output in the cover letter. Either way is possible
> but the latter one likely complicates the scripts I mentioned above.
>
> perf=tools/perf/perf
> ${perf} stat -D -1 -e cpu-cycles -a -I 1000 \
> --control fd:${ctl_fd},${ctl_fd_ack} \
> -- sleep 40 &
>
> Events disabled
> # time counts unit events
> 1.001100723cpu-cycles
>
> 2.003146566cpu-cycles
>
> 3.005073317cpu-cycles
>
> 4.006337062cpu-cycles
>
> Events enabled
> enable acked(ack)
> 5.011182000 54,128,692 cpu-cycles <===
>
> 6.012300167 3,648,804,827 cpu-cycles
>
> 7.013631689590,438,536 cpu-cycles
>
> 8.015558583406,935,663 cpu-cycles
>
> 9.017455505407,806,862 cpu-cycles
>
> 10.019300780399,351,824 cpu-cycles
>
> 11.021180025404,584,417 cpu-cycles
>
> 12.023033661537,787,981 cpu-cycles
>
> 13.024422354699,395,364 cpu-cycles
>
> 14.026325749397,871,324 cpu-cycles
>
> disable acked()
> Events disabled
> 15.027857981396,956,159 cpu-cycles <===
> 16.029279264cpu-cycles
>
> 17.031131311cpu-cycles
>
> 18.033010580cpu-cycles
>
> 19.034918883cpu-cycles
>
> enable acked(ack)
> Events enabled
> 20.036758793183,544,975 cpu-cycles <===
>
> 21.038163289419,054,544 cpu-cycles
>
> 22.040108245413,993,309 cpu-cycles
>
> 23.042042365403,584,493 cpu-cycles
Re: [PATCH v9 11/15] perf stat: implement control commands handling
On 06.07.2020 15:34, Jiri Olsa wrote:
> On Fri, Jul 03, 2020 at 10:47:22AM +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
>> ---
>> tools/perf/builtin-stat.c | 75 ---
>> 1 file changed, 55 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index 9e4288ecf2b8..5021f7286422 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval, int
>> *times)
>> return false;
>> }
>>
>> +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, ) > 0) {
>> +switch (cmd) {
>> +case EVLIST_CTL_CMD_ENABLE:
>> +pr_info(EVLIST_ENABLED_MSG);
>> +stop = handle_interval(interval, times);
>> +break;
>> +case EVLIST_CTL_CMD_DISABLE:
>> +stop = handle_interval(interval, times);
>
> I still don't understand why you call handle_interval in here
>
> I don't see it being necessary.. you enable events and handle_interval,
> wil be called in the next iteration of dispatch_events, why complicate
> this function with that?
Printing event counts at the moment of command processing lets scripts
built on top of stat output to provide more plain and accurate metrics.
Otherwise it may get spikes in the beginning of the next time interval
because not all counts lay inside [Events enabled, Events disable]
If -I interval is large tail event count can be also large. Compare the
output below with the output in the cover letter. Either way is possible
but the latter one likely complicates the scripts I mentioned above.
perf=tools/perf/perf
${perf} stat -D -1 -e cpu-cycles -a -I 1000 \
--control fd:${ctl_fd},${ctl_fd_ack} \
-- sleep 40 &
Events disabled
# time counts unit events
1.001100723cpu-cycles
2.003146566cpu-cycles
3.005073317cpu-cycles
4.006337062cpu-cycles
Events enabled
enable acked(ack)
5.011182000 54,128,692 cpu-cycles <===
6.012300167 3,648,804,827 cpu-cycles
7.013631689590,438,536 cpu-cycles
8.015558583406,935,663 cpu-cycles
9.017455505407,806,862 cpu-cycles
10.019300780399,351,824 cpu-cycles
11.021180025404,584,417 cpu-cycles
12.023033661537,787,981 cpu-cycles
13.024422354699,395,364 cpu-cycles
14.026325749397,871,324 cpu-cycles
disable acked()
Events disabled
15.027857981396,956,159 cpu-cycles <===
16.029279264cpu-cycles
17.031131311cpu-cycles
18.033010580cpu-cycles
19.034918883cpu-cycles
enable acked(ack)
Events enabled
20.036758793183,544,975 cpu-cycles <===
21.038163289419,054,544 cpu-cycles
22.040108245413,993,309 cpu-cycles
23.042042365403,584,493 cpu-cycles
24.043985381416,512,094 cpu-cycles
25.045925682401,513,429 cpu-cycles
#
Re: [PATCH v9 11/15] perf stat: implement control commands handling
On 06.07.2020 15:37, Jiri Olsa wrote:
> On Fri, Jul 03, 2020 at 10:47:22AM +0300, Alexey Budankov wrote:
>
> SNIP
>
>>
>> while (1) {
>> if (forks)
>> @@ -574,11 +610,22 @@ static int dispatch_events(bool forks, int timeout,
>> int interval, int *times, st
>> if (done || stop || child_exited)
>> break;
>>
>> -nanosleep(ts, NULL);
>> -if (timeout)
>> -stop = true;
>> -else
>> -stop = handle_interval(interval, times);
>> +clock_gettime(CLOCK_MONOTONIC, _start);
>> +if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll
>> timeout or EINTR */
>> +if (timeout)
>> +stop = true;
>> +else
>> +stop = handle_interval(interval, times);
>> +time_to_sleep = sleep_time;
>> +} else { /* fd revent */
>> +stop = process_evlist(evsel_list, interval, times);
>> +clock_gettime(CLOCK_MONOTONIC, _stop);
>> +diff_timespec(_diff, _stop, _start);
>> +time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +
>> + time_diff.tv_nsec / NSEC_PER_MSEC;
>> +if (time_to_sleep < 0)
>> +time_to_sleep = 0;
>
> could this computation go to a separate function? something like:
>
> time_to_sleep = compute_tts(time_start, time_stop);
Accepted. In v10.
Alexey
Re: [PATCH v9 11/15] perf stat: implement control commands handling
On Fri, Jul 03, 2020 at 10:47:22AM +0300, Alexey Budankov wrote:
SNIP
>
> while (1) {
> if (forks)
> @@ -574,11 +610,22 @@ static int dispatch_events(bool forks, int timeout, int
> interval, int *times, st
> if (done || stop || child_exited)
> break;
>
> - nanosleep(ts, NULL);
> - if (timeout)
> - stop = true;
> - else
> - stop = handle_interval(interval, times);
> + clock_gettime(CLOCK_MONOTONIC, _start);
> + if (!(evlist__poll(evsel_list, time_to_sleep) > 0)) { /* poll
> timeout or EINTR */
> + if (timeout)
> + stop = true;
> + else
> + stop = handle_interval(interval, times);
> + time_to_sleep = sleep_time;
> + } else { /* fd revent */
> + stop = process_evlist(evsel_list, interval, times);
> + clock_gettime(CLOCK_MONOTONIC, _stop);
> + diff_timespec(_diff, _stop, _start);
> + time_to_sleep -= time_diff.tv_sec * MSEC_PER_SEC +
> + time_diff.tv_nsec / NSEC_PER_MSEC;
> + if (time_to_sleep < 0)
> + time_to_sleep = 0;
could this computation go to a separate function? something like:
time_to_sleep = compute_tts(time_start, time_stop);
thanks,
jirka
Re: [PATCH v9 11/15] perf stat: implement control commands handling
On Fri, Jul 03, 2020 at 10:47:22AM +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
> ---
> tools/perf/builtin-stat.c | 75 ---
> 1 file changed, 55 insertions(+), 20 deletions(-)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 9e4288ecf2b8..5021f7286422 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -485,6 +485,31 @@ static bool handle_interval(unsigned int interval, int
> *times)
> return false;
> }
>
> +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, ) > 0) {
> + switch (cmd) {
> + case EVLIST_CTL_CMD_ENABLE:
> + pr_info(EVLIST_ENABLED_MSG);
> + stop = handle_interval(interval, times);
> + break;
> + case EVLIST_CTL_CMD_DISABLE:
> + stop = handle_interval(interval, times);
I still don't understand why you call handle_interval in here
I don't see it being necessary.. you enable events and handle_interval,
wil be called in the next iteration of dispatch_events, why complicate
this function with that?
thanks,
jirka
