RE: [PATCH 1/1] perf,stat: reduce interval-print to 10ms

[Liang, Kan]

> > SNIP
> >
> > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > > index a96fb5c..5ef88f7 100644
> > > --- a/tools/perf/builtin-stat.c
> > > +++ b/tools/perf/builtin-stat.c
> > > @@ -1179,7 +1179,7 @@ int cmd_stat(int argc, const char **argv, const
> char *prefix __maybe_unused)
> > >  	OPT_STRING(0, "post", &post_cmd, "command",
> > >  			"command to run after to the measured
> command"),
> > >  	OPT_UINTEGER('I', "interval-print", &stat_config.interval,
> > > -		    "print counts at regular interval in ms (>= 100)"),
> > > +		    "print counts at regular interval in ms (>= 10)"),
> > >  	OPT_SET_UINT(0, "per-socket", &stat_config.aggr_mode,
> > >  		     "aggregate counts per processor socket",
> AGGR_SOCKET),
> > >  	OPT_SET_UINT(0, "per-core", &stat_config.aggr_mode, @@ -
> 1332,9
> > > +1332,14 @@ int cmd_stat(int argc, const char **argv, const char
> *prefix __maybe_unused)
> > >  		thread_map__read_comms(evsel_list->threads);
> > >
> > >  	if (interval && interval < 100) {
> > > -		pr_err("print interval must be >= 100ms\n");
> > > -		parse_options_usage(stat_usage, options, "I", 1);
> > > -		goto out;
> > > +		if (interval < 10) {
> > > +			pr_err("print interval must be >= 10ms\n");
> > > +			parse_options_usage(stat_usage, 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");
> >
> > with '-I' 10 this warning flies away quite fast ;-)
> >
> > I guess it's better than nothing, and I'm not sure about putting some
> > sleep after that warning..
> >
> > Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> 
> Right, this should appear in the man page as well, I added the patch below,
> we could also print that warning when the workload ends.
> 

OK. We can print the warning message again at the end as the patch below.

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 5ef88f7..01209c7 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1377,6 +1377,11 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (!forever && status != -1 && !interval)
 		print_counters(NULL, argc, argv);
 
+	if (interval && interval < 100)
+		pr_warning("print interval < 100ms. "
+			   "The overhead percentage could be high in some cases. "
+			   "Please proceed with caution.\n");
+
 	perf_evlist__free_stats(evsel_list);
 out:
 	perf_evlist__delete(evsel_list);

Thanks,
Kan

> - Arnaldo
> 
> diff --git a/tools/perf/Documentation/perf-stat.txt
> b/tools/perf/Documentation/perf-stat.txt
> index 7586fe08ac9a..641caf66ff26 100644
> --- a/tools/perf/Documentation/perf-stat.txt
> +++ b/tools/perf/Documentation/perf-stat.txt
> @@ -130,6 +130,8 @@ perf stat --repeat 10 --null --sync --pre 'make -s
> O=defconfig-build/clean' -- m  --interval-print msecs::
>  	Print count deltas every N milliseconds (minimum: 10ms)
>  	example: perf stat -I 1000 -e cycles -a sleep 5
> +	The overhead percentage could be high in some cases, for instance
> +	with small, sub 100ms intervals.  Use with caution.
> 
>  --per-socket::
>  Aggregate counts per processor socket for system-wide mode
> measurements.  This
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/