Re: [PATCH v1] perf stat: enable 1ms interval for printing event counters values

[Alexey Budankov]

On 03.04.2018 18:22, Jiri Olsa wrote:
> On Tue, Apr 03, 2018 at 06:04:13PM +0300, Alexey Budankov wrote:
>>
>> Currently print count interval for performance counters values is 
>> limited by 10ms so reading the values at frequencies higher than 100Hz 
>> is restricted by the tool.
>>
>> This change makes perf stat -I possible on frequencies up to 1KHz and, 
>> to some extent, makes perf stat -I to be on-par with perf record 
>> sampling profiling.
>>
>> When running perf stat -I for monitoring e.g. PCIe uncore counters and 
>> at the same time profiling some I/O workload by perf record e.g. for 
>> cpu-cycles and context switches, it is then possible to observe 
>> consolidated CPU/OS/IO(Uncore) performance picture for that workload.
>>
>> Tool overhead warning printed when specifying -v option can be missed 
>> due to screen scrolling in case you have output to the console 
>> so message is moved into help available by running perf stat -h.
>>
>> Signed-off-by: Alexey Budankov 
>> ---
>>  tools/perf/builtin-stat.c | 14 ++
>>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> I don't mind taking out the limit, but please update also
> the stat man page, it mentiones the 10ms minimum

Yep, will do. Thanks for noticing this.

Thanks,
Alexey

