Date: Wed, 7 Apr 2010 10:04:04 -0700 From: Pyun YongHyeon <pyunyh@gmail.com> To: Andre Albsmeier <Andre.Albsmeier@siemens.com> Cc: "svn-src-stable@freebsd.org" <svn-src-stable@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, Pyun YongHyeon <yongari@freebsd.org> Subject: Re: svn commit: r205614 - stable/7/sys/dev/msk Message-ID: <20100407170404.GA5734@michelle.cdnetworks.com> In-Reply-To: <20100407053417.GB2899@curry.mchp.siemens.de> 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> <20100407053417.GB2899@curry.mchp.siemens.de>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Apr 07, 2010 at 07:34:17AM +0200, Andre Albsmeier wrote:
> 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?
>
Hmm, after reading code again MSI does not seem to trigger the
issue. I'll revert that part of code.
Thank you very much for testing and reporting!
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100407170404.GA5734>