[PATCH 0/2] tracing/events: 2 fixes for dynamic arrays

[PATCH 0/2] tracing/events: 2 fixes for dynamic arrays

[Li Zefan]

These 2 fixes are necessary to make __dynamic_array/__string
parsable for users.

And also fix to pass the correct string length when filtering
on dynamic strings.

[PATCH 1/2] tracing/events: add missing type info of dynamic arrays
[PATCH 2/2] tracing/events: record the size of dynamic arrays
---
 include/trace/ftrace.h             |   18 ++++++++++--------
 kernel/trace/trace_events_filter.c |    6 ++++--
 2 files changed, 14 insertions(+), 10 deletions(-)

[PATCH 1/2] tracing/events: add missing type info of dynamic arrays

[Li Zefan]

From: Lai Jiangshan <laijs@cn.fujitsu.com>

The format file doesn't contain enough information for
__dynamic_array/__string. The type name is missing.

Before:
  # cat format:
  name: irq_handler_entry
  ...
          field:__data_loc name;  offset:16;      size:2;

After:
  # cat format:
  name: irq_handler_entry
  ...
          field:__data_loc char[] name;  offset:16;      size:2;

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 include/trace/ftrace.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 1867553..cc78943 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -120,7 +120,7 @@
 
 #undef __dynamic_array
 #define __dynamic_array(type, item, len)				       \
