From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756573AbbDVK6w (ORCPT ); Wed, 22 Apr 2015 06:58:52 -0400 Received: from cantor2.suse.de ([195.135.220.15]:59085 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751929AbbDVK6u (ORCPT ); Wed, 22 Apr 2015 06:58:50 -0400 Date: Wed, 22 Apr 2015 12:58:45 +0200 From: Jan Kara To: Tejun Heo Cc: Jens Axboe , Jan Kara , linux-kernel@vger.kernel.org, Fengguang Wu Subject: Re: [PATCH block/for-4.1-fixes] writeback: use |1 instead of +1 to protect against div by zero Message-ID: <20150422105845.GA11059@quack.suse.cz> References: <20150421204913.GE9455@htj.duckdns.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150421204913.GE9455@htj.duckdns.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 On Tue 21-04-15 16:49:13, Tejun Heo wrote: > mm/page-writeback.c has several places where 1 is added to the divisor > to prevent division by zero exceptions; however, if the original > divisor is equivalent to -1, adding 1 leads to division by zero. > > There are three places where +1 is used for this purpose - one in > pos_ratio_polynom() and two in bdi_position_ratio(). The second one > in bdi_position_ratio() actually triggered div-by-zero oops on a > machine running a 3.10 kernel. The divisor is > > x_intercept - bdi_setpoint + 1 == span + 1 > > span is confirmed to be (u32)-1. It isn't clear how it ended up that > but it could be from write bandwidth calculation underflow fixed by > c72efb658f7c ("writeback: fix possible underflow in write bandwidth > calculation"). > > At any rate, +1 isn't a proper protection against div-by-zero. This > patch converts all +1 protections to |1. Note that > bdi_update_dirty_ratelimit() was already using |1 before this patch. So IMHO the only proper protection against division by zero is if (unlikely(divisor == 0)) return some_value; else do division But we took a shortcut with +1 or |1 in those calculations since that should be OK given the *expected* numbers we deal with. Once numbers get out of expected range, both shortcuts have issues, just different ones (sure you avoid division by zero with |1 but the absurd values you'll be getting will have interesting effect on the system anyway). So I'm OK with changing +1 to |1 to avoid oops and make code consistent, I just wanted to comment on the fact that the change is for avoiding oops, not about getting better values. Otherwise feel free to add: Reviewed-by: Jan Kara Honza > diff --git a/mm/page-writeback.c b/mm/page-writeback.c > index 5daf556..eb59f7e 100644 > --- a/mm/page-writeback.c > +++ b/mm/page-writeback.c > @@ -580,7 +580,7 @@ static long long pos_ratio_polynom(unsigned long setpoint, > long x; > > x = div64_s64(((s64)setpoint - (s64)dirty) << RATELIMIT_CALC_SHIFT, > - limit - setpoint + 1); > + (limit - setpoint) | 1); > pos_ratio = x; > pos_ratio = pos_ratio * x >> RATELIMIT_CALC_SHIFT; > pos_ratio = pos_ratio * x >> RATELIMIT_CALC_SHIFT; > @@ -807,7 +807,7 @@ static unsigned long bdi_position_ratio(struct backing_dev_info *bdi, > * scale global setpoint to bdi's: > * bdi_setpoint = setpoint * bdi_thresh / thresh > */ > - x = div_u64((u64)bdi_thresh << 16, thresh + 1); > + x = div_u64((u64)bdi_thresh << 16, thresh | 1); > bdi_setpoint = setpoint * (u64)x >> 16; > /* > * Use span=(8*write_bw) in single bdi case as indicated by > @@ -822,7 +822,7 @@ static unsigned long bdi_position_ratio(struct backing_dev_info *bdi, > > if (bdi_dirty < x_intercept - span / 4) { > pos_ratio = div64_u64(pos_ratio * (x_intercept - bdi_dirty), > - x_intercept - bdi_setpoint + 1); > + (x_intercept - bdi_setpoint) | 1); > } else > pos_ratio /= 4; > -- Jan Kara SUSE Labs, CR