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
_______________________________________________
cvs-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/cvs-all
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
