* [PATCH] kfifo: remove unnecessary type check @ 2012-10-26 1:46 Yuanhan Liu 2012-10-26 5:38 ` Stefani Seibold 0 siblings, 1 reply; 12+ messages in thread From: Yuanhan Liu @ 2012-10-26 1:46 UTC (permalink / raw) To: linux-kernel Cc: Yuanhan Liu, Andrew Morton, Wei Yang, Stefani Seibold, Fengguang Wu, Stephen Rothwell From: Yuanhan Liu <yuanhan.liu@linux.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@linux-foundation.org> Cc: Wei Yang <weiyang@linux.vnet.ibm.com> Cc: Stefani Seibold <stefani@seibold.net> Cc: Fengguang Wu <fengguang.wu@intel.com> Cc: Stephen Rothwell <sfr@canb.auug.org.au> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> --- include/linux/kfifo.h | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h index 10308c6..b8c1d03 100644 --- a/include/linux/kfifo.h +++ b/include/linux/kfifo.h @@ -390,10 +390,6 @@ __kfifo_int_must_check_helper( \ unsigned int __ret; \ const size_t __recsize = sizeof(*__tmp->rectype); \ struct __kfifo *__kfifo = &__tmp->kfifo; \ - if (0) { \ - typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \ - __dummy = (typeof(__val))NULL; \ - } \ if (__recsize) \ __ret = __kfifo_in_r(__kfifo, __val, sizeof(*__val), \ __recsize); \ @@ -432,8 +428,6 @@ __kfifo_uint_must_check_helper( \ unsigned int __ret; \ const size_t __recsize = sizeof(*__tmp->rectype); \ struct __kfifo *__kfifo = &__tmp->kfifo; \ - if (0) \ - __val = (typeof(__tmp->ptr))0; \ if (__recsize) \ __ret = __kfifo_out_r(__kfifo, __val, sizeof(*__val), \ __recsize); \ @@ -473,8 +467,6 @@ __kfifo_uint_must_check_helper( \ unsigned int __ret; \ const size_t __recsize = sizeof(*__tmp->rectype); \ struct __kfifo *__kfifo = &__tmp->kfifo; \ - if (0) \ - __val = (typeof(__tmp->ptr))NULL; \ if (__recsize) \ __ret = __kfifo_out_peek_r(__kfifo, __val, sizeof(*__val), \ __recsize); \ @@ -512,10 +504,6 @@ __kfifo_uint_must_check_helper( \ unsigned long __n = (n); \ const size_t __recsize = sizeof(*__tmp->rectype); \ struct __kfifo *__kfifo = &__tmp->kfifo; \ - if (0) { \ - typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \ - __dummy = (typeof(__buf))NULL; \ - } \ (__recsize) ?\ __kfifo_in_r(__kfifo, __buf, __n, __recsize) : \ __kfifo_in(__kfifo, __buf, __n); \ @@ -565,10 +553,6 @@ __kfifo_uint_must_check_helper( \ unsigned long __n = (n); \ const size_t __recsize = sizeof(*__tmp->rectype); \ struct __kfifo *__kfifo = &__tmp->kfifo; \ - if (0) { \ - typeof(__tmp->ptr) __dummy = NULL; \ - __buf = __dummy; \ - } \ (__recsize) ?\ __kfifo_out_r(__kfifo, __buf, __n, __recsize) : \ __kfifo_out(__kfifo, __buf, __n); \ @@ -777,10 +761,6 @@ __kfifo_uint_must_check_helper( \ unsigned long __n = (n); \ const size_t __recsize = sizeof(*__tmp->rectype); \ struct __kfifo *__kfifo = &__tmp->kfifo; \ - if (0) { \ - typeof(__tmp->ptr) __dummy __attribute__ ((unused)) = NULL; \ - __buf = __dummy; \ - } \ (__recsize) ? \ __kfifo_out_peek_r(__kfifo, __buf, __n, __recsize) : \ __kfifo_out_peek(__kfifo, __buf, __n); \ -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] kfifo: remove unnecessary type check 2012-10-26 1:46 [PATCH] kfifo: remove unnecessary type check Yuanhan Liu @ 2012-10-26 5:38 ` Stefani Seibold 2012-10-26 6:11 ` Yuanhan Liu 0 siblings, 1 reply; 12+ messages in thread From: Stefani Seibold @ 2012-10-26 5:38 UTC (permalink / raw) To: Yuanhan Liu Cc: linux-kernel, Yuanhan Liu, Andrew Morton, Wei Yang, Fengguang Wu, Stephen Rothwell Am Freitag, den 26.10.2012, 09:46 +0800 schrieb Yuanhan Liu: > From: Yuanhan Liu <yuanhan.liu@linux.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@linux-foundation.org> > Cc: Wei Yang <weiyang@linux.vnet.ibm.com> > Cc: Stefani Seibold <stefani@seibold.net> > Cc: Fengguang Wu <fengguang.wu@intel.com> > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > --- > include/linux/kfifo.h | 20 -------------------- > 1 file changed, 20 deletions(-) > > diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h > index 10308c6..b8c1d03 100644 > --- a/include/linux/kfifo.h > +++ b/include/linux/kfifo.h > @@ -390,10 +390,6 @@ __kfifo_int_must_check_helper( \ > unsigned int __ret; \ > const size_t __recsize = sizeof(*__tmp->rectype); \ > struct __kfifo *__kfifo = &__tmp->kfifo; \ > - if (0) { \ > - typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \ > - __dummy = (typeof(__val))NULL; \ > - } \ > if (__recsize) \ > __ret = __kfifo_in_r(__kfifo, __val, sizeof(*__val), \ > __recsize); \ > @@ -432,8 +428,6 @@ __kfifo_uint_must_check_helper( \ > unsigned int __ret; \ > const size_t __recsize = sizeof(*__tmp->rectype); \ > struct __kfifo *__kfifo = &__tmp->kfifo; \ > - if (0) \ > - __val = (typeof(__tmp->ptr))0; \ > if (__recsize) \ > __ret = __kfifo_out_r(__kfifo, __val, sizeof(*__val), \ > __recsize); \ > @@ -473,8 +467,6 @@ __kfifo_uint_must_check_helper( \ > unsigned int __ret; \ > const size_t __recsize = sizeof(*__tmp->rectype); \ > struct __kfifo *__kfifo = &__tmp->kfifo; \ > - if (0) \ > - __val = (typeof(__tmp->ptr))NULL; \ > if (__recsize) \ > __ret = __kfifo_out_peek_r(__kfifo, __val, sizeof(*__val), \ > __recsize); \ > @@ -512,10 +504,6 @@ __kfifo_uint_must_check_helper( \ > unsigned long __n = (n); \ > const size_t __recsize = sizeof(*__tmp->rectype); \ > struct __kfifo *__kfifo = &__tmp->kfifo; \ > - if (0) { \ > - typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \ > - __dummy = (typeof(__buf))NULL; \ > - } \ > (__recsize) ?\ > __kfifo_in_r(__kfifo, __buf, __n, __recsize) : \ > __kfifo_in(__kfifo, __buf, __n); \ > @@ -565,10 +553,6 @@ __kfifo_uint_must_check_helper( \ > unsigned long __n = (n); \ > const size_t __recsize = sizeof(*__tmp->rectype); \ > struct __kfifo *__kfifo = &__tmp->kfifo; \ > - if (0) { \ > - typeof(__tmp->ptr) __dummy = NULL; \ > - __buf = __dummy; \ > - } \ > (__recsize) ?\ > __kfifo_out_r(__kfifo, __buf, __n, __recsize) : \ > __kfifo_out(__kfifo, __buf, __n); \ > @@ -777,10 +761,6 @@ __kfifo_uint_must_check_helper( \ > unsigned long __n = (n); \ > const size_t __recsize = sizeof(*__tmp->rectype); \ > struct __kfifo *__kfifo = &__tmp->kfifo; \ > - if (0) { \ > - typeof(__tmp->ptr) __dummy __attribute__ ((unused)) = NULL; \ > - __buf = __dummy; \ > - } \ > (__recsize) ? \ > __kfifo_out_peek_r(__kfifo, __buf, __n, __recsize) : \ > __kfifo_out_peek(__kfifo, __buf, __n); \ Did you tried to compile the whole kernel including all the drivers with your patch? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kfifo: remove unnecessary type check 2012-10-26 5:38 ` Stefani Seibold @ 2012-10-26 6:11 ` Yuanhan Liu 2012-10-26 6:51 ` Stefani Seibold [not found] ` <20121026095244.GA815@richard.(null)> 0 siblings, 2 replies; 12+ messages in thread From: Yuanhan Liu @ 2012-10-26 6:11 UTC (permalink / raw) To: Stefani Seibold Cc: Yuanhan Liu, linux-kernel, Andrew Morton, Wei Yang, Fengguang Wu, Stephen Rothwell 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@linux.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@linux-foundation.org> > > Cc: Wei Yang <weiyang@linux.vnet.ibm.com> > > Cc: Stefani Seibold <stefani@seibold.net> > > Cc: Fengguang Wu <fengguang.wu@intel.com> > > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > --- > > include/linux/kfifo.h | 20 -------------------- > > 1 file changed, 20 deletions(-) > > > > diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h > > index 10308c6..b8c1d03 100644 > > --- a/include/linux/kfifo.h > > +++ b/include/linux/kfifo.h > > @@ -390,10 +390,6 @@ __kfifo_int_must_check_helper( \ > > unsigned int __ret; \ > > const size_t __recsize = sizeof(*__tmp->rectype); \ > > struct __kfifo *__kfifo = &__tmp->kfifo; \ > > - if (0) { \ > > - typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \ > > - __dummy = (typeof(__val))NULL; \ > > - } \ > > if (__recsize) \ > > __ret = __kfifo_in_r(__kfifo, __val, sizeof(*__val), \ > > __recsize); \ > > @@ -432,8 +428,6 @@ __kfifo_uint_must_check_helper( \ > > unsigned int __ret; \ > > const size_t __recsize = sizeof(*__tmp->rectype); \ > > struct __kfifo *__kfifo = &__tmp->kfifo; \ > > - if (0) \ > > - __val = (typeof(__tmp->ptr))0; \ > > if (__recsize) \ > > __ret = __kfifo_out_r(__kfifo, __val, sizeof(*__val), \ > > __recsize); \ > > @@ -473,8 +467,6 @@ __kfifo_uint_must_check_helper( \ > > unsigned int __ret; \ > > const size_t __recsize = sizeof(*__tmp->rectype); \ > > struct __kfifo *__kfifo = &__tmp->kfifo; \ > > - if (0) \ > > - __val = (typeof(__tmp->ptr))NULL; \ > > if (__recsize) \ > > __ret = __kfifo_out_peek_r(__kfifo, __val, sizeof(*__val), \ > > __recsize); \ > > @@ -512,10 +504,6 @@ __kfifo_uint_must_check_helper( \ > > unsigned long __n = (n); \ > > const size_t __recsize = sizeof(*__tmp->rectype); \ > > struct __kfifo *__kfifo = &__tmp->kfifo; \ > > - if (0) { \ > > - typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \ > > - __dummy = (typeof(__buf))NULL; \ > > - } \ > > (__recsize) ?\ > > __kfifo_in_r(__kfifo, __buf, __n, __recsize) : \ > > __kfifo_in(__kfifo, __buf, __n); \ > > @@ -565,10 +553,6 @@ __kfifo_uint_must_check_helper( \ > > unsigned long __n = (n); \ > > const size_t __recsize = sizeof(*__tmp->rectype); \ > > struct __kfifo *__kfifo = &__tmp->kfifo; \ > > - if (0) { \ > > - typeof(__tmp->ptr) __dummy = NULL; \ > > - __buf = __dummy; \ > > - } \ > > (__recsize) ?\ > > __kfifo_out_r(__kfifo, __buf, __n, __recsize) : \ > > __kfifo_out(__kfifo, __buf, __n); \ > > @@ -777,10 +761,6 @@ __kfifo_uint_must_check_helper( \ > > unsigned long __n = (n); \ > > const size_t __recsize = sizeof(*__tmp->rectype); \ > > struct __kfifo *__kfifo = &__tmp->kfifo; \ > > - if (0) { \ > > - typeof(__tmp->ptr) __dummy __attribute__ ((unused)) = NULL; \ > > - __buf = __dummy; \ > > - } \ > > (__recsize) ? \ > > __kfifo_out_peek_r(__kfifo, __buf, __n, __recsize) : \ > > __kfifo_out_peek(__kfifo, __buf, __n); \ > > Did you tried to compile the whole kernel including all the drivers with > your patch? Hi Stefani, I did a build test, it did't introduce any new compile errors and warnings. While, I haven't tried make allmodconfig then. Does this patch seems wrong to you? Thanks, Yuanhan Liu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kfifo: remove unnecessary type check 2012-10-26 6:11 ` Yuanhan Liu @ 2012-10-26 6:51 ` Stefani Seibold 2012-10-26 7:17 ` Yuanhan Liu [not found] ` <20121026095244.GA815@richard.(null)> 1 sibling, 1 reply; 12+ messages in thread From: Stefani Seibold @ 2012-10-26 6:51 UTC (permalink / raw) To: Yuanhan Liu Cc: Yuanhan Liu, linux-kernel, Andrew Morton, Wei Yang, Fengguang Wu, Stephen Rothwell 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@linux.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@linux-foundation.org> > > > Cc: Wei Yang <weiyang@linux.vnet.ibm.com> > > > Cc: Stefani Seibold <stefani@seibold.net> > > > Cc: Fengguang Wu <fengguang.wu@intel.com> > > > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > > --- > > > > Did you tried to compile the whole kernel including all the drivers with > > your patch? > > Hi Stefani, > > I did a build test, it did't introduce any new compile errors and > warnings. While, I haven't tried make allmodconfig then. Does this patch > seems wrong to you? > > Thanks, > Yuanhan Liu Hi Liu, no the patch seems not wrong to me. But as you see with the previous patch it is not easy to predict the side effects. An allmodconfig together with C=2 is necessary to check if there is no side effects which current users of the kfifo API. That is exactly what i did again and again as i developed the kfifo API. 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. Greetings, Stefani ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kfifo: remove unnecessary type check 2012-10-26 6:51 ` Stefani Seibold @ 2012-10-26 7:17 ` Yuanhan Liu 2012-10-26 7:33 ` Yuanhan Liu 2012-10-26 9:26 ` Stefani Seibold 0 siblings, 2 replies; 12+ messages in thread From: Yuanhan Liu @ 2012-10-26 7:17 UTC (permalink / raw) To: Stefani Seibold Cc: Yuanhan Liu, linux-kernel, Andrew Morton, Wei Yang, Fengguang Wu, Stephen Rothwell 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@linux.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@linux-foundation.org> > > > > Cc: Wei Yang <weiyang@linux.vnet.ibm.com> > > > > Cc: Stefani Seibold <stefani@seibold.net> > > > > Cc: Fengguang Wu <fengguang.wu@intel.com> > > > > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > > > --- > > > > > > > Did you tried to compile the whole kernel including all the drivers with > > > your patch? > > > > Hi Stefani, > > > > I did a build test, it did't introduce any new compile errors and > > warnings. While, I haven't tried make allmodconfig then. Does this patch > > seems wrong to you? > > > > Thanks, > > Yuanhan Liu > > Hi Liu, > > no the patch seems not wrong to me. But as you see with the previous > patch it is not easy to predict the side effects. > > An allmodconfig together with C=2 is necessary to check if there is no > side effects which current users of the kfifo API. Hi Stefani, Make with C=2 will produce tons of warnings, hard to tell it introduces new warnings or not. I build some drivers used kfifo and samples as you suggested with C=2, find no new warnings. I will build all drivers that used kfifo with C=2 later, and will post the result here. > That is exactly what > i did again and again as i developed the kfifo API. > > 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. Thanks, Yuanhan Liu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kfifo: remove unnecessary type check 2012-10-26 7:17 ` Yuanhan Liu @ 2012-10-26 7:33 ` Yuanhan Liu 2012-10-26 9:26 ` Stefani Seibold 1 sibling, 0 replies; 12+ messages in thread From: Yuanhan Liu @ 2012-10-26 7:33 UTC (permalink / raw) To: Stefani Seibold Cc: Yuanhan Liu, linux-kernel, Andrew Morton, Wei Yang, Fengguang Wu, Stephen Rothwell On Fri, Oct 26, 2012 at 03:17:57PM +0800, Yuanhan Liu wrote: > 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@linux.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@linux-foundation.org> > > > > > Cc: Wei Yang <weiyang@linux.vnet.ibm.com> > > > > > Cc: Stefani Seibold <stefani@seibold.net> > > > > > Cc: Fengguang Wu <fengguang.wu@intel.com> > > > > > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > > > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > > > > --- > > > > > > > > > > Did you tried to compile the whole kernel including all the drivers with > > > > your patch? > > > > > > Hi Stefani, > > > > > > I did a build test, it did't introduce any new compile errors and > > > warnings. While, I haven't tried make allmodconfig then. Does this patch > > > seems wrong to you? > > > > > > Thanks, > > > Yuanhan Liu > > > > Hi Liu, > > > > no the patch seems not wrong to me. But as you see with the previous > > patch it is not easy to predict the side effects. > > > > An allmodconfig together with C=2 is necessary to check if there is no > > side effects which current users of the kfifo API. > > Hi Stefani, > > Make with C=2 will produce tons of warnings, hard to tell it introduces > new warnings or not. I build some drivers used kfifo and samples as you > suggested with C=2, find no new warnings. I will build all drivers that > used kfifo with C=2 later, and will post the result here. Hi Stefani, Done build all drivers used kfifo and kfifo samples with C=2, none new warnigs and erros found :D Thanks, Yuanhan Liu > > > That is exactly what > > i did again and again as i developed the kfifo API. > > > > 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. > > Thanks, > Yuanhan Liu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kfifo: remove unnecessary type check 2012-10-26 7:17 ` Yuanhan Liu 2012-10-26 7:33 ` Yuanhan Liu @ 2012-10-26 9:26 ` Stefani Seibold 2012-10-26 13:04 ` Yuanhan Liu 1 sibling, 1 reply; 12+ messages in thread From: Stefani Seibold @ 2012-10-26 9:26 UTC (permalink / raw) To: Yuanhan Liu Cc: Yuanhan Liu, linux-kernel, Andrew Morton, Wei Yang, Fengguang Wu, Stephen Rothwell 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@linux.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@linux-foundation.org> > > > > > Cc: Wei Yang <weiyang@linux.vnet.ibm.com> > > > > > Cc: Stefani Seibold <stefani@seibold.net> > > > > > Cc: Fengguang Wu <fengguang.wu@intel.com> > > > > > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > > > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > > > > > --- > > > > > > > > > > Did you tried to compile the whole kernel including all the drivers with > > > > your patch? > > > > > > Hi Stefani, > > > > > > I did a build test, it did't introduce any new compile errors and > > > warnings. While, I haven't tried make allmodconfig then. Does this patch > > > seems wrong to you? > > > > > > Thanks, > > > Yuanhan Liu > > > > Hi Liu, > > > > no the patch seems not wrong to me. But as you see with the previous > > patch it is not easy to predict the side effects. > > > > An allmodconfig together with C=2 is necessary to check if there is no > > side effects which current users of the kfifo API. > > Hi Stefani, > > Make with C=2 will produce tons of warnings, hard to tell it introduces > new warnings or not. I build some drivers used kfifo and samples as you > suggested with C=2, find no new warnings. I will build all drivers that > used kfifo with C=2 later, and will post the result here. > That will be great... > > > > 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. Greetings, Stefani ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kfifo: remove unnecessary type check 2012-10-26 9:26 ` Stefani Seibold @ 2012-10-26 13:04 ` Yuanhan Liu 2012-10-26 13:42 ` Stefani Seibold 0 siblings, 1 reply; 12+ messages in thread From: Yuanhan Liu @ 2012-10-26 13:04 UTC (permalink / raw) To: Stefani Seibold Cc: Yuanhan Liu, linux-kernel, Andrew Morton, Wei Yang, Fengguang Wu, Stephen Rothwell 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@linux.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@linux-foundation.org> > > > > > > Cc: Wei Yang <weiyang@linux.vnet.ibm.com> > > > > > > Cc: Stefani Seibold <stefani@seibold.net> > > > > > > Cc: Fengguang Wu <fengguang.wu@intel.com> > > > > > > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > > > > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.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? Thanks, Yuanhan Liu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kfifo: remove unnecessary type check 2012-10-26 13:04 ` Yuanhan Liu @ 2012-10-26 13:42 ` Stefani Seibold 2012-10-27 8:43 ` Yuanhan Liu 0 siblings, 1 reply; 12+ messages in thread From: Stefani Seibold @ 2012-10-26 13:42 UTC (permalink / raw) To: Yuanhan Liu Cc: Yuanhan Liu, linux-kernel, Andrew Morton, Wei Yang, Fengguang Wu, Stephen Rothwell 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@linux.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@linux-foundation.org> > > > > > > > Cc: Wei Yang <weiyang@linux.vnet.ibm.com> > > > > > > > Cc: Stefani Seibold <stefani@seibold.net> > > > > > > > Cc: Fengguang Wu <fengguang.wu@intel.com> > > > > > > > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > > > > > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.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@seibold.net Greetings, Stefani ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kfifo: remove unnecessary type check 2012-10-26 13:42 ` Stefani Seibold @ 2012-10-27 8:43 ` Yuanhan Liu 0 siblings, 0 replies; 12+ messages in thread From: Yuanhan Liu @ 2012-10-27 8:43 UTC (permalink / raw) To: Stefani Seibold Cc: Yuanhan Liu, linux-kernel, Andrew Morton, Wei Yang, Fengguang Wu, Stephen Rothwell 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@linux.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@linux-foundation.org> > > > > > > > > Cc: Wei Yang <weiyang@linux.vnet.ibm.com> > > > > > > > > Cc: Stefani Seibold <stefani@seibold.net> > > > > > > > > Cc: Fengguang Wu <fengguang.wu@intel.com> > > > > > > > > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > > > > > > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.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@seibold.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 ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <20121026095244.GA815@richard.(null)>]
* Re: [PATCH] kfifo: remove unnecessary type check [not found] ` <20121026095244.GA815@richard.(null)> @ 2012-10-26 12:31 ` Yuanhan Liu [not found] ` <20121027015558.GA3983@richard.(null)> 0 siblings, 1 reply; 12+ messages in thread From: Yuanhan Liu @ 2012-10-26 12:31 UTC (permalink / raw) To: Richard Yang Cc: Stefani Seibold, Yuanhan Liu, linux-kernel, Andrew Morton, Fengguang Wu, Stephen Rothwell On Fri, Oct 26, 2012 at 05:52:44PM +0800, Richard Yang wrote: > On Fri, Oct 26, 2012 at 02:11:45PM +0800, Yuanhan Liu wrote: > >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@linux.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@linux-foundation.org> > >> > Cc: Wei Yang <weiyang@linux.vnet.ibm.com> > >> > Cc: Stefani Seibold <stefani@seibold.net> > >> > Cc: Fengguang Wu <fengguang.wu@intel.com> > >> > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > >> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > >> > --- > >> > include/linux/kfifo.h | 20 -------------------- > >> > 1 file changed, 20 deletions(-) > >> > > >> > diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h > >> > index 10308c6..b8c1d03 100644 > >> > --- a/include/linux/kfifo.h > >> > +++ b/include/linux/kfifo.h > >> > @@ -390,10 +390,6 @@ __kfifo_int_must_check_helper( \ > >> > unsigned int __ret; \ > >> > const size_t __recsize = sizeof(*__tmp->rectype); \ > >> > struct __kfifo *__kfifo = &__tmp->kfifo; \ > >> > - if (0) { \ > >> > - typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \ > >> > - __dummy = (typeof(__val))NULL; \ > >> > - } \ > >> > if (__recsize) \ > >> > __ret = __kfifo_in_r(__kfifo, __val, sizeof(*__val), \ > >> > __recsize); \ > >> > @@ -432,8 +428,6 @@ __kfifo_uint_must_check_helper( \ > >> > unsigned int __ret; \ > >> > const size_t __recsize = sizeof(*__tmp->rectype); \ > >> > struct __kfifo *__kfifo = &__tmp->kfifo; \ > >> > - if (0) \ > >> > - __val = (typeof(__tmp->ptr))0; \ > >> > if (__recsize) \ > >> > __ret = __kfifo_out_r(__kfifo, __val, sizeof(*__val), \ > >> > __recsize); \ > >> > @@ -473,8 +467,6 @@ __kfifo_uint_must_check_helper( \ > >> > unsigned int __ret; \ > >> > const size_t __recsize = sizeof(*__tmp->rectype); \ > >> > struct __kfifo *__kfifo = &__tmp->kfifo; \ > >> > - if (0) \ > >> > - __val = (typeof(__tmp->ptr))NULL; \ > >> > if (__recsize) \ > >> > __ret = __kfifo_out_peek_r(__kfifo, __val, sizeof(*__val), \ > >> > __recsize); \ > >> > @@ -512,10 +504,6 @@ __kfifo_uint_must_check_helper( \ > >> > unsigned long __n = (n); \ > >> > const size_t __recsize = sizeof(*__tmp->rectype); \ > >> > struct __kfifo *__kfifo = &__tmp->kfifo; \ > >> > - if (0) { \ > >> > - typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \ > >> > - __dummy = (typeof(__buf))NULL; \ > >> > - } \ > >> > (__recsize) ?\ > >> > __kfifo_in_r(__kfifo, __buf, __n, __recsize) : \ > >> > __kfifo_in(__kfifo, __buf, __n); \ > >> > @@ -565,10 +553,6 @@ __kfifo_uint_must_check_helper( \ > >> > unsigned long __n = (n); \ > >> > const size_t __recsize = sizeof(*__tmp->rectype); \ > >> > struct __kfifo *__kfifo = &__tmp->kfifo; \ > >> > - if (0) { \ > >> > - typeof(__tmp->ptr) __dummy = NULL; \ > >> > - __buf = __dummy; \ > >> > - } \ > >> > (__recsize) ?\ > >> > __kfifo_out_r(__kfifo, __buf, __n, __recsize) : \ > >> > __kfifo_out(__kfifo, __buf, __n); \ > >> > @@ -777,10 +761,6 @@ __kfifo_uint_must_check_helper( \ > >> > unsigned long __n = (n); \ > >> > const size_t __recsize = sizeof(*__tmp->rectype); \ > >> > struct __kfifo *__kfifo = &__tmp->kfifo; \ > >> > - if (0) { \ > >> > - typeof(__tmp->ptr) __dummy __attribute__ ((unused)) = NULL; \ > >> > - __buf = __dummy; \ > >> > - } \ > >> > (__recsize) ? \ > >> > __kfifo_out_peek_r(__kfifo, __buf, __n, __recsize) : \ > >> > __kfifo_out_peek(__kfifo, __buf, __n); \ > >> > >> Did you tried to compile the whole kernel including all the drivers with > >> your patch? > > > >Hi Stefani, > > > >I did a build test, it did't introduce any new compile errors and > >warnings. While, I haven't tried make allmodconfig then. Does this patch > >seems wrong to you? > > Hmm, in my mind, those warnings are produced by the code you removed. > So it is reasonable that you see no new warnings. Hi Yang, Nope. It's not hard to tell they are old warnings. And as you can see from the email I posted, I did a build compile with and without applying this patch, and does make C=2 2>/tmp/out.{before,after}. I then compared the two file, they are exactly same. So, no new warnings introduced. Thanks, Yuanhan Liu ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <20121027015558.GA3983@richard.(null)>]
* Re: [PATCH] kfifo: remove unnecessary type check [not found] ` <20121027015558.GA3983@richard.(null)> @ 2012-10-27 8:48 ` Yuanhan Liu 0 siblings, 0 replies; 12+ messages in thread From: Yuanhan Liu @ 2012-10-27 8:48 UTC (permalink / raw) To: Richard Yang Cc: Stefani Seibold, Yuanhan Liu, linux-kernel, Andrew Morton, Fengguang Wu, Stephen Rothwell On Sat, Oct 27, 2012 at 09:55:58AM +0800, Richard Yang wrote: > On Fri, Oct 26, 2012 at 08:31:38PM +0800, Yuanhan Liu wrote: > >On Fri, Oct 26, 2012 at 05:52:44PM +0800, Richard Yang wrote: > >> On Fri, Oct 26, 2012 at 02:11:45PM +0800, Yuanhan Liu wrote: > >> >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@linux.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@linux-foundation.org> > >> >> > Cc: Wei Yang <weiyang@linux.vnet.ibm.com> > >> >> > Cc: Stefani Seibold <stefani@seibold.net> > >> >> > Cc: Fengguang Wu <fengguang.wu@intel.com> > >> >> > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > >> >> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com> > >> >> > --- > >> >> > include/linux/kfifo.h | 20 -------------------- > >> >> > 1 file changed, 20 deletions(-) > >> >> > > >> >> > diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h > >> >> > index 10308c6..b8c1d03 100644 > >> >> > --- a/include/linux/kfifo.h > >> >> > +++ b/include/linux/kfifo.h > >> >> > @@ -390,10 +390,6 @@ __kfifo_int_must_check_helper( \ > >> >> > unsigned int __ret; \ > >> >> > const size_t __recsize = sizeof(*__tmp->rectype); \ > >> >> > struct __kfifo *__kfifo = &__tmp->kfifo; \ > >> >> > - if (0) { \ > >> >> > - typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \ > >> >> > - __dummy = (typeof(__val))NULL; \ > >> >> > - } \ > >> >> > if (__recsize) \ > >> >> > __ret = __kfifo_in_r(__kfifo, __val, sizeof(*__val), \ > >> >> > __recsize); \ > >> >> > @@ -432,8 +428,6 @@ __kfifo_uint_must_check_helper( \ > >> >> > unsigned int __ret; \ > >> >> > const size_t __recsize = sizeof(*__tmp->rectype); \ > >> >> > struct __kfifo *__kfifo = &__tmp->kfifo; \ > >> >> > - if (0) \ > >> >> > - __val = (typeof(__tmp->ptr))0; \ > >> >> > if (__recsize) \ > >> >> > __ret = __kfifo_out_r(__kfifo, __val, sizeof(*__val), \ > >> >> > __recsize); \ > >> >> > @@ -473,8 +467,6 @@ __kfifo_uint_must_check_helper( \ > >> >> > unsigned int __ret; \ > >> >> > const size_t __recsize = sizeof(*__tmp->rectype); \ > >> >> > struct __kfifo *__kfifo = &__tmp->kfifo; \ > >> >> > - if (0) \ > >> >> > - __val = (typeof(__tmp->ptr))NULL; \ > >> >> > if (__recsize) \ > >> >> > __ret = __kfifo_out_peek_r(__kfifo, __val, sizeof(*__val), \ > >> >> > __recsize); \ > >> >> > @@ -512,10 +504,6 @@ __kfifo_uint_must_check_helper( \ > >> >> > unsigned long __n = (n); \ > >> >> > const size_t __recsize = sizeof(*__tmp->rectype); \ > >> >> > struct __kfifo *__kfifo = &__tmp->kfifo; \ > >> >> > - if (0) { \ > >> >> > - typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \ > >> >> > - __dummy = (typeof(__buf))NULL; \ > >> >> > - } \ > >> >> > (__recsize) ?\ > >> >> > __kfifo_in_r(__kfifo, __buf, __n, __recsize) : \ > >> >> > __kfifo_in(__kfifo, __buf, __n); \ > >> >> > @@ -565,10 +553,6 @@ __kfifo_uint_must_check_helper( \ > >> >> > unsigned long __n = (n); \ > >> >> > const size_t __recsize = sizeof(*__tmp->rectype); \ > >> >> > struct __kfifo *__kfifo = &__tmp->kfifo; \ > >> >> > - if (0) { \ > >> >> > - typeof(__tmp->ptr) __dummy = NULL; \ > >> >> > - __buf = __dummy; \ > >> >> > - } \ > >> >> > (__recsize) ?\ > >> >> > __kfifo_out_r(__kfifo, __buf, __n, __recsize) : \ > >> >> > __kfifo_out(__kfifo, __buf, __n); \ > >> >> > @@ -777,10 +761,6 @@ __kfifo_uint_must_check_helper( \ > >> >> > unsigned long __n = (n); \ > >> >> > const size_t __recsize = sizeof(*__tmp->rectype); \ > >> >> > struct __kfifo *__kfifo = &__tmp->kfifo; \ > >> >> > - if (0) { \ > >> >> > - typeof(__tmp->ptr) __dummy __attribute__ ((unused)) = NULL; \ > >> >> > - __buf = __dummy; \ > >> >> > - } \ > >> >> > (__recsize) ? \ > >> >> > __kfifo_out_peek_r(__kfifo, __buf, __n, __recsize) : \ > >> >> > __kfifo_out_peek(__kfifo, __buf, __n); \ > >> >> > >> >> Did you tried to compile the whole kernel including all the drivers with > >> >> your patch? > >> > > >> >Hi Stefani, > >> > > >> >I did a build test, it did't introduce any new compile errors and > >> >warnings. While, I haven't tried make allmodconfig then. Does this patch > >> >seems wrong to you? > >> > >> Hmm, in my mind, those warnings are produced by the code you removed. > >> So it is reasonable that you see no new warnings. > > > >Hi Yang, > > > >Nope. It's not hard to tell they are old warnings. And as you can see from > >the email I posted, I did a build compile with and without applying this > >patch, and does make C=2 2>/tmp/out.{before,after}. I then compared the > >two file, they are exactly same. So, no new warnings introduced. > > > > I don't understand this mail https://lkml.org/lkml/2012/10/25/584. > > So Andrew means just remove the type check? Hi, What do you mean by 'just'? Thanks, Yuanhan Liu ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-10-27 8:48 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-26 1:46 [PATCH] kfifo: remove unnecessary type check Yuanhan Liu
2012-10-26 5:38 ` Stefani Seibold
2012-10-26 6:11 ` Yuanhan Liu
2012-10-26 6:51 ` Stefani Seibold
2012-10-26 7:17 ` Yuanhan Liu
2012-10-26 7:33 ` Yuanhan Liu
2012-10-26 9:26 ` Stefani Seibold
2012-10-26 13:04 ` Yuanhan Liu
2012-10-26 13:42 ` Stefani Seibold
2012-10-27 8:43 ` Yuanhan Liu
[not found] ` <20121026095244.GA815@richard.(null)>
2012-10-26 12:31 ` Yuanhan Liu
[not found] ` <20121027015558.GA3983@richard.(null)>
2012-10-27 8:48 ` Yuanhan Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox