Re: [Patch v1 10/10] perf tools: add new mem command for memory access profiling

[Namhyung Kim <namhyung@kernel.org>]

On Mon, 29 Oct 2012 16:15:52 +0100, Stephane Eranian wrote:
> This new command is a wrapper on top of perf record and
> perf report to make it easier to configure for memory
> access profiling.

So this new command will be run only on speicific (PEBS > 2?) Intel
machines, right?  Is there anything we can do for others?  Or at least
it might emit a warning message..

>
> To record loads:
> $ perf mem -t load rec .....
>
> To record stores:
> $ perf mem -t store rec .....
>
> To get the report:
> $ perf mem -t load rep
>
> Signed-off-by: Stephane Eranian <eranian@google.com>
> ---
[snip]
> +perf-mem(1)
> +===========
> +
> +NAME
> +----
> +perf-mem - Profile memory accesses
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'perf mem' -t load record <command>
> +'perf mem' -t store record <command>
> +'perf mem' -t load report
> +'perf mem' -t store report

Is '-t' option mandatory?  AFAISC it seems optional and defaults to load.

And is <command> for record also mandatory?  Doesn't 'perf record' make
it optional?

If so, the above can be written like you did in 'mem_usage':

'perf mem' [<options>] (record [<command>] | report)


> +
> +DESCRIPTION
> +-----------
> +"perf mem -t <TYPE> record" runs a command and gathers memory operation data
> +from it, into perf.data. Perf record options are accepted and are passed through.
> +
> +"perf mem -t <TYPE> report" displays the result. It invokes perf report with the
> +right set of options to display a memory access profile.
> +
> +OPTIONS
> +-------
> +<command>...::
> +	Any command you can specify in a shell.
> +
> +-t::
> +--type=::
> +	Select the memory operation type: load or store

It'd better saying it defaults to load.

> +
> +-R::
> +--dump-raw-samples=::
> +	Dump the raw decoded samples on the screen in a format that is easy to parse with
> +	one sample per line.

Didn't we usually use -D switch for this?

> +
> +-x::
> +--field-separator::
> +	Specify the field separator used when dump raw samples (-R option). By default,
> +	The separator is the space character.

And using -t for this will make it consistent with perf report IMHO.

> +
> +-C::
> +--cpu-list::
> +	Restrict dump of raw samples to those provided via this option. Note that the same
> +	option can be passed in record mode. It will be interpreted the same way as perf
> +	record.
> +
> +SEE ALSO
> +--------
> +linkperf:perf-record[1], linkperf:perf-report[1]
[snip]
> +#define MEM_OPERATION_LOAD	"load"
> +#define MEM_OPERATION_STORE	"store"
> +
> +static char const	*input_name		= "perf.data";

We have a global 'input_name' as of commit 70cb4e963f77 ("perf tools:
Add a global variable 'const char *input_name'").


> +static const char	*mem_operation		= MEM_OPERATION_LOAD;
> +static const char	*csv_sep		= NULL;

Why not use symbol_conf.field_sep?

> +
> +struct perf_mem {
> +	struct perf_tool	tool;
> +	char const		*input_name;
> +	symbol_filter_t		annotate_init;
> +	bool			hide_unresolved;
> +	bool			dump_raw;
> +	const char		*cpu_list;
> +	DECLARE_BITMAP(cpu_bitmap, MAX_NR_CPUS);
> +};
> +
> +static const char * const mem_usage[] = {
> +	"perf mem [<options>] {record <command> |report}",
> +	NULL
> +};
[snip]
> +static int report_raw_events(struct perf_mem *mem)
> +{
> +	int err = -EINVAL;
> +	int ret;
> +	struct perf_session *session = perf_session__new(input_name, O_RDONLY,
> +							 0, false, &mem->tool);
> +
> +	if (mem->cpu_list) {
> +		ret = perf_session__cpu_bitmap(session, mem->cpu_list,
> +					       mem->cpu_bitmap);
> +		if (ret)
> +			goto out_delete;
> +	}
> +
> +	if (symbol__init() < 0)
> +		return -1;
> +
> +	if (session == NULL)
> +		return -ENOMEM;

This check should be moved before perf_session__cpu_bitmap() calls.

Thanks,
Namhyung

> +
> +	printf("# PID, TID, IP, ADDR, COST, DSRC, SYMBOL\n");
> +
> +	err = perf_session__process_events(session, &mem->tool);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +
> +out_delete:
> +	perf_session__delete(session);
> +	return err;
> +}