From owner-freebsd-net@FreeBSD.ORG Fri Dec 6 20:17:55 2013 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id B2017882; Fri, 6 Dec 2013 20:17:55 +0000 (UTC) Received: from h2.funkthat.com (gate2.funkthat.com [208.87.223.18]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 88DFA1D4E; Fri, 6 Dec 2013 20:17:55 +0000 (UTC) Received: from h2.funkthat.com (localhost [127.0.0.1]) by h2.funkthat.com (8.14.3/8.14.3) with ESMTP id rB6KHmgo092246 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 6 Dec 2013 12:17:48 -0800 (PST) (envelope-from jmg@h2.funkthat.com) Received: (from jmg@localhost) by h2.funkthat.com (8.14.3/8.14.3/Submit) id rB6KHmEr092245; Fri, 6 Dec 2013 12:17:48 -0800 (PST) (envelope-from jmg) Date: Fri, 6 Dec 2013 12:17:48 -0800 From: John-Mark Gurney To: Michael Tuexen Subject: Re: A small fix for if_em.c, if_igb.c, if_ixgbe.c Message-ID: <20131206201748.GF55638@funkthat.com> Mail-Followup-To: Michael Tuexen , Adrian Chadd , Yong-Hyeon Pyun , Jack F Vogel , "freebsd-net@freebsd.org list" References: <521B9C2A-EECC-4412-9F68-2235320EF324@lurchi.franken.de> <20131202022338.GA3500@michelle.cdnetworks.com> <20131203021658.GC2981@michelle.cdnetworks.com> <2D0F95A6-1321-4F8E-87FB-1B9DD33FD319@lurchi.franken.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2D0F95A6-1321-4F8E-87FB-1B9DD33FD319@lurchi.franken.de> User-Agent: Mutt/1.4.2.3i X-Operating-System: FreeBSD 7.2-RELEASE i386 X-PGP-Fingerprint: 54BA 873B 6515 3F10 9E88 9322 9CB1 8F74 6D3F A396 X-Files: The truth is out there X-URL: http://resnet.uoregon.edu/~gurney_j/ X-Resume: http://resnet.uoregon.edu/~gurney_j/resume.html X-to-the-FBI-CIA-and-NSA: HI! HOW YA DOIN? can i haz chizburger? X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.2.2 (h2.funkthat.com [127.0.0.1]); Fri, 06 Dec 2013 12:17:49 -0800 (PST) Cc: Yong-Hyeon Pyun , Jack F Vogel , Adrian Chadd , "freebsd-net@freebsd.org list" X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Dec 2013 20:17:55 -0000 Michael Tuexen wrote this message on Fri, Dec 06, 2013 at 21:08 +0100: > On Dec 5, 2013, at 7:29 PM, Adrian Chadd wrote: > > > Yes. Looking at the ixgbe code, ixgbe_mq_start_locked() returns an > > error from ixgbe_xmit() but if it fails, it puts the buffer back. But > > it's already successfully queued a frame to the driver, so in this > > instance it shouldn't return the error from ixgbe_mq_start_locked(). > > > > The same deal in if_em.c and igb.c > > > > Now, drbr_putback() used to fail and now it doesn't, as you've said. > > So we should change the xxx_mq_start_locked() to set err=0 if we go > > via the drbr_putback() routine, as it hasn't actually failed to > > transmit. > > > > Now the very dirty thing is this - the error from xxx_transmit() is > > for the mbuf being queued at the end; but xxx_mq_start_locked() > > failures are for transmitting from the front. If there's only packet > > in the queue and that fails then they're the same thing and returning > > the error from xxx_mq_start_locked() matches the current mbuf being > > queued. But otherwise, they're referring to totally different packets. > > For TCP this may hurt; the TCP stack treats ENOBUFS a certain way and > > kicks off a timer to schedule a retransmit. I don't think we can fix > > _this_ right now. > > > > So Michael - can you redo your patch to set err=0 if drbr_putback() is > > called, and retest? > Hi Adrian, > > I guess you are talking about a patch like: > > [bsd5:~/head/sys/dev] tuexen% svn diff -x -p > Index: e1000/if_em.c > =================================================================== > --- e1000/if_em.c (revision 259039) > +++ e1000/if_em.c (working copy) > @@ -935,6 +935,7 @@ em_mq_start_locked(struct ifnet *ifp, struct tx_ri > drbr_advance(ifp, txr->br); > else > drbr_putback(ifp, txr->br, next); > + err = 0; You probably want curly braces around this... -- John-Mark Gurney Voice: +1 415 225 5579 "All that I will do, has been done, All that I have, has not."