From owner-cvs-src@FreeBSD.ORG Wed Dec 13 00:51:42 2006 Return-Path: X-Original-To: cvs-src@FreeBSD.org Delivered-To: cvs-src@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 8F3CF16A492; Wed, 13 Dec 2006 00:51:42 +0000 (UTC) (envelope-from jkim@FreeBSD.org) Received: from anuket.mj.niksun.com (gwnew.niksun.com [65.115.46.162]) by mx1.FreeBSD.org (Postfix) with ESMTP id A5A7943D64; Wed, 13 Dec 2006 00:48:29 +0000 (GMT) (envelope-from jkim@FreeBSD.org) Received: from niksun.com (anuket [10.70.0.5]) by anuket.mj.niksun.com (8.13.6/8.13.6) with ESMTP id kBD0njmP090260; Tue, 12 Dec 2006 19:49:45 -0500 (EST) (envelope-from jkim@FreeBSD.org) From: Jung-uk Kim To: Oleg Bulyzhin Date: Tue, 12 Dec 2006 19:49:41 -0500 User-Agent: KMail/1.6.2 References: <200612111800.kBBI0Zv0047909@repoman.freebsd.org> <200612121824.45643.jkim@FreeBSD.org> <20061213000352.GH91560@lath.rinet.ru> In-Reply-To: <20061213000352.GH91560@lath.rinet.ru> MIME-Version: 1.0 Content-Disposition: inline Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <200612121949.43019.jkim@FreeBSD.org> X-Virus-Scanned: ClamAV 0.88.6/2319/Tue Dec 12 15:09:22 2006 on anuket.mj.niksun.com X-Virus-Status: Clean Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/bge if_bge.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Dec 2006 00:51:42 -0000 On Tuesday 12 December 2006 07:03 pm, 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 > > > > read/clear. - Correct RX packet drop counter for BCM5705+. > > > > This register is read/clear and it wraps very quickly under > > > > heavy packet drops because only the lower ten bits are valid > > > > according to the documentation. However, it seems few more > > > > bits are actually valid and the rest bits are always > > > > zeros[1]. Therefore, we don't mask them off here. To get > > > > accurate packet drop count, we need to check the register > > > > from bge_rxeof(). It is commented out for now, not to > > > > penalize normal operation. Actual performance impact should > > > > be measured later. > > > > - 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]. > > > > > > > > Tested by: bde[1] > > > > Suggested by: bde[2] > > > > > > > > Revision Changes Path > > > > 1.158 +13 -10 src/sys/dev/bge/if_bge.c > > > > > > 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: 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. > > > > Depending on the chip types, we use different methods to get the > > stats. However, both methods read only 32-bit register/memory. > > Therefore, there's no reason to use u_long at all and integer > > math is cheap. > > While hardware counters are 32bit long, interface & driver's > counters are 64bit long on 64bit chips. So you can keep numbers > even if hardware counter wraps around. I guess you are talking about collision counter for BCM570[0-4], right? Since we are adding four different 32-bit counters to make one counter, we have no way of knowing which one is wrapped unless we check upper-half's. To make things worse, if multiple counters wrap, the previous logic lose. If you want real solution for 64-bit machines, you have to read full 64-bit atomically. > uint32_t & u_long are the same on i386 and u_long arithmetic is not > slower than int one for 64 bit chips. Anyway we need few arithmetic > operations once per second, so it's unimportant. > > > > P.S. Your current change is unclear to me too: since ifp > > > counters and sc->_bge_ are both u_long i can not see any reason > > > of converting (u_long) cast to (uint32_t) one. > > > > To cut the long story short, it does not really matter as I said > > in the log but it is required if sizeof(uint32_t) < sizeof(int). > > Could you please explain this. As I noted in the log, it was suggested by bde. I didn't ask his permission but I guess it's okay to cut and paste his e-mail. ;-) (Sorry, Bruce.) ----------------- Of course it makes no difference on currently supported machines -- see previous mail :-). The following uses fake ints to demonstrate the problem. %%% #include #include /* * Fake ints with more than 32 bits. Unsigned integer types strictly * smaller than ints get promoted to signed ints for binary operations. * Here it is not the signedness change but the loss of accidental * wrapping at 32 bits that matters. */ typedef int64_t int_t; /* Fake unsigned longs no smaller than ints. */ typedef uintmax_t ulong_t; /* 32-bit hardware counters. */ uint32_t before_wrap = 0xffffffff; uint32_t after_wrap = 0; ulong_t if_ierrors = 0; int main(void) { /* Should be only 1 more error, but... */ if_ierrors += (int_t)after_wrap - (int_t)before_wrap; printf("%ju\n", (uintmax_t)if_ierrors); } %%% The output is a magic number slightly less recognizable than 2^32-1: 18446744069414584321 This looks a bit like 2^64-1, but is actually 2^64-2^32+1 (1-2^32 after overflow). ----------------- Jung-uk Kim