svn commit: r205614 - stable/7/sys/dev/msk

[Pyun YongHyeon]

On Tue, Apr 06, 2010 at 09:59:36PM +0200, Andre Albsmeier wrote:
> On Tue, 06-Apr-2010 at 20:44:56 +0200, Pyun YongHyeon wrote:
> > On Tue, Apr 06, 2010 at 08:00:27PM +0200, Andre Albsmeier wrote:
> > > On Tue, 06-Apr-2010 at 15:46:26 +0200, Andre Albsmeier wrote:
> > > > On Mon, 05-Apr-2010 at 20:06:42 +0200, Pyun YongHyeon wrote:
> > 
> > [...]
> > 
> > > > > As you know 1.18.2.38 removed taskqueue based interrupt handling so
> > > > > it could be culprit of the issue. But that revision also removed
> > > > > two register accesses in TX path so I'd like to know which one
> > > > > caused the issue. 
> > > > 
> > > > I have now tried rev. 1.18.2.38 with this patch (no idea if
> > > > this is right ;-)):
> > > > 
> > > > --- if_msk.c.1.18.2.38	2010-04-06 15:09:19.000000000 +0200
> > > > +++ if_msk.c.1.18.2.38.TRY	2010-04-06 15:38:13.000000000 +0200
> > > > @@ -3327,6 +3327,11 @@
> > > >  	uint32_t control, status;
> > > >  	int cons, len, port, rxprog;
> > > >  
> > > > +	int idx;
> > > > +	idx = CSR_READ_2(sc, STAT_PUT_IDX);
> > > > +	if (idx == sc->msk_stat_cons)
> > > > +		return (0);
> > > > +
> > > >  	/* Sync status LEs. */
> > > >  	bus_dmamap_sync(sc->msk_stat_tag, sc->msk_stat_map,
> > > >  	    BUS_DMASYNC_POSTREAD | BUS_DMASYNC_POSTWRITE);
> > > > @@ -3407,7 +3412,7 @@
> > > >  	if (rxput[MSK_PORT_B] > 0)
> > > >  		msk_rxput(sc->msk_if[MSK_PORT_B]);
> > > >  
> > > > -	return (rxprog > sc->msk_process_limit ? EAGAIN : 0);
> > > > +	return (sc->msk_stat_cons != CSR_READ_2(sc, STAT_PUT_IDX));
> > > >  }
> > > >  
> > > >  static void
> > > > 
> > > > Now performance seems to be the same as with the older
> > > > driver (at least here at work) and in both directions!
> > > > Some numbers:
> > > > 
> > > > em0 writes to rev. 1.18.2.36:				20 seconds
> > > > em0 writes to rev. 1.18.2.38:				50 seconds
> > > > em0 writes to rev. 1.18.2.38 with patch from above:	23 seconds
> > > > same as before but with int_holdoff: 100 -> 1:		20 seconds
> > > > 
> > > > rev. 1.18.2.36 writes to em0:				22 seconds
> > > > rev. 1.18.2.38 writes to em0:				40 seconds
> > > > rev. 1.18.2.38 with patch from above writes to em0:	21 seconds
> > > > same as before but with int_holdoff: 100 -> 1:		20 seconds
> > > > 
> > > > It seems that these two CSR_READ_2s really help ;-).
> > > > 
> > > > As I said, this is at work and with slightly different machines.
> > > > I will try things at home later but I am rather confident of
> > > > receiving good results there as well...
> > > 
> > > OK, tests at home show similar good results with the
> > > above patch. When setting int_holdoff to 3, performance
> > > seems equal to the older versions.
> > > 
> > 
> > Thanks a lot for narrowing down the issue.
> > Because the msk_handle_events() are called in interrupt handler
> > I'd like to remove the two register accesses in fast path. It seems
> > accessing STAT_PUT_IDX triggers status updates. Would you try
> > attached patch and let me know whether the patch makes any
> > difference?
> 
> > Index: sys/dev/msk/if_msk.c
> > ===================================================================
> > --- sys/dev/msk/if_msk.c	(revision 206204)
> > +++ sys/dev/msk/if_msk.c	(working copy)
> > @@ -1470,6 +1470,7 @@
> >  		/* WA for dev. #4.18 */
> >  		CSR_WRITE_1(sc, STAT_FIFO_WM, 0x21);
> >  		CSR_WRITE_1(sc, STAT_FIFO_ISR_WM, 0x07);
> > +		CSR_WRITE_4(sc, STAT_TX_TIMER_INI, MSK_USECS(sc, 10));
> >  	} else {
> >  		CSR_WRITE_2(sc, STAT_TX_IDX_TH, 0x0a);
> >  		CSR_WRITE_1(sc, STAT_FIFO_WM, 0x10);
> > @@ -1481,9 +1482,16 @@
> >  		CSR_WRITE_4(sc, STAT_ISR_TIMER_INI, 0x0190);
> >  	}
> >  	/*
> > -	 * Use default value for STAT_ISR_TIMER_INI, STAT_LEV_TIMER_INI.
> > +	 * STAT_TX_TIMER_INI, STAT_ISR_TIMER_INI and STAT_LEV_TIMER_INI
> > +	 * as well as various water mark registers seem to control when
> > +	 * controller initiates status LE update.
> > +	 * It's not clear how these registers interact with interrupt
> > +	 * state but STAT_TX_TIMER_INI seems to control status update
> > +	 * time after crossing a threshold(STAT_TX_IDX_TH) value. Due to
> > +	 * the complexity of various parameters that may affect status
> > +	 * update just use hardware default until we know better
> > +	 * the internal things of status updates.
> >  	 */
> > -	CSR_WRITE_4(sc, STAT_TX_TIMER_INI, MSK_USECS(sc, 1000));
> >  
> >  	/* Enable status unit. */
> >  	CSR_WRITE_4(sc, STAT_CTRL, SC_STAT_OP_ON);
> 
> After applying this patch against an original 1.18.2.39
> things became even worse. After 4 minutes of waiting I
> aborted untar'ing the file ;-). Setting int_holdoff=2
> made things better, now it took 65 seconds for untar'ing...
> 

:-(

By chance, did you disable MSI(msi_disable=1)?