Re: [PATCH 11/12] perf, x86: implement AMD IBS event configuration

[Robert Richter <robert.richter@amd.com>]

On 21.04.10 13:37:27, Stephane Eranian wrote:
> Robert,
> 
> Some more comments about model_spec.
> 
> > Except for the sample period IBS can only be set up with raw (model
> > specific) config values and raw data samples. The event attributes for
> > the syscall should be programmed like this (IBS_FETCH):
> >
> >        memset(&attr, 0, sizeof(attr));
> >        attr.type        = PERF_TYPE_RAW;
> >        attr.sample_type = PERF_SAMPLE_CPU | PERF_SAMPLE_RAW;
> >        attr.config      = IBS_FETCH_CONFIG_DEFAULT
> >        attr.config     |=
> >                ((unsigned long long)MODEL_SPEC_TYPE_IBS_FETCH << 32)
> >                & MODEL_SPEC_TYPE_MASK;
> >        attr.model_spec  = 1;
> >
> Why do you need model_spec, in addition to your special encoding?
> 
> >  /*
> > + * Model specific hardware events
> > + *
> > + * With the attr.model_spec bit set we can setup hardware events
> > + * others than generic performance counters. A special PMU 64 bit
> > + * config value can be passed through the perf_event interface. The
> > + * concept of PMU model-specific arguments was practiced already in
> > + * Perfmon2. The type of event (8 bits) is determinded from the config
> > + * value too, bit 32-39 are reserved for this.
> > + */
> Isn't the config field big enough to encode all the information you need?
> In the kernel, you could check bit 32-39 and based on host CPU determine
> whether it refers to IBS or is a bogus value. I am trying to figure out what
> model_spec buys you. I believe RAW does not mean the final value as
> accepted by HW but a value that must be interpreted by the model-specific
> code to eventually come up with a raw HW value. In the current code, the
> RAW value is never passed as is, it is assembled from various bits and
> pieces incl. attr.config of course.

What about introducing a raw_type? We could reuse the unused bp_type
space for this. There is no longer the model_spec flag necessary and
we could pass the raw 64 bit config value without shifting bits to
encode the type there.

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 4924c96..c9b51b3 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -222,7 +222,10 @@ struct perf_event_attr {
                __u32           wakeup_watermark; /* bytes before wakeup   */
        };
 
-       __u32                   bp_type;
+       union {
+               __u32           bp_type;
+               __u32           raw_type;
+       }
        __u64                   bp_addr;
        __u64                   bp_len;
 };

Raw type would be architecure specific, e.g. for x86:

enum perf_raw_type {
        PERF_RAW_PERFCTR                        = 0,
        PERF_RAW_IBS_FETCH                      = 1,
        PERF_RAW_IBS_OP                         = 2,

        PERF_RAW_MAX,
};

Null is the architecture's default.

The raw event syntax could be suffixed by the type (as for
breakpoints):

   -e rNNN[:TYPE]

Example:

 perf record -e r186A:1          # ... meaning IBD fetch, cycle count 100000
 perf record -e r0:1 -c 100000   # ... the same

Or with named types:

 perf record -e r186A:IBS_FETCH ...
 perf record -e r0:IBS_FETCH -c 100000 ...

I would prefer this solution as it has a number of advantages: A raw
event type may be specified without to encode the type in the config
value. The attr flags are not 'polluted'. We can follow the already
existing breakpoint concept in syntax and encoding.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center
email: robert.richter@amd.com