svn commit: r308345 - head/sys/dev/e1000

[Sean Bruno]

On 11/06/16 23:37, Sepherosa Ziehau wrote:
> On Sun, Nov 6, 2016 at 7:16 AM, John Baldwin <jhb at 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.

sean

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 585 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20161107/ae188a86/attachment.sig>