diff mbox

[v3,5/6] net: calxedaxgmac: rework transmit ring handling

Message ID 1352132544-15809-6-git-send-email-robherring2@gmail.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Rob Herring Nov. 5, 2012, 4:22 p.m. UTC 
From: Rob Herring <rob.herring@calxeda.com>

Only generate tx interrupts on every ring size / 4 descriptors. Move the
netif_stop_queue call to the end of the xmit function rather than
checking at the beginning.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 drivers/net/ethernet/calxeda/xgmac.c |   24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

Comments

David Miller Nov. 6, 2012, 11:57 p.m. UTC | #1 
From: Rob Herring <robherring2@gmail.com>
Date: Mon,  5 Nov 2012 10:22:23 -0600

> Only generate tx interrupts on every ring size / 4 descriptors.

I thought we told you that you cannot do this.

With this change if we get a few packets, then stop generating any
traffic, there will be SKBs that just sit dead in your TX queue.

This cannot ever happen.  All TX SKBs must be freed up in a short,
finite, amount of time.  Under all conditions, and in every situation.

Otherwise memory accounted to sockets is not liberated, and such
sockets cannot be destroyed or closed.

SKBs also hold onto other kinds of resources, for which it is critical
to liberate in a finite amount of time.

I'm not applying this series, it still needs more work.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Nov. 7, 2012, 12:29 a.m. UTC | #2 
David,

On 11/06/2012 05:57 PM, David Miller wrote:
> From: Rob Herring <robherring2@gmail.com>
> Date: Mon,  5 Nov 2012 10:22:23 -0600
> 
>> Only generate tx interrupts on every ring size / 4 descriptors.
> 
> I thought we told you that you cannot do this.
> 
> With this change if we get a few packets, then stop generating any
> traffic, there will be SKBs that just sit dead in your TX queue.

And as I previously mentioned, we do get a tx complete interrupt in
addition. The h/w will interrupt when all packets are transmitted and
there is not another descriptor ready. That is the only tx interrupt we
get without this patch. With this patch, we will get interrupts for
every N descriptors in addition to a tx complete/idle interrupt. This
patch is to avoid the transmitter from going idle and only refilling the
tx ring after finishing sending all frames. I can repost this patch and
make the commit message more clear, but I don't think there is any
functional change needed. This one is not so important compared to the
rest of the series, so you can just drop it if you still don't agree.

Rob

> 
> This cannot ever happen.  All TX SKBs must be freed up in a short,
> finite, amount of time.  Under all conditions, and in every situation.
> 
> Otherwise memory accounted to sockets is not liberated, and such
> sockets cannot be destroyed or closed.
> 
> SKBs also hold onto other kinds of resources, for which it is critical
> to liberate in a finite amount of time.
> 
> I'm not applying this series, it still needs more work.
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Nov. 7, 2012, 1:10 a.m. UTC | #3 
From: Rob Herring <rob.herring@calxeda.com>
Date: Tue, 06 Nov 2012 18:29:11 -0600

> David,
> 
> On 11/06/2012 05:57 PM, David Miller wrote:
>> From: Rob Herring <robherring2@gmail.com>
>> Date: Mon,  5 Nov 2012 10:22:23 -0600
>> 
>>> Only generate tx interrupts on every ring size / 4 descriptors.
>> 
>> I thought we told you that you cannot do this.
>> 
>> With this change if we get a few packets, then stop generating any
>> traffic, there will be SKBs that just sit dead in your TX queue.
> 
> And as I previously mentioned, we do get a tx complete interrupt in
> addition. The h/w will interrupt when all packets are transmitted and
> there is not another descriptor ready.

Ok, in that case it's fine.  I'll keep reviewing this series then.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/calxeda/xgmac.c b/drivers/net/ethernet/calxeda/xgmac.c
index 8263219..362b35e 100644
--- a/drivers/net/ethernet/calxeda/xgmac.c
+++ b/drivers/net/ethernet/calxeda/xgmac.c
@@ -211,7 +211,7 @@ 
 #define DMA_INTR_ENA_TIE	0x00000001	/* Transmit Interrupt */
 
 #define DMA_INTR_NORMAL		(DMA_INTR_ENA_NIE | DMA_INTR_ENA_RIE | \
