From owner-freebsd-net@FreeBSD.ORG Thu Feb 28 19:20:01 2013 Return-Path: Delivered-To: freebsd-net@smarthost.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id EF372349 for ; Thu, 28 Feb 2013 19:20:01 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:1900:2254:206c::16:87]) by mx1.freebsd.org (Postfix) with ESMTP id D0AA3DA1 for ; Thu, 28 Feb 2013 19:20:01 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.6/8.14.6) with ESMTP id r1SJK1dm040991 for ; Thu, 28 Feb 2013 19:20:01 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.6/8.14.6/Submit) id r1SJK1FU040990; Thu, 28 Feb 2013 19:20:01 GMT (envelope-from gnats) Date: Thu, 28 Feb 2013 19:20:01 GMT Message-Id: <201302281920.r1SJK1FU040990@freefall.freebsd.org> To: freebsd-net@FreeBSD.org Cc: From: "Charbon, Julien" Subject: Re: kern/176446: [netinet] [patch] Concurrency in ixgbe driving out-of-order packet process and spurious RST X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: "Charbon, Julien" List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Feb 2013 19:20:02 -0000 The following reply was made to PR kern/176446; it has been noted by GNATS. From: "Charbon, Julien" To: John Baldwin Cc: bug-followup@freebsd.org, "De La Gueronniere, Marc" , jfv@freebsd.org Subject: Re: kern/176446: [netinet] [patch] Concurrency in ixgbe driving out-of-order packet process and spurious RST Date: Thu, 28 Feb 2013 20:10:39 +0100 On 2/28/13 4:57 PM, John Baldwin wrote: > Can you try the fixes from http://svnweb.freebsd.org/base?view=revision&revision=240968? Actually, Marc (I CC'ed him) did find the r240968 fix for concurrency between ixgbe_msix_que() and ixgbe_handle_que(), and made a backport for release-8.3.0 (see patch [1] below). However, the issue was still reproducible, then Marc found another place for concurrency from ixgbe_local_timer() and fix it (see patch [2]). But it was still not enough, and he found a last place for concurrency due to ixgbe_rearm_queues() call (see patch [3]). We all these patches applied, we were not able to reproduce this issue. If patch [1] and [2] seems clearly legitimates, patch [3] would need more discussions/feedback I guess. -- Julien [1] Patch ixgbe (1/3): Backport r240968 in release-8.3.0 Index: sys/dev/ixgbe/ixgbe.c =================================================================== --- sys/dev/ixgbe/ixgbe.c +++ sys/dev/ixgbe/ixgbe.c @@ -102,13 +102,15 @@ static int ixgbe_attach(device_t); static int ixgbe_detach(device_t); static int ixgbe_shutdown(device_t); -static void ixgbe_start(struct ifnet *); -static void ixgbe_start_locked(struct tx_ring *, struct ifnet *); #if __FreeBSD_version >= 800000 static int ixgbe_mq_start(struct ifnet *, struct mbuf *); static int ixgbe_mq_start_locked(struct ifnet *, struct tx_ring *, struct mbuf *); static void ixgbe_qflush(struct ifnet *); +static void ixgbe_deferred_mq_start(void *, int); +#else +static void ixgbe_start(struct ifnet *); +static void ixgbe_start_locked(struct tx_ring *, struct ifnet *); #endif static int ixgbe_ioctl(struct ifnet *, u_long, caddr_t); static void ixgbe_init(void *); @@ -645,6 +647,7 @@ { struct adapter *adapter = device_get_softc(dev); struct ix_queue *que = adapter->queues; + struct tx_ring *txr = adapter->tx_rings; u32 ctrl_ext; INIT_DEBUGOUT("ixgbe_detach: begin"); @@ -659,8 +662,11 @@ ixgbe_stop(adapter); IXGBE_CORE_UNLOCK(adapter); - for (int i = 0; i < adapter->num_tx_queues; i++, que++) { + for (int i = 0; i < adapter->num_tx_queues; i++, que++, txr++) { if (que->tq) { +#if __FreeBSD_version >= 800000 + taskqueue_drain(que->tq, &txr->txq_task); +#endif taskqueue_drain(que->tq, &que->que_task); taskqueue_free(que->tq); } @@ -722,6 +728,7 @@ } +#if __FreeBSD_version < 800000 /********************************************************************* * Transmit entry point * @@ -793,7 +800,7 @@ return; } -#if __FreeBSD_version >= 800000 +#else /* ** Multiqueue Transmit driver ** @@ -821,7 +828,7 @@ IXGBE_TX_UNLOCK(txr); } else { err = drbr_enqueue(ifp, txr->br, m); - taskqueue_enqueue(que->tq, &que->que_task); + taskqueue_enqueue(que->tq, &txr->txq_task); } return (err); @@ -887,6 +894,22 @@ } /* + * Called from a taskqueue to drain queued transmit packets. + */ +static void +ixgbe_deferred_mq_start(void *arg, int pending) +{ + struct tx_ring *txr = arg; + struct adapter *adapter = txr->adapter; + struct ifnet *ifp = adapter->ifp; + + IXGBE_TX_LOCK(txr); + if (!drbr_empty(ifp, txr->br)) + ixgbe_mq_start_locked(ifp, txr, NULL); + IXGBE_TX_UNLOCK(txr); +} + +/* ** Flush all ring buffers */ static void @@ -2210,6 +2233,9 @@ { device_t dev = adapter->dev; struct ix_queue *que = adapter->queues; +#if __FreeBSD_version >= 800000 + struct tx_ring *txr = adapter->tx_rings; +#endif int error, rid = 0; /* MSI RID at 1 */ @@ -2229,6 +2255,9 @@ * Try allocating a fast interrupt and the associated deferred * processing contexts. */ +#if __FreeBSD_version >= 800000 + TASK_INIT(&txr->txq_task, 0, ixgbe_deferred_mq_start, txr); +#endif TASK_INIT(&que->que_task, 0, ixgbe_handle_que, que); que->tq = taskqueue_create_fast("ixgbe_que", M_NOWAIT, taskqueue_thread_enqueue, &que->tq); @@ -2275,9 +2304,10 @@ { device_t dev = adapter->dev; struct ix_queue *que = adapter->queues; + struct tx_ring *txr = adapter->tx_rings; int error, rid, vector = 0; - for (int i = 0; i < adapter->num_tx_queues; i++, vector++, que++) { + for (int i = 0; i < adapter->num_tx_queues; i++, vector++, que++, txr++) { rid = vector + 1; que->res = bus_alloc_resource_any(dev, SYS_RES_IRQ, &rid, RF_SHAREABLE | RF_ACTIVE); @@ -2307,6 +2337,9 @@ if (adapter->num_tx_queues > 1) bus_bind_intr(dev, que->res, i); +#if __FreeBSD_version >= 800000 + TASK_INIT(&txr->txq_task, 0, ixgbe_deferred_mq_start, txr); +#endif TASK_INIT(&que->que_task, 0, ixgbe_handle_que, que); que->tq = taskqueue_create_fast("ixgbe_que", M_NOWAIT, taskqueue_thread_enqueue, &que->tq); @@ -2555,12 +2588,13 @@ ifp->if_softc = adapter; ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST; ifp->if_ioctl = ixgbe_ioctl; - ifp->if_start = ixgbe_start; #if __FreeBSD_version >= 800000 ifp->if_transmit = ixgbe_mq_start; ifp->if_qflush = ixgbe_qflush; +#else + ifp->if_start = ixgbe_start; + IFQ_SET_MAXLEN(&ifp->if_snd, adapter->num_tx_desc - 2); #endif - ifp->if_snd.ifq_maxlen = adapter->num_tx_desc - 2; ether_ifattach(ifp, adapter->hw.mac.addr); Index: sys/dev/ixgbe/ixgbe.h =================================================================== --- sys/dev/ixgbe/ixgbe.h +++ sys/dev/ixgbe/ixgbe.h @@ -298,6 +298,7 @@ char mtx_name[16]; #if __FreeBSD_version >= 800000 struct buf_ring *br; + struct task txq_task; #endif #ifdef IXGBE_FDIR u16 atr_sample; [2] Patch ixgbe (2/3): Do not schedule ixgbe_handle_que() from ixgbe_local_timer(). Index: sys/dev/ixgbe/ixgbe.c =================================================================== --- sys/dev/ixgbe/ixgbe.c +++ sys/dev/ixgbe/ixgbe.c @@ -2033,7 +2033,7 @@ if (txr->queue_status & IXGBE_QUEUE_DEPLETED) ++busy; if ((txr->queue_status & IXGBE_QUEUE_IDLE) == 0) - taskqueue_enqueue(que->tq, &que->que_task); + taskqueue_enqueue(que->tq, &txr->txq_task); } /* Only truely watchdog if all queues show hung */ if (hung == adapter->num_tx_queues) [3] Patch ixgbe (3/3): ixgbe_rearm_queues() directly schedules an interruption and drives not wanted concurrency, should we called it at all? Index: sys/dev/ixgbe/ixgbe.c =================================================================== --- sys/dev/ixgbe/ixgbe.c +++ sys/dev/ixgbe/ixgbe.c @@ -2046,7 +2046,7 @@ ifp->if_drv_flags &= ~IFF_DRV_OACTIVE; out: - ixgbe_rearm_queues(adapter, adapter->que_mask); + // ixgbe_rearm_queues(adapter, adapter->que_mask); callout_reset(&adapter->timer, hz, ixgbe_local_timer, adapter); return; @@ -4674,7 +4674,7 @@ ** Schedule another interrupt if so. */ if ((staterr & IXGBE_RXD_STAT_DD) != 0) { - ixgbe_rearm_queues(adapter, (u64)(1 << que->msix)); + // ixgbe_rearm_queues(adapter, (u64)(1 << que->msix)); return (TRUE); }