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

[Jiri Olsa <jolsa@redhat.com>]

On Thu, Nov 18, 2021 at 10:51:45AM -0600, Kim Phillips wrote:
> On 11/18/21 1:28 AM, Sandipan Das wrote:
> > 
> > On 11/16/2021 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 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.
> > > 
> > 
> > Sorry I didn't know that events could be programmed with "config=...".
> > Thanks for pointing that out. For basic '-e r...' usage, modifying
> > parse_events_add_numeric() and leaving parse_events_add_pmu() as is
> > should take care of it.
> > 
> > I thought about using the same fixup for the pmu format i.e. cpu/r0xNNNNN/
> > but as you rightly mentioned, it breaks when using cpu/config=0xNNNNN/.
> 
> I think we'd be breaking compatibility with changing -r,
> even if the two (0x100 and 0x200) event bits used for most
> three-digit AMD events are being masked.
> 
> Shall we just add an AMD-specific real-world 0xE0000UUEE example
> to the {stat,record} documentation to clear the mud?

sorry for late answer, but that seems to me like good idea

jirka