From owner-svn-src-stable@FreeBSD.ORG Wed Apr 7 17:54:49 2010 Return-Path: Delivered-To: svn-src-stable@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 77400106564A; Wed, 7 Apr 2010 17:54:49 +0000 (UTC) (envelope-from Andre.Albsmeier@siemens.com) Received: from goliath.siemens.de (goliath.siemens.de [192.35.17.28]) by mx1.freebsd.org (Postfix) with ESMTP id E967F8FC0A; Wed, 7 Apr 2010 17:54:47 +0000 (UTC) Received: from mail3.siemens.de (localhost [127.0.0.1]) by goliath.siemens.de (8.12.11.20060308/8.12.11) with ESMTP id o37Hsko2018285; Wed, 7 Apr 2010 19:54:46 +0200 Received: from curry.mchp.siemens.de (curry.mchp.siemens.de [139.25.40.130]) by mail3.siemens.de (8.12.11.20060308/8.12.11) with ESMTP id o37HskkX021817; Wed, 7 Apr 2010 19:54:46 +0200 Received: (from localhost) by curry.mchp.siemens.de (8.14.4/8.14.4) id o37Hsk0a003747; Date: Wed, 7 Apr 2010 19:54:46 +0200 From: Andre Albsmeier To: Pyun YongHyeon Message-ID: <20100407175446.GA28015@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> <20100407170404.GA5734@michelle.cdnetworks.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100407170404.GA5734@michelle.cdnetworks.com> X-Echelon: X-Advice: Drop that crappy M$-Outlook, I'm tired of your viruses! User-Agent: Mutt/1.5.20 (2009-06-14) Cc: "svn-src-stable@freebsd.org" , "svn-src-all@freebsd.org" , "Albsmeier, Andre" , Pyun YongHyeon Subject: Re: svn commit: r205614 - stable/7/sys/dev/msk X-BeenThere: svn-src-stable@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for all the -stable branches of the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Apr 2010 17:54:49 -0000 On Wed, 07-Apr-2010 at 19:04:04 +0200, Pyun YongHyeon wrote: > 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! No problem. If you want me to do more tests just drop me a note... Thanks for your work on this! -Andre