From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1429001AbcBTBzk (ORCPT ); Fri, 19 Feb 2016 20:55:40 -0500 Received: from cdptpa-outbound-snat.email.rr.com ([107.14.166.228]:50602 "EHLO cdptpa-oedge-vip.email.rr.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S964982AbcBTBzj (ORCPT ); Fri, 19 Feb 2016 20:55:39 -0500 Date: Fri, 19 Feb 2016 20:55:36 -0500 From: Steven Rostedt To: Tom Zanussi Cc: masami.hiramatsu.pt@hitachi.com, namhyung@kernel.org, josh@joshtriplett.org, andi@firstfloor.org, mathieu.desnoyers@efficios.com, peterz@infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v13 09/29] tracing: Add 'hist' event trigger command Message-ID: <20160219205536.5aedf4aa@grimm.local.home> In-Reply-To: <1455924470.4713.30.camel@tzanussi-mobl.amr.corp.intel.com> References: <15dc2e4792676582404b398c020926c412c63dcb.1449767187.git.tom.zanussi@linux.intel.com> <20160219162318.10de33fe@gandalf.local.home> <1455924470.4713.30.camel@tzanussi-mobl.amr.corp.intel.com> X-Mailer: Claws Mail 3.13.0 (GTK+ 2.24.29; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-RR-Connecting-IP: 107.14.168.142:25 X-Cloudmark-Score: 0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 19 Feb 2016 17:27:50 -0600 Tom Zanussi wrote: > > > + > > > +struct hist_trigger_data { > > > + struct hist_field *fields[TRACING_MAP_FIELDS_MAX]; > > > > Hmm I'm confused, TRACING_MAP_FIELDS_MAX == 4 > > > > But below it we index into fields with n_fields. Which look to me can > > be much bigger than 4. > > > > Or is there some correlation between n_vals, n_keys and 4? > > > > Yeah, the correlation is: > > n_fields = n_vals + n_keys > > where n_keys can't be larger than TRACING_MAP_KEYS_MAX, and n_vals can't > be larger than TRACING_MAP_FIELDS_MAX - TRACING_MAP_KEYS_MAX, where > currently TRACING_MAP_KEYS_MAX = 2 and TRACING_MAP_FIELDS_MAX = 4, which > means the max number of values is currently 2 as well. So n_fields > shouldn't be bigger than 4, but... > > Looking through the code to make sure, I realized that it should be > fields[TRACING_MAP_FIELDS_MAX + 1] because of the hitcount, which is an > always-defined value field and takes the first slot in fields[] > (followed by the rest of the vals, then the keys). Previous versions > had separate keys[] and vals[] arrays, but that was changed to be more > uniform and somehow adding 1 for the hitcount was overlooked. Obviously > I'll fix that size, but the rest of the code should be ok wrt that. > When you respin, can you add conditions like: if (WARN_ON(n_vals > TRACING_MAP_FIELDS_MAX - TRACING_MAP_KEYS_MAX)) return; where those values are updated. Same for n_keys. Maybe even add some warnons where index is done, against TRACING_MAP_FIELDS_MAX. As this accounting is not that trivial to keep track of. I'd feel better if we have warnings to make sure the values are capped at what we expect them to be capped at. Thanks, -- Steve