Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 6 Dec 2013 22:04:41 +0100
From:      Michael Tuexen <Michael.Tuexen@lurchi.franken.de>
To:        John-Mark Gurney <jmg@funkthat.com>
Cc:        Yong-Hyeon Pyun <pyunyh@gmail.com>, Jack F Vogel <jfv@freebsd.org>, Adrian Chadd <adrian@freebsd.org>, "freebsd-net@freebsd.org list" <freebsd-net@freebsd.org>
Subject:   Re: A small fix for if_em.c, if_igb.c, if_ixgbe.c
Message-ID:  <956436B1-5E20-4470-B415-3311F5CC24B8@lurchi.franken.de>
In-Reply-To: <20131206201748.GF55638@funkthat.com>
References:  <521B9C2A-EECC-4412-9F68-2235320EF324@lurchi.franken.de> <20131202022338.GA3500@michelle.cdnetworks.com> <B9593E83-E687-49E9-ABDC-B2DD615180E9@lurchi.franken.de> <20131203021658.GC2981@michelle.cdnetworks.com> <CAJ-Vmo=kfoPMYjZ0WAtqmoJMz1utXH50SW9N92RA83EMUzY7WA@mail.gmail.com> <2D0F95A6-1321-4F8E-87FB-1B9DD33FD319@lurchi.franken.de> <20131206201748.GF55638@funkthat.com>

next in thread | previous in thread | raw e-mail | index | archive | help
On Dec 6, 2013, at 9:17 PM, John-Mark Gurney <jmg@funkthat.com> wrote:

> Michael Tuexen wrote this message on Fri, Dec 06, 2013 at 21:08 +0100:
>> On Dec 5, 2013, at 7:29 PM, Adrian Chadd <adrian@freebsd.org> 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...
For sure. Thanks for catching it:

[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)
@@ -933,8 +933,10 @@ em_mq_start_locked(struct ifnet *ifp, struct tx_ri
 		if ((err = em_xmit(txr, &next)) != 0) {
 			if (next == NULL)
 				drbr_advance(ifp, txr->br);
-			else 
+			else {
 				drbr_putback(ifp, txr->br, next);
+				err = 0;
+			}
 			break;
 		}
 		drbr_advance(ifp, txr->br);
Index: e1000/if_igb.c
===================================================================
--- 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 = 0;
 			}
 			break;
 		}
Index: ixgbe/ixgbe.c
===================================================================
--- 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 = 0;
 			}
 #endif
 			break;
Index: ixgbe/ixv.c
===================================================================
--- 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 = 0;
 			}
 			break;
 		}
Index: virtio/network/if_vtnet.c
===================================================================
--- 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 = drbr_peek(ifp, br)) != NULL) {
 		error = vtnet_txq_encap(txq, &m);
 		if (error) {
-			if (m != NULL)
+			if (m != NULL) {
 				drbr_putback(ifp, br, m);
-			else
+				error = 0;
+			} else
 				drbr_advance(ifp, br);
 			break;
 		}

> 
> -- 
>  John-Mark Gurney				Voice: +1 415 225 5579
> 
>     "All that I will do, has been done, All that I have, has not."
> 




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?956436B1-5E20-4470-B415-3311F5CC24B8>