linux-kernel - Re: [PATCH v3]: perf record: enable arbitrary event names thru name= modifier

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ed38dfcc-be73-e1d7-4eb5-862468955766@linux.intel.com>
Date:   Mon, 4 Jun 2018 17:51:03 +0300
From:   Alexey Budankov <alexey.budankov@...ux.intel.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Jiri Olsa <jolsa@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Andi Kleen <ak@...ux.intel.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v3]: perf record: enable arbitrary event names thru name=
 modifier

Hi,
On 04.06.2018 17:23, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jun 04, 2018 at 05:19:11PM +0300, Alexey Budankov escreveu:
>> Hi,
>>
>> On 04.06.2018 16:58, Arnaldo Carvalho de Melo wrote:
>>> Em Mon, Jun 04, 2018 at 09:56:02AM +0200, Jiri Olsa escreveu:
>>>> On Mon, Jun 04, 2018 at 09:50:56AM +0300, Alexey Budankov wrote:
>>>>> Enable complex event names containing [.:=,] symbols to be encoded into Perf 
>>>>> trace using name= modifier e.g. like this:
>>>
>>>>> perf record -e cpu/name=\'OFFCORE_RESPONSE:request=DEMAND_RFO:response=L3_HIT.SNOOP_HITM\',\
>>>>> 		period=0x3567e0,event=0x3c,cmask=0x1/Duk ./futex
>>>
>>>>> Below is how it looks like in the report output. Please note explicit escaped 
>>>>> quoting at cmdline string in the header so that thestring can be directly reused
>>>>> for another collection in shell:
>>>
>>> Applied, but there are other places where we show event names, such as:
>>>
>>> [root@...et ~]# perf record -e cpu/name=\'OFFCORE_RESPONSE:request=DEMAND_RFO:response=L3_HIT.SNOOP_HITM\',period=0x3567e0,event=0x3c,cmask=0x1/Duk
>>> ^C[ perf record: Woken up 1 times to write data ]
>>> [ perf record: Captured and wrote 1.599 MB perf.data (704 samples) ]
>>>
>>> [root@...et ~]# perf evlist
>>> OFFCORE_RESPONSE:request=DEMAND_RFO:response=L3_HIT.SNOOP_HITM
>>> [root@...et ~]# perf evlist -v
>>> OFFCORE_RESPONSE:request=DEMAND_RFO:response=L3_HIT.SNOOP_HITM: type: 4, size: 112, config: 0x100003c, { sample_period, sample_freq }: 3500000, sample_type: IP|TID|TIME|CPU, disabled: 1, inherit: 1, pinned: 1, exclude_hv: 1, mmap: 1, comm: 1, task: 1, sample_id_all: 1, mmap2: 1, comm_exec: 1
>>> [root@...et ~]# 
>>>
>>> That I used to check if the period, etc were correctly set, etc. Perhaps
>>> we should add that \'\' there as well?
>>
>> Like this?
>>
>> name: 'OFFCORE_RESPONSE:request=DEMAND_RFO:response=L3_HIT.SNOOP_HITM', type: 4, size: 112, config: 0x100003c, { sample_period, sample_freq }: 3500000, sample_type: IP|TID|TIME|CPU, disabled: 1, inherit: 1, pinned: 1, exclude_hv: 1, mmap: 1, comm: 1, task: 1, sample_id_all: 1, mmap2: 1, comm_exec: 1
> 
> Nope, I mean with the same intent you had when showing escaped single
> quotes in the 'perf report' header, that users would copy'n'paste and
> get something that works on the command line?

Well ok, if there are other cases aside perf report that tool shows full 
command line ready for reusing then it definitely makes sense to print 
event names quoted there. If such cases were pointed out then they also 
could be addressed along with the unit/regression testing mentioned above.

Thanks,
Alexey

> 
> - Arnaldo
>  
>> Thanks,
>> Alexey
>>
>>>
>>> Also please consider adding an entry to tools/perf/tests/attr/ to make
>>> sure this is checked everytime we run 'perf test attr' or plain 'perf
>>> test'.
>>>
>>> Those can be followup patches, so I'm applying this one, thanks.
>>>
>>> - Arnaldo
>>>
>