Date: Mon, 7 Nov 2016 07:05:59 -0700 From: Sean Bruno <sbruno@freebsd.org> To: Sepherosa Ziehau <sepherosa@gmail.com>, John Baldwin <jhb@freebsd.org> Cc: 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: <075f36ce-53c8-ce58-672f-6086d8decc41@freebsd.org> In-Reply-To: <CAMOc5czC4c0kpkKfq-uaK_FL=DhKj6rmtg%2BYSpDWa95FZaaXtg@mail.gmail.com> References: <201611051630.uA5GUhtk073581@repo.freebsd.org> <15572642.JMvQo5TC3D@ralph.baldwin.cx> <CAMOc5czC4c0kpkKfq-uaK_FL=DhKj6rmtg%2BYSpDWa95FZaaXtg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --7j8ti3piQR8DUwJAq8fPrfVnOOm8dSErp Content-Type: multipart/mixed; boundary="R9PVhDFbhknGeFBWBGSRNGBvjj516oHeu"; protected-headers="v1" From: Sean Bruno <sbruno@freebsd.org> To: Sepherosa Ziehau <sepherosa@gmail.com>, John Baldwin <jhb@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Message-ID: <075f36ce-53c8-ce58-672f-6086d8decc41@freebsd.org> Subject: Re: svn commit: r308345 - head/sys/dev/e1000 References: <201611051630.uA5GUhtk073581@repo.freebsd.org> <15572642.JMvQo5TC3D@ralph.baldwin.cx> <CAMOc5czC4c0kpkKfq-uaK_FL=DhKj6rmtg+YSpDWa95FZaaXtg@mail.gmail.com> In-Reply-To: <CAMOc5czC4c0kpkKfq-uaK_FL=DhKj6rmtg+YSpDWa95FZaaXtg@mail.gmail.com> --R9PVhDFbhknGeFBWBGSRNGBvjj516oHeu Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 t= his >>> 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 CSU= M_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 >>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D >>> --- 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 para= meters"); >>> @@ -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 =3D=3D 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 =3D=3D 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 automat= ically >>> + ** when at lower speeds. -jfv >>> + */ >>> + if (adapter->link_speed !=3D 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). >=20 > I believe simply clearing CSUM_TSO should work for the TCP stack; > messing administrative like capenable and hwcaps does not sound > correct to me. >=20 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 --R9PVhDFbhknGeFBWBGSRNGBvjj516oHeu-- --7j8ti3piQR8DUwJAq8fPrfVnOOm8dSErp Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQF8BAEBCgBmBQJYIIpHXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRCQUFENDYzMkU3MTIxREU4RDIwOTk3REQx MjAxRUZDQTFFNzI3RTY0AAoJEBIB78oecn5kNgkH/jm1x6D4T1mAXZ0Eqrkkui2z fCzr1RMnSYM1vfIhjUR18VPn4c9Dze4X81gnrPv99YUHk9zecs3HtDbUTN3TRLAp ILs9jOo6PKwvLCI2f0rnzn4OguPqy+nTCMRi5/pT0MTBIVXeoBaLy5OfblctFG8x srhrZydr6O2swttCNz/HErUFyLOHs1yFVtQFQiNrAAikpniEb3fD1o1NPDYp3OX2 WJTDeElXhL+rITwrkd4DQSzbAISS3VGGrJF9NJRMSrNb4RXQDpzVFMoKm4Pp0kiC tMWwfNOAO+YblwUJ4oiUljwk67zlfpc1J9ADYkI+V176c4emO4Lh2bevVxkEtMM= =+Jfz -----END PGP SIGNATURE----- --7j8ti3piQR8DUwJAq8fPrfVnOOm8dSErp--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?075f36ce-53c8-ce58-672f-6086d8decc41>