Date:	Thu, 26 Feb 2009 18:43:03 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>,
	Lai Jiangshan <laijs@...fujitsu.com>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH 1/3] add binary printf


* Frederic Weisbecker <fweisbec@...il.com> wrote:

> On Thu, Feb 26, 2009 at 02:02:43PM +0100, Ingo Molnar wrote:
> > 
> > * Frederic Weisbecker <fweisbec@...il.com> wrote:
> > 
> > > From: Lai Jiangshan <laijs@...fujitsu.com>
> > > 
> > > Impact: Add APIs for binary trace printk infrastructure
> > > 
> > > vbin_printf(): write args to binary buffer, string is copied
> > > when "%s" is occurred.
> > > 
> > > bstr_printf(): read from binary buffer for args and format a string
> > > 
> > > [fweisbec@...il.com: ported to latest -tip]
> > > 
> > > Signed-off-by: Lai Jiangshan <laijs@...fujitsu.com>
> > > Signed-off-by: Frederic Weisbecker <fweisbec@...il.com>
> > > Cc: Steven Rostedt <rostedt@...dmis.org>
> > > ---
> > >  include/linux/string.h |    7 +
> > >  lib/Kconfig            |    3 +
> > >  lib/vsprintf.c         |  442 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 452 insertions(+), 0 deletions(-)
> > 
> > OK, it's a nice idea and speedup for printf based tracing - 
> > which is common and convenient. Would you mind to post the 
> > performance measurements you've done using the new bstr_printf() 
> > facility? (the nanoseconds latency figures you did in the timer 
> > irq in a system under load and on a system that is idle)
> 
> 
> Ok.
> 
>  
> > The new printf code itself should be done cleaner i think and is 
> > not acceptable in its current form.
> > 
> > These two new functions:
> > 
> > > +#ifdef CONFIG_BINARY_PRINTF
> > > +/*
> > > + * bprintf service:
> > > + * vbin_printf() - VA arguments to binary data
> > > + * bstr_printf() - Binary data to text string
> > > + */
> > 
> > Duplicate hundreds of lines of code into three large functions 
> > (vsnprintf, vbin_printf, bstr_printf). These functions only have 
> > a difference in the way the argument list is iterated and the 
> > way the parsed result is stored:
> > 
> >   vsnprintf:   iterates va_list, stores into string
> >   bstr_printf: iterates bin_buf, stores into string
> >   vbin_printf: iterates va_list, stores into bin_buf
> > 
> > We should try _much_ harder at unifying these functions before 
> > giving up and duplicating them...
> > 
> > An opaque in_buf/out_buf handle plus two helper function 
> > pointers passed in would be an obvious implementation.
> > 
> > That way we'd have a single generic (inline) function that knows 
> > about the printf format itself:
> > 
> >  __generic_printf(void *in_buf,
> > 		  void *out_buf,
> > 		  void * (*read_in_buf)(void **),
> > 		  void * (*store_out_buf)(void **));
> > 
> > And we'd have various variants for read_in_buf and 
> > store_out_buf. The generic function iterates the following way:
> > 
> > 	in_val = read_in_buf(&in_buf);
> > 	...
> > 	store_out_buf(&out_buf, in_val);
> > 
> > (where in_val is wide enough to store a single argument.) The 
> > iterators modify the in_buf / out_buf pointers. Argument 
> > skipping can be done by reading the in-buf and not using it. I 
> > think we can do it with just two iterator methods.
> > 
> > Or something like that - you get the idea. It can all be inlined 
> > so that we'd end up with essentially the same vsnprint() 
> > instruction sequence we have today.
> > 
> > 	Ingo
> 
> 
> Ok, I just looked deeply inside vsnprintf, and I don't think 
> such a generic interface would allow that. We need to know the 
> size of the argument, it's precision, width and flags.... And 
> we need to know if we want to skip the non format char.
> 
> 
> What do you think of the following:
> 
> __ generic_printf(void *in,
> 		  void *out,
> 		  void *(*read_in)(void **buf, int size),
> 		  void *(store_char)(char *dst, char *end, char val, int field_width, int flags),
> 		  void *(*store_string)(char *dst, char *end, char *val, int field_width, int precision, int flags),
> 		  void *(*store_pointer)(char type, char *dst, char *end, void *val,
> 					int field_width, int precision, int flags),
> 		  void *(*store_number)(char *dst, char *size, int base,int field_width, int precision, int flags),
> 		  bool skip_non_format
> 		  )
> 
> 
> Well, something like that...
> 
> read_in can advance the pointer to the buffer itself (buf can 
> be a va_args or u32 *) and it returns a value, void * is 
> generic for the type.
> 
> The storage functions are more specialized because of the 
> interpretation of flags, precision... So we can easily pass 
> the usual string(), pointer(), .... that are already present 
> in vsnprintf.c or use custom ones. They return the advanced 
> dst pointer.
> 
> And at last, skip_non_format will decide if we want to 
> consider non-format characters from fmt to be copied as common 
> %c characters or if we want to ignore them (useful for 
> vbin_printf()).

hm, that indeed looks very wide - storing into a binary buffer 
does complicate the iterator interface significantly.

But at least vsnprintf() and bstr_printf() could be unified - 
they both output into a string buffer, just have different 
methods to iterate arguments.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists