Date: Mon, 7 Nov 2016 14:37:43 +0800 From: Sepherosa Ziehau <sepherosa@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: Sean Bruno <sbruno@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r308345 - head/sys/dev/e1000 Message-ID: <CAMOc5czC4c0kpkKfq-uaK_FL=DhKj6rmtg%2BYSpDWa95FZaaXtg@mail.gmail.com> In-Reply-To: <15572642.JMvQo5TC3D@ralph.baldwin.cx> References: <201611051630.uA5GUhtk073581@repo.freebsd.org> <15572642.JMvQo5TC3D@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Nov 6, 2016 at 7:16 AM, John Baldwin <jhb@freebsd.org> wrote: > 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). I believe simply clearing CSUM_TSO should work for the TCP stack; messing administrative like capenable and hwcaps does not sound correct to me. As for this patch, do you need to re-enable TSO once link speed becomes 1000Mbps? BTW, since the link status check/update is async w/ the TX path, does this really work, if there are TSO packets pending on the TX rings (let alone inflight TSO packets from the TCP stack) when the link speed changed to 100Mbps? Thanks, sephe -- Tomorrow Will Never Die
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAMOc5czC4c0kpkKfq-uaK_FL=DhKj6rmtg%2BYSpDWa95FZaaXtg>