Messages in this thread | | | Subject | Re: [PATCH v9 20/22] tracing: Remove restriction on string position in hist trigger keys | From | Tom Zanussi <> | Date | Tue, 21 Jul 2015 09:15:36 -0500 |
| |
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 <tom.zanussi@linux.intel.com> > > 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 > >
| |