Date: Fri, 6 Dec 2013 21:08:13 +0100 From: Michael Tuexen <Michael.Tuexen@lurchi.franken.de> To: Adrian Chadd <adrian@freebsd.org> Cc: Yong-Hyeon Pyun <pyunyh@gmail.com>, Jack F Vogel <jfv@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: <2D0F95A6-1321-4F8E-87FB-1B9DD33FD319@lurchi.franken.de> In-Reply-To: <CAJ-Vmo=kfoPMYjZ0WAtqmoJMz1utXH50SW9N92RA83EMUzY7WA@mail.gmail.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>
next in thread | previous in thread | raw e-mail | index | archive | help
On Dec 5, 2013, at 7:29 PM, Adrian Chadd <adrian@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2D0F95A6-1321-4F8E-87FB-1B9DD33FD319>