[PATCH RFC] rcu: change return type to bool

[PATCH RFC] rcu: change return type to bool

[Nicholas Mc Guire]

Type-checking coccinelle spatches are being used to locate type mismatches
between function signatures and return values in this case this produced:
./kernel/rcu/srcu.c:271 WARNING: return of wrong type
        int != unsigned long,

srcu_readers_active() returns an int that is the sum of per_cpu unsigned
long but the only user is cleanup_srcu_struct() which is using it as a
boolean (condition) to see if there is any readers rather than actually
using the approximate number of readers. The theoretically possible
unsigned long overflow case does not need to be handled explicitly - if
we had 4G++ readers then something else went wrong a long time ago.

proposal: change the return type to boolean. The function name is left
          unchanged as it fits the naming expectation for a boolean.

patch was compile teseted for x86_64_defconfig (implies CONFIG_SRCU=y)

patch is against 4.1-rc4 (localversion-next is -next-20150522)

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
 kernel/rcu/srcu.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/rcu/srcu.c b/kernel/rcu/srcu.c
index fb33d35..7c4fbeb 100644
--- a/kernel/rcu/srcu.c
+++ b/kernel/rcu/srcu.c
@@ -252,14 +252,15 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *sp, int idx)
 }
 
 /**
- * srcu_readers_active - returns approximate number of readers.
+ * srcu_readers_active - returns true if there are readers. and false
+ *                       otherwise
  * @sp: which srcu_struct to count active readers (holding srcu_read_lock).
  *
  * Note that this is not an atomic primitive, and can therefore suffer
  * severe errors when invoked on an active srcu_struct.  That said, it
  * can be useful as an error check at cleanup time.
  */
-static int srcu_readers_active(struct srcu_struct *sp)
+static bool srcu_readers_active(struct srcu_struct *sp)
 {
 	int cpu;
 	unsigned long sum = 0;
@@ -268,7 +269,7 @@ static int srcu_readers_active(struct srcu_struct *sp)
 		sum += READ_ONCE(per_cpu_ptr(sp->per_cpu_ref, cpu)->c[0]);
 		sum += READ_ONCE(per_cpu_ptr(sp->per_cpu_ref, cpu)->c[1]);
 	}
-	return sum;
+	return !!sum;
 }
 
 /**
-- 
1.7.10.4

Re: [PATCH RFC] rcu: change return type to bool

[Steven Rostedt]

On Sat, 23 May 2015 16:47:52 +0200
Nicholas Mc Guire <hofrat@osadl.org> wrote:

> Type-checking coccinelle spatches are being used to locate type mismatches
> between function signatures and return values in this case this produced:
> ./kernel/rcu/srcu.c:271 WARNING: return of wrong type
>         int != unsigned long,
> 
> srcu_readers_active() returns an int that is the sum of per_cpu unsigned
> long but the only user is cleanup_srcu_struct() which is using it as a
> boolean (condition) to see if there is any readers rather than actually
> using the approximate number of readers. The theoretically possible
> unsigned long overflow case does not need to be handled explicitly - if
> we had 4G++ readers then something else went wrong a long time ago.
> 
> proposal: change the return type to boolean. The function name is left
>           unchanged as it fits the naming expectation for a boolean.
> 
> patch was compile teseted for x86_64_defconfig (implies CONFIG_SRCU=y)
> 
> patch is against 4.1-rc4 (localversion-next is -next-20150522)
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
>  kernel/rcu/srcu.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/srcu.c b/kernel/rcu/srcu.c
> index fb33d35..7c4fbeb 100644
> --- a/kernel/rcu/srcu.c
> +++ b/kernel/rcu/srcu.c
> @@ -252,14 +252,15 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *sp, int idx)
>  }
>  
>  /**
> - * srcu_readers_active - returns approximate number of readers.
> + * srcu_readers_active - returns true if there are readers. and false
> + *                       otherwise
>   * @sp: which srcu_struct to count active readers (holding srcu_read_lock).
>   *
>   * Note that this is not an atomic primitive, and can therefore suffer
>   * severe errors when invoked on an active srcu_struct.  That said, it
>   * can be useful as an error check at cleanup time.
>   */
> -static int srcu_readers_active(struct srcu_struct *sp)
> +static bool srcu_readers_active(struct srcu_struct *sp)
>  {
>  	int cpu;
>  	unsigned long sum = 0;
> @@ -268,7 +269,7 @@ static int srcu_readers_active(struct srcu_struct *sp)
>  		sum += READ_ONCE(per_cpu_ptr(sp->per_cpu_ref, cpu)->c[0]);
>  		sum += READ_ONCE(per_cpu_ptr(sp->per_cpu_ref, cpu)->c[1]);
>  	}
> -	return sum;
> +	return !!sum;

Hmm I wonder if gcc is smart enough to do the above without the need
for !!? That is, will it turn to !! because the return of the function
is bool, or does gcc complain about it not being bool without the !!?
Not a criticism of the patch, just a curiosity.

-- Steve


>  }
>  
>  /**

Re: [PATCH RFC] rcu: change return type to bool

[Nicholas Mc Guire]

On Sat, 23 May 2015, Steven Rostedt wrote:

> On Sat, 23 May 2015 16:47:52 +0200
> Nicholas Mc Guire <hofrat@osadl.org> wrote:
> 
> > Type-checking coccinelle spatches are being used to locate type mismatches
> > between function signatures and return values in this case this produced:
> > ./kernel/rcu/srcu.c:271 WARNING: return of wrong type
> >         int != unsigned long,
> > 
> > srcu_readers_active() returns an int that is the sum of per_cpu unsigned
> > long but the only user is cleanup_srcu_struct() which is using it as a
> > boolean (condition) to see if there is any readers rather than actually
> > using the approximate number of readers. The theoretically possible
> > unsigned long overflow case does not need to be handled explicitly - if
> > we had 4G++ readers then something else went wrong a long time ago.
> > 
> > proposal: change the return type to boolean. The function name is left
> >           unchanged as it fits the naming expectation for a boolean.
> > 
> > patch was compile teseted for x86_64_defconfig (implies CONFIG_SRCU=y)
> > 
> > patch is against 4.1-rc4 (localversion-next is -next-20150522)
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> >  kernel/rcu/srcu.c |    7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/rcu/srcu.c b/kernel/rcu/srcu.c
> > index fb33d35..7c4fbeb 100644
> > --- a/kernel/rcu/srcu.c
> > +++ b/kernel/rcu/srcu.c
> > @@ -252,14 +252,15 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *sp, int idx)
> >  }
> >  
> >  /**
> > - * srcu_readers_active - returns approximate number of readers.
> > + * srcu_readers_active - returns true if there are readers. and false
> > + *                       otherwise
> >   * @sp: which srcu_struct to count active readers (holding srcu_read_lock).
> >   *
> >   * Note that this is not an atomic primitive, and can therefore suffer
> >   * severe errors when invoked on an active srcu_struct.  That said, it
> >   * can be useful as an error check at cleanup time.
> >   */
> > -static int srcu_readers_active(struct srcu_struct *sp)
> > +static bool srcu_readers_active(struct srcu_struct *sp)
> >  {
> >  	int cpu;
> >  	unsigned long sum = 0;
> > @@ -268,7 +269,7 @@ static int srcu_readers_active(struct srcu_struct *sp)
> >  		sum += READ_ONCE(per_cpu_ptr(sp->per_cpu_ref, cpu)->c[0]);
> >  		sum += READ_ONCE(per_cpu_ptr(sp->per_cpu_ref, cpu)->c[1]);
> >  	}
> > -	return sum;
> > +	return !!sum;
> 
> Hmm I wonder if gcc is smart enough to do the above without the need
> for !!? That is, will it turn to !! because the return of the function
> is bool, or does gcc complain about it not being bool without the !!?
> Not a criticism of the patch, just a curiosity.
>
gcc will not complain if you assign a unsigned long to a boolean
as I understand it it is a macro and is not doing any type 
checking/promotion at all - so anything can be assigned to a bool
without warning (including double and pointers).
The !! will though always make the type compatible with int so it is 
a well defined type atleast as far as __builtin_types_compatible_p()
goes, and !! also makes static code checkers happy (that are maybe not
as smart as gcc) and it does make the intent of sum being treated 
as boolean here clear.

thx!
hofrat 

Re: [PATCH RFC] rcu: change return type to bool

[Joe Perches]

On Sun, 2015-05-24 at 09:27 +0200, Nicholas Mc Guire wrote:
> On Sat, 23 May 2015, Steven Rostedt wrote:
[]
> > > -	return sum;
> > > +	return !!sum;
> > 
> > Hmm I wonder if gcc is smart enough to do the above without the need
> > for !!? That is, will it turn to !! because the return of the function
> > is bool, or does gcc complain about it not being bool without the !!?
> > Not a criticism of the patch, just a curiosity.
> >
> gcc will not complain if you assign a unsigned long to a boolean
> as I understand it it is a macro and is not doing any type 
> checking/promotion at all - so anything can be assigned to a bool
> without warning (including double and pointers).
> The !! will though always make the type compatible with int so it is 
> a well defined type atleast as far as __builtin_types_compatible_p()
> goes, and !! also makes static code checkers happy (that are maybe not
> as smart as gcc) and it does make the intent of sum being treated 
> as boolean here clear.

