Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755306AbbGUOPy (ORCPT ); Tue, 21 Jul 2015 10:15:54 -0400 Received: from mga01.intel.com ([192.55.52.88]:59981 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753238AbbGUOPx (ORCPT ); Tue, 21 Jul 2015 10:15:53 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,516,1432623600"; d="scan'208";a="732767769" Message-ID: <1437488136.22524.3.camel@tzanussi-mobl.amr.corp.intel.com> Subject: Re: [PATCH v9 20/22] tracing: Remove restriction on string position in hist trigger keys From: Tom Zanussi To: Namhyung Kim Cc: rostedt@goodmis.org, daniel.wagner@bmw-carit.de, masami.hiramatsu.pt@hitachi.com, josh@joshtriplett.org, andi@firstfloor.org, linux-kernel@vger.kernel.org Date: Tue, 21 Jul 2015 09:15:36 -0500 In-Reply-To: <20150719133152.GE25163@danjae.kornet> References: <226f0a817bd72c2fd78c3b72c0fa79e9e8f394b1.1437066836.git.tom.zanussi@linux.intel.com> <20150719133152.GE25163@danjae.kornet> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4 (3.10.4-4.fc20) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5058 Lines: 151 On Sun, 2015-07-19 at 22:31 +0900, Namhyung Kim wrote: > On Thu, Jul 16, 2015 at 12:22:53PM -0500, Tom Zanussi wrote: > > If we assume the maximum size for a string field, we don't have to > > worry about its position. Since we only allow two keys in a compound > > key and having more than one string key in a given compound key > > doesn't make much sense anyway, trading a bit of extra space instead > > of introducing an arbitrary restriction makes more sense. > > > > We also need to use the event field size for static strings when > > copying the contents, otherwise we get random garbage in the key. > > > > Finally, rearrange the code without changing any functionality by > > moving the compound key updating code into a separate function. > > > > Signed-off-by: Tom Zanussi > > Looks good to me. Just a nitpick below.. > > > > --- > > kernel/trace/trace_events_hist.c | 65 +++++++++++++++++++++++----------------- > > 1 file changed, 37 insertions(+), 28 deletions(-) > > > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > > index 67fffee..4ba7645 100644 > > --- a/kernel/trace/trace_events_hist.c > > +++ b/kernel/trace/trace_events_hist.c > > @@ -508,8 +508,8 @@ static int create_key_field(struct hist_trigger_data *hist_data, > > goto out; > > } > > > > - if (is_string_field(field)) /* should be last key field */ > > - key_size = HIST_KEY_SIZE_MAX - key_offset; > > + if (is_string_field(field)) > > + key_size = MAX_FILTER_STR_VAL; > > else > > key_size = field->size; > > } > > @@ -781,9 +781,36 @@ static void hist_trigger_elt_update(struct hist_trigger_data *hist_data, > > } > > } > > > > +static inline void add_to_key(char *compound_key, void *key, > > + struct hist_field *key_field, void *rec) > > +{ > > + size_t size = key_field->size; > > + > > + if (key_field->flags & HIST_FIELD_STRING) { > > + struct ftrace_event_field *field; > > + > > + /* ensure NULL-termination */ > > + size--; > > This is unnecessary since the size value will be updated below anyway. > I think it's enough just to move the comment to ... > > > > + > > + field = key_field->field; > > + if (field->filter_type == FILTER_DYN_STRING) > > + size = *(u32 *)(rec + field->offset) >> 16; > > + else if (field->filter_type == FILTER_PTR_STRING) > > + size = strlen(key); > > + else if (field->filter_type == FILTER_STATIC_STRING) > > + size = field->size; > > + > > ... here. :) > Yep, makes sense, will do. Tom > > + if (size > key_field->size - 1) > > + size = key_field->size - 1; > > + } > > + > > + memcpy(compound_key + key_field->offset, key, size); > > +} > > + > > static void event_hist_trigger(struct event_trigger_data *data, void *rec) > > { > > struct hist_trigger_data *hist_data = data->private_data; > > + bool use_compound_key = (hist_data->n_keys > 1); > > unsigned long entries[HIST_STACKTRACE_DEPTH]; > > char compound_key[HIST_KEY_SIZE_MAX]; > > struct stack_trace stacktrace; > > @@ -798,8 +825,7 @@ static void event_hist_trigger(struct event_trigger_data *data, void *rec) > > return; > > } > > > > - if (hist_data->n_keys > 1) > > - memset(compound_key, 0, hist_data->key_size); > > + memset(compound_key, 0, hist_data->key_size); > > > > for (i = hist_data->n_vals; i < hist_data->n_fields; i++) { > > key_field = hist_data->fields[i]; > > @@ -816,35 +842,18 @@ static void event_hist_trigger(struct event_trigger_data *data, void *rec) > > key = entries; > > } else { > > field_contents = key_field->fn(key_field, rec); > > - if (key_field->flags & HIST_FIELD_STRING) > > + if (key_field->flags & HIST_FIELD_STRING) { > > key = (void *)field_contents; > > - else > > + use_compound_key = true; > > + } else > > key = (void *)&field_contents; > > - > > - if (hist_data->n_keys > 1) { > > - /* ensure NULL-termination */ > > - size_t size = key_field->size - 1; > > - > > - if (key_field->flags & HIST_FIELD_STRING) { > > - struct ftrace_event_field *field; > > - > > - field = key_field->field; > > - if (field->filter_type == FILTER_DYN_STRING) > > - size = *(u32 *)(rec + field->offset) >> 16; > > - else if (field->filter_type == FILTER_PTR_STRING) > > - size = strlen(key); > > - > > - if (size > key_field->size - 1) > > - size = key_field->size - 1; > > - } > > - > > - memcpy(compound_key + key_field->offset, key, > > - size); > > - } > > } > > + > > + if (use_compound_key) > > + add_to_key(compound_key, key, key_field, rec); > > } > > > > - if (hist_data->n_keys > 1) > > + if (use_compound_key) > > key = compound_key; > > > > elt = tracing_map_insert(hist_data->map, key); > > -- > > 1.9.3 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/