From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753360AbdLHNJR (ORCPT ); Fri, 8 Dec 2017 08:09:17 -0500 Received: from mail-pf0-f196.google.com ([209.85.192.196]:38733 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752470AbdLHNJP (ORCPT ); Fri, 8 Dec 2017 08:09:15 -0500 X-Google-Smtp-Source: AGs4zMYnTjnbNvWmk/jcUeJBHdkUM7tRottHXOimx/NSAgi1cVXooxS2VrCGHhQ5VpFk9rG6npZxBg== Date: Fri, 8 Dec 2017 22:06:29 +0900 From: Namhyung Kim To: Tom Zanussi Cc: rostedt@goodmis.org, tglx@linutronix.de, mhiramat@kernel.org, vedang.patel@intel.com, bigeasy@linutronix.de, joel.opensrc@gmail.com, joelaf@google.com, mathieu.desnoyers@efficios.com, baohong.liu@intel.com, rajvi.jingar@intel.com, julia@ni.com, fengguang.wu@intel.com, linux-kernel@vger.kernel.org, linux-rt-users@vger.kernel.org, kernel-team@lge.com Subject: Re: [PATCH v7 19/37] tracing: Generalize per-element hist trigger data Message-ID: <20171208130629.GB7312@danjae.aot.lge.com> References: <28c717b25a6440648171c99fc2fc94f63164096a.1512593081.git.tom.zanussi@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <28c717b25a6440648171c99fc2fc94f63164096a.1512593081.git.tom.zanussi@linux.intel.com> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Dec 06, 2017 at 04:38:00PM -0600, Tom Zanussi wrote: > Up until now, hist triggers only needed per-element support for saving > 'comm' data, which was saved directly as a private data pointer. > > In anticipation of the need to save other data besides 'comm', add a > new hist_elt_data struct for the purpose, and switch the current > 'comm'-related code over to that. > > Signed-off-by: Tom Zanussi > --- > kernel/trace/trace_events_hist.c | 76 +++++++++++++++++++++++----------------- > 1 file changed, 43 insertions(+), 33 deletions(-) > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index f75f7bc..3aeab8e 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -289,6 +289,10 @@ static struct hist_field *find_var(struct hist_trigger_data *hist_data, > return NULL; > } > > +struct hist_elt_data { > + char *comm; > +}; > + > static const char *hist_field_name(struct hist_field *field, > unsigned int level) > { > @@ -503,45 +507,61 @@ static inline void save_comm(char *comm, struct task_struct *task) > memcpy(comm, task->comm, TASK_COMM_LEN); > } > > -static void hist_trigger_elt_comm_free(struct tracing_map_elt *elt) > +static void hist_elt_data_free(struct hist_elt_data *elt_data) > +{ > + kfree(elt_data->comm); > + kfree(elt_data); > +} > + > +static void hist_trigger_elt_data_free(struct tracing_map_elt *elt) > { > - kfree((char *)elt->private_data); > + struct hist_elt_data *elt_data = elt->private_data; > + > + hist_elt_data_free(elt_data); > } > > -static int hist_trigger_elt_comm_alloc(struct tracing_map_elt *elt) > +static int hist_trigger_elt_data_alloc(struct tracing_map_elt *elt) > { > struct hist_trigger_data *hist_data = elt->map->private_data; > + unsigned int size = TASK_COMM_LEN + 1; AFAIK you don't need to do '+ 1' since task->comm is always terminated by a NUL character using strlcpy(). Thanks, Namhyung > + struct hist_elt_data *elt_data; > struct hist_field *key_field; > unsigned int i; > > + elt_data = kzalloc(sizeof(*elt_data), GFP_KERNEL); > + if (!elt_data) > + return -ENOMEM; > + > for_each_hist_key_field(i, hist_data) { > key_field = hist_data->fields[i]; > > if (key_field->flags & HIST_FIELD_FL_EXECNAME) { > - unsigned int size = TASK_COMM_LEN + 1; > - > - elt->private_data = kzalloc(size, GFP_KERNEL); > - if (!elt->private_data) > + elt_data->comm = kzalloc(size, GFP_KERNEL); > + if (!elt_data->comm) { > + kfree(elt_data); > return -ENOMEM; > + } > break; > } > } > > + elt->private_data = elt_data; > + > return 0; > } > > -static void hist_trigger_elt_comm_init(struct tracing_map_elt *elt) > +static void hist_trigger_elt_data_init(struct tracing_map_elt *elt) > { > - char *comm = elt->private_data; > + struct hist_elt_data *elt_data = elt->private_data; > > - if (comm) > - save_comm(comm, current); > + if (elt_data->comm) > + save_comm(elt_data->comm, current); > } > > -static const struct tracing_map_ops hist_trigger_elt_comm_ops = { > - .elt_alloc = hist_trigger_elt_comm_alloc, > - .elt_free = hist_trigger_elt_comm_free, > - .elt_init = hist_trigger_elt_comm_init, > +static const struct tracing_map_ops hist_trigger_elt_data_ops = { > + .elt_alloc = hist_trigger_elt_data_alloc, > + .elt_free = hist_trigger_elt_data_free, > + .elt_init = hist_trigger_elt_data_init, > }; > > static const char *get_hist_field_flags(struct hist_field *hist_field) > @@ -1481,21 +1501,6 @@ static int create_tracing_map_fields(struct hist_trigger_data *hist_data) > return 0; > } > > -static bool need_tracing_map_ops(struct hist_trigger_data *hist_data) > -{ > - struct hist_field *key_field; > - unsigned int i; > - > - for_each_hist_key_field(i, hist_data) { > - key_field = hist_data->fields[i]; > - > - if (key_field->flags & HIST_FIELD_FL_EXECNAME) > - return true; > - } > - > - return false; > -} > - > static struct hist_trigger_data * > create_hist_data(unsigned int map_bits, > struct hist_trigger_attrs *attrs, > @@ -1521,8 +1526,7 @@ static bool need_tracing_map_ops(struct hist_trigger_data *hist_data) > if (ret) > goto free; > > - if (need_tracing_map_ops(hist_data)) > - map_ops = &hist_trigger_elt_comm_ops; > + map_ops = &hist_trigger_elt_data_ops; > > hist_data->map = tracing_map_create(map_bits, hist_data->key_size, > map_ops, hist_data); > @@ -1710,7 +1714,13 @@ static void hist_trigger_stacktrace_print(struct seq_file *m, > seq_printf(m, "%s: [%llx] %-55s", field_name, > uval, str); > } else if (key_field->flags & HIST_FIELD_FL_EXECNAME) { > - char *comm = elt->private_data; > + struct hist_elt_data *elt_data = elt->private_data; > + char *comm; > + > + if (WARN_ON_ONCE(!elt_data)) > + return; > + > + comm = elt_data->comm; > > uval = *(u64 *)(key + key_field->offset); > seq_printf(m, "%s: %-16s[%10llu]", field_name, > -- > 1.9.3 >