Date: Wed, 7 Apr 2010 07:34:17 +0200 From: Andre Albsmeier <Andre.Albsmeier@siemens.com> To: Pyun YongHyeon <pyunyh@gmail.com> Cc: "svn-src-stable@freebsd.org" <svn-src-stable@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "Albsmeier, Andre" <andre.albsmeier@siemens.com>, Pyun YongHyeon <yongari@freebsd.org> Subject: Re: svn commit: r205614 - stable/7/sys/dev/msk Message-ID: <20100407053417.GB2899@curry.mchp.siemens.de> In-Reply-To: <20100406202949.GB1087@michelle.cdnetworks.com> References: <201003241721.o2OHL5K9063538@svn.freebsd.org> <20100405145937.GA78871@curry.mchp.siemens.de> <20100405180642.GD1225@michelle.cdnetworks.com> <20100406134626.GA1727@curry.mchp.siemens.de> <20100406180027.GA3724@curry.mchp.siemens.de> <20100406184456.GA1087@michelle.cdnetworks.com> <20100406195936.GA48023@curry.mchp.siemens.de> <20100406202949.GB1087@michelle.cdnetworks.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 06-Apr-2010 at 22:29:49 +0200, Pyun YongHyeon wrote:
> 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)?
Yes. Do you want me to test again with MSI enabled?
-Andre
--
"Regression testing? What's that? If it compiles,
it is good, if it boots up, it is perfect."
- Linus Torvalds
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100407053417.GB2899>