Re: [RFC][PATCH 2/3] perf: Fix u16 overflows

[Ingo Molnar <mingo@kernel.org>]

* 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