> 
> thanks,
> jirka
> 
>>
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index f5c454855908..147a27e8c937 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -1943,7 +1943,8 @@ static const struct option stat_options[] = {
>>  OPT_STRING(0, "post", _cmd, "command",
>>  "command to run after to the measured command"),
>>  OPT_UINTEGER('I', "interval-print", _config.interval,
>> -"print counts at regular interval in ms (>= 10)"),
>> +"print counts at regular interval in ms "
>> +"(overhead is possible for values <= 100ms)"),
>>  OPT_INTEGER(0, "interval-count", _config.times,
>>  "print counts for fixed number of times"),
>>  OPT_UINTEGER(0, "timeout", _config.timeout,
>> @@ -2923,17 +2924,6 @@ int cmd_stat(int argc, const char **argv)
>>  }
>>  }
>>  
>> -if (interval && interval < 100) {
>> -if (interval < 10) {
>> -pr_err("print interval must be >= 10ms\n");
>> -parse_options_usage(stat_usage, stat_options, "I", 1);
>> -goto out;
>> -} else
>> -pr_warning("print interval < 100ms. "
>> -   "The overhead percentage could be high in 
>> some cases. "
>> -   "Please proceed with caution.\n");
>> -}
>> -
>>  if (stat_config.times && interval)
>>  interval_count = true;
>>  else if (stat_config.times && !interval) {
> 

Re: [PATCH v1] perf stat: enable 1ms interval for printing event counters values

[Alexey Budankov]

On 03.04.2018 18:22, Jiri Olsa wrote:
> On Tue, Apr 03, 2018 at 06:04:13PM +0300, Alexey Budankov wrote:
>>
>> Currently print count interval for performance counters values is 
>> limited by 10ms so reading the values at frequencies higher than 100Hz 
>> is restricted by the tool.
>>
>> This change makes perf stat -I possible on frequencies up to 1KHz and, 
>> to some extent, makes perf stat -I to be on-par with perf record 
>> sampling profiling.
>>
>> When running perf stat -I for monitoring e.g. PCIe uncore counters and 
>> at the same time profiling some I/O workload by perf record e.g. for 
>> cpu-cycles and context switches, it is then possible to observe 
>> consolidated CPU/OS/IO(Uncore) performance picture for that workload.
>>
>> Tool overhead warning printed when specifying -v option can be missed 
>> due to screen scrolling in case you have output to the console 
>> so message is moved into help available by running perf stat -h.
>>
>> Signed-off-by: Alexey Budankov 
>> ---
>>  tools/perf/builtin-stat.c | 14 ++
>>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> I don't mind taking out the limit, but please update also
> the stat man page, it mentiones the 10ms minimum

Yep, will do. Thanks for noticing this.

Thanks,
Alexey

> 
> thanks,
> jirka
> 
>>
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index f5c454855908..147a27e8c937 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -1943,7 +1943,8 @@ static const struct option stat_options[] = {
>>  OPT_STRING(0, "post", _cmd, "command",
>>  "command to run after to the measured command"),
>>  OPT_UINTEGER('I', "interval-print", _config.interval,
>> -"print counts at regular interval in ms (>= 10)"),
>> +"print counts at regular interval in ms "
>> +"(overhead is possible for values <= 100ms)"),
>>  OPT_INTEGER(0, "interval-count", _config.times,
>>  "print counts for fixed number of times"),
>>  OPT_UINTEGER(0, "timeout", _config.timeout,
>> @@ -2923,17 +2924,6 @@ int cmd_stat(int argc, const char **argv)
>>  }
>>  }
>>  
>> -if (interval && interval < 100) {
>> -if (interval < 10) {
>> -pr_err("print interval must be >= 10ms\n");
>> -parse_options_usage(stat_usage, stat_options, "I", 1);
>> -goto out;
>> -} else
>> -pr_warning("print interval < 100ms. "
>> -   "The overhead percentage could be high in 
>> some cases. "
>> -   "Please proceed with caution.\n");
>> -}
>> -
>>  if (stat_config.times && interval)
>>  interval_count = true;
>>  else if (stat_config.times && !interval) {
> 

Re: [PATCH v1] perf stat: enable 1ms interval for printing event counters values

[Jiri Olsa]

On Tue, Apr 03, 2018 at 06:04:13PM +0300, Alexey Budankov wrote:
> 
> Currently print count interval for performance counters values is 
> limited by 10ms so reading the values at frequencies higher than 100Hz 
> is restricted by the tool.
> 
> This change makes perf stat -I possible on frequencies up to 1KHz and, 
> to some extent, makes perf stat -I to be on-par with perf record 
> sampling profiling.
> 
> When running perf stat -I for monitoring e.g. PCIe uncore counters and 
> at the same time profiling some I/O workload by perf record e.g. for 
> cpu-cycles and context switches, it is then possible to observe 
> consolidated CPU/OS/IO(Uncore) performance picture for that workload.
> 
> Tool overhead warning printed when specifying -v option can be missed 
> due to screen scrolling in case you have output to the console 
> so message is moved into help available by running perf stat -h.
> 
> Signed-off-by: Alexey Budankov 
> ---
>  tools/perf/builtin-stat.c | 14 ++
>  1 file changed, 2 insertions(+), 12 deletions(-)

I don't mind taking out the limit, but please update also
the stat man page, it mentiones the 10ms minimum

thanks,
jirka

> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index f5c454855908..147a27e8c937 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1943,7 +1943,8 @@ static const struct option stat_options[] = {
>   OPT_STRING(0, "post", _cmd, "command",
>   "command to run after to the measured command"),
>   OPT_UINTEGER('I', "interval-print", _config.interval,
> - "print counts at regular interval in ms (>= 10)"),
> + "print counts at regular interval in ms "
> + "(overhead is possible for values <= 100ms)"),
>   OPT_INTEGER(0, "interval-count", _config.times,
>   "print counts for fixed number of times"),
>   OPT_UINTEGER(0, "timeout", _config.timeout,
> @@ -2923,17 +2924,6 @@ int cmd_stat(int argc, const char **argv)
>   }
>   }
>  
> - if (interval && interval < 100) {
> - if (interval < 10) {
> - pr_err("print interval must be >= 10ms\n");
> - parse_options_usage(stat_usage, stat_options, "I", 1);
> - goto out;
> - } else
> - pr_warning("print interval < 100ms. "
> -"The overhead percentage could be high in 
> some cases. "
> -"Please proceed with caution.\n");
> - }
> -
>   if (stat_config.times && interval)
>   interval_count = true;
>   else if (stat_config.times && !interval) {

Re: [PATCH v1] perf stat: enable 1ms interval for printing event counters values

[Jiri Olsa]

On Tue, Apr 03, 2018 at 06:04:13PM +0300, Alexey Budankov wrote:
> 
> Currently print count interval for performance counters values is 
> limited by 10ms so reading the values at frequencies higher than 100Hz 
> is restricted by the tool.
> 
> This change makes perf stat -I possible on frequencies up to 1KHz and, 
> to some extent, makes perf stat -I to be on-par with perf record 
> sampling profiling.
> 
> When running perf stat -I for monitoring e.g. PCIe uncore counters and 
> at the same time profiling some I/O workload by perf record e.g. for 
> cpu-cycles and context switches, it is then possible to observe 
> consolidated CPU/OS/IO(Uncore) performance picture for that workload.
> 
> Tool overhead warning printed when specifying -v option can be missed 
> due to screen scrolling in case you have output to the console 
> so message is moved into help available by running perf stat -h.
> 
> Signed-off-by: Alexey Budankov 
> ---
>  tools/perf/builtin-stat.c | 14 ++
>  1 file changed, 2 insertions(+), 12 deletions(-)

I don't mind taking out the limit, but please update also
the stat man page, it mentiones the 10ms minimum

thanks,
jirka

> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index f5c454855908..147a27e8c937 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1943,7 +1943,8 @@ static const struct option stat_options[] = {
>   OPT_STRING(0, "post", _cmd, "command",
>   "command to run after to the measured command"),
>   OPT_UINTEGER('I', "interval-print", _config.interval,
> - "print counts at regular interval in ms (>= 10)"),
> + "print counts at regular interval in ms "
> + "(overhead is possible for values <= 100ms)"),
>   OPT_INTEGER(0, "interval-count", _config.times,
>   "print counts for fixed number of times"),
>   OPT_UINTEGER(0, "timeout", _config.timeout,
> @@ -2923,17 +2924,6 @@ int cmd_stat(int argc, const char **argv)
>   }
>   }
>  
> - if (interval && interval < 100) {
> - if (interval < 10) {
> - pr_err("print interval must be >= 10ms\n");
> - parse_options_usage(stat_usage, stat_options, "I", 1);
> - goto out;
> - } else
> - pr_warning("print interval < 100ms. "
> -"The overhead percentage could be high in 
> some cases. "
> -"Please proceed with caution.\n");
> - }
> -
>   if (stat_config.times && interval)
>   interval_count = true;
>   else if (stat_config.times && !interval) {