Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752289AbbEZOec (ORCPT ); Tue, 26 May 2015 10:34:32 -0400 Received: from foss.arm.com ([217.140.101.70]:50135 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751815AbbEZOea (ORCPT ); Tue, 26 May 2015 10:34:30 -0400 Date: Tue, 26 May 2015 15:36:16 +0100 From: Morten Rasmussen To: Nicholas Mc Guire Cc: Ingo Molnar , Peter Zijlstra , Oleg Nesterov , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH RFC] sched: remove implicit unsigned long - int - unsigned long conversion Message-ID: <20150526143616.GH26396@e105550-lin.cambridge.arm.com> References: <1432404085-24640-1-git-send-email-hofrat@osadl.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1432404085-24640-1-git-send-email-hofrat@osadl.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3051 Lines: 82 On Sat, May 23, 2015 at 07:01:25PM +0100, Nicholas Mc Guire wrote: > Type-checking coccinelle spatches are being used to locate type mismatches > between function signatures and return values in this case this produced: > kernel/sched/fair.c:4987 WARNING: return of wrong type > int != unsigned long > > get_cpu_usage() has one user update_sg_lb_stats() in which it is called: > sgs->group_usage += get_cpu_usage(i); > as group_usage (from struct sg_lb_stats) is unsigned long this > effectively is a unsigned long -> int -> unsigned long automatic type > conversion which has no effect as the return of get_cpu_usage() never > can exceed SCHED_LOAD_SCALE which is < INT_MAX. (on both 64 and 32bit > systems). > > proposal: make get_cpu_usage return unsigned long to make this type clean Agreed. I came across this inconsistency recently when messing around with this code and came to the same conclusion: https://lkml.org/lkml/2015/5/12/754 > > patch was compile tested for x86_64_defconfig > > patch is against 4.1-rc4 (localversion-next is -next-20150522) > > Signed-off-by: Nicholas Mc Guire > --- > > A question that popped up during code review: > "usage" in get_cpu_usage() is in the range of [0..SCHED_LOAD_SCALE] which > is 2**10 (could be up to 2**20 (currently disabled see sched.h:55) but > from code reading it seems that group_usage would eventually overflow as it > is only being incremented but never decremented or reset ? > > load_balance() > -> find_busiest_group() > -> update_sd_lb_stats() > -> update_sg_lb_stats() > ... > sgs->group_usage += get_cpu_usage(i); > > which returns (unsigned long) utilization_load_avg or cpu_capacity_orig > and seems to always be positive, so group_usage would eventually overflow. > On 64bit systems this overflow would probably simply never happen but on > 32bit ? > > what am I missing here ? There is a: memset(sgs, 0, sizeof(*sgs)); just before the loop with the sgs->group_usage increments which should reset it every time update_sg_lb_stats() is called. > > kernel/sched/fair.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index 1dbeea9..7c169a8 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -4978,7 +4978,7 @@ done: > * Without capping the usage, a group could be seen as overloaded (CPU0 usage > * at 121% + CPU1 usage at 80%) whereas CPU1 has 20% of available capacity > */ > -static int get_cpu_usage(int cpu) > +static unsigned long get_cpu_usage(int cpu) > { > unsigned long usage = cpu_rq(cpu)->cfs.utilization_load_avg; > unsigned long capacity = capacity_orig_of(cpu); > -- > 1.7.10.4 FWIW: Acked-by: Morten Rasmussen -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/