lkml.org 
[lkml]   [2013]   [Feb]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/5] perf, x86: Add PEBSv2 record support

* Andi Kleen <andi@firstfloor.org> wrote:

> From: Andi Kleen <ak@linux.intel.com>
>
> Add support for the v2 PEBS format. It has a superset of the v1 PEBS
> fields, but has a longer record so we need to adjust the code paths.
>
> The main advantage is the new "EventingRip" support which directly
> gives the instruction, not off-by-one instruction. So with precise == 2
> we use that directly and don't try to use LBRs and walking basic blocks.
> This lowers the overhead significantly.
>
> Some other features are added in later patches.
>
> Reviewed-by: Stephane Eranian <eranian@google.com>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
> arch/x86/kernel/cpu/perf_event.c | 2 +-
> arch/x86/kernel/cpu/perf_event_intel_ds.c | 101 ++++++++++++++++++++++-------
> 2 files changed, 79 insertions(+), 24 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index 6774c17..c95290a 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -397,7 +397,7 @@ int x86_pmu_hw_config(struct perf_event *event)
> * check that PEBS LBR correction does not conflict with
> * whatever the user is asking with attr->branch_sample_type
> */
> - if (event->attr.precise_ip > 1) {
> + if (event->attr.precise_ip > 1 && x86_pmu.intel_cap.pebs_format < 2) {
> u64 *br_type = &event->attr.branch_sample_type;
>
> if (has_branch_stack(event)) {
> diff --git a/arch/x86/kernel/cpu/perf_event_intel_ds.c b/arch/x86/kernel/cpu/perf_event_intel_ds.c
> index 826054a..9d0dae0 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel_ds.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c
> @@ -41,6 +41,12 @@ struct pebs_record_nhm {
> u64 status, dla, dse, lat;
> };
>
> +struct pebs_record_v2 {
> + struct pebs_record_nhm nhm;
> + u64 eventingrip;
> + u64 tsx_tuning;

'eventingrip' should be something more readable and obvious.
Also, comments for fields are needed when introducing new
structs.

> +};
> +
> void init_debug_store_on_cpu(int cpu)
> {
> struct debug_store *ds = per_cpu(cpu_hw_events, cpu).ds;
> @@ -559,8 +565,7 @@ static void __intel_pmu_pebs_event(struct perf_event *event,
> {
> /*
> * We cast to pebs_record_core since that is a subset of
> - * both formats and we don't use the other fields in this
> - * routine.
> + * both formats.
> */
> struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
> struct pebs_record_core *pebs = __pebs;
> @@ -588,7 +593,10 @@ static void __intel_pmu_pebs_event(struct perf_event *event,
> regs.bp = pebs->bp;
> regs.sp = pebs->sp;
>
> - if (event->attr.precise_ip > 1 && intel_pmu_pebs_fixup_ip(&regs))
> + if (event->attr.precise_ip > 1 && x86_pmu.intel_cap.pebs_format >= 2) {
> + regs.ip = ((struct pebs_record_v2 *)pebs)->eventingrip;
> + regs.flags |= PERF_EFLAGS_EXACT;

Here you messed up the record types in this function with the
introduction of the new pebs record format quite a bit.

First we have 'pebs' as type 'struct pebs_record_core *', but
here it's cast over to 'struct pebs_record_v2 *' just to access
a field that is not really in the pebs_record_core format...

If we continue down that road then code quickly becomes an
unmaintainable mess. I'm sure you can think of a better,
cleaner solution than mixing types and forcing casts like
that.

> + } else if (event->attr.precise_ip > 1 && intel_pmu_pebs_fixup_ip(&regs))
> regs.flags |= PERF_EFLAGS_EXACT;
> else
> regs.flags &= ~PERF_EFLAGS_EXACT;
> @@ -641,35 +649,21 @@ static void intel_pmu_drain_pebs_core(struct pt_regs *iregs)
> __intel_pmu_pebs_event(event, iregs, at);
> }
>
> -static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs)
> +static void intel_pmu_drain_pebs_common(struct pt_regs *iregs, void *at,
> + void *top)
> {
> struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
> struct debug_store *ds = cpuc->ds;
> - struct pebs_record_nhm *at, *top;
> struct perf_event *event = NULL;
> u64 status = 0;
> - int bit, n;
> -
> - if (!x86_pmu.pebs_active)
> - return;
> -
> - at = (struct pebs_record_nhm *)(unsigned long)ds->pebs_buffer_base;
> - top = (struct pebs_record_nhm *)(unsigned long)ds->pebs_index;
> + int bit;
>
> ds->pebs_index = ds->pebs_buffer_base;
>
> - n = top - at;
> - if (n <= 0)
> - return;
> + for ( ; at < top; at += x86_pmu.pebs_record_size) {
> + struct pebs_record_nhm *p = at;

This function shows record type confusion as well: the function
got renamed from intel_pmu_drain_pebs_nhm() to
intel_pmu_drain_pebs_common(), but then it deals with
pebs_record_nhm - which is obviously not 'common'.

> + case 2:
> + printk(KERN_CONT "PEBS fmt2%c, ", pebs_type);
> + x86_pmu.pebs_record_size = sizeof(struct pebs_record_v2);
> + x86_pmu.drain_pebs = intel_pmu_drain_pebs_v2;
> + break;
> +

That's inconsistent too - the data types and the code talks
about 'PEBS v2' but here it's printed out to the kernel log as
'PEBS fmt2'. Death by a thousand cuts of sloppiness and all
that.

Thanks,

Ingo


\
 
 \ /
  Last update: 2013-02-03 12:21    [W:0.105 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site