linux-kernel - Re: [PATCH 1/3] add binary printf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20090226170524.GB5889@nowhere>
Date:	Thu, 26 Feb 2009 18:05:25 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Ingo Molnar <mingo@...e.hu>
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

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()).

Hmm?

--
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/