From owner-svn-src-head@freebsd.org Fri Jan 8 06:05:07 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 3DDD1A67C78; Fri, 8 Jan 2016 06:05:07 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 0709A1BBF; Fri, 8 Jan 2016 06:05:06 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id 5725B782395; Fri, 8 Jan 2016 17:04:57 +1100 (AEDT) Date: Fri, 8 Jan 2016 17:04:55 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Jim Harris cc: Ravi Pokala , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r293328 - head/sys/dev/nvme In-Reply-To: Message-ID: <20160108160312.Y1626@besplex.bde.org> References: <201601071618.u07GIXdd054147@repo.freebsd.org> <6496C054-6FED-4B41-8EF8-8067E8B6C866@panasas.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=cK4dyQqN c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=JzwRw_2MAAAA:8 a=kj9zAlcOel0A:10 a=n-kJSqksAAAA:8 a=Ra8JCSpROcCdkStrUYgA:9 a=V9_opou4x4dvQzEj:21 a=LaXgxn80HHdOZiGp:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 Jan 2016 06:05:07 -0000 On Thu, 7 Jan 2016, Jim Harris wrote: > On Thu, Jan 7, 2016 at 9:27 AM, Ravi Pokala wrote: > ... >>> + * Used for calculating number of CPUs to assign to each core and number >> of I/O >>> + * queues to allocate per controller. >>> + */ >>> +#define NVME_CEILING(num, div) ((((num) - 1) / (div)) + 1) >>> + >>> >>> ... >> >> I'm surprised that this isn't in , along with >> roundup()/rounddown()/etc. Finding the ceiling like this is probably pretty >> common, so shouldn't it be added to the common header so everyone can use >> it? > > Good catch. howmany() does exactly this, just expressed differently. I'll > switch over to that. Thanks! howmany() doesn't do exactly this. It has diferent bugs / range of applicability, I see about 10. Subtracting 1 instead of adding more than 1 gives overflow in different cases. Even the simple and not unreasonable case num = 0 gives overflow in NVME_CEILING() if div happens to be unsigned and not 1: NVME_CEILING(0, 2U) = UINTMAX / 2 + 1 = 0x80000000 with 32-bit ints. Getting to 10 different bugs requires considering unreasonable cases with negative div or num, and nonsense cases with div = 0, and reasonable but unsupported cases with floating point args. C's broken division for negative integer values makes the details large. NVME_CEILING() is closer to supporting negative values than howmany() -- the magic 1 that it adds is to adjust for division of non-negative values rounding down. For division of negative values, the adjustment should be by -1 or 0, depending on the C bug and on whether you want to round negative results towards 0 or minus infinity. Howmany() is undocumented so its bugs are harder to see than in your own macro. Another one is that it is an unsafe macro with the name of a safe function-like API. This naming convention is more important for undocumented APIs. NVME_CEILING() follows it in reverse. Subtracting 1 instead of adding (div) - 1 is less robust but avoids multiple evaluation of div. Bruce