6.3.1.2 Boolean type

When any scalar value is converted to _Bool, the result is 0 if the
value compares equal to 0; otherwise, the result is 1.

Re: [PATCH RFC] rcu: change return type to bool

[Nicholas Mc Guire]

On Sun, 24 May 2015, Joe Perches wrote:

> On Sun, 2015-05-24 at 09:27 +0200, Nicholas Mc Guire wrote:
> > On Sat, 23 May 2015, Steven Rostedt wrote:
> []
> > > > -	return sum;
> > > > +	return !!sum;
> > > 
> > > Hmm I wonder if gcc is smart enough to do the above without the need
> > > for !!? That is, will it turn to !! because the return of the function
> > > is bool, or does gcc complain about it not being bool without the !!?
> > > Not a criticism of the patch, just a curiosity.
> > >
> > gcc will not complain if you assign a unsigned long to a boolean
> > as I understand it it is a macro and is not doing any type 
> > checking/promotion at all - so anything can be assigned to a bool
> > without warning (including double and pointers).
> > The !! will though always make the type compatible with int so it is 
> > a well defined type atleast as far as __builtin_types_compatible_p()
> > goes, and !! also makes static code checkers happy (that are maybe not
> > as smart as gcc) and it does make the intent of sum being treated 
> > as boolean here clear.
> 
> 6.3.1.2 Boolean type
> 
> When any scalar value is converted to _Bool, the result is 0 if the
> value compares equal to 0; otherwise, the result is 1.
>
As I understand this applies to arithmetic operations so for
bool x = false; int i = 42; x += i; x is defined to be true
but here it is the return type and not an arithmetic operation
so does this apply here without the !!?

the !! is ensuring that it is of type compatible to int as the rank
of _Bool is the lowest and

6.3.1 Arithmetic operands
6.3.1.1 Boolean, characters, and integers
...
2 The following may be used in an expression wherever an int or unsigned 
  int may be used:
  - An object or expression with an integer type whose integer conversion 
    rank is less than the rank of int and unsigned int.
  ...
"If an int can represent all values of the original type, the value is
 converted to an int; otherwise, it is converted to an unsigned int. 
 These are called the integer promotions.48) All other types are 
 unchanged by the integer promotions."


thx!
hofrat

Re: [PATCH RFC] rcu: change return type to bool

[Joe Perches]

On Sun, 2015-05-24 at 10:10 +0200, Nicholas Mc Guire wrote:
> On Sun, 24 May 2015, Joe Perches wrote:
> 
> > On Sun, 2015-05-24 at 09:27 +0200, Nicholas Mc Guire wrote:
> > > On Sat, 23 May 2015, Steven Rostedt wrote:
> > []
> > > > > -	return sum;
> > > > > +	return !!sum;
> > > > 
> > > > Hmm I wonder if gcc is smart enough to do the above without the need
> > > > for !!? That is, will it turn to !! because the return of the function
> > > > is bool, or does gcc complain about it not being bool without the !!?
> > > > Not a criticism of the patch, just a curiosity.
> > > >
> > > gcc will not complain if you assign a unsigned long to a boolean
> > > as I understand it it is a macro and is not doing any type 
> > > checking/promotion at all - so anything can be assigned to a bool
> > > without warning (including double and pointers).
> > > The !! will though always make the type compatible with int so it is 
> > > a well defined type atleast as far as __builtin_types_compatible_p()
> > > goes, and !! also makes static code checkers happy (that are maybe not
> > > as smart as gcc) and it does make the intent of sum being treated 
> > > as boolean here clear.
> > 
> > 6.3.1.2 Boolean type
> > 
> > When any scalar value is converted to _Bool, the result is 0 if the
> > value compares equal to 0; otherwise, the result is 1.
> >
> As I understand this applies to arithmetic operations so for
> bool x = false; int i = 42; x += i; x is defined to be true
> but here it is the return type and not an arithmetic operation
> so does this apply here without the !!?

Yes, it does.  return is an implicit conversion.

6.8.6.4 The return statement

3 If a return statement with an expression is executed, the value of
  the expression is returned to the caller as the value of the function
  call expression. If the expression has a type different from the
  return type of the function in which it appears, the value is
  converted as if by assignment to an object having the return type of
  the function.

Re: [PATCH RFC] rcu: change return type to bool

[Nicholas Mc Guire]

On Sun, 24 May 2015, Joe Perches wrote:

