[PATCH] kfifo: remove unnecessary type check

[PATCH] kfifo: remove unnecessary type check

[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); \
-- 
1.7.11.7

Re: [PATCH] kfifo: remove unnecessary type check

[Stefani Seibold]

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?

Re: [PATCH] kfifo: remove unnecessary type check

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

Re: [PATCH] kfifo: remove unnecessary type check

[Stefani Seibold]

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

Re: [PATCH] kfifo: remove unnecessary type check

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

Re: [PATCH] kfifo: remove unnecessary type check

[Yuanhan Liu]

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

Re: [PATCH] kfifo: remove unnecessary type check

[Stefani Seibold]

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

Re: [PATCH] kfifo: remove unnecessary type check

[Yuanhan Liu]

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

Re: [PATCH] kfifo: remove unnecessary type check

[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?

Thanks,
Yuanhan Liu

Re: [PATCH] kfifo: remove unnecessary type check

[Stefani Seibold]

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

Re: [PATCH] kfifo: remove unnecessary type check

[Yuanhan Liu]

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

Re: [PATCH] kfifo: remove unnecessary type check

[Yuanhan Liu]

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