Date: Tue, 20 Nov 2012 09:19:54 -0800 From: Jack Vogel <jfvogel@gmail.com> To: Gleb Smirnoff <glebius@freebsd.org> Cc: Karim Fodil-Lemelin <fodillemlinkarim@gmail.com>, jfv@freebsd.org, freebsd-net@freebsd.org Subject: Re: igb diver crashes in head@241037 Message-ID: <CAFOYbckbEPr3B=Kj5kiW16QKj5a6hWeTo%2BiU=vRpfy2jqXvd4w@mail.gmail.com> In-Reply-To: <20121120111833.GC67660@FreeBSD.org> References: <50AA8F24.7080604@gmail.com> <20121120111833.GC67660@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Nov 20, 2012 at 3:18 AM, Gleb Smirnoff <glebius@freebsd.org> wrote: > 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. > > Yes Gleb, I already have code in my internal tree which simply removes an mbuf pointer form the start_locked call and ALWAYS does a dequeue, start similarly will always enqueue. I just have been busy with ixgbe for a bit and have not gotten it committed yet. Jack
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFOYbckbEPr3B=Kj5kiW16QKj5a6hWeTo%2BiU=vRpfy2jqXvd4w>