Date: Mon, 7 Nov 2016 07:02:23 -0700 From: Sean Bruno <sbruno@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r308345 - head/sys/dev/e1000 Message-ID: <28ef0f40-f2a3-92ea-fac6-a586c81810b0@freebsd.org> 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
This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --lotim6AVPElTpsBaFLPlfNhgHjGJmm4L8 Content-Type: multipart/mixed; boundary="ncFn9WmclKOCqEXh9Dl8KoIvLJ9KdTJse"; protected-headers="v1" From: Sean Bruno <sbruno@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Message-ID: <28ef0f40-f2a3-92ea-fac6-a586c81810b0@freebsd.org> Subject: Re: svn commit: r308345 - head/sys/dev/e1000 References: <201611051630.uA5GUhtk073581@repo.freebsd.org> <15572642.JMvQo5TC3D@ralph.baldwin.cx> In-Reply-To: <15572642.JMvQo5TC3D@ralph.baldwin.cx> --ncFn9WmclKOCqEXh9Dl8KoIvLJ9KdTJse Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 11/05/16 17:16, John Baldwin 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 th= is >> adapter to work around bugs in TSO handling at this speed. >> =20 >> 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. >> =20 >> Move the handling of TSO deactivation to the link handler where we c= an >> more effectively make the decision about what to do. In addition, >> completely purge the TSO capabilities instead of disabling just CSUM= _TSO. >> =20 >> Thanks to jhb for explanation of the hw capabilites api. >> =20 >> Thanks to royger and cognet for testing the 100Mbit failure case to >> ensure that their adapters do indeed still work. >> =20 >> 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)) >> =20 >> -/* Allow common code without TSO */ >> -#ifndef CSUM_TSO >> -#define CSUM_TSO 0 >> -#endif >> - >> #define TSO_WORKAROUND 4 >> =20 >> static SYSCTL_NODE(_hw, OID_AUTO, em, CTLFLAG_RD, 0, "EM driver param= eters"); >> @@ -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); >> - /*=20 >> - ** 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); >=20 > Does this always disable TSO? Should this part be removed entirely? > (That is, it seems like this would disable TSO even on Gigabit links). >=20 I was confused by this question. The old code *always* disabled TSO because link_speed was always 0 here on boot. My intention is to ensure that CSUM_TSO is set if IFCAP_TSO4 is set. >> /* 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); >> + /*=20 >> + ** 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 !=3D SPEED_1000) { >> + if_sethwassistbits(ifp, 0, CSUM_TSO); >> + if_setcapenablebit(ifp, 0, IFCAP_TSO4); >> + if_setcapabilitiesbit(ifp, 0, IFCAP_TSO4); >> + >> + } >=20 > 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 spent several hours trying to come up with logic that would allow me to allow the user to do this. I am open to suggestions here, but it would require quite a bit more finesse than my "big hammer" approach. sean --ncFn9WmclKOCqEXh9Dl8KoIvLJ9KdTJse-- --lotim6AVPElTpsBaFLPlfNhgHjGJmm4L8 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQF8BAEBCgBmBQJYIIlvXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRCQUFENDYzMkU3MTIxREU4RDIwOTk3REQx MjAxRUZDQTFFNzI3RTY0AAoJEBIB78oecn5kTMoIAMtFj9sd1xxQxC1oxtaRSbnc NZ1qG1saA5s4olwt39U2d+kcWNsGvWWx/Mdeg5O7Zkbidu6dijdUyCVCYodiw/JF wHUXyLU+vruyIgR8gD0MEWzBiEfkjVo/Jj7bPSPF8nMNqrzUxCMpFvVHd/SdwZSR fEmKvvfFS7th50UtnxE8lCWBBbizvhvpjoTsH74bjEpzcrWgXwmXsS5WamQo5kNs tHATmjfd1QivAgtUWIUHnEYVh97pJeINsmu06Tim9PFlSY+aW0U9mFqvtj5U913J UtI6VhLsgKCeo8B8HIHYDuypiVAx4lSYs80qkTE62U0+wW5KP0yWI2VZ3D0ejcU= =cBbf -----END PGP SIGNATURE----- --lotim6AVPElTpsBaFLPlfNhgHjGJmm4L8--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?28ef0f40-f2a3-92ea-fac6-a586c81810b0>