From owner-svn-src-head@freebsd.org Mon Nov 7 06:22:28 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 54EEEC332E5; Mon, 7 Nov 2016 06:22:28 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from mail.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 0E1E227E; Mon, 7 Nov 2016 06:22:27 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by mail.baldwin.cx (Postfix) with ESMTPSA id D4E7310A718; Mon, 7 Nov 2016 01:22:18 -0500 (EST) From: John Baldwin To: Sean Bruno Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r308345 - head/sys/dev/e1000 Date: Sat, 05 Nov 2016 16:16:58 -0700 Message-ID: <15572642.JMvQo5TC3D@ralph.baldwin.cx> User-Agent: KMail/4.14.10 (FreeBSD/11.0-PRERELEASE; KDE/4.14.10; amd64; ; ) In-Reply-To: <201611051630.uA5GUhtk073581@repo.freebsd.org> References: <201611051630.uA5GUhtk073581@repo.freebsd.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.4.3 (mail.baldwin.cx); Mon, 07 Nov 2016 01:22:18 -0500 (EST) X-Virus-Scanned: clamav-milter 0.99.2 at mail.baldwin.cx X-Virus-Status: Clean X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 Nov 2016 06:22:28 -0000 On Saturday, November 05, 2016 04:30:43 PM Sean Bruno wrote: > Author: sbruno > Date: Sat Nov 5 16:30:42 2016 > New Revision: 308345 > URL: https://svnweb.freebsd.org/changeset/base/308345 > > Log: > r295133 attempted to deactivate TSO in the 100Mbit link case with this > adapter to work around bugs in TSO handling at this speed. > > em_init_locked is called during first boot of the adapter and will > see that link_speed is unitialized, effectively turning off tso for > all cards at all speeds, which I believe was *not* the intent. > > Move the handling of TSO deactivation to the link handler where we can > more effectively make the decision about what to do. In addition, > completely purge the TSO capabilities instead of disabling just CSUM_TSO. > > Thanks to jhb for explanation of the hw capabilites api. > > Thanks to royger and cognet for testing the 100Mbit failure case to > ensure that their adapters do indeed still work. > > MFC after: 1 week > Sponsored by: Limelight Networks > > Modified: > head/sys/dev/e1000/if_em.c > > Modified: head/sys/dev/e1000/if_em.c > ============================================================================== > --- head/sys/dev/e1000/if_em.c Sat Nov 5 16:23:33 2016 (r308344) > +++ head/sys/dev/e1000/if_em.c Sat Nov 5 16:30:42 2016 (r308345) > @@ -369,11 +369,6 @@ MODULE_DEPEND(em, netmap, 1, 1, 1); > #define MAX_INTS_PER_SEC 8000 > #define DEFAULT_ITR (1000000000/(MAX_INTS_PER_SEC * 256)) > > -/* Allow common code without TSO */ > -#ifndef CSUM_TSO > -#define CSUM_TSO 0 > -#endif > - > #define TSO_WORKAROUND 4 > > static SYSCTL_NODE(_hw, OID_AUTO, em, CTLFLAG_RD, 0, "EM driver parameters"); > @@ -1396,15 +1391,9 @@ em_init_locked(struct adapter *adapter) > if_clearhwassist(ifp); > if (if_getcapenable(ifp) & IFCAP_TXCSUM) > if_sethwassistbits(ifp, CSUM_TCP | CSUM_UDP, 0); > - /* > - ** There have proven to be problems with TSO when not > - ** at full gigabit speed, so disable the assist automatically > - ** when at lower speeds. -jfv > - */ > - if (if_getcapenable(ifp) & IFCAP_TSO4) { > - if (adapter->link_speed == SPEED_1000) > - if_sethwassistbits(ifp, CSUM_TSO, 0); > - } > + > + if (if_getcapenable(ifp) & IFCAP_TSO4) > + if_sethwassistbits(ifp, CSUM_TSO, 0); Does this always disable TSO? Should this part be removed entirely? (That is, it seems like this would disable TSO even on Gigabit links). > /* Configure for OS presence */ > em_init_manageability(adapter); > @@ -2412,6 +2401,18 @@ em_update_link_status(struct adapter *ad > if (link_check && (adapter->link_active == 0)) { > e1000_get_speed_and_duplex(hw, &adapter->link_speed, > &adapter->link_duplex); > + /* > + ** There have proven to be problems with TSO when not > + ** at full gigabit speed, so disable the assist automatically > + ** when at lower speeds. -jfv > + */ > + if (adapter->link_speed != SPEED_1000) { > + if_sethwassistbits(ifp, 0, CSUM_TSO); > + if_setcapenablebit(ifp, 0, IFCAP_TSO4); > + if_setcapabilitiesbit(ifp, 0, IFCAP_TSO4); > + > + } Even though I suggested it, I wonder if it wouldn't be better to only modify if_capenable and not if_capabilities, that way the admin can decide to use 'ifconfig em0 tso' to force it back on (e.g. if moving an adapter from 100 to 1G). -- John Baldwin