From owner-svn-src-all@freebsd.org Tue Nov 8 08:32:23 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id CD120C3668E; Tue, 8 Nov 2016 08:32:23 +0000 (UTC) (envelope-from sepherosa@gmail.com) Received: from mail-vk0-x22c.google.com (mail-vk0-x22c.google.com [IPv6:2607:f8b0:400c:c05::22c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 87B6B34E; Tue, 8 Nov 2016 08:32:23 +0000 (UTC) (envelope-from sepherosa@gmail.com) Received: by mail-vk0-x22c.google.com with SMTP id p9so142770500vkd.3; Tue, 08 Nov 2016 00:32:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=KHEpvPhJ3fCL519L2TKFZpf1I3P0e9FP/d/I5kn6kHg=; b=OfSh1etH/ZJplN5VAYTT8GNoi3yr1LUHQPPYu61qtWXpKeX82wV0XT4SsHrTQroyYS gsuqSWYwC9zh4/LhEprbU2Ol7s2U+CKOTELg+9QxQZO3/a9R/yiSkfNuGjYkaJvwcjbv FlRa2/hGQHry+PvAR96vpQ4Rae4PTEs9ZmKRsBKf7pY/dlEN3+puYi6UKx8tB+mofmCF kEPvA8QdiuinrXoBFlLd6nsI88rXaqZJ4SSvzr1qFXQrCvlWWWg/2N8OVAuzMm15yzd8 3AIXA3NYnxdy0Z/g5mju7sckwyAAQPpkn4UjpUGRYSHRwq8fFruWrnRTmKRAqq+INObq WoOA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=KHEpvPhJ3fCL519L2TKFZpf1I3P0e9FP/d/I5kn6kHg=; b=Tv6VmTLtby18AC6MceoO7MpYGBS4/zlY5Wf2Eak4PBgdEy0O0wja4lGm5cIOONmiAL Y0tvp5r7PEMpce4SUjhvh8VImGlyN/w1IH4JRLQiSdf7jdmrexLpmT/qxj6KTmR6IPCg iN0JdVBkO8SLEJNxfwI1IJA1hve9PkK8atbbsN4WQUOtdVIB4sOF09HAOcwZf30y5RrZ aLbiUIg96P10rKDpz9FpkOawULrx5SSnyYq7sZG+IFdK+67LPv3QyQUx3QdKPSybA5aI m19BrmRlBCKoHbUW3fKJQgu9VDfLTyxMjuUUJZIdU6zafxjWGg50IlDZ5YBjj7AIu8HO RS3A== X-Gm-Message-State: ABUngvcYlz/VxsDfgnolxslFD8Kk7L48Chai19LW94qaXEC04HmMnR/dqWn1gC/E6r2ywvTNBJPMfifTu2vP7A== X-Received: by 10.31.155.208 with SMTP id d199mr7336396vke.55.1478593942326; Tue, 08 Nov 2016 00:32:22 -0800 (PST) MIME-Version: 1.0 Received: by 10.159.36.247 with HTTP; Tue, 8 Nov 2016 00:32:21 -0800 (PST) In-Reply-To: <075f36ce-53c8-ce58-672f-6086d8decc41@freebsd.org> References: <201611051630.uA5GUhtk073581@repo.freebsd.org> <15572642.JMvQo5TC3D@ralph.baldwin.cx> <075f36ce-53c8-ce58-672f-6086d8decc41@freebsd.org> From: Sepherosa Ziehau Date: Tue, 8 Nov 2016 16:32:21 +0800 Message-ID: Subject: Re: svn commit: r308345 - head/sys/dev/e1000 To: Sean Bruno Cc: John Baldwin , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Content-Type: text/plain; charset=UTF-8 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Nov 2016 08:32:23 -0000 On Mon, Nov 7, 2016 at 10:05 PM, Sean Bruno wrote: > > > On 11/06/16 23:37, Sepherosa Ziehau wrote: >> On Sun, Nov 6, 2016 at 7:16 AM, 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 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