| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20130203103500.GD9330@gmail.com>
Date: Sun, 3 Feb 2013 11:35:00 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Andi Kleen <andi@...stfloor.org>
Cc: linux-kernel@...r.kernel.org, a.p.zijlstra@...llo.nl,
akpm@...ux-foundation.org, acme@...hat.com, eranian@...gle.com,
jolsa@...hat.com, namhyung@...nel.org,
Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH 1/5] perf, x86: Add PEBSv2 record support
* Andi Kleen <andi@...stfloor.org> wrote:
> From: Andi Kleen <ak@...ux.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@...gle.com>
> Signed-off-by: Andi Kleen <ak@...ux.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(®s))
> + 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(®s))
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists