[PATCH -tip] tracing: use defined fields to print formats

[PATCH -tip] tracing: use defined fields to print formats

[Lai Jiangshan]

It seems that ftrace_format_##call() and ftrace_define_fields_##call()
are duplicate more or less.

trace_define_field() defines fields and links them into
strcut ftrace_event_call. We reuse them to print formats
and remove ftrace_format_##call(). It make all things simpler.

TRACE_EVENT_FORMAT_NOFILTER is dropped. Because we should
"trace_define_field()" fields for all struct ftrace_event_call,
even it's no filter.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 5c093ff..738b33d 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -111,8 +111,8 @@ struct ftrace_event_call {
 	int			(*regfunc)(void);
 	void			(*unregfunc)(void);
 	int			id;
+	const char		*print_fmt;
 	int			(*raw_init)(void);
-	int			(*show_format)(struct trace_seq *s);
 	int			(*define_fields)(void);
 	struct list_head	fields;
 	int			filter_active;
diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 3cbb96e..d9b56fe 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -82,56 +82,11 @@
 #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
 
 /*
- * Setup the showing format of trace point.
+ * Setup the print format of trace point.
  *
- * int
- * ftrace_format_##call(struct trace_seq *s)
- * {
- *	struct ftrace_raw_##call field;
- *	int ret;
- *
- *	ret = trace_seq_printf(s, #type " " #item ";"
- *			       " offset:%u; size:%u;\n",
- *			       offsetof(struct ftrace_raw_##call, item),
- *			       sizeof(field.type));
- *
- * }
+ * static const char *print_fmt_<call> = <TP_printk>;
  */
 
-#undef TP_STRUCT__entry
-#define TP_STRUCT__entry(args...) args
-
-#undef __field
-#define __field(type, item)					\
-	ret = trace_seq_printf(s, "\tfield:" #type " " #item ";\t"	\
-			       "offset:%u;\tsize:%u;\n",		\
-			       (unsigned int)offsetof(typeof(field), item), \
-			       (unsigned int)sizeof(field.item));	\
-	if (!ret)							\
-		return 0;
-
-#undef __array
-#define __array(type, item, len)						\
-	ret = trace_seq_printf(s, "\tfield:" #type " " #item "[" #len "];\t"	\
-			       "offset:%u;\tsize:%u;\n",		\
-			       (unsigned int)offsetof(typeof(field), item), \
-			       (unsigned int)sizeof(field.item));	\
-	if (!ret)							\
-		return 0;
-
-#undef __dynamic_array
-#define __dynamic_array(type, item, len)				       \
-	ret = trace_seq_printf(s, "\tfield:__data_loc " #type "[] " #item ";\t"\
-			       "offset:%u;\tsize:%u;\n",		       \
-			       (unsigned int)offsetof(typeof(field),	       \
-					__data_loc_##item),		       \
-			       (unsigned int)sizeof(field.__data_loc_##item)); \
-	if (!ret)							       \
-		return 0;
-
-#undef __string
-#define __string(item, src) __dynamic_array(char, item, -1)
-
 #undef __entry
 #define __entry REC
 
@@ -140,25 +95,11 @@
 #undef __get_str
 
 #undef TP_printk
-#define TP_printk(fmt, args...) "%s, %s\n", #fmt, __stringify(args)
-
-#undef TP_fast_assign
-#define TP_fast_assign(args...) args
+#define TP_printk(fmt, args...) #fmt ", "  __stringify(args)
 
 #undef TRACE_EVENT
 #define TRACE_EVENT(call, proto, args, tstruct, func, print)		\
-static int								\
-ftrace_format_##call(struct trace_seq *s)				\
-{									\
-	struct ftrace_raw_##call field __attribute__((unused));		\
-	int ret = 0;							\
-									\
-	tstruct;							\
-									\
-	trace_seq_printf(s, "\nprint fmt: " print);			\
-									\
-	return ret;							\
-}
+static const char *print_fmt_##call = print;
 
 #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
 
@@ -429,6 +370,7 @@ static inline int ftrace_get_offsets_##call(				\
  *	if (!id)
  *		return -ENODEV;
  *	event_<call>.id = id;
+	event_<call>.print_fmt = print_fmt_<call>;
  *	return 0;
  * }
  *
@@ -440,14 +382,10 @@ static inline int ftrace_get_offsets_##call(				\
  *	.raw_init		= ftrace_raw_init_event_<call>,
  *	.regfunc		= ftrace_reg_event_<call>,
  *	.unregfunc		= ftrace_unreg_event_<call>,
- *	.show_format		= ftrace_format_<call>,
  * }
  *
  */
 
-#undef TP_FMT
-#define TP_FMT(fmt, args...)	fmt "\n", ##args
-
 #ifdef CONFIG_EVENT_PROFILE
 #define _TRACE_PROFILE(call, proto, args)				\
 static void ftrace_profile_##call(proto)				\
@@ -502,6 +440,9 @@ static void ftrace_profile_disable_##call(struct ftrace_event_call *event_call)\
 #define __assign_str(dst, src)						\
 	strcpy(__get_str(dst), src);
 
+#undef TP_fast_assign
+#define TP_fast_assign(args...) args
+
 #undef TRACE_EVENT
 #define TRACE_EVENT(call, proto, args, tstruct, assign, print)		\
 _TRACE_PROFILE(call, PARAMS(proto), PARAMS(args))			\
@@ -567,6 +508,7 @@ static int ftrace_raw_init_event_##call(void)				\
 	if (!id)							\
 		return -ENODEV;						\
 	event_##call.id = id;						\
+	event_##call.print_fmt = print_fmt_##call;			\
 	INIT_LIST_HEAD(&event_##call.fields);				\
 	init_preds(&event_##call);					\
 	return 0;							\
@@ -581,7 +523,6 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.raw_init		= ftrace_raw_init_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)					\
 }
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 06886a0..1551c45 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -867,8 +867,6 @@ extern const char *__stop___trace_bprintk_fmt[];
 #undef TRACE_EVENT_FORMAT
 #define TRACE_EVENT_FORMAT(call, proto, args, fmt, tstruct, tpfmt)	\
 	extern struct ftrace_event_call event_##call;
-#undef TRACE_EVENT_FORMAT_NOFILTER
-#define TRACE_EVENT_FORMAT_NOFILTER(call, proto, args, fmt, tstruct, tpfmt)
 #include "trace_event_types.h"
 
 #endif /* _LINUX_KERNEL_TRACE_H */
diff --git a/kernel/trace/trace_event_types.h b/kernel/trace/trace_event_types.h
index 6db005e..ab27fc1 100644
--- a/kernel/trace/trace_event_types.h
+++ b/kernel/trace/trace_event_types.h
@@ -60,7 +60,7 @@ TRACE_EVENT_FORMAT(context_switch, TRACE_CTX, ctx_switch_entry, ignore,
 	TP_RAW_FMT("%u:%u:%u  ==+ %u:%u:%u [%03u]")
 );
 
-TRACE_EVENT_FORMAT_NOFILTER(special, TRACE_SPECIAL, special_entry, ignore,
+TRACE_EVENT_FORMAT(special, TRACE_SPECIAL, special_entry, ignore,
 	TRACE_STRUCT(
 		TRACE_FIELD(unsigned long, arg1, arg1)
 		TRACE_FIELD(unsigned long, arg2, arg2)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 90cf936..965aa1f 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -47,7 +47,7 @@ int trace_define_field(struct ftrace_event_call *call, char *type,
 	field->offset = offset;
 	field->size = size;
 	field->is_signed = is_signed;
-	list_add(&field->link, &call->fields);
+	list_add_tail(&field->link, &call->fields);
 
 	return 0;
 
@@ -524,41 +524,16 @@ out:
 	return ret;
 }
 
-extern char *__bad_type_size(void);
-
-#undef FIELD
-#define FIELD(type, name)						\
-	sizeof(type) != sizeof(field.name) ? __bad_type_size() :	\
-	#type, "common_" #name, offsetof(typeof(field), name),		\
-		sizeof(field.name)
-
-static int trace_write_header(struct trace_seq *s)
-{
-	struct trace_entry field;
-
-	/* struct trace_entry */
-	return trace_seq_printf(s,
-				"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
-				"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
-				"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
-				"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
-				"\tfield:%s %s;\toffset:%zu;\tsize:%zu;\n"
-				"\n",
-				FIELD(unsigned short, type),
-				FIELD(unsigned char, flags),
-				FIELD(unsigned char, preempt_count),
-				FIELD(int, pid),
-				FIELD(int, tgid));
-}
-
 static ssize_t
 event_format_read(struct file *filp, char __user *ubuf, size_t cnt,
 		  loff_t *ppos)
 {
 	struct ftrace_event_call *call = filp->private_data;
+	struct ftrace_event_field *field;
 	struct trace_seq *s;
+	int common_field_count = 5;
 	char *buf;
-	int r;
+	int r = 0;
 
 	if (*ppos)
 		return 0;
@@ -569,14 +544,30 @@ event_format_read(struct file *filp, char __user *ubuf, size_t cnt,
 
 	trace_seq_init(s);
 
-	/* If any of the first writes fail, so will the show_format. */
+	/* If any of the first writes fail, so will the formats. */
 
 	trace_seq_printf(s, "name: %s\n", call->name);
 	trace_seq_printf(s, "ID: %d\n", call->id);
 	trace_seq_printf(s, "format:\n");
-	trace_write_header(s);
 
-	r = call->show_format(s);
+	list_for_each_entry(field, &call->fields, link) {
+		r = trace_seq_printf(s, "\tfield:%s %s;"
+				"\toffset:%u;\tsize:%u;\n",
+				field->type, field->name,
+				field->offset, field->size);
+
+		if (--common_field_count == 0)
+			r = trace_seq_printf(s, "\n");
+
+		if (!r)
+			break;
+	}
+
+	if (r) {
+		r = trace_seq_printf(s, "\nprint fmt: %s\n",
+				call->print_fmt);
+	}
+
 	if (!r) {
 		/*
 		 * ug!  The format output is bigger than a PAGE!!
@@ -986,10 +977,6 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events,
 					  filter);
 	}
 
-	/* A trace may not want to export its format */
-	if (!call->show_format)
-		return 0;
-
 	entry = trace_create_file("format", 0444, call->dir, call,
 				  format);
 
diff --git a/kernel/trace/trace_export.c b/kernel/trace/trace_export.c
index d06cf89..3a4f9bd 100644
--- a/kernel/trace/trace_export.c
+++ b/kernel/trace/trace_export.c
@@ -19,77 +19,6 @@
 #undef TRACE_STRUCT
 #define TRACE_STRUCT(args...) args
 
-extern void __bad_type_size(void);
-
-#undef TRACE_FIELD
-#define TRACE_FIELD(type, item, assign)					\
-	if (sizeof(type) != sizeof(field.item))				\
-		__bad_type_size();					\
-	ret = trace_seq_printf(s, "\tfield:" #type " " #item ";\t"	\
-			       "offset:%u;\tsize:%u;\n",		\
-			       (unsigned int)offsetof(typeof(field), item), \
-			       (unsigned int)sizeof(field.item));	\
-	if (!ret)							\
-		return 0;
-
-
-#undef TRACE_FIELD_SPECIAL
-#define TRACE_FIELD_SPECIAL(type_item, item, len, cmd)			\
-	ret = trace_seq_printf(s, "\tfield special:" #type_item ";\t"	\
-			       "offset:%u;\tsize:%u;\n",		\
-			       (unsigned int)offsetof(typeof(field), item), \
-			       (unsigned int)sizeof(field.item));	\
-	if (!ret)							\
-		return 0;
-
-#undef TRACE_FIELD_ZERO_CHAR
-#define TRACE_FIELD_ZERO_CHAR(item)					\
-	ret = trace_seq_printf(s, "\tfield:char " #item ";\t"		\
-			       "offset:%u;\tsize:0;\n",			\
-			       (unsigned int)offsetof(typeof(field), item)); \
-	if (!ret)							\
-		return 0;
-
-#undef TRACE_FIELD_SIGN
-#define TRACE_FIELD_SIGN(type, item, assign, is_signed)	\
-	TRACE_FIELD(type, item, assign)
-
-#undef TP_RAW_FMT
-#define TP_RAW_FMT(args...) args
-
-#undef TRACE_EVENT_FORMAT
-#define TRACE_EVENT_FORMAT(call, proto, args, fmt, tstruct, tpfmt)	\
-static int								\
-ftrace_format_##call(struct trace_seq *s)				\
-{									\
-	struct args field;						\
-	int ret;							\
-									\
-	tstruct;							\
-									\
-	trace_seq_printf(s, "\nprint fmt: \"%s\"\n", tpfmt);		\
-									\
-	return ret;							\
-}
-
-#undef TRACE_EVENT_FORMAT_NOFILTER
-#define TRACE_EVENT_FORMAT_NOFILTER(call, proto, args, fmt, tstruct,	\
-				    tpfmt)				\
-static int								\
-ftrace_format_##call(struct trace_seq *s)				\
-{									\
-	struct args field;						\
-	int ret;							\
-									\
-	tstruct;							\
-									\
-	trace_seq_printf(s, "\nprint fmt: \"%s\"\n", tpfmt);		\
-									\
-	return ret;							\
-}
-
-#include "trace_event_types.h"
-
 #undef TRACE_ZERO_CHAR
 #define TRACE_ZERO_CHAR(arg)
 
@@ -115,6 +44,9 @@ ftrace_format_##call(struct trace_seq *s)				\
 #define TRACE_FIELD_SPECIAL(type_item, item, len, cmd)	\
 	cmd;
 
+#undef TP_RAW_FMT
+#define TP_RAW_FMT(args...) args
+
 #undef TRACE_EVENT_FORMAT
 #define TRACE_EVENT_FORMAT(call, proto, args, fmt, tstruct, tpfmt)	\
 int ftrace_define_fields_##call(void);					\
@@ -124,10 +56,10 @@ struct ftrace_event_call __used						\
 __attribute__((__aligned__(4)))						\
 __attribute__((section("_ftrace_events"))) event_##call = {		\
 	.name			= #call,				\
-	.id			= proto,				\
 	.system			= __stringify(TRACE_SYSTEM),		\
+	.id			= proto,				\
+	.print_fmt		= tpfmt,				\
 	.raw_init		= ftrace_raw_init_event_##call,		\
-	.show_format		= ftrace_format_##call,			\
 	.define_fields		= ftrace_define_fields_##call,		\
 };									\
 static int ftrace_raw_init_event_##call(void)				\
@@ -137,19 +69,6 @@ static int ftrace_raw_init_event_##call(void)				\
 	return 0;							\
 }									\
 
-#undef TRACE_EVENT_FORMAT_NOFILTER
-#define TRACE_EVENT_FORMAT_NOFILTER(call, proto, args, fmt, tstruct,	\
-				    tpfmt)				\
-									\
-struct ftrace_event_call __used						\
-__attribute__((__aligned__(4)))						\
-__attribute__((section("_ftrace_events"))) event_##call = {		\
-	.name			= #call,				\
-	.id			= proto,				\
-	.system			= __stringify(TRACE_SYSTEM),		\
-	.show_format		= ftrace_format_##call,			\
-};
-
 #include "trace_event_types.h"
 
 #undef TRACE_FIELD
@@ -199,8 +118,4 @@ ftrace_define_fields_##call(void)					\
 	return ret;							\
 }
 
-#undef TRACE_EVENT_FORMAT_NOFILTER
-#define TRACE_EVENT_FORMAT_NOFILTER(call, proto, args, fmt, tstruct,	\
-				    tpfmt)
-
 #include "trace_event_types.h"

