Hi Kevin,
Thanks for the message! I did not realize the issue with the wake-ups affected the EMAC, but I have been running my kernel with 'nohlt' since we transitioned to linux-omap-3.4rcX several weeks back. It was required to keep the kernel from crashing on boot. Running with that option does not appear to have altered my EMAC performance really at all, but I suppose I could be missing something else somewhere...
To try to debug this myself, I did spend a bit of time diving into the driver. I found several things that caused an eyebrow to raise, but I do not believe any of them are "smoking guns". I believe there are a few places where skb's could leak or simply be dropped from the RX pool. So I put some prints there, but I've been testing for a few days and have never seen any of my prints hit. I suspect they are holes in the code, but clearly not my problem. I'm a novice with the EMAC and CPDMA drivers. I could not even find where the "rx_dropped" stat was being incremented, so obviously this was not a thorough review.
Below is a patch I've been using to test these areas with. My main "discoveries", if valid, are:
1.) emac_rx_alloc() is redundant to netdev_alloc_skb_ip_align(). So, I swapped it out.
2.) skbs can be _leaked_ by "failed" cpdma_chan_submit()?
dma_map_single() is not checked for a valid return in cpdma_chan_submit(). So if it fails, the code continues to shove forward. I don't know what that would do. So my patch below calls dma_mapping_error() to check that return, and returns an error back to the calling function if a dma_map_single() failed. This lets the calling function, like emac_rx_handler(), free the skb.
3.) losing skb's from our rx pool?
IF I'm understanding this correctly... I noticed that up front, in emac_dev_open(), EMAC_DEF_RX_NUM_DESC skb's are allocated and fed to cpdma. However, any errors in that loop do not appear to stop things from continuing on. I suspect this is a bug, albeit one that really never gets hit. So I put a little code in there to not just bust out of the loop, but rather clean up an dangling skbs and return an error code.
Once the driver is up and running, it looks to me like the emac_rx_handler() is supposed to basically replace every skb it processes with a new one. Sounds reasonable. However, I think there are gaps in the logic that could allow this replacement to fail, presumably leaving the cpdma with less skbs than desired. In essence, if the repalcement fails, that skb is _never_ resubmitted to cpdma, so it operates forever with 1 less. The patch below just has dev_err() messages in there that I put in place to trap that if it occurs. My plan to fix this, if my prints showed it was worth fixing, would be to insert a value into priv that told me how many skbs are presently submitted to cpdma. So, in that initial loop, I'd increment that value up to the limit. Everytime emac_rx_handler() was called, I would initially decrement the value to record the one I was pushing up the stack. Then, instead of just allocating 1 skb to replace that packet, I would loop against
that value (how many skbs *should* be in cpdma) and insert enough to get my buffer count back up to where it should be. From a logic flow perspective, if I failed to either alloc or submit a replacement skb on that particular emac_rx_handler() call, it would be noted and next time around I would try to insert 2 to make up for the one lost previously.
Thoughts on this? Regardless, as I noted at the top, none of these tweaks or prints are showing. So I'm sure these are not core issues at the moment. I want to track down where the rx_dropped stat is being set so I can have a look there. Still, I suspect my performance issue is something DMA-related. I'm just not well-versed enough in all that to track it down.
Thanks again for all your assistance!
------------------------------------------------------------------------------------------------------------
--- linux/drivers/net/ethernet/ti/davinci_emac.c 2012-06-28 17:11:52.155508605 -0400
+++ linux-omap_3.5rc4/drivers/net/ethernet/ti/davinci_emac.c 2012-06-26 22:39:57.000000000 -0400
@@ -991,6 +991,7 @@
return IRQ_HANDLED;
}
+/* SAME AS: netdev_alloc_skb_ip_align()
static struct sk_buff *emac_rx_alloc(struct emac_priv *priv)
{
struct sk_buff *skb = netdev_alloc_skb(priv->ndev, priv->rx_buf_size);
@@ -999,6 +1000,7 @@
skb_reserve(skb, NET_IP_ALIGN);
return skb;
}
+*/
static void emac_rx_handler(void *token, int len, int status)
{
@@ -1028,10 +1030,12 @@
ndev->stats.rx_packets++;
/* alloc a new packet for receive */
- skb = emac_rx_alloc(priv);
- if (!skb) {
+// skb = emac_rx_alloc(priv);
+ skb = netdev_alloc_skb_ip_align(priv->ndev, priv->rx_buf_size);
+ if (unlikely(!skb)) {
if (netif_msg_rx_err(priv) && net_ratelimit())
dev_err(emac_dev, "failed rx buffer alloc\n");
+dev_err(emac_dev, "(1) lost rx skb?\n");
return;
}
@@ -1041,7 +1045,10 @@
WARN_ON(ret == -ENOMEM);
if (unlikely(ret < 0))
+ {
dev_kfree_skb_any(skb);
+dev_err(emac_dev, "(2) lost rx skb?\n");
+ }
}
static void emac_tx_handler(void *token, int len, int status)
@@ -1053,7 +1060,7 @@
atomic_dec(&priv->cur_tx);
if (unlikely(netif_queue_stopped(ndev)))
- netif_start_queue(ndev);
+ netif_start_queue(ndev); /* maybe consider using 'netif_wake_queue(ndev);' here instead? */
ndev->stats.tx_packets++;
ndev->stats.tx_bytes += len;
dev_kfree_skb_any(skb);
@@ -1105,6 +1112,7 @@
return NETDEV_TX_OK;
fail_tx:
+dev_err(emac_dev, "(3) lost TX skb, queue stopped - needs restarted (tx_handler)?\n");
ndev->stats.tx_dropped++;
netif_stop_queue(ndev);
return NETDEV_TX_BUSY;
@@ -1540,7 +1548,8 @@
ndev->dev_addr[cnt] = priv->mac_addr[cnt];
/* Configuration items */
- priv->rx_buf_size = EMAC_DEF_MAX_FRAME_SIZE + NET_IP_ALIGN;
+// priv->rx_buf_size = EMAC_DEF_MAX_FRAME_SIZE + NET_IP_ALIGN;
+ priv->rx_buf_size = EMAC_DEF_MAX_FRAME_SIZE;
priv->mac_hash1 = 0;
priv->mac_hash2 = 0;
@@ -1548,15 +1557,29 @@
emac_write(EMAC_MACHASH2, 0);
for (i = 0; i < EMAC_DEF_RX_NUM_DESC; i++) {
- struct sk_buff *skb = emac_rx_alloc(priv);
+// struct sk_buff *skb = emac_rx_alloc(priv);
+ struct sk_buff *skb = netdev_alloc_skb_ip_align(priv->ndev, priv->rx_buf_size);
- if (!skb)
+ if (unlikely(!skb))
+ {
+ ret = -ENOMEM;
break;
+ }
ret = cpdma_chan_submit(priv->rxchan, skb, skb->data,
skb_tailroom(skb), GFP_KERNEL);
if (WARN_ON(ret < 0))
+ {
+ dev_kfree_skb_any(skb);
+ ret = -ENOMEM;
break;
+ }
+ }
+
+ if (unlikely(ret < 0))
+ {
+ dev_err(emac_dev, "failed prestaging rx skbs in cpdma");
+ return ret;
}
/* Request IRQ */
@@ -1579,6 +1602,7 @@
coal.rx_coalesce_usecs = (priv->coal_intvl << 4);
emac_set_coalesce(ndev, &coal);
+dev_info(emac_dev, "interrupt pacing enabled");
}
cpdma_ctlr_start(priv->dma);
--- linux/drivers/net/ethernet/ti/davinci_cpdma.c 2012-06-28 17:11:52.155508605 -0400
+++ linux-omap_3.5rc4/drivers/net/ethernet/ti/davinci_cpdma.c 2012-06-26 17:15:31.000000000 -0400
@@ -679,6 +679,12 @@
}
buffer = dma_map_single(ctlr->dev, data, len, chan->dir);
+ if (dma_mapping_error(ctlr->dev, buffer)) {
+ dev_err(ctlr->dev, "CPDMA: dma_map_single failed!");
+ ret = -EINVAL;
+ goto unlock_ret;
+ }
+
mode = CPDMA_DESC_OWNER | CPDMA_DESC_SOP | CPDMA_DESC_EOP;
desc_write(desc, hw_next, 0);
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
