Re: [RFC][PATCH 1/2] tracing/events: provide string with undefined size support

[Frederic Weisbecker <fweisbec@gmail.com>]

On Wed, Apr 15, 2009 at 09:33:15PM -0400, Steven Rostedt wrote:
> 
> On Thu, 16 Apr 2009, Frederic Weisbecker wrote:
> 
> > Impact: less memory usage for tracing
> > 
> > This patch provides the support for dynamic size strings on
> > event tracing.
> > 
> > The key concept is to use a structure with an ending char array field of
> > undefined size and use such ability to allocate the minimal size on the ring
> > buffer to make the entry fit inside as opposite to a fixed length strings with
> > upper bound.
> > 
> > This patch provides two new macros:
> > 
> > -__ending_string(name)
> > 
> > This one declares the string to the structure inside TP_STRUCT__entry.
> > Only the name is needed.
> > Two constraints: only one __ending_string() per TRACE_EVENT can be added and
> > it must be the last field to be declared. Hence the __ending prefix.
> > 
> > - open_string_assign(call, dst, src)
> > 
> > This one does the copy inside TP__fast_assign() of the source
> > string to the destination. The name of the tracepoint (call) must be provided
> > for now. Hopefully I will find a solution to avoid it later.
> > 
> > Two constraint: can be used only once and always on the beginning because
> > it needs to manage the ring buffer reservation by itself. Hence the open prefix.
> > 
> > How does it works?
> > 
> > A new has_ending_string field has been added to struct ftrace_event_call and is
> > false by default.
> > Once a TRACE_EVENT uses an __ending_string field, it is set to 1.
> > 
> > Until now, the ring buffer reservation was done in ftrace_raw_event_##call().
> > It is still the case if we don't have an __ending_string() field. If we have one,
> > open_string_assign() will manage it itself to allocate the appropriate size,
> > depending of the size of the string to be copied for each trace.
> > 
> > The choice between the usual static allocation and the new dynamic one depends
> > on the "has_ending_string" value.
> > 
> > It also support filtering because these strings behave essentially
> > like usual fixed length string.
> 
> And I thought I was the only one that could master CPP wackiness ;-)


:-)