-				 DMA_INTR_ENA_TUE)
+				 DMA_INTR_ENA_TUE | DMA_INTR_ENA_TIE)
 
 #define DMA_INTR_ABNORMAL	(DMA_INTR_ENA_AIE | DMA_INTR_ENA_FBE | \
 				 DMA_INTR_ENA_RWE | DMA_INTR_ENA_RSE | \
@@ -374,6 +374,7 @@  struct xgmac_priv {
 	struct sk_buff **tx_skbuff;
 	unsigned int tx_head;
 	unsigned int tx_tail;
+	int tx_irq_cnt;
 
 	void __iomem *base;
 	unsigned int dma_buf_sz;
@@ -886,7 +887,7 @@  static void xgmac_tx_complete(struct xgmac_priv *priv)
 	}
 
 	if (dma_ring_space(priv->tx_head, priv->tx_tail, DMA_TX_RING_SZ) >
-	    TX_THRESH)
+	    MAX_SKB_FRAGS)
 		netif_wake_queue(priv->dev);
 }
 
@@ -1057,19 +1058,15 @@  static netdev_tx_t xgmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct xgmac_priv *priv = netdev_priv(dev);
 	unsigned int entry;
 	int i;
+	u32 irq_flag;
 	int nfrags = skb_shinfo(skb)->nr_frags;
 	struct xgmac_dma_desc *desc, *first;
 	unsigned int desc_flags;
 	unsigned int len;
 	dma_addr_t paddr;
 
-	if (dma_ring_space(priv->tx_head, priv->tx_tail, DMA_TX_RING_SZ) <
-	    (nfrags + 1)) {
-		writel(DMA_INTR_DEFAULT_MASK | DMA_INTR_ENA_TIE,
-			priv->base + XGMAC_DMA_INTR_ENA);
-		netif_stop_queue(dev);
-		return NETDEV_TX_BUSY;
-	}
+	priv->tx_irq_cnt = (priv->tx_irq_cnt + 1) & (DMA_TX_RING_SZ/4 - 1);
+	irq_flag = priv->tx_irq_cnt ? 0 : TXDESC_INTERRUPT;
 
 	desc_flags = (skb->ip_summed == CHECKSUM_PARTIAL) ?
 		TXDESC_CSUM_ALL : 0;
@@ -1110,9 +1107,9 @@  static netdev_tx_t xgmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Interrupt on completition only for the latest segment */
 	if (desc != first)
 		desc_set_tx_owner(desc, desc_flags |
-			TXDESC_LAST_SEG | TXDESC_INTERRUPT);
+			TXDESC_LAST_SEG | irq_flag);
 	else
-		desc_flags |= TXDESC_LAST_SEG | TXDESC_INTERRUPT;
+		desc_flags |= TXDESC_LAST_SEG | irq_flag;
 
 	/* Set owner on first desc last to avoid race condition */
 	wmb();
@@ -1121,6 +1118,9 @@  static netdev_tx_t xgmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	priv->tx_head = dma_ring_incr(entry, DMA_TX_RING_SZ);
 
 	writel(1, priv->base + XGMAC_DMA_TX_POLL);
+	if (dma_ring_space(priv->tx_head, priv->tx_tail, DMA_TX_RING_SZ) <
+	    MAX_SKB_FRAGS)
+		netif_stop_queue(dev);
 
 	return NETDEV_TX_OK;
 }
@@ -1397,7 +1397,7 @@  static irqreturn_t xgmac_interrupt(int irq, void *dev_id)
 	}
 
 	/* TX/RX NORMAL interrupts */
-	if (intr_status & (DMA_STATUS_RI | DMA_STATUS_TU)) {
+	if (intr_status & (DMA_STATUS_RI | DMA_STATUS_TU | DMA_STATUS_TI)) {
 		__raw_writel(DMA_INTR_ABNORMAL, priv->base + XGMAC_DMA_INTR_ENA);
 		napi_schedule(&priv->napi);
 	}