Re: [PATCH 4/5] perf, x86: Add INST_RETIRED.ALL workarounds

[Stephane Eranian]

On Thu, Aug 14, 2014 at 3:17 AM, Andi Kleen <andi@xxxxxxxxxxxxxx> wrote:
> From: Andi Kleen <ak@xxxxxxxxxxxxxxx>
>
> On Broadwell INST_RETIRED.ALL cannot be used with any period
> that doesn't have the lowest 6 bits cleared. And the period
> should not be smaller than 128.
>
If you have frequency mode enabled, then I suspect this works okay.
It may be that the kernel will keep trying to set a period lower than 128
but you will correct it.

I am more worried about the case where the user sets a fixed period
with some of the bottom 6 bits set.  The apps thinks it is sampling at
X occurences per sample, when it is in fact at X - 63 (worst case).
I think this would be okay, if there was a trace of this in the sampling
buffer, i.e., PERF_SAMPLE_PERIOD. But I recall that perf record
does not request this flag when using a fixed period. So no way
of knowing what the actual period was, at least when using the
perf tool. I understand also that for this event, 64 occurrences
may not matter as much, maybe except if you use some filters
such as cmask.


> Add a new callback to enforce this, and set it for Broadwell.
>
> This is erratum BDM57 and BDM11.
>
> v2: Use correct event name in description. Use EVENT() macro.
> Signed-off-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/perf_event.c       |  3 +++
>  arch/x86/kernel/cpu/perf_event.h       |  1 +
>  arch/x86/kernel/cpu/perf_event_intel.c | 19 +++++++++++++++++++
>  3 files changed, 23 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index 0adc5e3..a0adf58 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -980,6 +980,9 @@ int x86_perf_event_set_period(struct perf_event *event)
>         if (left > x86_pmu.max_period)
>                 left = x86_pmu.max_period;
>
> +       if (x86_pmu.limit_period)
> +               left = x86_pmu.limit_period(event, left);
> +
>         per_cpu(pmc_prev_left[idx], smp_processor_id()) = left;
>
>         /*
> diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
> index de81627..a46e391 100644
> --- a/arch/x86/kernel/cpu/perf_event.h
> +++ b/arch/x86/kernel/cpu/perf_event.h
> @@ -456,6 +456,7 @@ struct x86_pmu {
>         struct x86_pmu_quirk *quirks;
>         int             perfctr_second_write;
>         bool            late_ack;
> +       unsigned        (*limit_period)(struct perf_event *event, unsigned l);
>
>         /*
>          * sysfs attrs
> diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
> index 4bfb0ec..66260e1 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -2034,6 +2034,24 @@ hsw_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event)
>         return c;
>  }
>
> +/*
> + * Broadwell:
> + * The INST_RETIRED.ALL period always needs to have lowest
> + * 6bits cleared (BDM57). It shall not use a period smaller
> + * than 100 (BDM11). We combine the two to enforce
> + * a min-period of 128.
> + */
> +static unsigned bdw_limit_period(struct perf_event *event, unsigned left)
> +{
> +       if ((event->hw.config & 0xffff) ==
> +                       X86_CONFIG(.event=0xc0, .umask=0x01)) {
> +               if (left < 128)
> +                       left = 128;
> +               left &= ~0x3fu;
> +       }
> +       return left;
> +}
> +
>  PMU_FORMAT_ATTR(event, "config:0-7"    );
>  PMU_FORMAT_ATTR(umask, "config:8-15"   );
>  PMU_FORMAT_ATTR(edge,  "config:18"     );
> @@ -2711,6 +2729,7 @@ __init int intel_pmu_init(void)
>                 x86_pmu.hw_config = hsw_hw_config;
>                 x86_pmu.get_event_constraints = hsw_get_event_constraints;
>                 x86_pmu.cpu_events = hsw_events_attrs;
> +               x86_pmu.limit_period = bdw_limit_period;
>                 pr_cont("Broadwell events, ");
>                 break;
>
> --
> 1.9.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/