[v8,04/13] perf stat: factor out body of event handling loop for system wide
diff mbox series

Message ID eeeff629-925a-530b-9803-f274337ae473@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:37 a.m. UTC
Introduce process_timeout() and process_interval() functions that
factor out body of event handling loop for attach and system wide
monitoring use cases.

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

Comments

Jiri Olsa June 23, 2020, 2:56 p.m. UTC | #1
On Wed, Jun 17, 2020 at 11:37:43AM +0300, Alexey Budankov wrote:
> 
> Introduce process_timeout() and process_interval() functions that
> factor out body of event handling loop for attach and system wide
> monitoring use cases.
> 
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> ---
>  tools/perf/builtin-stat.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 9be020e0098a..31f7ccf9537b 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -475,6 +475,23 @@ static void process_interval(void)
>  	print_counters(&rs, 0, NULL);
>  }
>  
> +static bool print_interval(unsigned int interval, int *times)
> +{
> +	if (interval) {
> +		process_interval();
> +		if (interval_count && !(--(*times)))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static bool process_timeout(int timeout, unsigned int interval, int *times)
> +{
> +	if (timeout)
> +		return true;
> +	return print_interval(interval, times);
> +}

I think it's confusing to keep this together, that
process_timeout triggers also interval processing

I think you can keep the timeout separated from interval
processing and rename the print_interval to process_interval
and process_interval to __process_interval

jirka

> +
>  static void enable_counters(void)
>  {
>  	if (stat_config.initial_delay)
> @@ -611,6 +628,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>  	struct affinity affinity;
>  	int i, cpu;
>  	bool second_pass = false;
> +	bool stop = false;
>  
>  	if (interval) {
>  		ts.tv_sec  = interval / USEC_PER_MSEC;
> @@ -805,17 +823,11 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>  			psignal(WTERMSIG(status), argv[0]);
>  	} else {
>  		enable_counters();
> -		while (!done) {
> +		while (!done && !stop) {
>  			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;
> -			}
> +			stop = process_timeout(timeout, interval, &times);
>  		}
>  	}
>  
> -- 
> 2.24.1
> 
>
Alexey Budankov June 24, 2020, 2:27 p.m. UTC | #2
On 23.06.2020 17:56, Jiri Olsa wrote:
> On Wed, Jun 17, 2020 at 11:37:43AM +0300, Alexey Budankov wrote:
>>
>> Introduce process_timeout() and process_interval() functions that
>> factor out body of event handling loop for attach and system wide
>> monitoring use cases.
>>
>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>> ---
>>  tools/perf/builtin-stat.c | 28 ++++++++++++++++++++--------
>>  1 file changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index 9be020e0098a..31f7ccf9537b 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -475,6 +475,23 @@ static void process_interval(void)
>>  	print_counters(&rs, 0, NULL);
>>  }
>>  
>> +static bool print_interval(unsigned int interval, int *times)
>> +{
>> +	if (interval) {
>> +		process_interval();
>> +		if (interval_count && !(--(*times)))
>> +			return true;
>> +	}
>> +	return false;
>> +}
>> +
>> +static bool process_timeout(int timeout, unsigned int interval, int *times)
>> +{
>> +	if (timeout)
>> +		return true;
>> +	return print_interval(interval, times);
>> +}
> 
> I think it's confusing to keep this together, that
> process_timeout triggers also interval processing
> 
> I think you can keep the timeout separated from interval
> processing and rename the print_interval to process_interval
> and process_interval to __process_interval

Well, ok.

I will rename process_interval() to __process_interval() and
then print_interval() to process_interval().

Regarding timeout let's have it like this:

static bool process_timeout(int timeout)
{
	return timeout ? true : false;
}

static bool process_timing_settings(int timeout, unsigned int interval, int *times)
{
        bool res = process_timeout(timeout);
        if (!res)
		res = process_interval(interval, times);
	return res; 
}

Ok?

~Alexey

> 
> jirka
> 
>> +
>>  static void enable_counters(void)
>>  {
>>  	if (stat_config.initial_delay)
>> @@ -611,6 +628,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>>  	struct affinity affinity;
>>  	int i, cpu;
>>  	bool second_pass = false;
>> +	bool stop = false;
>>  
>>  	if (interval) {
>>  		ts.tv_sec  = interval / USEC_PER_MSEC;
>> @@ -805,17 +823,11 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>>  			psignal(WTERMSIG(status), argv[0]);
>>  	} else {
>>  		enable_counters();
>> -		while (!done) {
>> +		while (!done && !stop) {
>>  			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;
>> -			}
>> +			stop = process_timeout(timeout, interval, &times);
>>  		}
>>  	}
>>  
>> -- 
>> 2.24.1
>>
>>
>
Jiri Olsa June 25, 2020, 12:17 p.m. UTC | #3
On Wed, Jun 24, 2020 at 05:27:41PM +0300, Alexey Budankov wrote:
> 
> On 23.06.2020 17:56, Jiri Olsa wrote:
> > On Wed, Jun 17, 2020 at 11:37:43AM +0300, Alexey Budankov wrote:
> >>
> >> Introduce process_timeout() and process_interval() functions that
> >> factor out body of event handling loop for attach and system wide
> >> monitoring use cases.
> >>
> >> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> >> ---
> >>  tools/perf/builtin-stat.c | 28 ++++++++++++++++++++--------
> >>  1 file changed, 20 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> >> index 9be020e0098a..31f7ccf9537b 100644
> >> --- a/tools/perf/builtin-stat.c
> >> +++ b/tools/perf/builtin-stat.c
> >> @@ -475,6 +475,23 @@ static void process_interval(void)
> >>  	print_counters(&rs, 0, NULL);
> >>  }
> >>  
> >> +static bool print_interval(unsigned int interval, int *times)
> >> +{
> >> +	if (interval) {
> >> +		process_interval();
> >> +		if (interval_count && !(--(*times)))
> >> +			return true;
> >> +	}
> >> +	return false;
> >> +}
> >> +
> >> +static bool process_timeout(int timeout, unsigned int interval, int *times)
> >> +{
> >> +	if (timeout)
> >> +		return true;
> >> +	return print_interval(interval, times);
> >> +}
> > 
> > I think it's confusing to keep this together, that
> > process_timeout triggers also interval processing
> > 
> > I think you can keep the timeout separated from interval
> > processing and rename the print_interval to process_interval
> > and process_interval to __process_interval
> 
> Well, ok.
> 
> I will rename process_interval() to __process_interval() and
> then print_interval() to process_interval().
> 
> Regarding timeout let's have it like this:
> 
> static bool process_timeout(int timeout)
> {
> 	return timeout ? true : false;
> }

can't this just stay as value check after finished poll?

	if (timeout)
		break;

and then separate call to process_interval(interval, times)?

jirka

> 
> static bool process_timing_settings(int timeout, unsigned int interval, int *times)
> {
>         bool res = process_timeout(timeout);
>         if (!res)
> 		res = process_interval(interval, times);
> 	return res; 
> }
> 
> Ok?
> 
> ~Alexey
> 
> > 
> > jirka
> > 
> >> +
> >>  static void enable_counters(void)
> >>  {
> >>  	if (stat_config.initial_delay)
> >> @@ -611,6 +628,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> >>  	struct affinity affinity;
> >>  	int i, cpu;
> >>  	bool second_pass = false;
> >> +	bool stop = false;
> >>  
> >>  	if (interval) {
> >>  		ts.tv_sec  = interval / USEC_PER_MSEC;
> >> @@ -805,17 +823,11 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> >>  			psignal(WTERMSIG(status), argv[0]);
> >>  	} else {
> >>  		enable_counters();
> >> -		while (!done) {
> >> +		while (!done && !stop) {
> >>  			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;
> >> -			}
> >> +			stop = process_timeout(timeout, interval, &times);
> >>  		}
> >>  	}
> >>  
> >> -- 
> >> 2.24.1
> >>
> >>
> > 
>
Alexey Budankov June 25, 2020, 4:01 p.m. UTC | #4
On 25.06.2020 15:17, Jiri Olsa wrote:
> On Wed, Jun 24, 2020 at 05:27:41PM +0300, Alexey Budankov wrote:
>>
>> On 23.06.2020 17:56, Jiri Olsa wrote:
>>> On Wed, Jun 17, 2020 at 11:37:43AM +0300, Alexey Budankov wrote:
>>>>
>>>> Introduce process_timeout() and process_interval() functions that
>>>> factor out body of event handling loop for attach and system wide
>>>> monitoring use cases.
>>>>
>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>> ---
>>>>  tools/perf/builtin-stat.c | 28 ++++++++++++++++++++--------
>>>>  1 file changed, 20 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>>>> index 9be020e0098a..31f7ccf9537b 100644
>>>> --- a/tools/perf/builtin-stat.c
>>>> +++ b/tools/perf/builtin-stat.c
>>>> @@ -475,6 +475,23 @@ static void process_interval(void)
>>>>  	print_counters(&rs, 0, NULL);
>>>>  }
>>>>  
>>>> +static bool print_interval(unsigned int interval, int *times)
>>>> +{
>>>> +	if (interval) {
>>>> +		process_interval();
>>>> +		if (interval_count && !(--(*times)))
>>>> +			return true;
>>>> +	}
>>>> +	return false;
>>>> +}
>>>> +
>>>> +static bool process_timeout(int timeout, unsigned int interval, int *times)
>>>> +{
>>>> +	if (timeout)
>>>> +		return true;
>>>> +	return print_interval(interval, times);
>>>> +}
>>>
>>> I think it's confusing to keep this together, that
>>> process_timeout triggers also interval processing
>>>
>>> I think you can keep the timeout separated from interval
>>> processing and rename the print_interval to process_interval
>>> and process_interval to __process_interval
>>
>> Well, ok.
>>
>> I will rename process_interval() to __process_interval() and
>> then print_interval() to process_interval().
>>
>> Regarding timeout let's have it like this:
>>
>> static bool process_timeout(int timeout)
>> {
>> 	return timeout ? true : false;
>> }
> 
> can't this just stay as value check after finished poll?
> 
> 	if (timeout)
> 		break;
> 
> and then separate call to process_interval(interval, times)?

Like this? Still makes sense to have it in a single function.

static bool process_timing_settings(int timeout, unsigned int interval, int *times)
{
	bool res = timeout ? true : false;
        if (!res)
 		res = process_interval(interval, times);
 	return res;
}

~Alexey
Jiri Olsa June 25, 2020, 5:13 p.m. UTC | #5
On Thu, Jun 25, 2020 at 07:01:08PM +0300, Alexey Budankov wrote:

SNIP

> >>
> >> Well, ok.
> >>
> >> I will rename process_interval() to __process_interval() and
> >> then print_interval() to process_interval().
> >>
> >> Regarding timeout let's have it like this:
> >>
> >> static bool process_timeout(int timeout)
> >> {
> >> 	return timeout ? true : false;
> >> }
> > 
> > can't this just stay as value check after finished poll?
> > 
> > 	if (timeout)
> > 		break;
> > 
> > and then separate call to process_interval(interval, times)?
> 
> Like this? Still makes sense to have it in a single function.
> 
> static bool process_timing_settings(int timeout, unsigned int interval, int *times)
> {
> 	bool res = timeout ? true : false;
>         if (!res)
>  		res = process_interval(interval, times);
>  	return res;
> }

I don't see the connection between timeout and interval
IMO this just complicates things, is there a problem to
keep it separated as it is now?

jirka

> 
> ~Alexey
>
Alexey Budankov June 25, 2020, 6:43 p.m. UTC | #6
On 25.06.2020 20:13, Jiri Olsa wrote:
> On Thu, Jun 25, 2020 at 07:01:08PM +0300, Alexey Budankov wrote:
> 
> SNIP
> 
>>>>
>>>> Well, ok.
>>>>
>>>> I will rename process_interval() to __process_interval() and
>>>> then print_interval() to process_interval().
>>>>
>>>> Regarding timeout let's have it like this:
>>>>
>>>> static bool process_timeout(int timeout)
>>>> {
>>>> 	return timeout ? true : false;
>>>> }
>>>
>>> can't this just stay as value check after finished poll?
>>>
>>> 	if (timeout)
>>> 		break;
>>>
>>> and then separate call to process_interval(interval, times)?
>>
>> Like this? Still makes sense to have it in a single function.
>>
>> static bool process_timing_settings(int timeout, unsigned int interval, int *times)
>> {
>> 	bool res = timeout ? true : false;
>>         if (!res)
>>  		res = process_interval(interval, times);
>>  	return res;
>> }
> 
> I don't see the connection between timeout and interval
> IMO this just complicates things, is there a problem to
> keep it separated as it is now?

Not a problem. Can duplicate it in dispatch_events().

~Alexey

Patch
diff mbox series

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 9be020e0098a..31f7ccf9537b 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -475,6 +475,23 @@  static void process_interval(void)
 	print_counters(&rs, 0, NULL);
 }
 
+static bool print_interval(unsigned int interval, int *times)
+{
+	if (interval) {
+		process_interval();
+		if (interval_count && !(--(*times)))
+			return true;
+	}
+	return false;
+}
+
+static bool process_timeout(int timeout, unsigned int interval, int *times)
+{
+	if (timeout)
+		return true;
+	return print_interval(interval, times);
+}
+
 static void enable_counters(void)
 {
 	if (stat_config.initial_delay)
@@ -611,6 +628,7 @@  static int __run_perf_stat(int argc, const char **argv, int run_idx)
 	struct affinity affinity;
 	int i, cpu;
 	bool second_pass = false;
+	bool stop = false;
 
 	if (interval) {
 		ts.tv_sec  = interval / USEC_PER_MSEC;
@@ -805,17 +823,11 @@  static int __run_perf_stat(int argc, const char **argv, int run_idx)
 			psignal(WTERMSIG(status), argv[0]);
 	} else {
 		enable_counters();
-		while (!done) {
+		while (!done && !stop) {
 			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;
-			}
+			stop = process_timeout(timeout, interval, &times);
 		}
 	}