kern/150516: e1000 receive queue handling problem
Beezar Liu
beezarliu at yahoo.com.cn
Mon Sep 13 06:50:02 UTC 2010
>Number: 150516
>Category: kern
>Synopsis: e1000 receive queue handling problem
>Confidential: no
>Severity: non-critical
>Priority: medium
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Mon Sep 13 06:50:01 UTC 2010
>Closed-Date:
>Last-Modified:
>Originator: Beezar Liu
>Release: FreeBSD8.1
>Organization:
>Environment:
>Description:
I found em's receiving queue handling will have the following problems if system mbuf is used up.
1. NIC will hang because receive ring's tail pointer will not be updated (in em_rxeof).
2. "ifconfig up/down" may cause system panic because em_setup_receive_ring will free already-freed mbufs.
So I made some changes:
1. NIC's recieve ring's head/tail pointer is updated according to rxr's next_to_check/next_to_refresh.
So, on the position of next_to_refresh, no need to fill free mbuf because datasheet says
"When the head pointer(s) is equal to the tail pointer(s), the queue(s) is empty.
Hardware stops storing packets in system memory until software advances the tail
pointer(s), making more receive buffers available."
And (next_to_refresh + 1) % num_rx_desc == next_to_check means ring queue is full.
2. no need to reallocate the mbufs on receive queue when em re-initialize.
3. The mbufs on the queue are also freed according to these two indexs.
4. If ring queue is empty, em_refresh_mbufs is also called even if it doesn't handle any packet
>How-To-Repeat:
>Fix:
Patch attached with submission follows:
Index: if_em.c
===================================================================
--- if_em.c (revision 212405)
+++ if_em.c (working copy)
@@ -3671,11 +3671,13 @@
bus_dma_segment_t segs[1];
bus_dmamap_t map;
struct em_buffer *rxbuf;
- int i, error, nsegs, cleaned;
+ int i, j, error, nsegs, cleaned;
- i = rxr->next_to_refresh;
+ i = j = rxr->next_to_refresh;
cleaned = -1;
- while (i != limit) {
+ if (++j == adapter->num_rx_desc)
+ j = 0;
+ while (j != limit) {
m = m_getcl(M_DONTWAIT, MT_DATA, M_PKTHDR);
if (m == NULL)
goto update;
@@ -3711,9 +3713,10 @@
rxr->rx_base[i].buffer_addr = htole64(segs[0].ds_addr);
cleaned = i;
+ i = j;
/* Calculate next index */
- if (++i == adapter->num_rx_desc)
- i = 0;
+ if (++j == adapter->num_rx_desc)
+ j = 0;
/* This is the work marker for refresh */
rxr->next_to_refresh = i;
}
@@ -3722,7 +3725,7 @@
BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE);
if (cleaned != -1) /* Update tail index */
E1000_WRITE_REG(&adapter->hw,
- E1000_RDT(rxr->me), cleaned);
+ E1000_RDT(rxr->me), rxr->next_to_refresh);
return;
}
@@ -3809,32 +3812,22 @@
struct adapter *adapter = rxr->adapter;
struct em_buffer *rxbuf;
bus_dma_segment_t seg[1];
- int rsize, nsegs, error;
+ int i, j, nsegs, error;
/* Clear the ring contents */
EM_RX_LOCK(rxr);
- rsize = roundup2(adapter->num_rx_desc *
- sizeof(struct e1000_rx_desc), EM_DBA_ALIGN);
- bzero((void *)rxr->rx_base, rsize);
-
- /*
- ** Free current RX buffer structs and their mbufs
- */
- for (int i = 0; i < adapter->num_rx_desc; i++) {
- rxbuf = &rxr->rx_buffers[i];
- if (rxbuf->m_head != NULL) {
- bus_dmamap_sync(rxr->rxtag, rxbuf->map,
- BUS_DMASYNC_POSTREAD);
- bus_dmamap_unload(rxr->rxtag, rxbuf->map);
- m_freem(rxbuf->m_head);
- }
+ for (i = 0; i < adapter->num_rx_desc; i++) {
+ struct e1000_rx_desc* cur;
+ cur = &rxr->rx_base[i];
+ cur->status = 0;
}
- /* Now replenish the mbufs */
- for (int j = 0; j != adapter->num_rx_desc; ++j) {
-
- rxbuf = &rxr->rx_buffers[j];
+ i = j = rxr->next_to_refresh;
+ if (++j == adapter->num_rx_desc)
+ j = 0;
+ while(j != rxr->next_to_check) {
+ rxbuf = &rxr->rx_buffers[i];
rxbuf->m_head = m_getcl(M_DONTWAIT, MT_DATA, M_PKTHDR);
if (rxbuf->m_head == NULL)
panic("RX ring hdr initialization failed!\n");
@@ -3852,13 +3845,14 @@
rxbuf->map, BUS_DMASYNC_PREREAD);
/* Update descriptor */
- rxr->rx_base[j].buffer_addr = htole64(seg[0].ds_addr);
+ rxr->rx_base[i].buffer_addr = htole64(seg[0].ds_addr);
+ i = j;
+ if (++j == adapter->num_rx_desc)
+ j = 0;
}
-
/* Setup our descriptor indices */
- rxr->next_to_check = 0;
- rxr->next_to_refresh = 0;
+ rxr->next_to_refresh = i;
bus_dmamap_sync(rxr->rxdma.dma_tag, rxr->rxdma.dma_map,
BUS_DMASYNC_PREREAD | BUS_DMASYNC_PREWRITE);
@@ -3938,6 +3932,7 @@
{
struct adapter *adapter = rxr->adapter;
struct em_buffer *rxbuf = NULL;
+ int i;
INIT_DEBUGOUT("free_receive_buffers: begin");
@@ -3947,7 +3942,8 @@
}
if (rxr->rx_buffers != NULL) {
- for (int i = 0; i < adapter->num_rx_desc; i++) {
+ i = rxr->next_to_check;
+ while(i != rxr->next_to_refresh) {
rxbuf = &rxr->rx_buffers[i];
if (rxbuf->map != NULL) {
bus_dmamap_sync(rxr->rxtag, rxbuf->map,
@@ -3959,6 +3955,8 @@
m_freem(rxbuf->m_head);
rxbuf->m_head = NULL;
}
+ if (++i == adapter->num_rx_desc)
+ i = 0;
}
free(rxr->rx_buffers, M_DEVBUF);
rxr->rx_buffers = NULL;
@@ -4044,8 +4042,8 @@
E1000_WRITE_REG(hw, E1000_RDBAH(i), (u32)(bus_addr >> 32));
E1000_WRITE_REG(hw, E1000_RDBAL(i), (u32)bus_addr);
/* Setup the Head and Tail Descriptor Pointers */
- E1000_WRITE_REG(hw, E1000_RDH(i), 0);
- E1000_WRITE_REG(hw, E1000_RDT(i), adapter->num_rx_desc - 1);
+ E1000_WRITE_REG(hw, E1000_RDH(i), rxr->next_to_check);
+ E1000_WRITE_REG(hw, E1000_RDT(i), rxr->next_to_refresh);
}
/* Setup the Receive Control Register */
@@ -4206,7 +4204,7 @@
}
/* Catch any remaining refresh work */
- if (processed != 0) {
+ if (processed != 0 || i == rxr->next_to_refresh) {
em_refresh_mbufs(rxr, i);
processed = 0;
}
>Release-Note:
>Audit-Trail:
>Unformatted:
More information about the freebsd-bugs
mailing list