-	ret = trace_seq_printf(s, "\tfield:__data_loc " #item ";\t"	       \
+	ret = trace_seq_printf(s, "\tfield:__data_loc " #type "[] " #item ";\t"\
 			       "offset:%u;\tsize:%u;\n",		       \
 			       (unsigned int)offsetof(typeof(field),	       \
 					__data_loc_##item),		       \
@@ -279,7 +279,7 @@ ftrace_raw_output_##call(struct trace_iterator *iter, int flags)	\
 
 #undef __dynamic_array
 #define __dynamic_array(type, item, len)				       \
-	ret = trace_define_field(event_call, "__data_loc" "[" #type "]", #item,\
+	ret = trace_define_field(event_call, "__data_loc " #type "[]", #item,  \
 				offsetof(typeof(field), __data_loc_##item),    \
 				 sizeof(field.__data_loc_##item), 0);
 
-- 1.6.3 

[PATCH 2/2] tracing/events: record the size of dynamic arrays

[Li Zefan]

When a dynamic array is defined, we add __data_loc_foo in
trace_entry to record the offset of the array, but the
size of the array is not recorded, which causes 2 problems:

- the event filter just compares the first 2 chars of the strings.

- parsers can't parse dynamic arrays.

So we encode the size of each dynamic array in the higher 16 bits
of __data_loc_foo, while the offset is in lower 16 bits.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 include/trace/ftrace.h             |   14 ++++++++------
 kernel/trace/trace_events_filter.c |    6 ++++--
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index cc78943..3cbb96e 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -25,7 +25,7 @@
 #define __array(type, item, len)	type	item[len];
 
 #undef __dynamic_array
-#define __dynamic_array(type, item, len) unsigned short __data_loc_##item;
+#define __dynamic_array(type, item, len) u32 __data_loc_##item;
 
 #undef __string
 #define __string(item, src) __dynamic_array(char, item, -1)
@@ -51,13 +51,14 @@
  * Include the following:
  *
  * struct ftrace_data_offsets_<call> {
- *	int				<item1>;
- *	int				<item2>;
+ *	u32				<item1>;
+ *	u32				<item2>;
  *	[...]
  * };
  *
- * The __dynamic_array() macro will create each int <item>, this is
+ * The __dynamic_array() macro will create each u32 <item>, this is
  * to keep the offset of each array from the beginning of the event.
+ * The size of an array is also encoded, in the higher 16 bits of <item>.
  */
 
 #undef __field
@@ -67,7 +68,7 @@
 #define __array(type, item, len)
 
 #undef __dynamic_array
-#define __dynamic_array(type, item, len)	int item;
+#define __dynamic_array(type, item, len)	u32 item;
 
 #undef __string
 #define __string(item, src) __dynamic_array(char, item, -1)
@@ -207,7 +208,7 @@ ftrace_format_##call(struct trace_seq *s)				\
 
 #undef __get_dynamic_array
 #define __get_dynamic_array(field)	\
-		((void *)__entry + __entry->__data_loc_##field)
+		((void *)__entry + (__entry->__data_loc_##field & 0xffff))
 
 #undef __get_str
 #define __get_str(field) (char *)__get_dynamic_array(field)
@@ -325,6 +326,7 @@ ftrace_define_fields_##call(void)					\
 #define __dynamic_array(type, item, len)				\
 	__data_offsets->item = __data_size +				\
 			       offsetof(typeof(*entry), __data);	\
+	__data_offsets->item |= (len * sizeof(type)) << 16;		\
 	__data_size += (len) * sizeof(type);
 
 #undef __string
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index b9aae72..1c80ef7 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -176,11 +176,13 @@ static int filter_pred_string(struct filter_pred *pred, void *event,
 static int filter_pred_strloc(struct filter_pred *pred, void *event,
 			      int val1, int val2)
 {
-	unsigned short str_loc = *(unsigned short *)(event + pred->offset);
+	u32 str_item = *(u32 *)(event + pred->offset);
+	int str_loc = str_item & 0xffff;
+	int str_len = str_item >> 16;
 	char *addr = (char *)(event + str_loc);
 	int cmp, match;
 
-	cmp = strncmp(addr, pred->str_val, pred->str_len);
+	cmp = strncmp(addr, pred->str_val, str_len);
 
 	match = (!cmp) ^ pred->not;
 
-- 1.6.3 

Re: [PATCH 1/2] tracing/events: add missing type info of dynamic arrays

[Frederic Weisbecker]

On Thu, Jul 16, 2009 at 10:53:34AM +0800, Li Zefan wrote:
> From: Lai Jiangshan <laijs@cn.fujitsu.com>
> 
> The format file doesn't contain enough information for
> __dynamic_array/__string. The type name is missing.
> 
> Before:
>   # cat format:
>   name: irq_handler_entry
>   ...
>           field:__data_loc name;  offset:16;      size:2;
> 
> After:
>   # cat format:
>   name: irq_handler_entry
>   ...
>           field:__data_loc char[] name;  offset:16;      size:2;
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>


Looks good to me, but I'd prefer to wait for Steve Ack, since he wasn't
sure...

Thanks.


> ---
>  include/trace/ftrace.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> index 1867553..cc78943 100644
> --- a/include/trace/ftrace.h
> +++ b/include/trace/ftrace.h
> @@ -120,7 +120,7 @@
>  
>  #undef __dynamic_array
>  #define __dynamic_array(type, item, len)				       \
> -	ret = trace_seq_printf(s, "\tfield:__data_loc " #item ";\t"	       \
> +	ret = trace_seq_printf(s, "\tfield:__data_loc " #type "[] " #item ";\t"\
>  			       "offset:%u;\tsize:%u;\n",		       \
>  			       (unsigned int)offsetof(typeof(field),	       \
>  					__data_loc_##item),		       \
> @@ -279,7 +279,7 @@ ftrace_raw_output_##call(struct trace_iterator *iter, int flags)	\
>  
>  #undef __dynamic_array
>  #define __dynamic_array(type, item, len)				       \
> -	ret = trace_define_field(event_call, "__data_loc" "[" #type "]", #item,\
> +	ret = trace_define_field(event_call, "__data_loc " #type "[]", #item,  \
>  				offsetof(typeof(field), __data_loc_##item),    \
>  				 sizeof(field.__data_loc_##item), 0);
>  
> -- 1.6.3 

Re: [PATCH 2/2] tracing/events: record the size of dynamic arrays

[Frederic Weisbecker]

On Thu, Jul 16, 2009 at 10:54:02AM +0800, Li Zefan wrote:
> When a dynamic array is defined, we add __data_loc_foo in
> trace_entry to record the offset of the array, but the
> size of the array is not recorded, which causes 2 problems:
> 
> - the event filter just compares the first 2 chars of the strings.
> 
> - parsers can't parse dynamic arrays.
> 
> So we encode the size of each dynamic array in the higher 16 bits
> of __data_loc_foo, while the offset is in lower 16 bits.
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  include/trace/ftrace.h             |   14 ++++++++------
>  kernel/trace/trace_events_filter.c |    6 ++++--
>  2 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> index cc78943..3cbb96e 100644
> --- a/include/trace/ftrace.h
> +++ b/include/trace/ftrace.h
> @@ -25,7 +25,7 @@
>  #define __array(type, item, len)	type	item[len];
>  
>  #undef __dynamic_array
> -#define __dynamic_array(type, item, len) unsigned short __data_loc_##item;
> +#define __dynamic_array(type, item, len) u32 __data_loc_##item;
>  
>  #undef __string
>  #define __string(item, src) __dynamic_array(char, item, -1)
> @@ -51,13 +51,14 @@
>   * Include the following:
>   *
>   * struct ftrace_data_offsets_<call> {
> - *	int				<item1>;
> - *	int				<item2>;
> + *	u32				<item1>;
> + *	u32				<item2>;
>   *	[...]
>   * };
>   *
> - * The __dynamic_array() macro will create each int <item>, this is
> + * The __dynamic_array() macro will create each u32 <item>, this is
>   * to keep the offset of each array from the beginning of the event.
> + * The size of an array is also encoded, in the higher 16 bits of <item>.
>   */
>  
>  #undef __field
> @@ -67,7 +68,7 @@
>  #define __array(type, item, len)
>  
>  #undef __dynamic_array
> -#define __dynamic_array(type, item, len)	int item;
> +#define __dynamic_array(type, item, len)	u32 item;
>  
>  #undef __string
>  #define __string(item, src) __dynamic_array(char, item, -1)
> @@ -207,7 +208,7 @@ ftrace_format_##call(struct trace_seq *s)				\
>  
>  #undef __get_dynamic_array
>  #define __get_dynamic_array(field)	\
> -		((void *)__entry + __entry->__data_loc_##field)
> +		((void *)__entry + (__entry->__data_loc_##field & 0xffff))
>  
>  #undef __get_str
>  #define __get_str(field) (char *)__get_dynamic_array(field)
> @@ -325,6 +326,7 @@ ftrace_define_fields_##call(void)					\
>  #define __dynamic_array(type, item, len)				\
>  	__data_offsets->item = __data_size +				\
>  			       offsetof(typeof(*entry), __data);	\
> +	__data_offsets->item |= (len * sizeof(type)) << 16;		\
>  	__data_size += (len) * sizeof(type);
>  
>  #undef __string
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index b9aae72..1c80ef7 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -176,11 +176,13 @@ static int filter_pred_string(struct filter_pred *pred, void *event,
>  static int filter_pred_strloc(struct filter_pred *pred, void *event,
>  			      int val1, int val2)
>  {
> -	unsigned short str_loc = *(unsigned short *)(event + pred->offset);
> +	u32 str_item = *(u32 *)(event + pred->offset);
> +	int str_loc = str_item & 0xffff;
> +	int str_len = str_item >> 16;
>  	char *addr = (char *)(event + str_loc);
>  	int cmp, match;
>  
> -	cmp = strncmp(addr, pred->str_val, pred->str_len);
> +	cmp = strncmp(addr, pred->str_val, str_len);
>  
>  	match = (!cmp) ^ pred->not;
>  
> -- 1.6.3 


Looks good.
Like the previous one, I prefer to wait for an Ack from Steve though.

Re: [PATCH 0/2] tracing/events: 2 fixes for dynamic arrays

[Li Zefan]

Hi Steven,

Could you have a look at these 2 patches?

> These 2 fixes are necessary to make __dynamic_array/__string
> parsable for users.
> 
> And also fix to pass the correct string length when filtering
> on dynamic strings.
> 
> [PATCH 1/2] tracing/events: add missing type info of dynamic arrays
> [PATCH 2/2] tracing/events: record the size of dynamic arrays
> ---
>  include/trace/ftrace.h             |   18 ++++++++++--------
>  kernel/trace/trace_events_filter.c |    6 ++++--
>  2 files changed, 14 insertions(+), 10 deletions(-)

Re: [PATCH 0/2] tracing/events: 2 fixes for dynamic arrays

[Steven Rostedt]

On Mon, 20 Jul 2009, Li Zefan wrote:

> Hi Steven,
> 
> Could you have a look at these 2 patches?

Thanks Li,

I just pulled them in

-- Steve

> 
> > These 2 fixes are necessary to make __dynamic_array/__string
> > parsable for users.
> > 
> > And also fix to pass the correct string length when filtering
> > on dynamic strings.
> > 
> > [PATCH 1/2] tracing/events: add missing type info of dynamic arrays
> > [PATCH 2/2] tracing/events: record the size of dynamic arrays
> > ---
> >  include/trace/ftrace.h             |   18 ++++++++++--------
> >  kernel/trace/trace_events_filter.c |    6 ++++--
> >  2 files changed, 14 insertions(+), 10 deletions(-)
> 

[tip:tracing/core] tracing/events: add missing type info of dynamic arrays

[tip-bot for Lai Jiangshan]

Commit-ID:  68fd60a8c8bca6af51805c45f286f0f2572ac977
Gitweb:     http://git.kernel.org/tip/68fd60a8c8bca6af51805c45f286f0f2572ac977
Author:     Lai Jiangshan <laijs@cn.fujitsu.com>
AuthorDate: Thu, 16 Jul 2009 10:53:34 +0800
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Mon, 20 Jul 2009 10:52:51 -0400

tracing/events: add missing type info of dynamic arrays

The format file doesn't contain enough information for
__dynamic_array/__string. The type name is missing.

Before:
  # cat format:
  name: irq_handler_entry
  ...
          field:__data_loc name;  offset:16;      size:2;

After:
  # cat format:
  name: irq_handler_entry
  ...
          field:__data_loc char[] name;  offset:16;      size:2;

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
LKML-Reference: <4A5E962E.9020900@cn.fujitsu.com>
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>


---
 include/trace/ftrace.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 1867553..cc78943 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -120,7 +120,7 @@
 
 #undef __dynamic_array
 #define __dynamic_array(type, item, len)				       \
-	ret = trace_seq_printf(s, "\tfield:__data_loc " #item ";\t"	       \
+	ret = trace_seq_printf(s, "\tfield:__data_loc " #type "[] " #item ";\t"\
 			       "offset:%u;\tsize:%u;\n",		       \
 			       (unsigned int)offsetof(typeof(field),	       \
 					__data_loc_##item),		       \
@@ -279,7 +279,7 @@ ftrace_raw_output_##call(struct trace_iterator *iter, int flags)	\
 
 #undef __dynamic_array
 #define __dynamic_array(type, item, len)				       \
-	ret = trace_define_field(event_call, "__data_loc" "[" #type "]", #item,\
+	ret = trace_define_field(event_call, "__data_loc " #type "[]", #item,  \
 				offsetof(typeof(field), __data_loc_##item),    \
 				 sizeof(field.__data_loc_##item), 0);
 

[tip:tracing/core] tracing/events: record the size of dynamic arrays

[tip-bot for Li Zefan]

Commit-ID:  7d536cb3fb9993bdcd5a2fbaa6b0670ded4e101c
Gitweb:     http://git.kernel.org/tip/7d536cb3fb9993bdcd5a2fbaa6b0670ded4e101c
Author:     Li Zefan <lizf@cn.fujitsu.com>
AuthorDate: Thu, 16 Jul 2009 10:54:02 +0800
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Mon, 20 Jul 2009 11:38:44 -0400

tracing/events: record the size of dynamic arrays

When a dynamic array is defined, we add __data_loc_foo in
trace_entry to record the offset of the array, but the
size of the array is not recorded, which causes 2 problems:

- the event filter just compares the first 2 chars of the strings.

- parsers can't parse dynamic arrays.

So we encode the size of each dynamic array in the higher 16 bits
of __data_loc_foo, while the offset is in lower 16 bits.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
LKML-Reference: <4A5E964A.9000403@cn.fujitsu.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>


---
 include/trace/ftrace.h             |   14 ++++++++------
 kernel/trace/trace_events_filter.c |    6 ++++--
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index cc78943..3cbb96e 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -25,7 +25,7 @@
 #define __array(type, item, len)	type	item[len];
 
 #undef __dynamic_array
-#define __dynamic_array(type, item, len) unsigned short __data_loc_##item;
+#define __dynamic_array(type, item, len) u32 __data_loc_##item;
 
 #undef __string
 #define __string(item, src) __dynamic_array(char, item, -1)
@@ -51,13 +51,14 @@
  * Include the following:
  *
  * struct ftrace_data_offsets_<call> {
- *	int				<item1>;
- *	int				<item2>;
+ *	u32				<item1>;
+ *	u32				<item2>;
  *	[...]
  * };
  *
- * The __dynamic_array() macro will create each int <item>, this is
+ * The __dynamic_array() macro will create each u32 <item>, this is
  * to keep the offset of each array from the beginning of the event.
+ * The size of an array is also encoded, in the higher 16 bits of <item>.
  */
 
 #undef __field
@@ -67,7 +68,7 @@
 #define __array(type, item, len)
 
 #undef __dynamic_array
-#define __dynamic_array(type, item, len)	int item;
+#define __dynamic_array(type, item, len)	u32 item;
 
 #undef __string
 #define __string(item, src) __dynamic_array(char, item, -1)
@@ -207,7 +208,7 @@ ftrace_format_##call(struct trace_seq *s)				\
 
 #undef __get_dynamic_array
 #define __get_dynamic_array(field)	\
-		((void *)__entry + __entry->__data_loc_##field)
+		((void *)__entry + (__entry->__data_loc_##field & 0xffff))
 
 #undef __get_str
 #define __get_str(field) (char *)__get_dynamic_array(field)
@@ -325,6 +326,7 @@ ftrace_define_fields_##call(void)					\
 #define __dynamic_array(type, item, len)				\
 	__data_offsets->item = __data_size +				\
 			       offsetof(typeof(*entry), __data);	\
+	__data_offsets->item |= (len * sizeof(type)) << 16;		\
 	__data_size += (len) * sizeof(type);
 
 #undef __string
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index b9aae72..1c80ef7 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -176,11 +176,13 @@ static int filter_pred_string(struct filter_pred *pred, void *event,
 static int filter_pred_strloc(struct filter_pred *pred, void *event,
 			      int val1, int val2)
 {
-	unsigned short str_loc = *(unsigned short *)(event + pred->offset);
+	u32 str_item = *(u32 *)(event + pred->offset);
+	int str_loc = str_item & 0xffff;
+	int str_len = str_item >> 16;
 	char *addr = (char *)(event + str_loc);
 	int cmp, match;
 
-	cmp = strncmp(addr, pred->str_val, pred->str_len);
+	cmp = strncmp(addr, pred->str_val, str_len);
 
 	match = (!cmp) ^ pred->not;