Date: Tue, 20 Nov 2012 15:18:33 +0400 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Karim Fodil-Lemelin <fodillemlinkarim@gmail.com>, jfv@FreeBSD.org Cc: freebsd-net@FreeBSD.org Subject: Re: igb diver crashes in head@241037 Message-ID: <20121120111833.GC67660@FreeBSD.org> In-Reply-To: <50AA8F24.7080604@gmail.com> References: <50AA8F24.7080604@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--6sX45UoQRIJXqkqR
Content-Type: text/plain; charset=koi8-r
Content-Disposition: inline
Karim,
On Mon, Nov 19, 2012 at 02:57:24PM -0500, Karim Fodil-Lemelin wrote:
K> While testing the latest igb driver in CURRENT I came across an issue
K> with igb_mq_start(). More specifically this code:
K>
K> ...
K>
K> struct mbuf *pm = NULL;
K> /*
K> ** Try to queue first to avoid
K> ** out-of-order delivery, but
K> ** settle for it if that fails
K> */
K> if (m && drbr_enqueue(ifp, txr->br, m))
K> pm = m;
K> err = igb_mq_start_locked(ifp, txr, pm);
K>
K> ...
K>
K>
K> The problem comes from the fact that drbr_enqueue() can return an error
K> and delete the mbuf as seen in drbr_enqueue():
K>
K> ...
K> error = buf_ring_enqueue(br, m);
K> if (error)
K> m_freem(m);
K> ...
Good catch!
K> diff --git a/sys/dev/e1000/if_igb.c b/sys/dev/e1000/if_igb.c
K> index 1318910..be1719a 100644
K> --- a/sys/dev/e1000/if_igb.c
K> +++ b/sys/dev/e1000/if_igb.c
K> @@ -961,15 +961,7 @@ igb_mq_start(struct ifnet *ifp, struct mbuf *m)
K> que = &adapter->queues[i];
K> if (((txr->queue_status & IGB_QUEUE_DEPLETED) == 0) &&
K> IGB_TX_TRYLOCK(txr)) {
K> - struct mbuf *pm = NULL;
K> - /*
K> - ** Try to queue first to avoid
K> - ** out-of-order delivery, but
K> - ** settle for it if that fails
K> - */
K> - if (m && drbr_enqueue(ifp, txr->br, m))
K> - pm = m;
K> - err = igb_mq_start_locked(ifp, txr, pm);
K> + err = igb_mq_start_locked(ifp, txr, m);
K> IGB_TX_UNLOCK(txr);
K> } else {
K> err = drbr_enqueue(ifp, txr->br, m);
Well, the idea to prevent out-of-order delivery is important. TCP
suffers a lot when this happens.
I'd suggest the following code:
if (m)
drbr_enqueue(ifp, txr->br, m);
err = igb_mq_start_locked(ifp, txr, NULL);
Which eventually leads us to all invocations of igb_mq_start_locked() called
with third argument as NULL. This allows us to simplify this function.
Patch for review attached.
--
Totus tuus, Glebius.
--6sX45UoQRIJXqkqR
Content-Type: text/x-diff; charset=koi8-r
Content-Disposition: attachment; filename="if_igb.c.diff"
Index: if_igb.c
===================================================================
--- if_igb.c (revision 243329)
+++ if_igb.c (working copy)
@@ -181,8 +181,7 @@
static int igb_resume(device_t);
#if __FreeBSD_version >= 800000
static int igb_mq_start(struct ifnet *, struct mbuf *);
-static int igb_mq_start_locked(struct ifnet *,
- struct tx_ring *, struct mbuf *);
+static int igb_mq_start_locked(struct ifnet *, struct tx_ring *);
static void igb_qflush(struct ifnet *);
static void igb_deferred_mq_start(void *, int);
#else
@@ -845,7 +844,7 @@
/* Process the stack queue only if not depleted */
if (((txr->queue_status & IGB_QUEUE_DEPLETED) == 0) &&
!drbr_empty(ifp, txr->br))
- igb_mq_start_locked(ifp, txr, NULL);
+ igb_mq_start_locked(ifp, txr);
#else
if (!IFQ_DRV_IS_EMPTY(&ifp->if_snd))
igb_start_locked(txr, ifp);
@@ -961,15 +960,14 @@
que = &adapter->queues[i];
if (((txr->queue_status & IGB_QUEUE_DEPLETED) == 0) &&
IGB_TX_TRYLOCK(txr)) {
- struct mbuf *pm = NULL;
/*
** Try to queue first to avoid
** out-of-order delivery, but
** settle for it if that fails
*/
- if (m && drbr_enqueue(ifp, txr->br, m))
- pm = m;
- err = igb_mq_start_locked(ifp, txr, pm);
+ if (m)
+ drbr_enqueue(ifp, txr->br, m);
+ err = igb_mq_start_locked(ifp, txr);
IGB_TX_UNLOCK(txr);
} else {
err = drbr_enqueue(ifp, txr->br, m);
@@ -980,7 +978,7 @@
}
static int
-igb_mq_start_locked(struct ifnet *ifp, struct tx_ring *txr, struct mbuf *m)
+igb_mq_start_locked(struct ifnet *ifp, struct tx_ring *txr)
{
struct adapter *adapter = txr->adapter;
struct mbuf *next;
@@ -990,24 +988,13 @@
if (((ifp->if_drv_flags & IFF_DRV_RUNNING) == 0) ||
(txr->queue_status & IGB_QUEUE_DEPLETED) ||
- adapter->link_active == 0) {
- if (m != NULL)
- err = drbr_enqueue(ifp, txr->br, m);
+ adapter->link_active == 0)
return (err);
- }
enq = 0;
- if (m == NULL) {
- next = drbr_dequeue(ifp, txr->br);
- } else if (drbr_needs_enqueue(ifp, txr->br)) {
- if ((err = drbr_enqueue(ifp, txr->br, m)) != 0)
- return (err);
- next = drbr_dequeue(ifp, txr->br);
- } else
- next = m;
/* Process the queue */
- while (next != NULL) {
+ while ((next = drbr_dequeue(ifp, txr->br)) != NULL) {
if ((err = igb_xmit(txr, &next)) != 0) {
if (next != NULL)
err = drbr_enqueue(ifp, txr->br, next);
@@ -1020,7 +1007,6 @@
ETHER_BPF_MTAP(ifp, next);
if ((ifp->if_drv_flags & IFF_DRV_RUNNING) == 0)
break;
- next = drbr_dequeue(ifp, txr->br);
}
if (enq > 0) {
/* Set the watchdog */
@@ -1046,7 +1032,7 @@
IGB_TX_LOCK(txr);
if (!drbr_empty(ifp, txr->br))
- igb_mq_start_locked(ifp, txr, NULL);
+ igb_mq_start_locked(ifp, txr);
IGB_TX_UNLOCK(txr);
}
@@ -1398,7 +1384,7 @@
/* Process the stack queue only if not depleted */
if (((txr->queue_status & IGB_QUEUE_DEPLETED) == 0) &&
!drbr_empty(ifp, txr->br))
- igb_mq_start_locked(ifp, txr, NULL);
+ igb_mq_start_locked(ifp, txr);
#else
if (!IFQ_DRV_IS_EMPTY(&ifp->if_snd))
igb_start_locked(txr, ifp);
@@ -1449,7 +1435,7 @@
/* Process the stack queue only if not depleted */
if (((txr->queue_status & IGB_QUEUE_DEPLETED) == 0) &&
!drbr_empty(ifp, txr->br))
- igb_mq_start_locked(ifp, txr, NULL);
+ igb_mq_start_locked(ifp, txr);
#else
if (!IFQ_DRV_IS_EMPTY(&ifp->if_snd))
igb_start_locked(txr, ifp);
@@ -1549,7 +1535,7 @@
} while (loop-- && more);
#if __FreeBSD_version >= 800000
if (!drbr_empty(ifp, txr->br))
- igb_mq_start_locked(ifp, txr, NULL);
+ igb_mq_start_locked(ifp, txr);
#else
if (!IFQ_DRV_IS_EMPTY(&ifp->if_snd))
igb_start_locked(txr, ifp);
@@ -1586,7 +1572,7 @@
/* Process the stack queue only if not depleted */
if (((txr->queue_status & IGB_QUEUE_DEPLETED) == 0) &&
!drbr_empty(ifp, txr->br))
- igb_mq_start_locked(ifp, txr, NULL);
+ igb_mq_start_locked(ifp, txr);
#else
if (!IFQ_DRV_IS_EMPTY(&ifp->if_snd))
igb_start_locked(txr, ifp);
--6sX45UoQRIJXqkqR--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121120111833.GC67660>