Re: [PATCH -tip] tracing: use defined fields to print formats

[Ingo Molnar]

* Lai Jiangshan <laijs@cn.fujitsu.com> wrote:

> It seems that ftrace_format_##call() and 
> ftrace_define_fields_##call() are duplicate more or less.
> 
> trace_define_field() defines fields and links them into strcut 
> ftrace_event_call. We reuse them to print formats and remove 
> ftrace_format_##call(). It make all things simpler.
> 
> TRACE_EVENT_FORMAT_NOFILTER is dropped. Because we should 
> "trace_define_field()" fields for all struct ftrace_event_call, 
> even it's no filter.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> ---
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 5c093ff..738b33d 100644

Minor request for future patches: could you please include the 
diffstat all the time? Makes it much easier to see the general 
impact of a patch.

	Ingo

Re: [PATCH -tip] tracing: use defined fields to print formats

[Steven Rostedt]

On Thu, 16 Jul 2009, Lai Jiangshan wrote:

> 
> It seems that ftrace_format_##call() and ftrace_define_fields_##call()
> are duplicate more or less.
> 
> trace_define_field() defines fields and links them into
> strcut ftrace_event_call. We reuse them to print formats
> and remove ftrace_format_##call(). It make all things simpler.
> 
> TRACE_EVENT_FORMAT_NOFILTER is dropped. Because we should
> "trace_define_field()" fields for all struct ftrace_event_call,
> even it's no filter.