> On Sun, 2015-05-24 at 10:10 +0200, Nicholas Mc Guire wrote:
> > On Sun, 24 May 2015, Joe Perches wrote:
> > 
> > > On Sun, 2015-05-24 at 09:27 +0200, Nicholas Mc Guire wrote:
> > > > On Sat, 23 May 2015, Steven Rostedt wrote:
> > > []
> > > > > > -	return sum;
> > > > > > +	return !!sum;
> > > > > 
> > > > > Hmm I wonder if gcc is smart enough to do the above without the need
> > > > > for !!? That is, will it turn to !! because the return of the function
> > > > > is bool, or does gcc complain about it not being bool without the !!?
> > > > > Not a criticism of the patch, just a curiosity.
> > > > >
> > > > gcc will not complain if you assign a unsigned long to a boolean
> > > > as I understand it it is a macro and is not doing any type 
> > > > checking/promotion at all - so anything can be assigned to a bool
> > > > without warning (including double and pointers).
> > > > The !! will though always make the type compatible with int so it is 
> > > > a well defined type atleast as far as __builtin_types_compatible_p()
> > > > goes, and !! also makes static code checkers happy (that are maybe not
> > > > as smart as gcc) and it does make the intent of sum being treated 
> > > > as boolean here clear.
> > > 
> > > 6.3.1.2 Boolean type
> > > 
> > > When any scalar value is converted to _Bool, the result is 0 if the
> > > value compares equal to 0; otherwise, the result is 1.
> > >
> > As I understand this applies to arithmetic operations so for
> > bool x = false; int i = 42; x += i; x is defined to be true
> > but here it is the return type and not an arithmetic operation
> > so does this apply here without the !!?
> 
> Yes, it does.  return is an implicit conversion.
> 
> 6.8.6.4 The return statement
> 
> 3 If a return statement with an expression is executed, the value of
>   the expression is returned to the caller as the value of the function
>   call expression. If the expression has a type different from the
>   return type of the function in which it appears, the value is
>   converted as if by assignment to an object having the return type of
>   the function.
>
get it - thanks for the clarification !

thx!
hofrat 

Re: [PATCH RFC] rcu: change return type to bool

[Paul E. McKenney]

On Sun, May 24, 2015 at 10:46:50AM +0200, Nicholas Mc Guire wrote:
> On Sun, 24 May 2015, Joe Perches wrote:
> 
> > On Sun, 2015-05-24 at 10:10 +0200, Nicholas Mc Guire wrote:
> > > On Sun, 24 May 2015, Joe Perches wrote:
> > > 
> > > > On Sun, 2015-05-24 at 09:27 +0200, Nicholas Mc Guire wrote:
> > > > > On Sat, 23 May 2015, Steven Rostedt wrote:
> > > > []
> > > > > > > -	return sum;
> > > > > > > +	return !!sum;
> > > > > > 
> > > > > > Hmm I wonder if gcc is smart enough to do the above without the need
> > > > > > for !!? That is, will it turn to !! because the return of the function
> > > > > > is bool, or does gcc complain about it not being bool without the !!?
> > > > > > Not a criticism of the patch, just a curiosity.
> > > > > >
> > > > > gcc will not complain if you assign a unsigned long to a boolean
> > > > > as I understand it it is a macro and is not doing any type 
> > > > > checking/promotion at all - so anything can be assigned to a bool
> > > > > without warning (including double and pointers).
> > > > > The !! will though always make the type compatible with int so it is 
> > > > > a well defined type atleast as far as __builtin_types_compatible_p()
> > > > > goes, and !! also makes static code checkers happy (that are maybe not
> > > > > as smart as gcc) and it does make the intent of sum being treated 
> > > > > as boolean here clear.
> > > > 
> > > > 6.3.1.2 Boolean type
> > > > 
> > > > When any scalar value is converted to _Bool, the result is 0 if the
> > > > value compares equal to 0; otherwise, the result is 1.
> > > >
> > > As I understand this applies to arithmetic operations so for
> > > bool x = false; int i = 42; x += i; x is defined to be true
> > > but here it is the return type and not an arithmetic operation
> > > so does this apply here without the !!?
> > 
> > Yes, it does.  return is an implicit conversion.
> > 
> > 6.8.6.4 The return statement
> > 
> > 3 If a return statement with an expression is executed, the value of
> >   the expression is returned to the caller as the value of the function
> >   call expression. If the expression has a type different from the
> >   return type of the function in which it appears, the value is
> >   converted as if by assignment to an object having the return type of
> >   the function.
> >
> get it - thanks for the clarification !

Hello, Nicholas,

Were you planning to send an updated patch?

							Thanx, Paul