[PATCH] locking: type cleanup when accessing fast_read_ctr

[PATCH] locking: type cleanup when accessing fast_read_ctr

[Nicholas Mc Guire]

static code checking with coccinelle was unhappy with:
./kernel/locking/percpu-rwsem.c:113 WARNING: return of wrong type
	int != unsigned int


current state:

include/linux/percpu-rwsem.h:
struct percpu_rw_semaphore {
  unsigned int __percpu   *fast_read_ctr;
  ...
  atomic_t                slow_read_ctr;

allocated as int:

int __percpu_init_rwsem(struct percpu_rw_semaphore *brw,
                        const char *name, struct lock_class_key *rwsem_key)
{
	brw->fast_read_ctr = alloc_percpu(int); 

used compliant with type int in:

static bool update_fast_ctr(struct percpu_rw_semaphore *brw, 
			    unsigned int val)
usage (2):
  update_fast_ctr(brw, +1)
  update_fast_ctr(brw, -1)
 
so val should be int here not unsigned int 

static int clear_fast_ctr(struct percpu_rw_semaphore *brw)
usage (1):
  atomic_add(clear_fast_ctr(brw), &brw->slow_read_ctr);

slow_read_ctr is atomic_t which is int so it would be consistent here to
have fast_read_ctr being changed to int as well.

This patch changes:
  fast_read_ctr to int
  clear_fast_ctr():sum to int
  update_fast_ctr():val to int
to make usage of fast_read_ctr consistent with respect to type.

  ( Patch was build tested with x86_64_defconfig + 
    CONFIG_UPROBE_EVENT (implies CONFIG_PERCPU_RWSEM=y))

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
 include/linux/percpu-rwsem.h  |    2 +-
 kernel/locking/percpu-rwsem.c |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h
index 3e88c9a..ba37dbe 100644
--- a/include/linux/percpu-rwsem.h
+++ b/include/linux/percpu-rwsem.h
@@ -8,7 +8,7 @@
 #include <linux/lockdep.h>
 
 struct percpu_rw_semaphore {
-	unsigned int __percpu	*fast_read_ctr;
+	int __percpu		*fast_read_ctr;
 	atomic_t		write_ctr;
 	struct rw_semaphore	rw_sem;
 	atomic_t		slow_read_ctr;
diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
index 652a8ee..002837d 100644
--- a/kernel/locking/percpu-rwsem.c
+++ b/kernel/locking/percpu-rwsem.c
@@ -52,7 +52,7 @@ void percpu_free_rwsem(struct percpu_rw_semaphore *brw)
  * reader inside the critical section. See the comments in down_write and
  * up_write below.
  */
-static bool update_fast_ctr(struct percpu_rw_semaphore *brw, unsigned int val)
+static bool update_fast_ctr(struct percpu_rw_semaphore *brw, int val)
 {
 	bool success = false;
 
@@ -102,7 +102,7 @@ void percpu_up_read(struct percpu_rw_semaphore *brw)
 
 static int clear_fast_ctr(struct percpu_rw_semaphore *brw)
 {
-	unsigned int sum = 0;
+	int sum = 0;
 	int cpu;
 
 	for_each_possible_cpu(cpu) {
-- 
1.7.10.4

Re: [PATCH] locking: type cleanup when accessing fast_read_ctr

[Peter Zijlstra]

On Mon, May 18, 2015 at 07:48:02PM +0200, Nicholas Mc Guire wrote:
> static code checking with coccinelle was unhappy with:
> ./kernel/locking/percpu-rwsem.c:113 WARNING: return of wrong type
> 	int != unsigned int
> 
> 
> current state:
> 
> include/linux/percpu-rwsem.h:
> struct percpu_rw_semaphore {
>   unsigned int __percpu   *fast_read_ctr;
>   ...
>   atomic_t                slow_read_ctr;
> 
> allocated as int:
> 
> int __percpu_init_rwsem(struct percpu_rw_semaphore *brw,
>                         const char *name, struct lock_class_key *rwsem_key)
> {
> 	brw->fast_read_ctr = alloc_percpu(int); 
> 
> used compliant with type int in:
> 
> static bool update_fast_ctr(struct percpu_rw_semaphore *brw, 
> 			    unsigned int val)
> usage (2):
>   update_fast_ctr(brw, +1)
>   update_fast_ctr(brw, -1)
>  
> so val should be int here not unsigned int 
> 
> static int clear_fast_ctr(struct percpu_rw_semaphore *brw)
> usage (1):
>   atomic_add(clear_fast_ctr(brw), &brw->slow_read_ctr);
> 
> slow_read_ctr is atomic_t which is int so it would be consistent here to
> have fast_read_ctr being changed to int as well.
> 
> This patch changes:
>   fast_read_ctr to int
>   clear_fast_ctr():sum to int
>   update_fast_ctr():val to int
> to make usage of fast_read_ctr consistent with respect to type.
> 
>   ( Patch was build tested with x86_64_defconfig + 
>     CONFIG_UPROBE_EVENT (implies CONFIG_PERCPU_RWSEM=y))
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>

So the value is unsigned by purpose; that said we should never cross the
2G I think, so it really doesn't matter much.

Cc'ed Oleg who wrote this code, left the rest of the msg for his
convencience.

That said; Oleg, should I post those patches implementing percpu-rwsem
with the stuff we did for the hotplug lock a while back? I think I
actually have those around somewhere.

> ---
>  include/linux/percpu-rwsem.h  |    2 +-
>  kernel/locking/percpu-rwsem.c |    4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h
> index 3e88c9a..ba37dbe 100644
> --- a/include/linux/percpu-rwsem.h
> +++ b/include/linux/percpu-rwsem.h
> @@ -8,7 +8,7 @@
>  #include <linux/lockdep.h>
>  
>  struct percpu_rw_semaphore {
> -	unsigned int __percpu	*fast_read_ctr;
> +	int __percpu		*fast_read_ctr;
>  	atomic_t		write_ctr;
>  	struct rw_semaphore	rw_sem;
>  	atomic_t		slow_read_ctr;
> diff --git a/kernel/locking/percpu-rwsem.c b/kernel/locking/percpu-rwsem.c
> index 652a8ee..002837d 100644
> --- a/kernel/locking/percpu-rwsem.c
> +++ b/kernel/locking/percpu-rwsem.c
> @@ -52,7 +52,7 @@ void percpu_free_rwsem(struct percpu_rw_semaphore *brw)
>   * reader inside the critical section. See the comments in down_write and
>   * up_write below.
>   */
> -static bool update_fast_ctr(struct percpu_rw_semaphore *brw, unsigned int val)
> +static bool update_fast_ctr(struct percpu_rw_semaphore *brw, int val)
>  {
>  	bool success = false;
>  
> @@ -102,7 +102,7 @@ void percpu_up_read(struct percpu_rw_semaphore *brw)
>  
>  static int clear_fast_ctr(struct percpu_rw_semaphore *brw)
>  {
> -	unsigned int sum = 0;
> +	int sum = 0;
>  	int cpu;
>  
>  	for_each_possible_cpu(cpu) {
> -- 
> 1.7.10.4
> 

Re: [PATCH] locking: type cleanup when accessing fast_read_ctr

[Nicholas Mc Guire]

On Tue, 19 May 2015, Peter Zijlstra wrote:

> On Mon, May 18, 2015 at 07:48:02PM +0200, Nicholas Mc Guire wrote:
> > static code checking with coccinelle was unhappy with:
> > ./kernel/locking/percpu-rwsem.c:113 WARNING: return of wrong type
> > 	int != unsigned int
> > 
> > 
> > current state:
> > 
> > include/linux/percpu-rwsem.h:
> > struct percpu_rw_semaphore {
> >   unsigned int __percpu   *fast_read_ctr;
> >   ...
> >   atomic_t                slow_read_ctr;
> > 
> > allocated as int:
> > 
> > int __percpu_init_rwsem(struct percpu_rw_semaphore *brw,
> >                         const char *name, struct lock_class_key *rwsem_key)
> > {
> > 	brw->fast_read_ctr = alloc_percpu(int); 
> > 
> > used compliant with type int in:
> > 
> > static bool update_fast_ctr(struct percpu_rw_semaphore *brw, 
> > 			    unsigned int val)
> > usage (2):
> >   update_fast_ctr(brw, +1)
> >   update_fast_ctr(brw, -1)
> >  
> > so val should be int here not unsigned int 
> > 
> > static int clear_fast_ctr(struct percpu_rw_semaphore *brw)
> > usage (1):
> >   atomic_add(clear_fast_ctr(brw), &brw->slow_read_ctr);
> > 
> > slow_read_ctr is atomic_t which is int so it would be consistent here to
> > have fast_read_ctr being changed to int as well.
> > 
> > This patch changes:
> >   fast_read_ctr to int
> >   clear_fast_ctr():sum to int
> >   update_fast_ctr():val to int
> > to make usage of fast_read_ctr consistent with respect to type.
> > 
> >   ( Patch was build tested with x86_64_defconfig + 
> >     CONFIG_UPROBE_EVENT (implies CONFIG_PERCPU_RWSEM=y))
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> 
> So the value is unsigned by purpose; that said we should never cross the
> 2G I think, so it really doesn't matter much.

I assumed it would not matter but did not see a simple way of getting it
type clean with unsigned either mainly due to the atomic_t being int and
val in update_fast_ctr() being passed as -1.

thanks for the review comments.

thx!
hofrat

Re: [PATCH] locking: type cleanup when accessing fast_read_ctr

[Oleg Nesterov]

On 05/19, Peter Zijlstra wrote:
>
> So the value is unsigned by purpose; that said we should never cross the
> 2G I think, so it really doesn't matter much.

Yes, thanks, this is on purpose. Actually I was asked to make it "unsigned"
during the review, but I agree it looks better than "signed int" because
of overflows.

> That said; Oleg, should I post those patches implementing percpu-rwsem
> with the stuff we did for the hotplug lock a while back? I think I
> actually have those around somewhere.

Yes, this would be nice ;)

Oleg.

Re: [PATCH] locking: type cleanup when accessing fast_read_ctr

[Oleg Nesterov]

On 05/19, Nicholas Mc Guire wrote:
>
> I assumed it would not matter but did not see a simple way of getting it
> type clean with unsigned either mainly due to the atomic_t being int and
> val in update_fast_ctr() being passed as -1.

Perhaps clear_fast_ctr() should have a comment to explain why it returns
"int"... even if "unsigned" should work too.

Please see https://lkml.org/lkml/2012/11/2/346

Oleg.

Re: [PATCH] locking: type cleanup when accessing fast_read_ctr

[Nicholas Mc Guire]

On Wed, 20 May 2015, Oleg Nesterov wrote:

> On 05/19, Nicholas Mc Guire wrote:
> >
> > I assumed it would not matter but did not see a simple way of getting it
> > type clean with unsigned either mainly due to the atomic_t being int and
> > val in update_fast_ctr() being passed as -1.
> 
> Perhaps clear_fast_ctr() should have a comment to explain why it returns
> "int"... even if "unsigned" should work too.
> 
> Please see https://lkml.org/lkml/2012/11/2/346
>
thanks - makes it more clear to me where this came from - will give it
another run and see how this could be resolved type clean. in any case
I think that for such deliberate type missmatch it would be good to
add an explicit cast so that it is clear that it was actually intended.
 
thx!
hofrat
 

Re: [PATCH] locking: type cleanup when accessing fast_read_ctr

[Nicholas Mc Guire]

On Wed, 20 May 2015, Oleg Nesterov wrote:

> On 05/19, Nicholas Mc Guire wrote:
> >
> > I assumed it would not matter but did not see a simple way of getting it
> > type clean with unsigned either mainly due to the atomic_t being int and
> > val in update_fast_ctr() being passed as -1.
> 
> Perhaps clear_fast_ctr() should have a comment to explain why it returns
> "int"... even if "unsigned" should work too.
>
Might not be into c99 standard far enough but from reviewing 6.5/J.2
I do not see this argument here.

The "well defined modulo 2**n" behavior for unsigned int can be
found stated in a few places - but not in the c99 standard for 
arithmetic overflow.

The "well defined overflow behavior" as far as I understand c99, 
*only* applies to left shift operations when overflowing - see 6.5.7 "
Bitwise shift operators" -> Semantics -> 4) -  further for the counter
problem this well defined behavior is of little help as the sum would 
be wrong in both cases.

I still do not see the point in the implicit/automatic type conversion
here and why that should be an advantage - could somone point me to 
the right c99 clauses ?

thx!
hofrat

Re: [PATCH] locking: type cleanup when accessing fast_read_ctr

[Oleg Nesterov]

On 05/23, Nicholas Mc Guire wrote:
>
> On Wed, 20 May 2015, Oleg Nesterov wrote:
>
> > On 05/19, Nicholas Mc Guire wrote:
> > >
> > > I assumed it would not matter but did not see a simple way of getting it
> > > type clean with unsigned either mainly due to the atomic_t being int and
> > > val in update_fast_ctr() being passed as -1.
> >
> > Perhaps clear_fast_ctr() should have a comment to explain why it returns
> > "int"... even if "unsigned" should work too.
> >
> Might not be into c99 standard far enough but from reviewing 6.5/J.2
> I do not see this argument here.
>
> The "well defined modulo 2**n" behavior for unsigned int can be
> found stated in a few places - but not in the c99 standard for
> arithmetic overflow.
>
> The "well defined overflow behavior" as far as I understand c99,
> *only* applies to left shift operations when overflowing - see 6.5.7 "
> Bitwise shift operators" -> Semantics -> 4) -  further for the counter
> problem this well defined behavior is of little help as the sum would
> be wrong in both cases.
>
> I still do not see the point in the implicit/automatic type conversion
> here and why that should be an advantage - could somone point me to
> the right c99 clauses ?

Sorry, I don't really understand your question...

Once again. Signed overflow is undefined behaviour according to C standard.
That is why ->fast_read_ctr is "unsigned long", this counter can actually
over/underflow if down/up happens on different CPU's.

clear_fast_ctr() returns "signed int" just because this looks better to me,
it can actually return the negative number. If you make it return "unsigned"
you will simply shift this implicit/automatic type conversion to atomic_add()
which accepts "int i".

Let me also quote Linus:

	Now, I doubt you'll find an architecture or C compiler where this will
	actually ever make a difference,

Yes. So this all is actually cosmetic.

Oleg.

Re: [PATCH] locking: type cleanup when accessing fast_read_ctr

[Nicholas Mc Guire]

On Sun, 24 May 2015, Oleg Nesterov wrote:

> On 05/23, Nicholas Mc Guire wrote:
> >
> > On Wed, 20 May 2015, Oleg Nesterov wrote:
> >
> > > On 05/19, Nicholas Mc Guire wrote:
> > > >
> > > > I assumed it would not matter but did not see a simple way of getting it
> > > > type clean with unsigned either mainly due to the atomic_t being int and
> > > > val in update_fast_ctr() being passed as -1.
> > >
> > > Perhaps clear_fast_ctr() should have a comment to explain why it returns
> > > "int"... even if "unsigned" should work too.
> > >
> > Might not be into c99 standard far enough but from reviewing 6.5/J.2
> > I do not see this argument here.
> >
> > The "well defined modulo 2**n" behavior for unsigned int can be
> > found stated in a few places - but not in the c99 standard for
> > arithmetic overflow.
> >
> > The "well defined overflow behavior" as far as I understand c99,
> > *only* applies to left shift operations when overflowing - see 6.5.7 "
> > Bitwise shift operators" -> Semantics -> 4) -  further for the counter
> > problem this well defined behavior is of little help as the sum would
> > be wrong in both cases.
> >
> > I still do not see the point in the implicit/automatic type conversion
> > here and why that should be an advantage - could somone point me to
> > the right c99 clauses ?
> 
> Sorry, I don't really understand your question...
>
> Once again. Signed overflow is undefined behaviour according to C standard.
> That is why ->fast_read_ctr is "unsigned long", this counter can actually
> over/underflow if down/up happens on different CPU's.

that signed overflow is undefind is clear - just that unsigned overflow is
well defined is not clear - simply did not find this defined behavior for
unsigned int/long in c99 except for the bitshift operators. So my question
simply was where is the behavior on arithmetic overflow defined ? 

> 
> clear_fast_ctr() returns "signed int" just because this looks better to me,
> it can actually return the negative number. If you make it return "unsigned"
> you will simply shift this implicit/automatic type conversion to atomic_add()
> which accepts "int i".

yup - that is clear - which is one of the reasons why the conversion was 
towards int.

> 
> Let me also quote Linus:
> 
> 	Now, I doubt you'll find an architecture or C compiler where this will
> 	actually ever make a difference,
> 
> Yes. So this all is actually cosmetic.
>
If there is no simple way of getting it out - and there are sound reasons 
to do it this way then I'll just mark it as false positive. 
The intent of the patch was just to cleanup type conversions that did not 
really look necessary and simplify automatic code checking.

thanks for the clarification !

thx!
hofrat