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

[Jung-uk Kim]

On Tuesday 12 December 2006 07:44 pm, Oleg Bulyzhin wrote:
> 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!)

ifp->if_ierrors does not reset between ifconfig down/up.

> >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.

Well, new code works for me on multiple chips/revisions.

Jung-uk Kim