From owner-freebsd-net@FreeBSD.ORG Tue Nov 20 11:18:41 2012 Return-Path: Delivered-To: freebsd-net@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 4C0E117E; Tue, 20 Nov 2012 11:18:41 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebius.int.ru (glebius.int.ru [81.19.64.117]) by mx1.freebsd.org (Postfix) with ESMTP id 7D4C78FC08; Tue, 20 Nov 2012 11:18:40 +0000 (UTC) Received: from cell.glebius.int.ru (localhost [127.0.0.1]) by cell.glebius.int.ru (8.14.5/8.14.5) with ESMTP id qAKBIXfJ068342; Tue, 20 Nov 2012 15:18:33 +0400 (MSK) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebius.int.ru (8.14.5/8.14.5/Submit) id qAKBIXgU068341; Tue, 20 Nov 2012 15:18:33 +0400 (MSK) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebius.int.ru: glebius set sender to glebius@FreeBSD.org using -f Date: Tue, 20 Nov 2012 15:18:33 +0400 From: Gleb Smirnoff To: Karim Fodil-Lemelin , jfv@FreeBSD.org Subject: Re: igb diver crashes in head@241037 Message-ID: <20121120111833.GC67660@FreeBSD.org> References: <50AA8F24.7080604@gmail.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="6sX45UoQRIJXqkqR" Content-Disposition: inline In-Reply-To: <50AA8F24.7080604@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: freebsd-net@FreeBSD.org X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Nov 2012 11:18:41 -0000 --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--