Re: [PATCH v1 0/2] perf stat: Support --all-kernel and --all-user

["Jin, Yao" <yao.jin@linux.intel.com>]

On 10/11/2019 3:21 PM, Jiri Olsa wrote:
> On Fri, Oct 11, 2019 at 10:50:35AM +0800, Jin, Yao wrote:
>>
>>
>> On 10/10/2019 8:33 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Thu, Oct 10, 2019 at 04:33:57PM +0800, Jin, Yao escreveu:
>>>>
>>>>
>>>> On 10/10/2019 4:00 PM, Jiri Olsa wrote:
>>>>> On Thu, Oct 10, 2019 at 02:46:36PM +0800, Jin, Yao wrote:
>>>>>>
>>>>>>
>>>>>> On 10/1/2019 10:17 AM, Andi Kleen wrote:
>>>>>>>>> I think it's useful. Makes it easy to do kernel/user break downs.
>>>>>>>>> perf record should support the same.
>>>>>>>>
>>>>>>>> Don't we have this already with:
>>>>>>>>
>>>>>>>> [root@quaco ~]# perf stat -e cycles:u,instructions:u,cycles:k,instructions:k -a -- sleep 1
>>>>>>>
>>>>>>> This only works for simple cases. Try it for --topdown or multiple -M metrics.
>>>>>>>
>>>>>>> -Andi
>>>>>>>
>>>>>>
>>>>>> Hi Arnaldo, Jiri,
>>>>>>
>>>>>> We think it should be very useful if --all-user / --all-kernel can be
>>>>>> specified together, so that we can get a break down between user and kernel
>>>>>> easily.
>>>>>>
>>>>>> But yes, the patches for supporting this new semantics is much complicated
>>>>>> than the patch which just follows original perf-record behavior. I fully
>>>>>> understand this concern.
>>>>>>
>>>>>> So if this new semantics can be accepted, that would be very good. But if
>>>>>> you think the new semantics is too complicated, I'm also fine for posting a
>>>>>> new patch which just follows the perf-record behavior.
>>>>>
>>>>> I still need to think a bit more about this.. did you consider
>>>>> other options like cloning of the perf_evlist/perf_evsel and
>>>>> changing just the exclude* bits? might be event worse actualy ;-)
>>>>>
>>>>
>>>> That should be another approach, but it might be a bit more complicated than
>>>> just appending ":u"/":k" modifiers to the event name string.
>>>>
>>>>> or maybe if we add modifier we could add extra events/groups
>>>>> within the parser.. like:
>>>>>
>>>>>      "{cycles,instructions}:A,{cache-misses,cache-references}:A,cycles:A"
>>>>>
>>>>> but that might be still more complicated then what you did
>>>>>
>>>>
>>>> Yes agree.
>>>>
>>>>> also please add the perf record changes so we have same code
>>>>> and logic for both if we are going to change it
>>>> If this new semantics can be accepted, I'd like to add perf record
>>>> supporting as well. :)
>>>
>>> Changes in semantics should be avoided, when we add an option already
>>> present in some other tool, we should strive to keep the semantics, so
>>> that people can reuse their knowledge and just switch tools to go from
>>> sampling to counting, say.
>>>
>>
>> Yes, that makes sense. We need to try our best to keep the original
>> semantics. I will post a patch for perf-stat which just follows the
>> semantics in perf-record.
>>
>>> So if at all possible, and without having really looked deep in this
>>> specific case, I would prefer that new semantics come with a new syntax,
>>> would that be possible?
>>>
>>
>> Yes, that's possible. Maybe we can use a new option for automatically adding
>> two copies of the events (one copy for user and the other copy for kernel).
>> The option something like "--all-space"?
> 
> some other ideas:
> 
> 	--all
> 	--uk
> 	--both
> 	--full
> 	-e {cycles,cache-misses}:A,cycles,instructions:A
> 	-e {cycles,cache-misses}:B,cycles,instructions:B
> 	--duplicate-every-event-or-group-of-events-for-each-address-space ;-)
> 
> jirka
> 

I like '--uk'. :)

Thanks
Jin Yao