From owner-cvs-all@FreeBSD.ORG Wed Dec 13 02:13:45 2006 Return-Path: X-Original-To: cvs-all@freebsd.org Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id F379D16A417; Wed, 13 Dec 2006 02:13:44 +0000 (UTC) (envelope-from bde@zeta.org.au) Received: from mailout1.pacific.net.au (mailout1-3.pacific.net.au [61.8.2.210]) by mx1.FreeBSD.org (Postfix) with ESMTP id 9C85C43C9F; Wed, 13 Dec 2006 02:12:17 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailproxy1.pacific.net.au (mailproxy1.pacific.net.au [61.8.2.162]) by mailout1.pacific.net.au (Postfix) with ESMTP id 2699F328272; Wed, 13 Dec 2006 13:13:40 +1100 (EST) Received: from katana.zip.com.au (katana.zip.com.au [61.8.7.246]) by mailproxy1.pacific.net.au (Postfix) with ESMTP id 0DABB8C19; Wed, 13 Dec 2006 13:13:37 +1100 (EST) Date: Wed, 13 Dec 2006 13:13:32 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Oleg Bulyzhin In-Reply-To: <20061213000352.GH91560@lath.rinet.ru> Message-ID: <20061213114547.X833@delplex.bde.org> References: <200612111800.kBBI0Zv0047909@repoman.freebsd.org> <20061212222549.GD91560@lath.rinet.ru> <200612121824.45643.jkim@FreeBSD.org> <20061213000352.GH91560@lath.rinet.ru> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org, Jung-uk Kim Subject: Re: cvs commit: src/sys/dev/bge if_bge.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Dec 2006 02:13:45 -0000 On Wed, 13 Dec 2006, Oleg Bulyzhin wrote: > On Tue, Dec 12, 2006 at 06:24:43PM -0500, Jung-uk Kim wrote: >> On Tuesday 12 December 2006 05:25 pm, Oleg Bulyzhin wrote: >>> On Mon, Dec 11, 2006 at 06:00:35PM +0000, Jung-uk Kim wrote: >>>> jkim 2006-12-11 18:00:35 UTC >>>> >>>> FreeBSD src repository >>>> >>>> Modified files: >>>> sys/dev/bge if_bge.c >>>> Log: >>>> - Correct collision counter for BCM5705+. This register is >>>> ... >>>> - Correct integer casting from u_long to uint32_t. Casting is >>>> not really needed for all supported platforms but we better do >>>> this correctly[2]. >>> ... >>> I didnt get the point of your u_long -> uint32_t changes. >>> As i can see your change will cause more often wraps on 64bit >>> archs: Wrapping is the point of the changes. The hardware counters (the part that is actually used) are 32 bits, so they must be subtracted in 32 bits, so that when the counter wraps from 0xffffffff to 0 the result is 1 and not -4294967295. Without the cast, the following would happen on machines with > 32 bit ints: uint32_t val_before = 0xffffffff; uint32_t val_after = 0; expression (val_after - val_before): o Both terms are first promoted to int and the promotion is strict since uint32_t is smaller than int. o The result is -4294967295, provided there are no padding bits in ints (the result needs 32 value bits and 1 sign bit to represent). Also assume that this result is representable as an int. expression if_ierrors += (val_after - val_before): o The RHS is first promoted to the type of the LHS (u_long) since the type of the LHS is not smaller than u_int and the type of the RHS is not smaller than the type of the LHS and not smaller than int. o So the addition is of u_long values. Assume 64-bit u_longs, as actually occur on all supported machines that have u_longs with >= 33 bits. o u_long is an unsigned type, so it cannot really represent the negative int -4294967295. However, it can represernt values quite a bit larger than +4294967295, so there is no benign overflow overflow (wrapping) at 2^32. -44294967295 is converted to (u_long)((ULONG_MAX + 1) + (-4294967295)) where the additions are in infinite precision. This makes it (u_long)(2^64 + 1 - 2^32). The small positive difference of 1 has been mangled to to a huge unsigned value. o Adding the huge unsigned value may or may not overflow. If it doesn't overflow, then the result is bogus (a huger unsigned value). If it overflows then the result is just bogus due to the overflow (overflow of counters isn't benign, and with 64-bit counters can only ooccur due to bugs). o Assigning the result of the addition makes no difference. It either works right or preserves a value that is already bogus due to overflow. However, if if_ierrors were 32 bits or we converted to uint32_t before storing it then we would be back to the relatively benign overflow (wrap) at 32 bits which occurs natually on machines with 32-bit u_longs. >>> In rev. 1.153 you have converted cnt to uint32_t. Since cnt >>> is stored in sc->bge_* counters you have shortened(on 64bit archs) >>> them as well. This is better for similar reasons. The hardware has 64-bit registers, but the software only every read the low 32 bits and doesn't have locking necessary for reading 64-bit registers atomically, and anyway, reading 64 bit registers would be just a pessimization, especially with correct locking, since all the devices that I looked at always store 0 in the top 32 bits (0xffffffff wraps to 0). Before rev.1.153, the bug actually occurred on all supported 64-bit machines: if_ierrors += ((u_long)0 - (u_long)0xffffffff); The RHS is (u_long)(2^64 + 1 - 2^32). Casting as is necessary for the unsupported machines would probably fix this but then 64-bit values in bge counters would be just a small pessimization -- the top 32 bits would only be used transiently in cases where the compiler can't figure out that they will be discarded The problem might be clearer if the hardware only maintained the low 10 bits of the registers, as almost happens for the drop count on 5705's (the hardware actually helps by clearing the register on read, so the negative differences which become hige unsigned ones on overflow don't occur). The wrapping at 32-bit can't help either accidentally or intentionally: - if the hardware leaves garbage in the top 22 bits, then obviously we must clear the garbage. It's easiest to clear the garbage before using it. Otherwise we have to do a complicated analysis like the above to see that the garbage doesn't matter. Clearing it reduces to the next case. - if the hardware sets the top bits to 0 then we can safely subtract values. However, we must handle wrap at 0x400 so that we never get negative differences: /* Usual sign-extension/overflow bugs: */ if_ierrors += ((any_t)0 - (any_t)0x3ff); /* Working version depending on 2's complement magic. */ if_ierrors += (0 - 0x3ff) & 0x3ff; /* Working version depending on 2's complement non-magic. */ if_ierrors += ((u_int)0 - (u_int)0x3ff) & 0x3ff; /* Clearer(?) working version. Depends on h/w clearing top bits */ if_ierrors += ((0x400 + 0) - 0x3ff) & 0x3ff; Bruce