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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 31 Oct 2012 15:23:52 +0100
From:	Stephane Eranian <eranian@...gle.com>
To:	Namhyung Kim <namhyung@...nel.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	"mingo@...e.hu" <mingo@...e.hu>,
	"ak@...ux.intel.com" <ak@...ux.intel.com>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Jiri Olsa <jolsa@...hat.com>, Lin Ming <ming.m.lin@...el.com>
Subject: Re: [Patch v1 10/10] perf tools: add new mem command for memory
 access profiling

On Wed, Oct 31, 2012 at 7:57 AM, Namhyung Kim <namhyung@...nel.org> wrote:
> 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@...gle.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.
>
Yes, defaults to load. Fixed.

> 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)
>
Fixed.

>
>> +
>> +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.
>
Done.

>> +
>> +-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?
>
Using -D to be consistent with report.

>> +
>> +-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.
>
>> +
No it's -x there too.

>> +-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'").
>
>
Yes. Fixed.

>> +static const char    *mem_operation          = MEM_OPERATION_LOAD;
>> +static const char    *csv_sep                = NULL;
>
> Why not use symbol_conf.field_sep?
>
Done.

>> +
>> +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.
>
Yes.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/