Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755903AbbKDChg (ORCPT ); Tue, 3 Nov 2015 21:37:36 -0500 Received: from mga02.intel.com ([134.134.136.20]:1330 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755072AbbKDChf (ORCPT ); Tue, 3 Nov 2015 21:37:35 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,241,1444719600"; d="scan'208";a="677871272" Message-ID: <1446604653.17120.177.camel@tzanussi-mobl.amr.corp.intel.com> Subject: Re: [PATCH v11 17/28] tracing: Add hist trigger 'execname' modifier 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, mathieu.desnoyers@efficios.com, peterz@infradead.org, linux-kernel@vger.kernel.org Date: Tue, 03 Nov 2015 20:37:33 -0600 In-Reply-To: <20151102141012.GA31643@danjae.kornet> References: <48db482555d15fbedf6e0492b4d7d78712e0f7a0.1445530672.git.tom.zanussi@linux.intel.com> <20151102141012.GA31643@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: 7102 Lines: 223 On Mon, 2015-11-02 at 23:10 +0900, Namhyung Kim wrote: > On Thu, Oct 22, 2015 at 01:14:21PM -0500, Tom Zanussi wrote: > > Allow users to have pid fields displayed as program names in the output > > by appending '.execname' to field names: > > > > # echo hist:keys=aaa.execname ... \ > > [ if filter] > event/trigger > > So in this case, 'aaa' should be 'common_pid', right? > Right. > Also, it's just for display thus tasks which have different pid but > same comm name will be saved in different entries, right? If so, it'd > be better to note that somewhere as it might confuse users.. > Right, good point, will document that. > > > > > Signed-off-by: Tom Zanussi > > Tested-by: Masami Hiramatsu > > --- > > kernel/trace/trace.c | 3 +- > > kernel/trace/trace_events_hist.c | 86 +++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 87 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > > index 4f0968e..a143be0 100644 > > --- a/kernel/trace/trace.c > > +++ b/kernel/trace/trace.c > > @@ -3822,7 +3822,8 @@ static const char readme_msg[] = > > "\t following modifiers to the field name, as applicable:\n\n" > > "\t .hex display a number as a hex value\n" > > "\t .sym display an address as a symbol\n" > > - "\t .sym-offset display an address as a symbol and offset\n\n" > > + "\t .sym-offset display an address as a symbol and offset\n" > > + "\t .execname display a common_pid as a program name\n\n" > > "\t The 'pause' parameter can be used to pause an existing hist\n" > > "\t trigger or to start a hist trigger but not log any events\n" > > "\t until told to do so. 'continue' can be used to start or\n" > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > > index e6e5c6e..656973a 100644 > > --- a/kernel/trace/trace_events_hist.c > > +++ b/kernel/trace/trace_events_hist.c > > @@ -75,6 +75,7 @@ enum hist_field_flags { > > HIST_FIELD_HEX = 8, > > HIST_FIELD_SYM = 16, > > HIST_FIELD_SYM_OFFSET = 32, > > + HIST_FIELD_EXECNAME = 64, > > }; > > > > struct hist_trigger_attrs { > > @@ -218,6 +219,78 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str) > > return ERR_PTR(ret); > > } > > > > +static inline void save_comm(char *comm, struct task_struct *task) > > +{ > > + if (!task->pid) { > > + strcpy(comm, ""); > > + return; > > + } > > + > > + if (WARN_ON_ONCE(task->pid < 0)) { > > + strcpy(comm, ""); > > + return; > > + } > > + > > + if (task->pid > PID_MAX_DEFAULT) { > > + strcpy(comm, "<...>"); > > + return; > > + } > > I guess this comes from the saved_cmdlines code which uses a fixed > array of PID_MAX_DEFAULT size. But as it doesn't use the array, we > can simply copy the task->comm IMHO. > > > > + > > + memcpy(comm, task->comm, TASK_COMM_LEN); > > +} > > + > > +static void hist_trigger_elt_free(struct tracing_map_elt *elt) > > +{ > > + kfree((char *)elt->private_data); > > +} > > + > > +static int hist_trigger_elt_alloc(struct tracing_map_elt *elt) > > +{ > > + struct hist_trigger_data *hist_data = elt->map->private_data; > > + struct hist_field *key_field; > > + unsigned int i; > > + > > + for (i = hist_data->n_vals; i < hist_data->n_fields; i++) { > > I think it'd be better to add a list iterator macro like > for_each_key_field() or something since it's easy to miss that the key > fields always follows the value fields IMHO. > Yeah, that's a good idea (sometimes I even forget that myself ;-) > > > + key_field = hist_data->fields[i]; > > + > > + if (key_field->flags & HIST_FIELD_EXECNAME) { > > + unsigned int size = TASK_COMM_LEN + 1; > > + > > + elt->private_data = kzalloc(size, GFP_KERNEL); > > + if (!elt->private_data) > > + return -ENOMEM; > > + break; > > + } > > + } > > + > > + return 0; > > +} > > + > > +static void hist_trigger_elt_copy(struct tracing_map_elt *to, > > + struct tracing_map_elt *from) > > +{ > > + char *comm_from = from->private_data; > > + char *comm_to = to->private_data; > > + > > + if (comm_from) > > + memcpy(comm_to, comm_from, TASK_COMM_LEN + 1); > > +} > > + > > +static void hist_trigger_elt_init(struct tracing_map_elt *elt) > > +{ > > + char *comm = elt->private_data; > > + > > + if (comm) > > + save_comm(comm, current); > > +} > > + > > +static struct tracing_map_ops hist_trigger_ops = { > > + .elt_alloc = hist_trigger_elt_alloc, > > + .elt_copy = hist_trigger_elt_copy, > > + .elt_free = hist_trigger_elt_free, > > + .elt_init = hist_trigger_elt_init, > > +}; > > How about making it const? > Sure. > > > + > > static void destroy_hist_field(struct hist_field *hist_field) > > { > > kfree(hist_field); > > @@ -376,6 +449,9 @@ static int create_key_field(struct hist_trigger_data *hist_data, > > flags |= HIST_FIELD_SYM; > > else if (!strcmp(field_str, "sym-offset")) > > flags |= HIST_FIELD_SYM_OFFSET; > > + else if (!strcmp(field_str, "execname") && > > + !strcmp(field_name, "common_pid")) > > + flags |= HIST_FIELD_EXECNAME; > > else { > > ret = -EINVAL; > > goto out; > > @@ -612,7 +688,7 @@ create_hist_data(unsigned int map_bits, > > goto free; > > > > hist_data->map = tracing_map_create(map_bits, hist_data->key_size, > > - NULL, hist_data); > > + &hist_trigger_ops, hist_data); > > Why not using the hist_trigger_ops only if HIST_FIELD_EXECNAME is set > to one of the key fields actually? > Right, they're only currently needed in that case, so should only be set in that case, will change that. Thanks, Tom > Thanks, > Namhyung > > > > if (IS_ERR(hist_data->map)) { > > ret = PTR_ERR(hist_data->map); > > hist_data->map = NULL; > > @@ -732,6 +808,12 @@ hist_trigger_entry_print(struct seq_file *m, > > sprint_symbol(str, uval); > > seq_printf(m, "%s: [%llx] %-55s", > > key_field->field->name, uval, str); > > + } else if (key_field->flags & HIST_FIELD_EXECNAME) { > > + char *comm = elt->private_data; > > + > > + uval = *(u64 *)(key + key_field->offset); > > + seq_printf(m, "%s: %-16s[%10llu]", > > + key_field->field->name, comm, uval); > > } else if (key_field->flags & HIST_FIELD_STRING) { > > seq_printf(m, "%s: %-50s", key_field->field->name, > > (char *)(key + key_field->offset)); > > @@ -851,6 +933,8 @@ static const char *get_hist_field_flags(struct hist_field *hist_field) > > flags_str = "sym"; > > else if (hist_field->flags & HIST_FIELD_SYM_OFFSET) > > flags_str = "sym-offset"; > > + else if (hist_field->flags & HIST_FIELD_EXECNAME) > > + flags_str = "execname"; > > > > return flags_str; > > } > > -- > > 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/