OK, I added this and did a diff of the formats before this patch and 
after the patch. Here they are (with a lot of duplicats cut out).

[
  '<' represents the old format
  '>' represents the new format (with patch applied)
]

4c4
< 	field:unsigned short common_type;	offset:0;	size:2;
---
> 	field:int common_type;	offset:0;	size:2;

We changed the common type from "unsigned short" to "int"?

45,46c45,46
< 	field:char rwbs[6];	offset:32;	size:6;
< 	field:char comm[TASK_COMM_LEN];	offset:38;	size:16;
---
> 	field:char[6] rwbs;	offset:32;	size:6;
> 	field:char[TASK_COMM_LEN] comm;	offset:38;	size:16;

I have several parsers that expect the '[6]' to come after the 
declaration.


390c390
< print fmt: "type:%u call_site:%lx ptr:%p"
---
> print fmt: type:%u call_site:%lx ptr:%p

The 'ftrace' events lost their quotes around the print format.

394c394
< 	field:unsigned short common_type;	offset:0;	size:2;
---
> 	field:unsigned char common_type;	offset:0;	size:2;

And the ftrace events also now use "unsigned char" for the common_type 
instead of unsigned short?


408c408
< print fmt: "type:%u call_site:%lx ptr:%p req:%lu alloc:%lu flags:%x node:%d"
---
> print fmt: type:%u call_site:%lx ptr:%p req:%lu alloc:%lu flags:%x node:%d

