Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755612AbZCVSAb (ORCPT ); Sun, 22 Mar 2009 14:00:31 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751484AbZCVSAV (ORCPT ); Sun, 22 Mar 2009 14:00:21 -0400 Received: from mail-ew0-f165.google.com ([209.85.219.165]:59140 "EHLO mail-ew0-f165.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751341AbZCVSAT (ORCPT ); Sun, 22 Mar 2009 14:00:19 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; b=TceL8YF9jUxzcr1aRSW+mxueOnvrXhJ4hRqrAGKEdM/yTboqSjSXTJ6fKvBQQfgOyt qIBGKXEbuLhlJgUqDquUDA+vok6bgcVkApPuKeIyoA89lK7R7CB0C+cIty4T8VWKoj21 dvnEN0Flh1/YbqZSxb/y+G/YdPmIn/HCNqNyU= Date: Sun, 22 Mar 2009 19:00:13 +0100 From: Frederic Weisbecker To: Ingo Molnar Cc: Tom Zanussi , linux-kernel , Steven Rostedt Subject: Re: [PATCH 1/4] tracing: add run-time field descriptions for event filtering Message-ID: <20090322180012.GD6002@nowhere> References: <1237710639.7703.46.camel@charm-linux> <20090322173908.GB6002@nowhere> <20090322174443.GA5632@elte.hu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090322174443.GA5632@elte.hu> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8336 Lines: 254 On Sun, Mar 22, 2009 at 06:44:43PM +0100, Ingo Molnar wrote: > > * Frederic Weisbecker wrote: > > > Hi Tom, > > > > > > On Sun, Mar 22, 2009 at 03:30:39AM -0500, Tom Zanussi wrote: > > > This patch makes the field descriptions defined for event tracing > > > available at run-time, for the event-filtering mechanism introduced in a > > > subsequent patch. > > > > > > The common event fields are prepended with 'common_' in the format > > > display, allowing them to be distinguished from the other fields that > > > might internally have same name and can therefore be unambiguously used > > > in filters. > > > > > > Signed-off-by: Tom Zanussi > > > > > > --- > > > kernel/trace/trace.h | 30 ++++++++++++++++------- > > > kernel/trace/trace_events.c | 40 ++++++++++++++++++++++++++++++- > > > kernel/trace/trace_events_stage_2.h | 45 +++++++++++++++++++++++++++++++++++ > > > kernel/trace/trace_events_stage_3.h | 2 + > > > 4 files changed, 107 insertions(+), 10 deletions(-) > > > > > > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > > > index 7cfb741..9288dc7 100644 > > > --- a/kernel/trace/trace.h > > > +++ b/kernel/trace/trace.h > > > @@ -775,16 +775,26 @@ enum { > > > TRACE_EVENT_TYPE_RAW = 2, > > > }; > > > > > > +struct ftrace_event_field { > > > + struct list_head link; > > > + char *name; > > > + char *type; > > > + int offset; > > > + int size; > > > +}; > > > + > > > struct ftrace_event_call { > > > - char *name; > > > - char *system; > > > - struct dentry *dir; > > > - int enabled; > > > - int (*regfunc)(void); > > > - void (*unregfunc)(void); > > > - int id; > > > - int (*raw_init)(void); > > > - int (*show_format)(struct trace_seq *s); > > > + char *name; > > > + char *system; > > > + struct dentry *dir; > > > + int enabled; > > > + int (*regfunc)(void); > > > + void (*unregfunc)(void); > > > + int id; > > > + int (*raw_init)(void); > > > + int (*show_format)(struct trace_seq *s); > > > + int (*define_fields)(void); > > > + struct list_head fields; > > > > > > #ifdef CONFIG_EVENT_PROFILE > > > atomic_t profile_count; > > > @@ -793,6 +803,8 @@ struct ftrace_event_call { > > > #endif > > > }; > > > > > > +int trace_define_field(struct ftrace_event_call *call, char *type, > > > + char *name, int offset, int size); > > > void event_trace_printk(unsigned long ip, const char *fmt, ...); > > > extern struct ftrace_event_call __start_ftrace_events[]; > > > extern struct ftrace_event_call __stop_ftrace_events[]; > > > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c > > > index 3047b56..961b057 100644 > > > --- a/kernel/trace/trace_events.c > > > +++ b/kernel/trace/trace_events.c > > > @@ -19,6 +19,34 @@ > > > > > > static DEFINE_MUTEX(event_mutex); > > > > > > +int trace_define_field(struct ftrace_event_call *call, char *type, > > > + char *name, int offset, int size) > > > +{ > > > + struct ftrace_event_field *field; > > > + > > > + field = kmalloc(sizeof(*field), GFP_KERNEL); > > > + if (!field) > > > + goto err; > > > + field->name = kstrdup(name, GFP_KERNEL); > > > + if (!field->name) > > > + goto err; > > > + field->type = kstrdup(type, GFP_KERNEL); > > > + if (!field->type) > > > + goto err; > > > + field->offset = offset; > > > + field->size = size; > > > + list_add(&field->link, &call->fields); > > > + > > > + return 0; > > > +err: > > > + if (field) { > > > + kfree(field->name); > > > + kfree(field->type); > > > > > > You need kzalloc to allocate field. > > With kmalloc, field will point to random filled memory > > after a fresh allocation. > > > > Imagine this path: > > > > if (!field->name) > > goto err; > > ... > > err: > > kfree(field->name) <- field->name = NULL, correct > > kfree(field->type) <- field->type = ? > > > > > > > + } > > > + kfree(field); > > > + return -ENOMEM; > > > +} > > > + > > > static void ftrace_clear_events(void) > > > { > > > struct ftrace_event_call *call = (void *)__start_ftrace_events; > > > @@ -343,7 +371,8 @@ event_enable_write(struct file *filp, const char __user *ubuf, size_t cnt, > > > > > > #undef FIELD > > > #define FIELD(type, name) \ > > > - #type, #name, offsetof(typeof(field), name), sizeof(field.name) > > > + #type, "common_" #name, offsetof(typeof(field), name), \ > > > + sizeof(field.name) > > > > > > static int trace_write_header(struct trace_seq *s) > > > { > > > @@ -581,6 +610,15 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events) > > > call->name); > > > } > > > > > > + if (call->define_fields) { > > > + ret = call->define_fields(); > > > + if (ret < 0) { > > > + pr_warning("Could not initialize trace point" > > > + " events/%s\n", call->name); > > > + return ret; > > > + } > > > + } > > > + > > > /* A trace may not want to export its format */ > > > if (!call->show_format) > > > return 0; > > > diff --git a/kernel/trace/trace_events_stage_2.h b/kernel/trace/trace_events_stage_2.h > > > index 5117c43..30743f7 100644 > > > --- a/kernel/trace/trace_events_stage_2.h > > > +++ b/kernel/trace/trace_events_stage_2.h > > > @@ -129,3 +129,48 @@ ftrace_format_##call(struct trace_seq *s) \ > > > } > > > > > > #include > > > + > > > +#undef __field > > > +#define __field(type, item) \ > > > + ret = trace_define_field(event_call, #type, #item, \ > > > + offsetof(typeof(field), item), \ > > > + sizeof(field.item)); \ > > > + if (ret) \ > > > + return ret; > > > + > > > +#undef __array > > > +#define __array(type, item, len) \ > > > + ret = trace_define_field(event_call, #type "[" #len "]", #item, \ > > > + offsetof(typeof(field), item), \ > > > + sizeof(field.item)); \ > > > + if (ret) \ > > > + return ret; > > > + > > > +#define __common_field(type, item) \ > > > + ret = trace_define_field(event_call, #type, "common_" #item, \ > > > + offsetof(typeof(field.ent), item), \ > > > + sizeof(field.ent.item)); \ > > > + if (ret) \ > > > + return ret; > > > + > > > +#undef TRACE_EVENT > > > +#define TRACE_EVENT(call, proto, args, tstruct, func, print) \ > > > +int \ > > > +ftrace_define_fields_##call(void) \ > > > +{ \ > > > + struct ftrace_raw_##call field; \ > > > + struct ftrace_event_call *event_call = &event_##call; \ > > > + int ret; \ > > > + \ > > > + __common_field(unsigned char, type); \ > > > + __common_field(unsigned char, flags); \ > > > + __common_field(unsigned char, preempt_count); \ > > > + __common_field(int, pid); \ > > > + __common_field(int, tgid); \ > > > + \ > > > + tstruct; \ > > > + \ > > > + return ret; \ > > > +} > > > + > > > +#include > > > diff --git a/kernel/trace/trace_events_stage_3.h b/kernel/trace/trace_events_stage_3.h > > > index 6b3261c..468938f 100644 > > > --- a/kernel/trace/trace_events_stage_3.h > > > +++ b/kernel/trace/trace_events_stage_3.h > > > @@ -252,6 +252,7 @@ static int ftrace_raw_init_event_##call(void) \ > > > if (!id) \ > > > return -ENODEV; \ > > > event_##call.id = id; \ > > > + INIT_LIST_HEAD(&event_##call.fields); \ > > > return 0; \ > > > } \ > > > \ > > > @@ -264,6 +265,7 @@ __attribute__((section("_ftrace_events"))) event_##call = { \ > > > .regfunc = ftrace_raw_reg_event_##call, \ > > > .unregfunc = ftrace_raw_unreg_event_##call, \ > > > .show_format = ftrace_format_##call, \ > > > + .define_fields = ftrace_define_fields_##call, \ > > > _TRACE_PROFILE_INIT(call) \ > > > } > > > > > > > Other than the small possible memory leak, it looks good. > > it's not a memory leak but an incorrect kfree(), which can crash the > box. Yeah, indeed. > > > > Acked-by: Frederic Weisbecker > > thanks - i also queued up a fix for the kfree() bug. > > Ingo Thanks. -- 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/