From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755527AbbESLZU (ORCPT <rfc822;w@1wt.eu>);
	Tue, 19 May 2015 07:25:20 -0400
Received: from hofr.at ([212.69.189.236]:60322 "EHLO mail.hofr.at"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1754506AbbESLZQ (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Tue, 19 May 2015 07:25:16 -0400
Date: Tue, 19 May 2015 13:25:13 +0200
From: Nicholas Mc Guire <der.herr@hofr.at>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Nicholas Mc Guire <hofrat@osadl.org>, Ingo Molnar <mingo@redhat.com>,
        linux-kernel@vger.kernel.org, Oleg Nesterov <oleg@redhat.com>
Subject: Re: [PATCH] locking: type cleanup when accessing fast_read_ctr
Message-ID: <20150519112513.GA1337@opentech.at>
References: <1431971282-21412-1-git-send-email-hofrat@osadl.org> <20150519111105.GB3644@twins.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20150519111105.GB3644@twins.programming.kicks-ass.net>
User-Agent: Mutt/1.5.18 (2008-05-17)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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