Date: Tue, 8 Nov 2016 16:32:21 +0800 From: Sepherosa Ziehau <sepherosa@gmail.com> To: Sean Bruno <sbruno@freebsd.org> Cc: John Baldwin <jhb@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: <CAMOc5cxxu6bHHvWjhfH=ZJNjdstqygp6vDd9jE0oXXhYwAePGA@mail.gmail.com> In-Reply-To: <075f36ce-53c8-ce58-672f-6086d8decc41@freebsd.org> References: <201611051630.uA5GUhtk073581@repo.freebsd.org> <15572642.JMvQo5TC3D@ralph.baldwin.cx> <CAMOc5czC4c0kpkKfq-uaK_FL=DhKj6rmtg%2BYSpDWa95FZaaXtg@mail.gmail.com> <075f36ce-53c8-ce58-672f-6086d8decc41@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Nov 7, 2016 at 10:05 PM, Sean Bruno <sbruno@freebsd.org> wrote: > > > On 11/06/16 23:37, Sepherosa Ziehau wrote: >> 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. >> > > I don't disagree, but I also don't have an opinion. What I didn't want, > was a continuation of the half disabled/half enabled TSO code path that > we had prior to this change. > >> As for this patch, do you need to re-enable TSO once link speed >> becomes 1000Mbps? > > Probably? There wasn't a clear way to flip this back on that I could > find that would catch the case of "link speed was 100 and is now 1000". > > > 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? > > TSO packets that are "pending" will continue out their path, AFAIK. I > don't believe that a link speed change from 1000 to 100 is a very common > occurrence, but I'm willing to change the code to something more > "graceful" if you have an idea of how to do it. https://people.freebsd.org/~sephe/em1.diff Thanks, sephe -- Tomorrow Will Never Die
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAMOc5cxxu6bHHvWjhfH=ZJNjdstqygp6vDd9jE0oXXhYwAePGA>