Skip site navigation (1)Skip section navigation (2)
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>