423c423
< print fmt: "%llx->%llx type:%u state:%u"
---
> print fmt: %llx->%llx type:%u state:%u

436c436
< print fmt: "from: %llx to: %llx"
---
> print fmt: from: %llx to: %llx


Lots more quotes missing (I'll cut out the rest of the diff of quotes 
missing).

447,448c447,448
< 	field special:char func[TRACE_FUNC_SIZE+1];	offset:16;	size:31;
< 	field special:char file[TRACE_FUNC_SIZE+1];	offset:47;	size:21;
---
> 	field:char func[TRACE_FUNC_SIZE+1][TRACE_FUNC_SIZE+1] func;	offset:16;	size:31;
> 	field:char file[TRACE_FUNC_SIZE+1][TRACE_FUNC_SIZE+1] file;	offset:47;	size:21;

This is interesting.


> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>

I really like the clean up this patch does, but it must not modify the 
current format of the files unless there is a really good reason to do so.

Thanks,

-- Steve

Re: [PATCH -tip] tracing: use defined fields to print formats

[Lai Jiangshan]

Steven Rostedt wrote:
> On Thu, 16 Jul 2009, Lai Jiangshan wrote:
> 
>> It seems that ftrace_format_##call() and ftrace_define_fields_##call()
>> are duplicate more or less.
>>
>> trace_define_field() defines fields and links them into
>> strcut ftrace_event_call. We reuse them to print formats
>> and remove ftrace_format_##call(). It make all things simpler.
>>
>> TRACE_EVENT_FORMAT_NOFILTER is dropped. Because we should
>> "trace_define_field()" fields for all struct ftrace_event_call,
>> even it's no filter.
> 
> OK, I added this and did a diff of the formats before this patch and 
> after the patch. Here they are (with a lot of duplicats cut out).
> 
> [
>   '<' represents the old format
>   '>' represents the new format (with patch applied)
> ]
> 
> 4c4
> < 	field:unsigned short common_type;	offset:0;	size:2;
> ---
>> 	field:int common_type;	offset:0;	size:2;
> 
> We changed the common type from "unsigned short" to "int"?
> 
> 45,46c45,46
> < 	field:char rwbs[6];	offset:32;	size:6;
> < 	field:char comm[TASK_COMM_LEN];	offset:38;	size:16;
> ---
>> 	field:char[6] rwbs;	offset:32;	size:6;
>> 	field:char[TASK_COMM_LEN] comm;	offset:38;	size:16;
> 
> I have several parsers that expect the '[6]' to come after the 
> declaration.
> 
> 
> 390c390
> < print fmt: "type:%u call_site:%lx ptr:%p"
> ---
>> print fmt: type:%u call_site:%lx ptr:%p
> 
> The 'ftrace' events lost their quotes around the print format.
> 
> 394c394
> < 	field:unsigned short common_type;	offset:0;	size:2;
> ---
>> 	field:unsigned char common_type;	offset:0;	size:2;
> 
> And the ftrace events also now use "unsigned char" for the common_type 
> instead of unsigned short?
> 
> 
> 408c408
> < print fmt: "type:%u call_site:%lx ptr:%p req:%lu alloc:%lu flags:%x node:%d"
> ---
>> print fmt: type:%u call_site:%lx ptr:%p req:%lu alloc:%lu flags:%x node:%d
> 
> 423c423
> < print fmt: "%llx->%llx type:%u state:%u"
> ---
>> print fmt: %llx->%llx type:%u state:%u
> 
> 436c436
> < print fmt: "from: %llx to: %llx"
> ---
>> print fmt: from: %llx to: %llx
> 
> 
> Lots more quotes missing (I'll cut out the rest of the diff of quotes 
> missing).
> 
> 447,448c447,448
> < 	field special:char func[TRACE_FUNC_SIZE+1];	offset:16;	size:31;
> < 	field special:char file[TRACE_FUNC_SIZE+1];	offset:47;	size:21;
> ---
>> 	field:char func[TRACE_FUNC_SIZE+1][TRACE_FUNC_SIZE+1] func;	offset:16;	size:31;
>> 	field:char file[TRACE_FUNC_SIZE+1][TRACE_FUNC_SIZE+1] file;	offset:47;	size:21;
> 
> This is interesting.
> 
> 
>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> 
> I really like the clean up this patch does, but it must not modify the 
> current format of the files unless there is a really good reason to do so.
> 
> Thanks,
> 

