Date: Wed, 21 Nov 2012 11:44:57 -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: <CAFOYbc=NNF%2BgVHdd=g%2Bi13UnmZFAtX73KirWpiJWcdp6YgmyYA@mail.gmail.com> In-Reply-To: <20121121062631.GJ67660@glebius.int.ru> References: <50AA8F24.7080604@gmail.com> <20121120111833.GC67660@FreeBSD.org> <CAFOYbckbEPr3B=Kj5kiW16QKj5a6hWeTo%2BiU=vRpfy2jqXvd4w@mail.gmail.com> <20121121062631.GJ67660@glebius.int.ru>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --]
Gleb,
Here is a patch based on my latest igb internal code, I had not yet
committed this as I
was not completely confident about the start/queueing changes, I would love
to have a
wider testing base, so anyone that wishes to test this... Its against HEAD.
It does a few things: change mq_start to ALWAYS queue, hence
mq_start_locked no
longer takes an mbuf pointer arg.
Second, it gets rid of OACTIVE as far as the queues go, its still used only
in a device
wide up/down sense.
Last, there is a flow control display added, this follows what our linux
driver does, it
gives you the current flow control state when a link up event happens. I
was asked
to do this by my validation group, and it seemed kinda handy...
Let me know what you think,
Jack
On Tue, Nov 20, 2012 at 10:26 PM, Gleb Smirnoff <glebius@freebsd.org> wrote:
> Jack,
>
> On Tue, Nov 20, 2012 at 09:19:54AM -0800, Jack Vogel wrote:
> J> > I'd suggest the following code:
> J> >
> J> > if (m)
> J> > drbr_enqueue(ifp, txr->br, m);
> J> > err = igb_mq_start_locked(ifp, txr, NULL);
> J> >
> J> > Which eventually leads us to all invocations of igb_mq_start_locked()
> J> > called
> J> > with third argument as NULL. This allows us to simplify this function.
> J> >
> J> > Patch for review attached.
> J> >
> J> >
> J> Yes Gleb, I already have code in my internal tree which simply removes
> an
> J> mbuf
> J> pointer form the start_locked call and ALWAYS does a dequeue, start
> J> similarly
> J> will always enqueue. I just have been busy with ixgbe for a bit and have
> J> not gotten
> J> it committed yet.
>
> Since ixgbe work is performance tuning and this patch closes a kernel
> crash,
> I'd ask to preempt the ixgbe job with this patch. :)
>
> Or you can approve my patch and I will check it in.
>
> --
> Totus tuus, Glebius.
>
[-- Attachment #2 --]
--- if_igb.c 2012-10-30 09:46:52.478147221 -0700
+++ if_igb.new.c 2012-11-21 11:32:01.175605461 -0800
@@ -100,7 +100,7 @@
/*********************************************************************
* Driver version:
*********************************************************************/
-char igb_driver_version[] = "version - 2.3.5";
+char igb_driver_version[] = "version - 2.3.8";
/*********************************************************************
@@ -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);
@@ -959,68 +958,47 @@
txr = &adapter->tx_rings[i];
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);
- IGB_TX_UNLOCK(txr);
- } else {
- err = drbr_enqueue(ifp, txr->br, m);
- taskqueue_enqueue(que->tq, &txr->txq_task);
- }
+ taskqueue_enqueue(que->tq, &txr->txq_task);
return (err);
}
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;
- int err = 0, enq;
+ struct mbuf *m;
+ int err = 0, enq = 0;
IGB_TX_LOCK_ASSERT(txr);
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);
- return (err);
- }
+ adapter->link_active == 0)
+ return (ENETDOWN);
- 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;
+ if (txr->queue_status & IGB_QUEUE_DEPLETED)
+ igb_txeof(txr);
+ if (txr->tx_avail >= IGB_MAX_SCATTER)
+ txr->queue_status &= ~IGB_QUEUE_DEPLETED;
+ else
+ return (EIO);
/* Process the queue */
- while (next != NULL) {
- if ((err = igb_xmit(txr, &next)) != 0) {
- if (next != NULL)
- err = drbr_enqueue(ifp, txr->br, next);
+ m = drbr_dequeue(ifp, txr->br);
+ while (m != NULL) {
+ if ((err = igb_xmit(txr, &m)) != 0) {
+ if (m != NULL)
+ err = drbr_enqueue(ifp, txr->br, m);
break;
}
enq++;
- ifp->if_obytes += next->m_pkthdr.len;
- if (next->m_flags & M_MCAST)
+ ifp->if_obytes += m->m_pkthdr.len;
+ if (m->m_flags & M_MCAST)
ifp->if_omcasts++;
- ETHER_BPF_MTAP(ifp, next);
+ ETHER_BPF_MTAP(ifp, m);
if ((ifp->if_drv_flags & IFF_DRV_RUNNING) == 0)
break;
- next = drbr_dequeue(ifp, txr->br);
+ m = drbr_dequeue(ifp, txr->br);
}
if (enq > 0) {
/* Set the watchdog */
@@ -1046,7 +1024,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 +1376,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 +1427,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 +1527,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 +1564,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);
@@ -1695,7 +1673,6 @@
igb_media_status(struct ifnet *ifp, struct ifmediareq *ifmr)
{
struct adapter *adapter = ifp->if_softc;
- u_char fiber_type = IFM_1000_SX;
INIT_DEBUGOUT("igb_media_status: begin");
@@ -1712,26 +1689,31 @@
ifmr->ifm_status |= IFM_ACTIVE;
- if ((adapter->hw.phy.media_type == e1000_media_type_fiber) ||
- (adapter->hw.phy.media_type == e1000_media_type_internal_serdes))
- ifmr->ifm_active |= fiber_type | IFM_FDX;
- else {
- switch (adapter->link_speed) {
- case 10:
- ifmr->ifm_active |= IFM_10_T;
- break;
- case 100:
- ifmr->ifm_active |= IFM_100_TX;
- break;
- case 1000:
- ifmr->ifm_active |= IFM_1000_T;
- break;
- }
- if (adapter->link_duplex == FULL_DUPLEX)
- ifmr->ifm_active |= IFM_FDX;
+ switch (adapter->link_speed) {
+ case 10:
+ ifmr->ifm_active |= IFM_10_T;
+ break;
+ case 100:
+ /*
+ ** Support for 100Mb SFP - these are Fiber
+ ** but the media type appears as serdes
+ */
+ if (adapter->hw.phy.media_type ==
+ e1000_media_type_internal_serdes)
+ ifmr->ifm_active |= IFM_100_FX;
else
- ifmr->ifm_active |= IFM_HDX;
+ ifmr->ifm_active |= IFM_100_TX;
+ break;
+ case 1000:
+ ifmr->ifm_active |= IFM_1000_T;
+ break;
}
+
+ if (adapter->link_duplex == FULL_DUPLEX)
+ ifmr->ifm_active |= IFM_FDX;
+ else
+ ifmr->ifm_active |= IFM_HDX;
+
IGB_CORE_UNLOCK(adapter);
}
@@ -2180,7 +2162,7 @@
struct ifnet *ifp = adapter->ifp;
struct tx_ring *txr = adapter->tx_rings;
struct igb_queue *que = adapter->queues;
- int hung = 0, busy = 0;
+ int hung = 0;
IGB_CORE_LOCK_ASSERT(adapter);
@@ -2190,26 +2172,16 @@
/*
** Check the TX queues status
- ** - central locked handling of OACTIVE
- ** - watchdog only if all queues show hung
*/
for (int i = 0; i < adapter->num_queues; i++, que++, txr++) {
if ((txr->queue_status & IGB_QUEUE_HUNG) &&
(adapter->pause_frames == 0))
++hung;
- if (txr->queue_status & IGB_QUEUE_DEPLETED)
- ++busy;
if ((txr->queue_status & IGB_QUEUE_IDLE) == 0)
taskqueue_enqueue(que->tq, &que->que_task);
}
if (hung == adapter->num_queues)
goto timeout;
- if (busy == adapter->num_queues)
- ifp->if_drv_flags |= IFF_DRV_OACTIVE;
- else if ((ifp->if_drv_flags & IFF_DRV_OACTIVE) &&
- (busy < adapter->num_queues))
- ifp->if_drv_flags &= ~IFF_DRV_OACTIVE;
-
adapter->pause_frames = 0;
callout_reset(&adapter->timer, hz, igb_local_timer, adapter);
#ifndef DEVICE_POLLING
@@ -2234,11 +2206,13 @@
static void
igb_update_link_status(struct adapter *adapter)
{
- struct e1000_hw *hw = &adapter->hw;
- struct ifnet *ifp = adapter->ifp;
- device_t dev = adapter->dev;
- struct tx_ring *txr = adapter->tx_rings;
- u32 link_check, thstat, ctrl;
+ struct e1000_hw *hw = &adapter->hw;
+ struct e1000_fc_info *fc = &hw->fc;
+ struct ifnet *ifp = adapter->ifp;
+ device_t dev = adapter->dev;
+ struct tx_ring *txr = adapter->tx_rings;
+ u32 link_check, thstat, ctrl;
+ char *flowctl = NULL;
link_check = thstat = ctrl = 0;
@@ -2276,15 +2250,33 @@
ctrl = E1000_READ_REG(hw, E1000_CTRL_EXT);
}
+ /* Get the flow control for display */
+ switch (fc->current_mode) {
+ case e1000_fc_rx_pause:
+ flowctl = "RX";
+ break;
+ case e1000_fc_tx_pause:
+ flowctl = "TX";
+ break;
+ case e1000_fc_full:
+ flowctl = "Full";
+ break;
+ case e1000_fc_none:
+ default:
+ flowctl = "None";
+ break;
+ }
+
/* Now we check if a transition has happened */
if (link_check && (adapter->link_active == 0)) {
e1000_get_speed_and_duplex(&adapter->hw,
&adapter->link_speed, &adapter->link_duplex);
if (bootverbose)
- device_printf(dev, "Link is up %d Mbps %s\n",
+ device_printf(dev, "Link is up %d Mbps %s,"
+ " Flow Control: %s\n",
adapter->link_speed,
((adapter->link_duplex == FULL_DUPLEX) ?
- "Full Duplex" : "Half Duplex"));
+ "Full Duplex" : "Half Duplex"), flowctl);
adapter->link_active = 1;
ifp->if_baudrate = adapter->link_speed * 1000000;
if ((ctrl & E1000_CTRL_EXT_LINK_MODE_GMII) &&
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFOYbc=NNF%2BgVHdd=g%2Bi13UnmZFAtX73KirWpiJWcdp6YgmyYA>