From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755455AbbINJcU (ORCPT ); Mon, 14 Sep 2015 05:32:20 -0400 Received: from mail-wi0-f170.google.com ([209.85.212.170]:36301 "EHLO mail-wi0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755391AbbINJbq (ORCPT ); Mon, 14 Sep 2015 05:31:46 -0400 Date: Mon, 14 Sep 2015 11:31:42 +0200 From: Ingo Molnar To: Peter Zijlstra Cc: linux-kernel@vger.kernel.org, vincent.weaver@maine.edu, acme@kernel.org, eranian@google.com, jolsa@redhat.com, alexander.shishkin@linux.intel.com Subject: Re: [RFC][PATCH 2/3] perf: Fix u16 overflows Message-ID: <20150914093142.GA24362@gmail.com> References: <20150910161621.363774906@infradead.org> <20150910162007.946742153@infradead.org> <20150912081120.GB9737@gmail.com> <20150914090632.GO18489@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150914090632.GO18489@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Peter Zijlstra wrote: > On Sat, Sep 12, 2015 at 10:11:20AM +0200, Ingo Molnar wrote: > > > > * Peter Zijlstra 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