Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755271AbbFKUmj (ORCPT ); Thu, 11 Jun 2015 16:42:39 -0400 Received: from mga14.intel.com ([192.55.52.115]:35917 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751767AbbFKUmg (ORCPT ); Thu, 11 Jun 2015 16:42:36 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,597,1427785200"; d="scan'208";a="745175492" Message-ID: <1434055353.15529.10.camel@picadillo> Subject: Re: [PATCH v7 00/10] tracing: 'hist' triggers 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: Thu, 11 Jun 2015 15:42:33 -0500 In-Reply-To: <20150611030503.GA6446@sejong> References: <20150611030503.GA6446@sejong> 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: 5444 Lines: 163 Hi Namhyung, On Thu, 2015-06-11 at 12:05 +0900, Namhyung Kim wrote: > Hi Tom, > > I tried to play with this patchset, and it worked greatly. Thanks for > your work! > Great, and thanks for trying it out and especially for sending this patch. > But I found that it doesn't handle string fields properly so I can see > failure on accessing 'filename' field in sched_process_exec event. > Below is my fix for that.. Can you take a look at it? > It looks good to me - I was able to verify both the problem and that this fixes it - thanks very much for taking the time to figure this out. I'll include this with my next revision of the patchset, which basically just splits things up into smaller patches for review. I may also make a couple modifications in addition to this - at minimum, enforcing the 'should be the last key field' comment, but I'll try to think of a better way, so that that comment can be removed instead... Thanks again, Tom > Thanks, > Namhyung > > > > From e0442e9aec9740e4f20931ff10545f07c8d6d346 Mon Sep 17 00:00:00 2001 > From: Namhyung Kim > Date: Thu, 11 Jun 2015 11:48:41 +0900 > Subject: [PATCH] tracing: Support string type key properly > > The string in a trace event is usually recorded as dynamic array which > is variable length. But current hist code only support fixed length > array so it cannot support most strings. > > This patch fixes it by checking filter_type of the field and get > proper pointer with it. With this, it can get a histogram of exec() > based on filenames like below: > > # cd /sys/kernel/tracing/events/sched/sched_process_exec > # cat 'hist:key=filename' > trigger > # ps > PID TTY TIME CMD > 1 ? 00:00:00 init > 29 ? 00:00:00 sh > 38 ? 00:00:00 ps > # ls > enable filter format hist id trigger > # cat hist > # trigger info: hist:keys=filename:vals=hitcount:sort=hitcount:size=2048 [active] > > { filename: /usr/bin/ps } hitcount: 1 > { filename: /usr/bin/ls } hitcount: 1 > { filename: /usr/bin/cat } hitcount: 1 > > Totals: > Hits: 3 > Entries: 3 > Dropped: 0 > > Cc: Tom Zanussi > Cc: Masami Hiramatsu > Signed-off-by: Namhyung Kim > --- > kernel/trace/trace_events_hist.c | 47 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 44 insertions(+), 3 deletions(-) > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index 5ea18956497d..d5edf9fa5aea 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -52,6 +52,22 @@ static u64 hist_field_string(struct hist_field *hist_field, void *event) > return (u64)addr; > } > > +static u64 hist_field_dynstring(struct hist_field *hist_field, void *event) > +{ > + u32 str_item = *(u32 *)(event + hist_field->field->offset); > + int str_loc = str_item & 0xffff; > + char *addr = (char *)(event + str_loc); > + > + return (u64)addr; > +} > + > +static u64 hist_field_pstring(struct hist_field *hist_field, void *event) > +{ > + char **addr = (char **)(event + hist_field->field->offset); > + > + return (u64)*addr; > +} > + > #define DEFINE_HIST_FIELD_FN(type) \ > static u64 hist_field_##type(struct hist_field *hist_field, void *event)\ > { \ > @@ -330,7 +346,13 @@ static struct hist_field *create_hist_field(struct ftrace_event_field *field, > > if (is_string_field(field)) { > flags |= HIST_FIELD_STRING; > - hist_field->fn = hist_field_string; > + > + if (field->filter_type == FILTER_STATIC_STRING) > + hist_field->fn = hist_field_string; > + else if (field->filter_type == FILTER_DYN_STRING) > + hist_field->fn = hist_field_dynstring; > + else > + hist_field->fn = hist_field_pstring; > } else { > hist_field->fn = select_value_fn(field->size, > field->is_signed); > @@ -485,7 +507,10 @@ static int create_key_field(struct hist_trigger_data *hist_data, > goto out; > } > > - key_size = field->size; > + if (is_string_field(field)) /* should be last key field */ > + key_size = HIST_KEY_SIZE_MAX - key_offset; > + else > + key_size = field->size; > } > > hist_data->fields[key_idx] = create_hist_field(field, flags); > @@ -791,8 +816,24 @@ static void event_hist_trigger(struct event_trigger_data *data, void *rec) > 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, > - key_field->size); > + size); > } > } > } -- 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/