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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: if_igb.c.diff
Type: text/x-diff
Size: 4020 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-net/attachments/20121120/71e299d1/attachment.diff>