Re: [RFC PATCH] tools/perf/x86: Use alternative format for AMD raw events

[kajoljain <kjain@linux.ibm.com>]

On 11/16/21 9:31 PM, Kim Phillips wrote:
> On 11/15/21 10:38 PM, kajoljain wrote:
>> On 11/15/21 3:28 PM, Sandipan Das wrote:
>>>
>>> On 11/14/2021 10:11 PM, Jiri Olsa wrote:
>>>> good question.. I see raw events as a way to put to config whatever
>>>> the user wants and IMO adding changes/quirks that silently changes
>>>> config could create confusion and angry users ;-)
>>>>
>>
>> I agree, making quirks in config will only going to confuse users as
>> It's against the encodings we had in
>> /sys/bus/event_source/devices/<pmu>/format/*
>>
>>>> I'd think that if user is composing/using raw events then using
>>>> directly r100001f8e is not such a big deal?
>>>>
>>>
>>> Sure. Some of the confusion may be due to the fact that the man pages
>>> for perf-{stat,record,top} state that raw events are "eventsel+umask".
>>> While that is technically true, it does not describe the encoding
>>> scheme (/sys/bus/event_source/devices/<pmu>/format/*)
>>>
>>> Would another option be to update the man pages with a reference to
>>> these sysfs files when describing raw events?
>>>
>>> Something like:
>>>
>>> diff --git a/tools/perf/Documentation/perf-record.txt
>>> b/tools/perf/Documentation/perf-record.txt
>>> index 2d7df8703cf2..5dfdfeba594b 100644
>>> --- a/tools/perf/Documentation/perf-record.txt
>>> +++ b/tools/perf/Documentation/perf-record.txt
>>> @@ -30,8 +30,10 @@ OPTIONS
>>>
>>>           - a symbolic event name        (use 'perf list' to list all
>>> events)
>>>
>>> -        - a raw PMU event (eventsel+umask) in the form of rNNN where
>>> NNN is a
>>> -         hexadecimal event descriptor.
>>> +        - a raw PMU event (eventsel+umask) in the form of rN..NN
>>> where N..NN
>>> +          is a hexadecimal value representing the raw encoding with
>>> the layout
>>> +          of the corresponding event control register as defined by
>>> entries in
>>> +          /sys/bus/event_sources/devices/<pmu>/format/*
>>
>> Do we need to specify (eventsel+umask) in the raw event description? As
>> the format/fields totally depend on PMU and umask name convention is
>> specific to one arch.
>>
>> Can we just update it to:
>>
>> - a raw PMU event in the form of rNNN where NNN is a hexadecimal value
>> representing the raw encoding, with the layout of the corresponding
>> event control register as defined by entries in
>> /sys/bus/event_sources/devices/<pmu>/format/*
> 
> The r notation is for the cpu pmu only.
> 
> The triple-digit 'NNN' is what's most misleading for 12-bit
> event implementation users, such as AMD's core PMUs.  It
> tells users 'see your processor's documentation's triple-
> hex-digit PMCx18e event?  All you need to do is make that
> "-e r18e" on the perf stat/record command line.
> 
> So all notions of what size of parameter 'r' takes, esp. 'NNN'
> should be removed IMO.  Perhaps an AMD/12-bit-specific
> example can be provided.

I agree. Maybe we can remove specification of digits with r
and separately specify for  different scenario like AMD/12-bit-specific.
Can we use something like r<hex-digit> ?

Thanks,
Kajol Jain

> 
> I had thought that the shorthand command line r spec alone
> could be modified to be smarter and more accommodating
> for conventional rUUEE users migrating to AMD/12-bit,
> which AFAICT is not what this patch does.  I think it
> modifies even the  cpu/config=0xNNNNN/ specification,
> which, granted, is bad.
> 
> Thanks,
> 
> Kim