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

[Oleg Bulyzhin]

On Tue, Dec 12, 2006 at 07:31:24PM -0500, Jung-uk Kim wrote:
> On Tuesday 12 December 2006 06:44 pm, Oleg Bulyzhin wrote:
> > On Tue, Dec 12, 2006 at 06:09:17PM -0500, Jung-uk Kim wrote:
> > > On Tuesday 12 December 2006 05:05 pm, Oleg Bulyzhin wrote:
> > > > On Fri, Dec 01, 2006 at 01:08:52AM +0000, Jung-uk Kim wrote:
> > > > > jkim        2006-12-01 01:08:52 UTC
> > > > >
> > > > >   FreeBSD src repository
> > > > >
> > > > >   Modified files:
> > > > >     sys/dev/bge          if_bge.c if_bgereg.h
> > > > >   Log:
> > > > >   Simplify statistics updates, remove redundant register
> > > > > reads, and add discarded RX packets to input error for
> > > > > BCM5705 or newer chipset as the others. Unfortunately we
> > > > > cannot do the same for output errors because ifOutDiscards
> > > > > equivalent register does not exist.  While I am here, replace
> > > > > misleading and wrong BGE_RX_STATS/BGE_TX_STATS with
> > > > > BGE_MAC_STATS.  They were reversed but worked accidently.
> > > > >
> > > > >   Revision  Changes    Path
> > > > >   1.153     +15 -23    src/sys/dev/bge/if_bge.c
> > > > >   1.58      +4 -5      src/sys/dev/bge/if_bgereg.h
> > > >
> > > > I would say you have simplified it too much. With your change
> > > > you will get wrong numbers after ifconfig down/up (since it
> > > > implies hardware counters reset while sc->bge_* counters are
> > > > not cleared).
> > >
> > > I did clear sc->bge_* counter:
> > >
> > > @@ -3368,6 +3357,9 @@ bge_init_locked(struct bge_softc *sc)
> > >
> > >  	/* Init our RX return ring index. */
> > >  	sc->bge_rx_saved_considx = 0;
> > > +
> > > +	/* Init our RX/TX stat counters. */
> > > +	sc->bge_rx_discards = sc->bge_tx_discards =
> > > sc->bge_tx_collisions = 0;
> > >
> > >  	/* Init TX ring. */
> > >  	bge_init_tx_ring(sc);
> > >
> > > While ifconfig down/up, bge_init_locked() should be called.
> > > Did I miss something?
> >
> > Oh, i didnt noticed that, but this makes your change even worse:
> > you will loose statistic counters on every interface reset.
> 
> I don't understand why you have to keep the old counters. 
In order to keep statistics across chip resets.( It's required for correct
ipkt/ierror ratio. Imagine situation: i have bge0 with 1M input packets
and 1M input errors, so packet loss i 50%. Then I do ifconfig bge0 down;
iconfig bge0 up and input errors are gone!)

>As you 
> said, when the controller resets, stat registers are reset as well.  
true.
> Therefore, the old offsets must be reset as well.
>Otherwise, you get bogus stats.
This is not true, old code (prior to rev.1.153) was able to deal with it,
until you 'simplified' it.

> 
> Jung-uk Kim

-- 
Oleg.

================================================================
=== Oleg Bulyzhin -- OBUL-RIPN -- OBUL-RIPE -- oleg at rinet.ru ===
================================================================