Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753535AbbGSNeN (ORCPT ); Sun, 19 Jul 2015 09:34:13 -0400 Received: from mail-pd0-f177.google.com ([209.85.192.177]:34633 "EHLO mail-pd0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753406AbbGSNeM (ORCPT ); Sun, 19 Jul 2015 09:34:12 -0400 Date: Sun, 19 Jul 2015 22:31:52 +0900 From: Namhyung Kim To: Tom Zanussi 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 Subject: Re: [PATCH v9 20/22] tracing: Remove restriction on string position in hist trigger keys Message-ID: <20150719133152.GE25163@danjae.kornet> References: <226f0a817bd72c2fd78c3b72c0fa79e9e8f394b1.1437066836.git.tom.zanussi@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <226f0a817bd72c2fd78c3b72c0fa79e9e8f394b1.1437066836.git.tom.zanussi@linux.intel.com> User-Agent: Mutt/1.5.23+89 (0255b37be491) (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4691 Lines: 143 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. :) > + 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/