From owner-freebsd-net@FreeBSD.ORG Fri Dec 6 20:08:16 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 9E6301F7; Fri, 6 Dec 2013 20:08:16 +0000 (UTC) Received: from mail-n.franken.de (drew.ipv6.franken.de [IPv6:2001:638:a02:a001:20e:cff:fe4a:feaa]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id C44A51B48; Fri, 6 Dec 2013 20:08:15 +0000 (UTC) Received: from [192.168.1.200] (p508F3521.dip0.t-ipconnect.de [80.143.53.33]) (Authenticated sender: macmic) by mail-n.franken.de (Postfix) with ESMTP id A43481C0C0692; Fri, 6 Dec 2013 21:08:12 +0100 (CET) Content-Type: text/plain; charset=iso-8859-1 Mime-Version: 1.0 (Mac OS X Mail 6.6 \(1510\)) Subject: Re: A small fix for if_em.c, if_igb.c, if_ixgbe.c From: Michael Tuexen In-Reply-To: Date: Fri, 6 Dec 2013 21:08:13 +0100 Content-Transfer-Encoding: quoted-printable Message-Id: <2D0F95A6-1321-4F8E-87FB-1B9DD33FD319@lurchi.franken.de> References: <521B9C2A-EECC-4412-9F68-2235320EF324@lurchi.franken.de> <20131202022338.GA3500@michelle.cdnetworks.com> <20131203021658.GC2981@michelle.cdnetworks.com> To: Adrian Chadd X-Mailer: Apple Mail (2.1510) Cc: Yong-Hyeon Pyun , Jack F Vogel , "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:08:16 -0000 On Dec 5, 2013, at 7:29 PM, Adrian Chadd wrote: > Hi, >=20 > 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(). >=20 > The same deal in if_em.c and igb.c >=20 > 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=3D0 if we go > via the drbr_putback() routine, as it hasn't actually failed to > transmit. >=20 > 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. >=20 > So Michael - can you redo your patch to set err=3D0 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 =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 --- 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=20 drbr_putback(ifp, txr->br, next); + err =3D 0; break; } drbr_advance(ifp, txr->br); Index: e1000/if_igb.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 --- e1000/if_igb.c (revision 259039) +++ e1000/if_igb.c (working copy) @@ -1024,6 +1024,7 @@ igb_mq_start_locked(struct ifnet *ifp, struct tx_r * may have changed it. */ drbr_putback(ifp, txr->br, next); + err =3D 0; } break; } Index: ixgbe/ixgbe.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 --- ixgbe/ixgbe.c (revision 259039) +++ ixgbe/ixgbe.c (working copy) @@ -864,6 +864,7 @@ ixgbe_mq_start_locked(struct ifnet *ifp, struct tx drbr_advance(ifp, txr->br); } else { drbr_putback(ifp, txr->br, next); + err =3D 0; } #endif break; Index: ixgbe/ixv.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 --- ixgbe/ixv.c (revision 259039) +++ ixgbe/ixv.c (working copy) @@ -629,6 +629,7 @@ ixv_mq_start_locked(struct ifnet *ifp, struct tx_r drbr_advance(ifp, txr->br); } else { drbr_putback(ifp, txr->br, next); + err =3D 0; } break; } Index: virtio/network/if_vtnet.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 --- virtio/network/if_vtnet.c (revision 259039) +++ virtio/network/if_vtnet.c (working copy) @@ -2242,9 +2242,10 @@ vtnet_txq_mq_start_locked(struct vtnet_txq *txq, = s while ((m =3D drbr_peek(ifp, br)) !=3D NULL) { error =3D vtnet_txq_encap(txq, &m); if (error) { - if (m !=3D NULL) + if (m !=3D NULL) { drbr_putback(ifp, br, m); - else + error =3D 0; + } else drbr_advance(ifp, br); break; } I looked for drivers using drbr_putback() and used a similar fix. Please = note that sys/dev/oce/oce_if.c seems strange. It uses drbr_putback() and = drbr_enqueue(), so I left it out for now. I tested the igb driver and the above patch fixes the problem I saw. =46rom your above description I think the above patch is a valid patch. However, xxx_transmit() can still return an error, even if there is no problem with the provided packet. This is an issue for transport = protocols... Best regards Michael >=20 > Thanks! >=20 >=20 >=20 >=20 > -adrian >=20