Messages in this thread | | | Date | Mon, 14 Sep 2015 11:31:42 +0200 | From | Ingo Molnar <> | Subject | Re: [RFC][PATCH 2/3] perf: Fix u16 overflows |
| |
* Peter Zijlstra <peterz@infradead.org> wrote:
> On Sat, Sep 12, 2015 at 10:11:20AM +0200, Ingo Molnar wrote: > > > > * Peter Zijlstra <peterz@infradead.org> wrote: > > > > > Vince reported that its possible to overflow the various size fields > > > and get weird stuff if you stick too many events in a group. > > > > > > Put a lid on this by requiring the fixed record size not exceed 16k. > > > This is still a fair amount of events (silly amount really) and leaves > > > plenty room for callchains and stack dwarves while also avoiding > > > overflowing the u16 variables. > > > > Does this leave a natural ABI extension route here, in case in the future it > > becomes a problem? We should take aside a value to mean 'larger record' or such? > > So this all is a result of: > > struct perf_event_header { > __u32 type; > __u16 misc; > __u16 size; > }; > > And we've not even done the 'sensible' thing of interpreting @size as > @size*8 :/ That is, because entries must be u64 aligned, the lower 3 > bits of @size will always be 0. > > Now there are of course ways we can 'grow' if we really have to. One > would be to set aside a MISC bit to indicate we should do that *8 thing, > which would allow up to 512 Kb records. > > __u32 type; > __u16 misc; > __u16 size; > };
Makes sense!
Btw., it appears that header->type is using only about 4 bits at the moment, out of 32.
So future extensions could split it into two and use the other __u16 half as more header->misc fields, should we run out of them (we seem to be close to). Such user-space requesting extended misc bits would have to parse the new format records.
> That said, 64k is already quite a lot of data, and I'm not sure we want to have > records bigger than that. Certainly not for samples, copying that much data on > an interrupt is just not going to be fast. > > And I'm not sure there's a sensible use-case for having this many events in a > group (and there's good reasons not to do it). > > In any case, the patch only pokes at internal stuff, the ABI isn't affected > beyond refusing to create humongous groups.
Fair enough!
Thanks,
Ingo
| |