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@xxxxxxxxx>
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/