Re: [RFC PATCH v4 1/3] tracing/events: Fix wrong sample output by storing array length instead of size

["Alex Bennée" <alex.bennee@linaro.org>]

Steven Rostedt <rostedt@goodmis.org> writes:

> On Fri, 17 Jul 2015 10:32:15 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
>
>> This change affects all callers of dymanic_array, not just bitmasks.
>> 
>> >  	__data_size += __item_length;
>> >  
>> >  #undef __string
>> 
>> BTW, if I revert commit ac01ce1410fc2 "tracing: Make
>> ftrace_print_array_seq compute buf_len" it works again.
>> 
>> I'm going to look into this some more, and maybe the answer is to go
>> back and just pass in buffer length here. I can't see what was broken
>> before that change.
>
> OK, the print_array() code is already being used by the thermal events
> and can't be changed. But we can't make the proposed change because
> that changes the user interface.
>
> What we can change is the sample code!
>
> -- Steve
>
> From 95de1e9721a2f9d05831a53d228e181a33001c55 Mon Sep 17 00:00:00 2001
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> Date: Fri, 17 Jul 2015 14:03:26 -0400
> Subject: [PATCH] tracing: Fix sample output of dynamic arrays
>
> He Kuang noticed that the trace event samples for arrays was broken:
>
> "The output result of trace_foo_bar event in traceevent samples is
>  wrong. This problem can be reproduced as following:
>
>   (Build kernel with SAMPLE_TRACE_EVENTS=m)
>
>   $ insmod trace-events-sample.ko
>
>   $ echo 1 > /sys/kernel/debug/tracing/events/sample-trace/foo_bar/enable
>
>   $ cat /sys/kernel/debug/tracing/trace
>
>   event-sample-980 [000] ....  43.649559: foo_bar: foo hello 21 0x15
>   BIT1|BIT3|0x10 {0x1,0x6f6f6e53,0xff007970,0xffffffff} Snoopy
>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                  The array length is not right, should be {0x1}.
>   (ffffffff,ffffffff)
>
>   event-sample-980 [000] ....  44.653827: foo_bar: foo hello 22 0x16
>   BIT2|BIT3|0x10
>   {0x1,0x2,0x646e6147,0x666c61,0xffffffff,0xffffffff,0x750aeffe,0x7}
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                  The array length is not right, should be {0x1,0x2}.
>   Gandalf (ffffffff,ffffffff)"
>
> This was caused by an update to have __print_array()'s second parameter
> be the count of items in the array and not the size of the array.
>
> As there is already users of __print_array(), it can not change. But
> the sample code can and we can also improve on the documentation about
> __print_array() and __get_dynamic_array_len().
>
> Link: http://lkml.kernel.org/r/1436839171-31527-2-git-send-email-hekuang@huawei.com
>
> Fixes: ac01ce1410fc2 ("tracing: Make ftrace_print_array_seq compute buf_len")
> Reported-by: He Kuang <hekuang@huawei.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  samples/trace_events/trace-events-sample.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/samples/trace_events/trace-events-sample.h b/samples/trace_events/trace-events-sample.h
> index 8965d1bb8811..125d6402f64f 100644
> --- a/samples/trace_events/trace-events-sample.h
> +++ b/samples/trace_events/trace-events-sample.h
> @@ -168,7 +168,10 @@
>   *
>   *      For __dynamic_array(int, foo, bar) use __get_dynamic_array(foo)
>   *            Use __get_dynamic_array_len(foo) to get the length of the array
> - *            saved.
> + *            saved. Note, __get_dynamic_array_len() returns the total allocated
> + *            length of the dynamic array; __print_array() expects the second
> + *            parameter to be the number of elements. To get that, the array length
> + *            needs to be divided by the element size.
>   *
>   *      For __string(foo, bar) use __get_str(foo)
>   *
> @@ -288,7 +291,7 @@ TRACE_EVENT(foo_bar,
>   *    This prints out the array that is defined by __array in a nice format.
>   */
>  		  __print_array(__get_dynamic_array(list),
> -				__get_dynamic_array_len(list),
> +				__get_dynamic_array_len(list) / sizeof(int),
>  				sizeof(int)),
>  		  __get_str(str), __get_bitmask(cpus))
>  );

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée