Messages in this thread Patch in this message | | | Subject | Re: [PATCH] perf_events: fix bogus warn_on(_once) in perf_prepare_sample() | From | Peter Zijlstra <> | Date | Thu, 08 Apr 2010 23:03:19 +0200 |
| |
On Thu, 2010-04-08 at 22:55 +0200, Peter Zijlstra wrote: > On Thu, 2010-04-08 at 22:45 +0200, Stephane Eranian wrote: > > There is a warn_on_once() check for PERF_SAMPLE_RAW which trips > > when using PEBS on both Core and Nehalem. Core PEBS sample size is 144 > > bytes and 176 bytes for Nehalem. Both are multiples of 8, but the size > > field is encoded as int, thus the total is never a multiple of 8 which > > trips the check. I think the size should have been u64, but now it is > > too late to change given it is ABI. > > PEBS hasn't seen -linus yet, so we can fix that. > > There's various things that do indeed rely on the perf buffer to always > be u64 aligned, so this warning isn't bogus at all.
On the subject of PEBS, we need to change the ABI before it does hit -linus, I've got something like the below,. but I'm not quite sure of it.
We could keep the PERF_RECORD_MISC_EXACT bit and allow non-fixed up IPs in PERF_SAMPLE_EVENT_IP fields, that way we can avoid having to add both IP and EVENT_IP to samples.
--- Subject: perf: Change the PEBS interface From: Peter Zijlstra <a.p.zijlstra@chello.nl> Date: Tue Mar 30 13:35:58 CEST 2010
This patch changes the PEBS interface from perf_event_attr::precise and PERF_RECORD_MISC_EXACT to perf_event_attr::precise and PERF_SAMPLE_EVENT_IP.
The rationale is that .precise=1 !PERF_SAMPLE_EVENT_IP could be used to implement buffered PEBS.
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> LKML-Reference: <new-submission> --- arch/x86/include/asm/perf_event.h | 9 -- arch/x86/kernel/cpu/perf_event_intel_ds.c | 133 +++++++++++++----------------- include/linux/perf_event.h | 7 + kernel/perf_event.c | 6 + tools/perf/builtin-top.c | 9 +- 5 files changed, 79 insertions(+), 85 deletions(-)
Index: linux-2.6/arch/x86/include/asm/perf_event.h =================================================================== --- linux-2.6.orig/arch/x86/include/asm/perf_event.h +++ linux-2.6/arch/x86/include/asm/perf_event.h @@ -128,21 +128,12 @@ extern void perf_events_lapic_init(void) #define PERF_EVENT_INDEX_OFFSET 0 -/* - * Abuse bit 3 of the cpu eflags register to indicate proper PEBS IP fixups. - * This flag is otherwise unused and ABI specified to be 0, so nobody should - * care what we do with it. - */ -#define PERF_EFLAGS_EXACT (1UL << 3) - #define perf_misc_flags(regs) \ ({ int misc = 0; \ if (user_mode(regs)) \ misc |= PERF_RECORD_MISC_USER; \ else \ misc |= PERF_RECORD_MISC_KERNEL; \ - if (regs->flags & PERF_EFLAGS_EXACT) \ - misc |= PERF_RECORD_MISC_EXACT; \ misc; }) #define perf_instruction_pointer(regs) ((regs)->ip) Index: linux-2.6/arch/x86/kernel/cpu/perf_event_intel_ds.c =================================================================== --- linux-2.6.orig/arch/x86/kernel/cpu/perf_event_intel_ds.c +++ linux-2.6/arch/x86/kernel/cpu/perf_event_intel_ds.c @@ -330,7 +330,8 @@ static void intel_pmu_pebs_enable(struct cpuc->pebs_enabled |= 1ULL << hwc->idx; WARN_ON_ONCE(cpuc->enabled); - if (x86_pmu.intel_cap.pebs_trap) + if (x86_pmu.intel_cap.pebs_trap && + (event->attr.sample_type & PERF_SAMPLE_EVENT_IP)) intel_pmu_lbr_enable(event); } @@ -345,7 +346,8 @@ static void intel_pmu_pebs_disable(struc hwc->config |= ARCH_PERFMON_EVENTSEL_INT; - if (x86_pmu.intel_cap.pebs_trap) + if (x86_pmu.intel_cap.pebs_trap && + (event->attr.sample_type & PERF_SAMPLE_EVENT_IP)) intel_pmu_lbr_disable(event); } @@ -376,12 +378,12 @@ static inline bool kernel_ip(unsigned lo #endif } -static int intel_pmu_pebs_fixup_ip(struct pt_regs *regs) +static int intel_pmu_pebs_fixup_ip(unsigned long *ipp) { struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events); unsigned long from = cpuc->lbr_entries[0].from; unsigned long old_to, to = cpuc->lbr_entries[0].to; - unsigned long ip = regs->ip; + unsigned long ip = *ipp; /* * We don't need to fixup if the PEBS assist is fault like @@ -412,7 +414,7 @@ static int intel_pmu_pebs_fixup_ip(struc * We sampled a branch insn, rewind using the LBR stack */ if (ip == to) { - regs->ip = from; + *ipp = from; return 1; } @@ -439,7 +441,7 @@ static int intel_pmu_pebs_fixup_ip(struc } while (to < ip); if (to == ip) { - regs->ip = old_to; + *ipp = old_to; return 1; } @@ -452,15 +454,62 @@ static int intel_pmu_pebs_fixup_ip(struc static int intel_pmu_save_and_restart(struct perf_event *event); +static void __intel_pmu_pebs_event(struct perf_event *event, + struct pt_regs *iregs, void *__pebs) +{ + /* + * 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. + */ + struct pebs_record_core *pebs = __pebs; + struct perf_sample_data data; + struct perf_raw_record raw; + struct pt_regs regs; + + if (!intel_pmu_save_and_restart(event)) + return; + + perf_sample_data_init(&data, 0); + data.period = event->hw.last_period; + + if (event->attr.sample_type & PERF_SAMPLE_RAW) { + raw.size = x86_pmu.pebs_record_size; + raw.data = pebs; + data.raw = &raw; + } + + /* + * We use the interrupt regs as a base because the PEBS record + * does not contain a full regs set, specifically it seems to + * lack segment descriptors, which get used by things like + * user_mode(). + * + * In the simple case fix up only the IP and BP,SP regs, for + * PERF_SAMPLE_IP and PERF_SAMPLE_CALLCHAIN to function properly. + * A possible PERF_SAMPLE_REGS will have to transfer all regs. + */ + regs = *iregs; + regs.ip = pebs->ip; + regs.bp = pebs->bp; + regs.sp = pebs->sp; + + if (event->attr.sample_type & PERF_SAMPLE_EVENT_IP) { + unsigned long event_ip = pebs->ip; + if (intel_pmu_pebs_fixup_ip(&event_ip)) + data.event_ip = event_ip; + } + + if (perf_event_overflow(event, 1, &data, ®s)) + x86_pmu_stop(event); +} + static void intel_pmu_drain_pebs_core(struct pt_regs *iregs) { struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events); struct debug_store *ds = cpuc->ds; struct perf_event *event = cpuc->events[0]; /* PMC0 only */ struct pebs_record_core *at, *top; - struct perf_sample_data data; - struct perf_raw_record raw; - struct pt_regs regs; int n; if (!ds || !x86_pmu.pebs) @@ -486,9 +535,6 @@ static void intel_pmu_drain_pebs_core(st if (n <= 0) return; - if (!intel_pmu_save_and_restart(event)) - return; - /* * Should not happen, we program the threshold at 1 and do not * set a reset value. @@ -496,37 +542,7 @@ static void intel_pmu_drain_pebs_core(st WARN_ON_ONCE(n > 1); at += n - 1; - perf_sample_data_init(&data, 0); - data.period = event->hw.last_period; - - if (event->attr.sample_type & PERF_SAMPLE_RAW) { - raw.size = x86_pmu.pebs_record_size; - raw.data = at; - data.raw = &raw; - } - - /* - * We use the interrupt regs as a base because the PEBS record - * does not contain a full regs set, specifically it seems to - * lack segment descriptors, which get used by things like - * user_mode(). - * - * In the simple case fix up only the IP and BP,SP regs, for - * PERF_SAMPLE_IP and PERF_SAMPLE_CALLCHAIN to function properly. - * A possible PERF_SAMPLE_REGS will have to transfer all regs. - */ - regs = *iregs; - regs.ip = at->ip; - regs.bp = at->bp; - regs.sp = at->sp; - - if (intel_pmu_pebs_fixup_ip(®s)) - regs.flags |= PERF_EFLAGS_EXACT; - else - regs.flags &= ~PERF_EFLAGS_EXACT; - - if (perf_event_overflow(event, 1, &data, ®s)) - x86_pmu_stop(event); + __intel_pmu_pebs_event(event, iregs, at); } static void intel_pmu_drain_pebs_nhm(struct pt_regs *iregs) @@ -534,10 +550,7 @@ static void intel_pmu_drain_pebs_nhm(str 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_sample_data data; struct perf_event *event = NULL; - struct perf_raw_record raw; - struct pt_regs regs; u64 status = 0; int bit, n; @@ -579,33 +592,7 @@ static void intel_pmu_drain_pebs_nhm(str if (!event || bit >= MAX_PEBS_EVENTS) continue; - if (!intel_pmu_save_and_restart(event)) - continue; - - perf_sample_data_init(&data, 0); - data.period = event->hw.last_period; - - if (event->attr.sample_type & PERF_SAMPLE_RAW) { - raw.size = x86_pmu.pebs_record_size; - raw.data = at; - data.raw = &raw; - } - - /* - * See the comment in intel_pmu_drain_pebs_core() - */ - regs = *iregs; - regs.ip = at->ip; - regs.bp = at->bp; - regs.sp = at->sp; - - if (intel_pmu_pebs_fixup_ip(®s)) - regs.flags |= PERF_EFLAGS_EXACT; - else - regs.flags &= ~PERF_EFLAGS_EXACT; - - if (perf_event_overflow(event, 1, &data, ®s)) - x86_pmu_stop(event); + __intel_pmu_pebs_event(event, iregs, at); } } Index: linux-2.6/include/linux/perf_event.h =================================================================== --- linux-2.6.orig/include/linux/perf_event.h +++ linux-2.6/include/linux/perf_event.h @@ -125,8 +125,9 @@ enum perf_event_sample_format { PERF_SAMPLE_PERIOD = 1U << 8, PERF_SAMPLE_STREAM_ID = 1U << 9, PERF_SAMPLE_RAW = 1U << 10, + PERF_SAMPLE_EVENT_IP = 1U << 11, - PERF_SAMPLE_MAX = 1U << 11, /* non-ABI */ + PERF_SAMPLE_MAX = 1U << 12, /* non-ABI */ }; /* @@ -294,7 +295,6 @@ struct perf_event_mmap_page { #define PERF_RECORD_MISC_USER (2 << 0) #define PERF_RECORD_MISC_HYPERVISOR (3 << 0) -#define PERF_RECORD_MISC_EXACT (1 << 14) /* * Reserve the last bit to indicate some extended misc field */ @@ -389,6 +389,7 @@ enum perf_event_type { * struct perf_event_header header; * * { u64 ip; } && PERF_SAMPLE_IP + * { u64 event_ip; } && PERF_SAMPLE_EVENT_IP * { u32 pid, tid; } && PERF_SAMPLE_TID * { u64 time; } && PERF_SAMPLE_TIME * { u64 addr; } && PERF_SAMPLE_ADDR @@ -804,6 +805,7 @@ struct perf_sample_data { u64 type; u64 ip; + u64 event_ip; struct { u32 pid; u32 tid; @@ -824,6 +826,7 @@ struct perf_sample_data { static inline void perf_sample_data_init(struct perf_sample_data *data, u64 addr) { + data->event_ip = 0; data->addr = addr; data->raw = NULL; } Index: linux-2.6/kernel/perf_event.c =================================================================== --- linux-2.6.orig/kernel/perf_event.c +++ linux-2.6/kernel/perf_event.c @@ -3158,6 +3158,9 @@ void perf_output_sample(struct perf_outp if (sample_type & PERF_SAMPLE_IP) perf_output_put(handle, data->ip); + if (sample_type & PERF_SAMPLE_EVENT_IP) + perf_output_put(handle, data->event_ip); + if (sample_type & PERF_SAMPLE_TID) perf_output_put(handle, data->tid_entry); @@ -3237,6 +3240,9 @@ void perf_prepare_sample(struct perf_eve header->size += sizeof(data->ip); } + if (sample_type & PERF_SAMPLE_EVENT_IP) + header->size += sizeof(data->event_ip); + if (sample_type & PERF_SAMPLE_TID) { /* namespace issues */ data->tid_entry.pid = perf_event_pid(event, current); Index: linux-2.6/tools/perf/builtin-top.c =================================================================== --- linux-2.6.orig/tools/perf/builtin-top.c +++ linux-2.6/tools/perf/builtin-top.c @@ -975,8 +975,10 @@ static void event__process_sample(const return; } - if (self->header.misc & PERF_RECORD_MISC_EXACT) + if (ip) exact_samples++; + else + return; if (event__preprocess_sample(self, session, &al, symbol_filter) < 0 || al.filtered) @@ -1169,6 +1171,11 @@ static void start_counter(int i, int cou attr->sample_type = PERF_SAMPLE_IP | PERF_SAMPLE_TID; + if (attr->precise) { + attr->sample_type &= ~PERF_SAMPLE_IP; + attr->sample_type |= PERF_SAMPLE_EVENT_IP; + } + if (freq) { attr->sample_type |= PERF_SAMPLE_PERIOD; attr->freq = 1;
| |