Re: [PATCH 01/13] tracing/events: Add __vstring() and __assign_vstr() helper macros

[Chunfeng Yun <chunfeng.yun@mediatek.com>]

On Tue, 2022-07-05 at 18:44 -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> There's several places that open code the following logic:
> 
>   TP_STRUCT__entry(__dynamic_array(char, msg, MSG_MAX)),
>   TP_fast_assign(vsnprintf(__get_str(msg), MSG_MAX, vaf->fmt, *vaf-
> >va);)
> 
> To load a string created by variable array va_list.
> 
> The main issue with this approach is that "MSG_MAX" usage in the
> __dynamic_array() portion. That actually just reserves the MSG_MAX in
> the
> event, and even wastes space because there's dynamic meta data also
> saved
> in the event to denote the offset and size of the dynamic array. It
> would
> have been better to just use a static __array() field.
> 
> Instead, create __vstring() and __assign_vstr() that work like
> __string
> and __assign_str() but instead of taking a destination string to
> copy,
> take a format string and a va_list pointer and fill in the values.
> 
> It uses the helper:
> 
>  #define __trace_event_vstr_len(fmt, va)	\
>  ({						\
> 	va_list __ap;				\
> 	int __ret;				\
> 						\
> 	va_copy(__ap, *(va));			\
> 	__ret = vsnprintf(NULL, 0, fmt, __ap);	\
> 	va_end(__ap);				\
> 						\
> 	min(__ret, TRACE_EVENT_STR_MAX);	\
>  })
> 
> To figure out the length to store the string. It may be slightly
> slower as
> it needs to run the vsnprintf() twice, but it now saves space on the
> ring
> buffer.
> 
> Cc: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Leon Romanovsky <leon@kernel.org>
> Cc: Kalle Valo <kvalo@kernel.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Arend van Spriel <aspriel@gmail.com>
> Cc: Franky Lin <franky.lin@broadcom.com>
> Cc: Hante Meuleman <hante.meuleman@broadcom.com>
> Cc: Gregory Greenman <gregory.greenman@intel.com>
> Cc: Peter Chen <peter.chen@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Mathias Nyman <mathias.nyman@intel.com>
> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Cc: Bin Liu <b-liu@ti.com>
> Cc: Marek Lindner <mareklindner@neomailbox.ch>
> Cc: Simon Wunderlich <sw@simonwunderlich.de>
> Cc: Antonio Quartulli <a@unstable.cc>
> Cc: Sven Eckelmann <sven@narfation.org>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Cc: Jim Cromie <jim.cromie@gmail.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Seems difficult to get this patch without, such as, '--cc="
linux-kernel@vger.kernel.org"'

> ---
>  include/linux/trace_events.h                 | 18 ++++++++++++++++++
>  include/trace/stages/stage1_struct_define.h  |  3 +++
>  include/trace/stages/stage2_data_offsets.h   |  3 +++
>  include/trace/stages/stage4_event_fields.h   |  3 +++
>  include/trace/stages/stage5_get_offsets.h    |  4 ++++
>  include/trace/stages/stage6_event_callback.h |  7 +++++++
>  6 files changed, 38 insertions(+)
> 
> diff --git a/include/linux/trace_events.h
> b/include/linux/trace_events.h
> index e6e95a9f07a5..e6f8ba52a958 100644
> --- a/include/linux/trace_events.h
> +++ b/include/linux/trace_events.h
> @@ -916,6 +916,24 @@ perf_trace_buf_submit(void *raw_data, int size,
> int rctx, u16 type,
>  
>  #endif
>  
> +#define TRACE_EVENT_STR_MAX	512
> +
> +/*
> + * gcc warns that you can not use a va_list in an inlined
> + * function. But lets me make it into a macro :-/
> + */
> +#define __trace_event_vstr_len(fmt, va)		\
> +({						\
> +	va_list __ap;				\
> +	int __ret;				\
> +						\
> +	va_copy(__ap, *(va));			\
> +	__ret = vsnprintf(NULL, 0, fmt, __ap);	\
> +	va_end(__ap);				\
> +						\
> +	min(__ret, TRACE_EVENT_STR_MAX);	\
> +})
> +
>  #endif /* _LINUX_TRACE_EVENT_H */
>  
>  /*
> diff --git a/include/trace/stages/stage1_struct_define.h
> b/include/trace/stages/stage1_struct_define.h
> index a16783419687..1b7bab60434c 100644
> --- a/include/trace/stages/stage1_struct_define.h
> +++ b/include/trace/stages/stage1_struct_define.h
> @@ -26,6 +26,9 @@
>  #undef __string_len
>  #define __string_len(item, src, len) __dynamic_array(char, item, -1)
>  
> +#undef __vstring
> +#define __vstring(item, fmt, ap) __dynamic_array(char, item, -1)
> +
>  #undef __bitmask
>  #define __bitmask(item, nr_bits) __dynamic_array(char, item, -1)
>  
> diff --git a/include/trace/stages/stage2_data_offsets.h
> b/include/trace/stages/stage2_data_offsets.h
> index 42fd1e8813ec..1b7a8f764fdd 100644
> --- a/include/trace/stages/stage2_data_offsets.h
> +++ b/include/trace/stages/stage2_data_offsets.h
> @@ -32,6 +32,9 @@
>  #undef __string_len
>  #define __string_len(item, src, len) __dynamic_array(char, item, -1)
>  
> +#undef __vstring
> +#define __vstring(item, fmt, ap) __dynamic_array(char, item, -1)
> +
>  #undef __bitmask
>  #define __bitmask(item, nr_bits) __dynamic_array(unsigned long,
> item, -1)
>  
> diff --git a/include/trace/stages/stage4_event_fields.h
> b/include/trace/stages/stage4_event_fields.h
> index e80cdc397a43..c3790ec7a453 100644
> --- a/include/trace/stages/stage4_event_fields.h
> +++ b/include/trace/stages/stage4_event_fields.h
> @@ -38,6 +38,9 @@
>  #undef __string_len
>  #define __string_len(item, src, len) __dynamic_array(char, item, -1)
>  
> +#undef __vstring
> +#define __vstring(item, fmt, ap) __dynamic_array(char, item, -1)
> +
>  #undef __bitmask
>  #define __bitmask(item, nr_bits) __dynamic_array(unsigned long,
> item, -1)
>  
> diff --git a/include/trace/stages/stage5_get_offsets.h
> b/include/trace/stages/stage5_get_offsets.h
> index 7ee5931300e6..fba4c24ed9e6 100644
> --- a/include/trace/stages/stage5_get_offsets.h
> +++ b/include/trace/stages/stage5_get_offsets.h
> @@ -39,6 +39,10 @@
>  #undef __string_len
>  #define __string_len(item, src, len) __dynamic_array(char, item,
> (len) + 1)
>  
> +#undef __vstring
> +#define __vstring(item, fmt, ap) __dynamic_array(char, item,		
> \
> +		      __trace_event_vstr_len(fmt, ap))
> +
>  #undef __rel_dynamic_array
>  #define __rel_dynamic_array(type, item, len)				
> \
>  	__item_length = (len) * sizeof(type);				
> \
> diff --git a/include/trace/stages/stage6_event_callback.h
> b/include/trace/stages/stage6_event_callback.h
> index e1724f73594b..0f51f6b3ab70 100644
> --- a/include/trace/stages/stage6_event_callback.h
> +++ b/include/trace/stages/stage6_event_callback.h
> @@ -24,6 +24,9 @@
>  #undef __string_len
>  #define __string_len(item, src, len) __dynamic_array(char, item, -1)
>  
> +#undef __vstring
> +#define __vstring(item, fmt, ap) __dynamic_array(char, item, -1)
> +
>  #undef __assign_str
>  #define __assign_str(dst, src)					
> 	\
>  	strcpy(__get_str(dst), (src) ? (const char *)(src) : "(null)");
> @@ -35,6 +38,10 @@
>  		__get_str(dst)[len] = '\0';				
> \
>  	} while(0)
>  
> +#undef __assign_vstr
> +#define __assign_vstr(dst, fmt, va)					
> \
> +	vsnprintf(__get_str(dst), TRACE_EVENT_STR_MAX, fmt, *(va))
> +
>  #undef __bitmask
>  #define __bitmask(item, nr_bits) __dynamic_array(unsigned long,
> item, -1)
>