> 
> BTW, I was confused on IRC, because I was thinking this was printf like,
> but no, we only need to be concerned about a string.
> 
> But I have another idea:
> 
> 
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > ---
> >  include/linux/ftrace_event.h |    1 +
> >  include/trace/ftrace.h       |   62 ++++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 55 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> > index 75f3ac0..b49bfbf 100644
> > --- a/include/linux/ftrace_event.h
> > +++ b/include/linux/ftrace_event.h
> > @@ -100,6 +100,7 @@ struct ftrace_event_call {
> >  	int			n_preds;
> >  	struct filter_pred	**preds;
> >  	void			*mod;
> > +	bool			has_ending_string;
> >  
> >  #ifdef CONFIG_EVENT_PROFILE
> >  	atomic_t	profile_count;
> > diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> > index 60c5323..8ea750e 100644
> > --- a/include/trace/ftrace.h
> > +++ b/include/trace/ftrace.h
> > @@ -27,6 +27,9 @@
> >  #undef __field
> >  #define __field(type, item)		type	item;
> >  
> > +#undef __ending_string
> > +#define __ending_string(item)			char	item[];
> > +
> >  #undef TP_STRUCT__entry
> >  #define TP_STRUCT__entry(args...) args
> >  
> > @@ -146,12 +149,23 @@ ftrace_raw_output_##call(struct trace_iterator *iter, int flags)	\
> >  	if (!ret)							\
> >  		return 0;
> >  
> > +#undef __ending_string
> > +#define __ending_string(item)						      \
> > +	ret = trace_seq_printf(s, "\tfield: char " #item "[];\t"	      \
> > +			       "offset:%u;\n",				      \
> > +			       (unsigned int)offsetof(typeof(field), item));   \
> > +	if (!ret)							      \
> > +		return 0;
> > +
> >  #undef __entry
> >  #define __entry REC
> >  
> >  #undef TP_printk
> >  #define TP_printk(fmt, args...) "%s, %s\n", #fmt, __stringify(args)
> >  
> > +#undef open_string_assign
> > +#define open_string_assign(call, dst, src)	strcpy(__entry->dst, src)
> > +
> >  #undef TP_fast_assign
> >  #define TP_fast_assign(args...) args
> >  
> > @@ -189,6 +203,19 @@ ftrace_format_##call(struct trace_seq *s)				\
> >  	if (ret)							\
> >  		return ret;
> >  
> > +/*
> > + * We choose a size of MAX_FILTER_STR_VAL, then we behave like
> > + * a usual string with the maximum size to keep being filterable.
> > + */
> > +#undef __ending_string
> > +#define __ending_string(item)						\
> > +	ret = trace_define_field(event_call, "char []", #item,		\
> > +				 offsetof(typeof(field), item),		\
> > +				 MAX_FILTER_STR_VAL);			\
> > +	if (ret)							\
> > +		return ret;						\
> > +	event_call->has_ending_string = true;
> > +
> >  #undef TRACE_EVENT
> >  #define TRACE_EVENT(call, proto, args, tstruct, func, print)		\
> >  int									\
> > @@ -409,6 +436,22 @@ __attribute__((section("_ftrace_events"))) event_##call = {		\
> >  #undef __entry
> >  #define __entry entry
> >  
> > +/*
> > + * If we have and ending undefined string size, then the size of
> > + * the entry is dynamic. In such case we override the ring buffer
> > + * reservation to manage it ourselves with our dynamic string size.
> > + */
> > +#undef open_string_assign
> > +#define open_string_assign(call, dst, src)				       \
> > +	event = trace_current_buffer_lock_reserve(			       \
> > +			event_##call.id,				       \
> > +			sizeof(struct ftrace_raw_##call) + strlen(src) + 1,    \
> > +			irq_flags, pc);					       \
> > +	if (!event)							       \
> > +		return;							       \
> > +	entry	= ring_buffer_event_data(event);			       \
> > +	strcpy(entry->dst, src);
> > +
> >  #undef TRACE_EVENT
> >  #define TRACE_EVENT(call, proto, args, tstruct, assign, print)		\
> >  _TRACE_PROFILE(call, PARAMS(proto), PARAMS(args))			\
> > @@ -418,20 +461,23 @@ static struct ftrace_event_call event_##call;				\
> >  static void ftrace_raw_event_##call(proto)				\
> >  {									\
> >  	struct ftrace_event_call *call = &event_##call;			\
> > -	struct ring_buffer_event *event;				\
> > -	struct ftrace_raw_##call *entry;				\
> > +	struct ring_buffer_event *event = NULL;				\
> > +	struct ftrace_raw_##call *entry = NULL;				\
> >  	unsigned long irq_flags;					\
> >  	int pc;								\
> >  									\
> >  	local_save_flags(irq_flags);					\
> >  	pc = preempt_count();						\
> >  									\
> > -	event = trace_current_buffer_lock_reserve(event_##call.id,	\
> > -				  sizeof(struct ftrace_raw_##call),	\
> > -				  irq_flags, pc);			\
> > -	if (!event)							\
> > -		return;							\
> > -	entry	= ring_buffer_event_data(event);			\
> > +	if (!call->has_ending_string) {					\
> > +		event = trace_current_buffer_lock_reserve(		\
> > +				event_##call.id,			\
> > +				sizeof(struct ftrace_raw_##call),	\
> > +				irq_flags, pc);				\
> > +		if (!event)						\
> > +			return;						\
> > +		entry	= ring_buffer_event_data(event);		\
> > +	}								\
> >  									\
> >  	assign;								\
> >  									\
> 
> 
> We can get rid of the has_ending_string and open_string_assign and have 
> this:
> 
>  
>         TP_STRUCT__entry(
>                 __field(unsigned long, wait_usec)
>                 __field(unsigned long, wait_nsec_rem)
>                 __ending_string(name)
>         ),
>         TP_fast_assign(
>                 __entry->wait_nsec_rem = do_div(waittime, NSEC_PER_USEC);
>                 __entry->wait_usec = (unsigned long) waittime;
>                 strcpy(__entry->name, lock->name);
>         ),
> 
> 
> Then we could do the following in the print the assign:
> 
> #undef __field
> #define __field(a,b)
> #undef __array
> #define __array(a,b,c)
> #undef __ending_sting
> #define __ending_string(name) __str_size__ += strlen(name) + 1;
> 
> #define TRACE_EVENT(call, proto, args, tstruct, assign, print)
> static void ftrace_raw_event_##call(proto)
> {
> 	int __str_size__ = 0;
> 
> 	tstruct
> 
> 	event = trace_current_buffer_lock_reserve(event_##call.id,
> 				sizeof(struct ftrace_raw_##call) +
> 				__str_size__,
> 				irq_flags, pc)
> 	if (!event)
> 		return;
> 
> 	entry = ring_buffer_event_data(event);
> 
> 	assign
> }
> 
> 
> Yes, I did not add other declarations nor the backslashes for the macro, 
> but you get what I'm doing, right?



Oh!
I'm surprised, this is much more proper this way and also so obvious too!

Thanks a lot, I will integrate this simplification in a v2.



> -- Steve
>