From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754934AbZBZSwZ (ORCPT ); Thu, 26 Feb 2009 13:52:25 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752137AbZBZSwQ (ORCPT ); Thu, 26 Feb 2009 13:52:16 -0500 Received: from an-out-0708.google.com ([209.85.132.241]:5730 "EHLO an-out-0708.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752123AbZBZSwP (ORCPT ); Thu, 26 Feb 2009 13:52:15 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; b=fPvj15cIL1TOgaPBKR5Y5ymBT7BRqHtxOLQh/Oxd113havSmInBPAFhMeeeaSa5bPG 6Udj+GqoZZWLT+RAo6NUO6sIOEDjlvT0AAEgjoAwj7TYKFSBqFl6qnBO3h8CQ6KMUFqY oazh0lYdymAYc0YKFci507N9lO6H7vJkh8uJE= Date: Thu, 26 Feb 2009 19:52:09 +0100 From: Frederic Weisbecker To: Ingo Molnar Cc: Linus Torvalds , Andrew Morton , linux-kernel@vger.kernel.org, Steven Rostedt , Lai Jiangshan , Peter Zijlstra Subject: Re: [PATCH 1/3] add binary printf Message-ID: <20090226185208.GA6658@nowhere> References: <49a38304.0506d00a.1f4b.406d@mx.google.com> <20090226130243.GA22460@elte.hu> <20090226170524.GB5889@nowhere> <20090226174303.GC29439@elte.hu> <20090226174547.GC5889@nowhere> <20090226175225.GA4527@elte.hu> <20090226183415.GE5889@nowhere> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20090226183415.GE5889@nowhere> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 wrote: > > > > > On Thu, Feb 26, 2009 at 06:43:03PM +0100, Ingo Molnar wrote: > > > > > > > > * Frederic Weisbecker wrote: > > > > > > > > > On Thu, Feb 26, 2009 at 02:02:43PM +0100, Ingo Molnar wrote: > > > > > > > > > > > > * Frederic Weisbecker wrote: > > > > > > > > > > > > > From: Lai Jiangshan > > > > > > > > > > > > > > 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@gmail.com: ported to latest -tip] > > > > > > > > > > > > > > Signed-off-by: Lai Jiangshan > > > > > > > Signed-off-by: Frederic Weisbecker > > > > > > > Cc: Steven Rostedt > > > > > > > --- > > > > > > > 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.