cvs commit: src/sys/dev/bge if_bge.c

[Jung-uk Kim]

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 <stdint.h>
#include <stdio.h>

/*
 * 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