Date:	Sat, 27 Oct 2012 16:43:54 +0800
From:	Yuanhan Liu <yuanhan.liu@...ux.intel.com>
To:	Stefani Seibold <stefani@...bold.net>
Cc:	Yuanhan Liu <yliu.null@...il.com>, linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Wei Yang <weiyang@...ux.vnet.ibm.com>,
	Fengguang Wu <fengguang.wu@...el.com>,
	Stephen Rothwell <sfr@...b.auug.org.au>
Subject: Re: [PATCH] kfifo: remove unnecessary type check

On Fri, Oct 26, 2012 at 03:42:44PM +0200, Stefani Seibold wrote:
> Am Freitag, den 26.10.2012, 21:04 +0800 schrieb Yuanhan Liu:
> > On Fri, Oct 26, 2012 at 11:26:31AM +0200, Stefani Seibold wrote:
> > > Am Freitag, den 26.10.2012, 15:17 +0800 schrieb Yuanhan Liu:
> > > > On Fri, Oct 26, 2012 at 08:51:06AM +0200, Stefani Seibold wrote:
> > > > > Am Freitag, den 26.10.2012, 14:11 +0800 schrieb Yuanhan Liu:
> > > > > > On Fri, Oct 26, 2012 at 07:38:31AM +0200, Stefani Seibold wrote:
> > > > > > > Am Freitag, den 26.10.2012, 09:46 +0800 schrieb Yuanhan Liu:
> > > > > > > > From: Yuanhan Liu <yuanhan.liu@...ux.intel.com>
> > > > > > > > 
> > > > > > > > Firstly, this kind of type check doesn't work. It does something similay
> > > > > > > > like following:
> > > > > > > > 	void * __dummy = NULL;
> > > > > > > > 	__buf = __dummy;
> > > > > > > > 
> > > > > > > > __dummy is defined as void *. Thus it will not trigger warnings as
> > > > > > > > expected.
> > > > > > > > 
> > > > > > > > Second, we don't need that kind of check. Since the prototype
> > > > > > > > of __kfifo_out is:
> > > > > > > > 	unsigned int __kfifo_out(struct __kfifo *fifo,  void *buf, unsigned int len)
> > > > > > > > 
> > > > > > > > buf is defined as void *, so we don't need do the type check. Remove it.
> > > > > > > > 
> > > > > > > > LINK: https://lkml.org/lkml/2012/10/25/386
> > > > > > > > LINK: https://lkml.org/lkml/2012/10/25/584
> > > > > > > > 
> > > > > > > > Cc: Andrew Morton <akpm@...ux-foundation.org>
> > > > > > > > Cc: Wei Yang <weiyang@...ux.vnet.ibm.com>
> > > > > > > > Cc: Stefani Seibold <stefani@...bold.net>
> > > > > > > > Cc: Fengguang Wu <fengguang.wu@...el.com>
> > > > > > > > Cc: Stephen Rothwell <sfr@...b.auug.org.au>
> > > > > > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@...ux.intel.com>
> > > > > > > > ---
> > > > > 
> > 
> > [snip]...
> > 
> > > > > 
> > > > > Also you have to build the kfifo samples, since this example code use
> > > > > all features of the kfifo API.
> > > > > 
> > > > > And again: The kfifo is designed to do the many things at compile time,
> > > > > not at runtime. If you modify the code, you have to check the compiler
> > > > > assembler output for no degradation, especially in kfifo_put, kfifo_get,
> > > > > kfifo_in, kfifo_out, __kfifo_in and __kfifo_out. Prevent runtime checks
> > > > > if you can do it at compile time. This is the basic reasons to do it in
> > > > > macros.
> > > > 
> > > > Is it enought to check kernel/kfifo.o only? I build that file with
> > > > and without this patch. And then  dump it by objdump -D kernel/fifo.o to
> > > > /tmp/kfifo.dump.with and /tmp/kfifo.dump.without, respectively. And the
> > > > two dump file are exactly same.
> > > > 
> > > 
> > > No, since most of the code is inlined due performace reasons, you have
> > > to hack the kfifo examples output code for regressions and code
> > > increase.
> > 
> > In my test, this patch doesn't change anything. Here are some data to
> > prove that:
> > 
> > $ make samples/kfifo/
> > $ cp samples/kfifo/*.o /tmp/before/
> > 
> > $ git am this-patch
> > $ make samples/kfifo/
> > $ cp samples/kfifo/*.o /tmp/after/
> > 
> > $ for i in /tmp/before/*.o; do size $i /tmp/after/`basename $i`; done
> >    text    data     bss     dec     hex filename
> >    1939     464     456    2859     b2b /tmp/before/bytestream-example.o
> >    1939     464     456    2859     b2b /tmp/after/bytestream-example.o
> >    text    data     bss     dec     hex filename
> >    1423     112     296    1831     727 /tmp/before/dma-example.o
> >    1423     112     296    1831     727 /tmp/after/dma-example.o
> >    text    data     bss     dec     hex filename
> >    1864     624     376    2864     b30 /tmp/before/inttype-example.o
> >    1864     624     376    2864     b30 /tmp/after/inttype-example.o
> >    text    data     bss     dec     hex filename
> >    1916     464     472    2852     b24 /tmp/before/record-example.o
> >    1916     464     472    2852     b24 /tmp/after/record-example.o
> > # You will see that it changed nothing.
> > 
> > 
> > $ objdump -d /tmp/before/bytestream-example.o >/tmp/bytestream-example.before
> > $ objdump -d /tmp/after/bytestream-example.o >/tmp/bytestream-example.after
> > $ diff /tmp/bytestream.before /tmp/bytestream.after -urN
> > --- bytestream.before   2012-10-26 20:55:33.645578668 +0800
> > +++ bytestream.after    2012-10-26 20:55:26.520578669 +0800
> > @@ -1,5 +1,5 @@
> > 
> > -/tmp/bytestream-example.o:     file format elf64-x86-64
> > +/tmp/bytestream-example.o:     file format elf64-x86-64
> > 
> > # So, as you can see, expect the filename, they are same.
> > 
> > 
> > So, Stefani, is it what you want? Does this looks OK to you?
> 
> Perfect. It looks okay for me and i hope for you too ;-)
> 
> Acked by stefani@...bold.net

Thanks for that.  Well, I checked this code a bit more, and found I
forgot to remove something: ptr and const_ptr, which were used for type
checking only. Since we are gonna remove type checking, we don't need
them, too.

I did same allmodconfig build, size, objdump test to all kfifo users for
v2 patch as well, and found no new warning, error or assembler changes.

Will sent out v2 soon. And sorry for not noticing this issue early. :(

Thanks,
Yuanhan Liu
--
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