perf_events: fix bogus warn_on(_once) in perf_prepare_sample()
Subsystems (2)
PATCH 1/1 perf_events: fix bogus warn_on(_once) in perf_prepare_sample()
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.
kernel/perf_event.c +0 -1
@@ -3311,7 +3311,6 @@ void perf_prepare_sample(struct perf_event_header *header,else size +=sizeof (u32); -WARN_ON_ONCE (size & (sizeof (u64)-1 )); header->size += size; }
Peter Zijlstra
maintainerQuoted 6 lines · Stephane Eranian
There is a
warn_on_once()check forPERF_SAMPLE_RAWwhich 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.
Peter Zijlstra
maintainerQuoted 8 lines · Peter Zijlstra
Quoted 6 lines · Stephane Eranian
There is a
warn_on_once()check forPERF_SAMPLE_RAWwhich 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.
Quoted 4 lines
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 <[email protected]>
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.
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(-)
arch/x86/include/asm/perf_event.h +0 -9
@@ -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)
arch/x86/kernel/cpu/perf_event_intel_ds.c +60 -73
204 lines not on this page · raw message
include/linux/perf_event.h +5 -2
@@ -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 ; }
kernel/perf_event.c +6 -0
@@ -3158,6 +3158,9 @@ void perf_output_sample(struct perf_outpif (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);
tools/perf/builtin-top.c +8 -1
@@ -975,8 +975,10 @@ static void event__process_sample(constreturn ; } -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 ;
On Thu, Apr 8, 2010 at 11:03 PM, Peter Zijlstra <[email protected]> wrote:> 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.>You always have the original IP in the PEBS sample, so why add yet another wayof getting to the same data?
Quoted 3 lines
---> Subject: perf: Change the PEBS interface> From: Peter Zijlstra <[email protected]>> Date: Tue Mar 30 13:35:58 CEST 2010>> This patch changes the PEBS interface from
perf_event_attr::precise> andPERF_RECORD_MISC_EXACTtoperf_event_attr::preciseand>PERF_SAMPLE_EVENT_IP.>> The rationale is that .precise=1 !PERF_SAMPLE_EVENT_IP could be used> to implement buffered PEBS.>I am not sure I understand what you mean by buffered PEBS. Are you talkingabout using PEBS buffer bigger than one entry?If so, how can you do: - the LBR based fixups for multiple samples (on PMU interrupt) - attribute PID/TID in system-wide mode
Peter Zijlstra
maintainerQuoted 2 lines · Stephane Eranian
I am not sure I understand what you mean by buffered PEBS. Are you talking about using PEBS buffer bigger than one entry?
Yep.
Quoted 2 lines
If so, how can you do:
- the LBR based fixups for multiple samples (on PMU interrupt)
Not, so it would be the offset on.
Quoted 1 line
- attribute PID/TID in system-wide mode
flush on context switches.
Quoted 11 lines · Peter Zijlstra
Quoted 2 lines · Stephane Eranian
I am not sure I understand what you mean by buffered PEBS. Are you talking about using PEBS buffer bigger than one entry?
Yep.
Quoted 2 lines
If so, how can you do:
- the LBR based fixups for multiple samples (on PMU interrupt)Not, so it would be the offset on.
Not sure I understand what you mean here.
Quoted 4 lines
Quoted 1 line
- attribute PID/TID in system-wide mode
flush on context switches.
cost?
Quoted 10 lines · Peter Zijlstra
Quoted 6 lines · Stephane Eranian
There is a
warn_on_once()check forPERF_SAMPLE_RAWwhich 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.
Are you suggesting you add some padding the PEBS raw sample you return as PERF_SAMPLE_RAW? Then you need to define what RAW actually means? Seems here, it would mean more than what the HW returns.
Quoted 3 lines
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.
I assume this has to do with the wrap-around detection.
Peter Zijlstra
maintainerQuoted 5 lines · Stephane Eranian
Are you suggesting you add some padding the PEBS raw sample you return as
PERF_SAMPLE_RAW? Then you need to define what RAW actually means? Seems here, it would mean more than what the HW returns.
Well, RAW doesn't mean anything much at all, its really a fugly pass some crap around thing.
So yeah, adding padding seems just fine.
Quoted 7 lines · Peter Zijlstra
Quoted 5 lines · Stephane Eranian
Are you suggesting you add some padding the PEBS raw sample you return as
PERF_SAMPLE_RAW? Then you need to define what RAW actually means? Seems here, it would mean more than what the HW returns.
Quoted 5 lines
Well, RAW doesn't mean anything much at all, its really a fugly pass some crap around thing.
So yeah, adding padding seems just fine.
I would rather see size as u64. Who's using raw today anyway?
The PEBS record itself is always a multiple of u64. The size header is the one causing the problems.
Quoted 1 line
Quoted 8 lines · Stephane Eranian
Quoted 7 lines · Peter Zijlstra
Quoted 5 lines · Stephane Eranian
Are you suggesting you add some padding the PEBS raw sample you return as
PERF_SAMPLE_RAW? Then you need to define what RAW actually means? Seems here, it would mean more than what the HW returns.
Quoted 6 lines
Quoted 5 lines
Well, RAW doesn't mean anything much at all, its really a fugly pass some crap around thing.
So yeah, adding padding seems just fine.
I would rather see size as u64. Who's using raw today anyway?
The trace events. Hence the size of the size shouldn't be touched, it is an ABI now.
Quoted 12 lines · Frederic Weisbecker
Quoted 11 lines · Stephane Eranian
Quoted 10 lines · Peter Zijlstra
Quoted 5 lines · Stephane Eranian
Are you suggesting you add some padding the PEBS raw sample you return as
PERF_SAMPLE_RAW? Then you need to define what RAW actually means? Seems here, it would mean more than what the HW returns.Well, RAW doesn't mean anything much at all, its really a fugly pass some crap around thing.
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines
So yeah, adding padding seems just fine.
I would rather see size as u64. Who's using raw today anyway?
The trace events. Hence the size of the size shouldn't be touched, it is an ABI now.
Given your alignment constraints, it seems like it was a bad choice to pick u32 for size to begin with.
Quoted 15 lines · Stephane Eranian
Quoted 14 lines · Frederic Weisbecker
Quoted 13 lines · Stephane Eranian
Quoted 12 lines · Peter Zijlstra
Quoted 5 lines · Stephane Eranian
Are you suggesting you add some padding the PEBS raw sample you return as
PERF_SAMPLE_RAW? Then you need to define what RAW actually means? Seems here, it would mean more than what the HW returns.Well, RAW doesn't mean anything much at all, its really a fugly pass some crap around thing.
So yeah, adding padding seems just fine.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
I would rather see size as u64. Who's using raw today anyway?
The trace events. Hence the size of the size shouldn't be touched, it is an ABI now.
Given your alignment constraints, it seems like it was a bad choice to pick u32 for size to begin with.
Indeed, I'm not exactly sure how this is dealt since this is indeed u32 and the buffer requires to align to u64...
Quoted 15 lines · Stephane Eranian
Quoted 14 lines · Frederic Weisbecker
Quoted 13 lines · Stephane Eranian
Quoted 12 lines · Peter Zijlstra
Quoted 5 lines · Stephane Eranian
Are you suggesting you add some padding the PEBS raw sample you return as
PERF_SAMPLE_RAW? Then you need to define what RAW actually means? Seems here, it would mean more than what the HW returns.Well, RAW doesn't mean anything much at all, its really a fugly pass some crap around thing.
So yeah, adding padding seems just fine.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
I would rather see size as u64. Who's using raw today anyway?
The trace events. Hence the size of the size shouldn't be touched, it is an ABI now.
Given your alignment constraints, it seems like it was a bad choice to pick u32 for size to begin with.
Ah I remember now how we did that. We align the trace events raw sample size such that sizeof(raw_sample) + sizeof(size) is aligned to u64.
Well, indeed that complexifies a bit the raw_sample size handling but at least it makes the traces a bit more compact.
Peter Zijlstra
maintainerQuoted 8 lines · Stephane Eranian
Quoted 7 lines · Peter Zijlstra
Quoted 5 lines · Stephane Eranian
Are you suggesting you add some padding the PEBS raw sample you return as
PERF_SAMPLE_RAW? Then you need to define what RAW actually means? Seems here, it would mean more than what the HW returns.
Quoted 6 lines
Quoted 5 lines
Well, RAW doesn't mean anything much at all, its really a fugly pass some crap around thing.
So yeah, adding padding seems just fine.
I would rather see size as u64. Who's using raw today anyway?
the tracepoint stuff does
Peter Zijlstra
maintainerQuoted 5 lines · Stephane Eranian
Quoted 4 lines
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.
I assume this has to do with the wrap-around detection.
Nah, mostly dealing with architectures that don't do well with unaligned accesses.
Peter Zijlstra
maintainerQuoted 9 lines · Stephane Eranian
Quoted 8 lines · Peter Zijlstra
Quoted 6 lines · Stephane Eranian
There is a
warn_on_once()check forPERF_SAMPLE_RAWwhich 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.
Quoted 6 lines
Quoted 2 lines
PEBS hasn't seen -linus yet, so we can fix that.
Are you suggesting you add some padding the PEBS raw sample you return as
PERF_SAMPLE_RAW? Then you need to define what RAW actually means? Seems here, it would mean more than what the HW returns.
The best fix here is to simply remove the PERF_SAMPLE_RAW support and implement PERF_SAMPLE_REGS. Its just that we need to come up with a way to deal with compat pt_regs muck.
Quoted 12 lines · Peter Zijlstra
Quoted 11 lines · Stephane Eranian
Quoted 10 lines · Peter Zijlstra
Quoted 6 lines · Stephane Eranian
There is a
warn_on_once()check forPERF_SAMPLE_RAWwhich 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.
Quoted 10 lines
Quoted 4 lines
Are you suggesting you add some padding the PEBS raw sample you return as
PERF_SAMPLE_RAW? Then you need to define what RAW actually means? Seems here, it would mean more than what the HW returns.The best fix here is to simply remove the
PERF_SAMPLE_RAWsupport and implementPERF_SAMPLE_REGS. Its just that we need to come up with a way to deal with compatpt_regsmuck.
But isn't RAW already part of the ABI?
It is true that you need to also provide the interrupted state (or part of it) to the user, i.e.,
beyond just the IP.
But the interrupt state and PEBS state are distinct and should not be swapped. In fact,
both may be useful at the same time to evaluate the skid.
So yes, you need PERF_SAMPLE_REGS. But just like your proposal with the two IPs. I think you need to export two versions of REGS: IREGS and PREGS for instance. The
issue is that PEBS does not record the whole state but only a very small subset.
Then, on Nehalem, there is PEBS more where you collect more that just machine state,
you collect cache miss information. That's not regs anymore.
Peter Zijlstra
maintainerQuoted 18 lines · Stephane Eranian
Quoted 17 lines · Peter Zijlstra
Quoted 15 lines · Stephane Eranian
Quoted 10 lines · Peter Zijlstra
Quoted 6 lines · Stephane Eranian
There is a
warn_on_once()check forPERF_SAMPLE_RAWwhich 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.
Are you suggesting you add some padding the PEBS raw sample you return as
PERF_SAMPLE_RAW? Then you need to define what RAW actually means? Seems here, it would mean more than what the HW returns.
Quoted 6 lines
Quoted 5 lines
The best fix here is to simply remove the
PERF_SAMPLE_RAWsupport and implementPERF_SAMPLE_REGS. Its just that we need to come up with a way to deal with compatpt_regsmuck.But isn't RAW already part of the ABI?
The existance of it, yes, the content less so.
Quoted 2 lines
It is true that you need to also provide the interrupted state (or part of it) to the user, i.e., beyond just the IP.
Why?
The only thing I would do is maybe use the interrupt state to fill out the PEBS reg data, its mostly things like segment regs that go missing, and for those the interrupt state ought to be good enough.
Quoted 2 lines
But the interrupt state and PEBS state are distinct and should not be swapped. In fact, both may be useful at the same time to evaluate the skid.
That doesn't seem like something a regular person would be interested in, and for those who do they can easily hack the kernel.
I don't think that is a use-case worth expanding the ABI over.
Quoted 6 lines
So yes, you need
PERF_SAMPLE_REGS. But just like your proposal with the two IPs. I think you need to export two versions of REGS: IREGS and PREGS for
instance. The issue is that PEBS does not record the whole state but only a very small subset.Then, on Nehalem, there is PEBS more where you collect more that just machine state, you collect cache miss information. That's not regs anymore.
Yes, the whole load-latency train-wreck is something we need to come up with a sensible interface for, that's definitely not something I would do through RAW.