From owner-svn-src-all@freebsd.org Mon Nov 7 06:37:45 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 99221C337CB; Mon, 7 Nov 2016 06:37:45 +0000 (UTC) (envelope-from sepherosa@gmail.com) Received: from mail-ua0-x22e.google.com (mail-ua0-x22e.google.com [IPv6:2607:f8b0:400c:c08::22e]) (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 4E87EBB1; Mon, 7 Nov 2016 06:37:45 +0000 (UTC) (envelope-from sepherosa@gmail.com) Received: by mail-ua0-x22e.google.com with SMTP id 20so111960323uak.0; Sun, 06 Nov 2016 22:37:45 -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=sFPcs9wAlicYRZ5Wj75V2abdBy28rH+nOQlcyk/2y8A=; b=h4q+atGpXcSrM0OXjNGX195fLc2TDyP+Fm8ITeqf+fhODOjyzZ+FxdEbJNZPuK7dBr s+WsnCIocEMNZ/VdPnv76gIJuBncM/gS3Jsw9Zbs7h4ufqxRQT5I1aPRddk7CQV3PgYN 1x422FNJCz2zrMR9xzdiYXy2PmDaA7Q8uMKZO7+Xi6A6BgPbFLMSg7l2UvrtTG5KAmS/ zAHT8EZTOg2xUo8DegE17a3fYyGIpYDeacLX75YifgvsxdMf5C958Nqx6c0PoK45IM1G IebXk4tcEpQ3ZIXtL14tEEIWDMeHpYkAv47Ck+Qx25tU0TuQGzQ+DneXqhyoS5lfyJcw 5WFg== 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=sFPcs9wAlicYRZ5Wj75V2abdBy28rH+nOQlcyk/2y8A=; b=TyoqYfFFjUxhVEfmnTKLqGeHABsxV97PFoWyDY0eAjXHdE9zrayxbEzmqOPLOgynBK un1UgHiP3porogQ2dMuQQ8H5QrQNolmJB0cOOKe4oM2jM2XDwmS7l6jx2kiGcdLqRTHW Xhc/lsSc3+g972Njt4UeOVgq+GbfkGD8sMBJCQNHZqWCxoz3pW9cQVGl/KwJdzVII+So B5giARDFN+Cd+6AhJHUMDk2lWkVAPebWuE/7nwWfGAT7oahkpNV5yoxi83y3C2/7DzLF ZH5exsq5AGOdjgTBlxDQJkFkXi6jB92HFMgVRrpfoLFn8nqdLxSFI3S5TS4FprMTXyA4 Kr3Q== X-Gm-Message-State: ABUngvdt7jUC+81apvnNhg8934kfutQQHLeWssEZU+L9bOQIhCv6vx4YlGN2O6GCxfywdZXZR03kC/q56nzLXA== X-Received: by 10.176.68.103 with SMTP id m94mr2925531uam.75.1478500664140; Sun, 06 Nov 2016 22:37:44 -0800 (PST) MIME-Version: 1.0 Received: by 10.159.36.247 with HTTP; Sun, 6 Nov 2016 22:37:43 -0800 (PST) In-Reply-To: <15572642.JMvQo5TC3D@ralph.baldwin.cx> References: <201611051630.uA5GUhtk073581@repo.freebsd.org> <15572642.JMvQo5TC3D@ralph.baldwin.cx> From: Sepherosa Ziehau Date: Mon, 7 Nov 2016 14:37:43 +0800 Message-ID: Subject: Re: svn commit: r308345 - head/sys/dev/e1000 To: John Baldwin Cc: Sean Bruno , 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: Mon, 07 Nov 2016 06:37:45 -0000 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. As for this patch, do you need to re-enable TSO once link speed becomes 1000Mbps? 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? Thanks, sephe -- Tomorrow Will Never Die