I'll fix it.

There're 4 differences:

1) the type of "common_type" is changed.
It's brought by a bug `__common_field(int, type, 1);`
It'll be fixed.

2) Type Item[Len] ==> Type[Len] Item.
(There are the same, )But it can't not be recovered as before.
Because it's brought by `trace_define_field(event_call, #type "[" #len "]", #item,`
I'll fix trace-cmd and make it supports these two style together.

3) double quotation marks for the format is missing.
It'll be fixed.

4) a line in events/ftrace/print/format is missing.
It'll be fixed.

At the end(fixed patch), the only change is "Type Item[Len] ==> Type[Len] Item".

The values of this patch are:
1) A big function ftrace_format_##call() is defined for every event type.
it waste a lot of memory. this patch saves these memory.

2) reduce coupling: ftrace_format_##call() and
ftrace_define_fields_##call() are almost the same.
We need to maintain them together, it's not good design.

The difference between ftrace_format_##call() and
ftrace_define_fields_##call() implies a bug.

Example: `__common_field(int, type, 1);` in
ftrace_define_fields_##call() is a bug.

Thank you.

Lai.

Re: [PATCH -tip] tracing: use defined fields to print formats

[Steven Rostedt]

On Tue, 21 Jul 2009, Lai Jiangshan wrote:
> 
> There're 4 differences:
> 
> 1) the type of "common_type" is changed.
> It's brought by a bug `__common_field(int, type, 1);`
> It'll be fixed.
> 
> 2) Type Item[Len] ==> Type[Len] Item.
> (There are the same, )But it can't not be recovered as before.
> Because it's brought by `trace_define_field(event_call, #type "[" #len "]", #item,`
> I'll fix trace-cmd and make it supports these two style together.

I'll wait for your next patch and see if I can take a crack at getting it
back to the old way.

> 
> 3) double quotation marks for the format is missing.
> It'll be fixed.
> 
> 4) a line in events/ftrace/print/format is missing.
> It'll be fixed.
> 
> At the end(fixed patch), the only change is "Type Item[Len] ==> Type[Len] Item".
> 
> The values of this patch are:
> 1) A big function ftrace_format_##call() is defined for every event type.
> it waste a lot of memory. this patch saves these memory.

I agree.

> 
> 2) reduce coupling: ftrace_format_##call() and
> ftrace_define_fields_##call() are almost the same.
> We need to maintain them together, it's not good design.

I agree.

> 
> The difference between ftrace_format_##call() and
> ftrace_define_fields_##call() implies a bug.
> 
> Example: `__common_field(int, type, 1);` in
> ftrace_define_fields_##call() is a bug.

The problem is that this will not make it until 32, and 31 already has the 
type item[x] format. Which means tools will be expecting it that way. We 
can make the tools handle both, but I would really like to prevent that.

I totally agree that your patch is a nice clean up, and saves space and 
prevents more bugs. On the other hand, we are changing an API to users 
where all tools that parse this must be able to cope.

I'll wait for your next patch and see if I can't pull something out of my 
CPP magic hat that can save the day ;-)

-- Steve