| 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
| ||
|
Message-ID: <20090226185208.GA6658@nowhere>
Date: Thu, 26 Feb 2009 19:52:09 +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 07:34:16PM +0100, Frederic Weisbecker wrote:
> On Thu, Feb 26, 2009 at 06:52:25PM +0100, Ingo Molnar wrote:
> >
> > * Frederic Weisbecker <fweisbec@...il.com> wrote:
> >
> > > On Thu, Feb 26, 2009 at 06:43:03PM +0100, Ingo Molnar wrote:
> > > >
> > > > * 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
> > >
> > > Right, ok I'm on it.
> >
> > hm, it would still be nice to get vbin_printf() into the same
> > scheme too.
> >
> > And i think we can do it by unconditionally tracking field type
> > and width in the generic helper - instead of passing this across
> > the abstraction barrier like your proposal did. vsnprintf() and
> > bstr_printf() wont make use of it - but vbin_printf() will.
> >
> > Am i missing anything?
> >
> > Ingo
>
> Hm, I have some trouble to visualize it globally.
>
> vsnprintf: takes char * for dest and va_list as src, write formatted in dest
> bstr_printf: takes char * as dest and u32 * as src, read from src is specialized
> vbin_printf: takes u32 * buf as dest and va_list as src, write to dest is specialized
>
> The only thing that can be generic between the three is the format decoding.
> All in-out operations must be abstracted if we want a common interface from the three.
>
> So I'm not sure I have the choice.
>
Instead of calling the in/out helper from the decoder,
why not calling the decoder from these three functions and let them
take the appropriate actions for each decoded format token?
Naive example:
bstr_printf() {
while (decode_format(...))
if (type == number)
read_number_from_buf()
str = number(....)
....
}
vsnprintf {
while (decode_format(...))
if (type == number)
var_arg(...)
str = number(....)
....
}
vbin_printf {
while (decode_format(...))
if (type == number)
var_arg(...)
write_number_to_buffer()
...
}
And the standard in/out pieces can be invoked through helpers.
--
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