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

[Oleg Bulyzhin]

On Tue, Dec 12, 2006 at 08:07:46PM -0500, Jung-uk Kim wrote:
> 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

I have to apologize for waisting your time. I was inattentive while reading
code and i should confess i'm wrong and you are right.

Thanks for your explanation!

P.S. I should have go to bed earlier...

-- 
Oleg.

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