Re: NetBSD 5.1 TCP performance issue (lots of ACK)

[David Young <dyoung%pobox.com@localhost>]

On Fri, Oct 28, 2011 at 09:21:08PM +0200, Manuel Bouyer wrote:
> On Fri, Oct 28, 2011 at 10:27:56AM -0500, David Young wrote:
> > > Here is an updated patch. The key point to avoid the receive errors is
> > > to do another BUS_DMASYNC after reading wrx_status, before reading the
> > > other values to avoid reading e.g. len before status gets updated.
> > > The errors were because of 0-len receive descriptors.
> > 
> > Good catch!  Question, though:
> > 
> > > Index: sys/dev/pci/if_wm.c
> > > ===================================================================
> > > RCS file: /cvsroot/src/sys/dev/pci/if_wm.c,v
> > > retrieving revision 1.162.4.15
> > > diff -u -p -u -r1.162.4.15 if_wm.c
> > > --- sys/dev/pci/if_wm.c   7 Mar 2011 04:14:19 -0000       1.162.4.15
> > > +++ sys/dev/pci/if_wm.c   28 Oct 2011 14:03:33 -0000
> > > @@ -2879,11 +2907,7 @@ wm_rxintr(struct wm_softc *sc)
> > >               device_xname(sc->sc_dev), i));
> > >  
> > >           WM_CDRXSYNC(sc, i, BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);
> > > -
> > >           status = sc->sc_rxdescs[i].wrx_status;
> > > -         errors = sc->sc_rxdescs[i].wrx_errors;
> > > -         len = le16toh(sc->sc_rxdescs[i].wrx_len);
> > > -         vlantag = sc->sc_rxdescs[i].wrx_special;
> > >  
> > >           if ((status & WRX_ST_DD) == 0) {
> > >                   /*
> > > @@ -2892,6 +2916,14 @@ wm_rxintr(struct wm_softc *sc)
> > >                   WM_CDRXSYNC(sc, i, BUS_DMASYNC_PREREAD);
> > >                   break;
> > >           }
> > 
> > Should
> > 
> > >                   WM_CDRXSYNC(sc, i, BUS_DMASYNC_PREREAD);
> > 
> > move above 
> > 
> > >           if ((status & WRX_ST_DD) == 0) {
> > 
> > ?
> 
> I don't think so: if WRX_ST_DD is not set, we won't read anything more frm
> this descriptor so there's no need to sync it again.

Currently, if WRX_ST_DD is not set, we sync the descriptor and
get out of the loop:

                        WM_CDRXSYNC(sc, i, BUS_DMASYNC_PREREAD);
                        break;

If WRX_ST_DD is set, however, we do read more from the descriptor.  That
is why I ask whether we should sync it again.

It is strange and possibly unnecessary to have two sync calls
back-to-back, for it would be:

                WM_CDRXSYNC(sc, i, BUS_DMASYNC_PREREAD);
                if ((status & WRX_ST_DD) == 0) {
                        break;
                }

                /*                                                              
                 * sync again, to make sure the values below have been read     
                 * after status.                                                
                 */                                                             
                WM_CDRXSYNC(sc, i, BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE);

Dave

-- 
David Young             OJC Technologies is now Pixo
dyoung%pixotech.com@localhost     Urbana, IL   (217) 344-0444 x24