Re: [PATCH V9 3/6] perf, record: introduce --freq-perf option

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Tue, Sep 08, 2015 at 03:32:46PM -0400, kan.liang@intel.com escreveu:
> From: Kan Liang <kan.liang@intel.com>
> 
> To generate the frequency and performance output, perf must sample read
> special events like cycles, ref-cycles, msr/tsc/, msr/aperf/ or
> msr/mperf/.
> With the --freq-perf option, perf record can automatically check and add
> those event into evlist for sampling read.
> 
> Signed-off-by: Kan Liang <kan.liang@intel.com>
> ---
>  tools/perf/Documentation/perf-record.txt |  4 ++++
>  tools/perf/builtin-record.c              | 39 +++++++++++++++++++++++++++++++-
>  tools/perf/util/session.h                | 10 ++++++++
>  3 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
> index 2e9ce77..3f40712 100644
> --- a/tools/perf/Documentation/perf-record.txt
> +++ b/tools/perf/Documentation/perf-record.txt
> @@ -308,6 +308,10 @@ This option sets the time out limit. The default value is 500 ms.
>  Record context switch events i.e. events of type PERF_RECORD_SWITCH or
>  PERF_RECORD_SWITCH_CPU_WIDE.
>  
> +--freq-perf::
> +Add frequency and performance related events to do sample read.
> +These events include cycles, ref-cycles, msr/tsc/, msr/aperf/ and msr/mperf/.
> +
>  SEE ALSO
>  --------
>  linkperf:perf-stat[1], linkperf:perf-list[1]
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 142eeb3..e87dda3 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -13,7 +13,7 @@
>  #include "util/util.h"
>  #include "util/parse-options.h"
>  #include "util/parse-events.h"
> -
> +#include "util/pmu.h"
>  #include "util/callchain.h"
>  #include "util/cgroup.h"
>  #include "util/header.h"
> @@ -50,6 +50,7 @@ struct record {
>  	bool			no_buildid;
>  	bool			no_buildid_cache;
>  	long			samples;
> +	bool			freq_perf;
>  };
>  
>  static int record__write(struct record *rec, void *bf, size_t size)
> @@ -948,6 +949,35 @@ out_free:
>  	return ret;
>  }
>  
> +const char *freq_perf_events[FREQ_PERF_MAX][3] = {
> +	{ "msr", "tsc", "msr/tsc/" },
> +	{ "msr", "aperf", "msr/aperf/" },
> +	{ "msr", "mperf", "msr/mperf/" },
> +	{ NULL, "cycles", "cycles" },
> +	{ NULL, "ref-cycles", "ref-cycles" },
> +};
> +
> +static int
> +record_add_freq_perf_events(struct perf_evlist *evlist)
> +{
> +	int i;
> +	char freq_perf_attrs[100];
> +
> +	strcpy(freq_perf_attrs, "{cycles,ref-cycles");
> +	for (i = 0; i < FREQ_PERF_MAX; i++) {
> +		if ((i == FREQ_PERF_CYCLES) ||
> +		    (i == FREQ_PERF_REF_CYCLES))
> +			continue;
> +		if (pmu_have_event(freq_perf_events[i][0], freq_perf_events[i][1])) {
> +			strcat(freq_perf_attrs, ",");
> +			strcat(freq_perf_attrs, freq_perf_events[i][2]);
> +		}
> +	}
> +	strcat(freq_perf_attrs, "}:S");
> +
> +	return parse_events(evlist, freq_perf_attrs, NULL);
> +}
> +
>  static const char * const __record_usage[] = {
>  	"perf record [<options>] [<command>]",
>  	"perf record [<options>] -- <command> [<options>]",
> @@ -1096,6 +1126,8 @@ struct option __record_options[] = {
>  			"per thread proc mmap processing timeout in ms"),
>  	OPT_BOOLEAN(0, "switch-events", &record.opts.record_switch_events,
>  		    "Record context switch events"),
> +	OPT_BOOLEAN(0, "freq-perf", &record.freq_perf,
> +		    "Add frequency and performance related events to do sample read."),

	Isn't this too vague? Can you try rewriting it? what do you mean
with "performance related events"?

	You forgot to add the entry to
tools/perf/Documentation/perf-record.txt, where a less terse explanation
could have helped me understand the part above, oops, sorry, you added
it, but it was really terse! Can you elaborate a bit? Why would one want
to use this? To achieve what? Don't assume everybody knows :-)

>  	OPT_END()
>  };
>  
> @@ -1157,6 +1189,11 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
>  	if (rec->no_buildid_cache || rec->no_buildid)
>  		disable_buildid_cache();
>  
> +	if (rec->freq_perf && record_add_freq_perf_events(rec->evlist)) {
> +		pr_err("Cannot set up freq and performance events\n");
> +		goto out_symbol_exit;
> +	}
> +
>  	if (rec->evlist->nr_entries == 0 &&
>  	    perf_evlist__add_default(rec->evlist) < 0) {
>  		pr_err("Not enough memory for event selector list\n");
> diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
> index b44afc7..3915be7 100644
> --- a/tools/perf/util/session.h
> +++ b/tools/perf/util/session.h
> @@ -42,6 +42,16 @@ struct perf_session {
>  #define PRINT_IP_OPT_ONELINE	(1<<4)
>  #define PRINT_IP_OPT_SRCLINE	(1<<5)
>  
> +enum perf_freq_perf_index {

That is really an overly long enum name! What about:

	enum perf_freqs {
		PERF_FREQ_TSC,
		...
	}

> +	FREQ_PERF_TSC		= 0,
> +	FREQ_PERF_APERF		= 1,
> +	FREQ_PERF_MPERF		= 2,
> +	FREQ_PERF_CYCLES	= 3,
> +	FREQ_PERF_REF_CYCLES	= 4,
> +
> +	FREQ_PERF_MAX
> +};
> +
>  struct perf_tool;
>  
>  struct perf_session *perf_session__new(struct perf_data_file *file,
> -- 
> 1.8.3.1