netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/4] net: bcmgenet: interrupt coalescing support
@ 2014-04-21 15:45 Florian Fainelli
  2014-04-21 15:45 ` [PATCH net-next 1/4] net: bcmgenet: prepare for MBDONE interrupts support Florian Fainelli
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Florian Fainelli @ 2014-04-21 15:45 UTC (permalink / raw)
  To: netdev; +Cc: davem, Florian Fainelli

Hi David,

This patch series adds RX and TX interrupt coalescing support to the BCMGENET
driver. Thanks!

Florian Fainelli (4):
  net: bcmgenet: prepare for MBDONE interrupts support
  net: bcmgenet: add support for ethtool tx-frames
  net: bcmgenet: add support for ethtool rx-frames
  net: bcmgenet: do not set RX buffer length

 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 98 +++++++++++++++++++++-----
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  1 +
 drivers/net/ethernet/broadcom/genet/bcmmii.c   |  3 +
 3 files changed, 85 insertions(+), 17 deletions(-)

-- 
1.9.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net-next 1/4] net: bcmgenet: prepare for MBDONE interrupts support
  2014-04-21 15:45 [PATCH net-next 0/4] net: bcmgenet: interrupt coalescing support Florian Fainelli
@ 2014-04-21 15:45 ` Florian Fainelli
  2014-04-21 15:45 ` [PATCH net-next 2/4] net: bcmgenet: add support for ethtool tx-frames Florian Fainelli
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2014-04-21 15:45 UTC (permalink / raw)
  To: netdev; +Cc: davem, Florian Fainelli

Multiple buffer done interrupts, as their name suggests will generate an
interrupt after N packets/buffers are done, prepare the interrupt
handlers and TX and RX path for handling these interrupts in preparation
for ethtool coalescing support.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 0966bd04375f..b093a48c427f 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -78,6 +78,15 @@
 #define GENET_RDMA_REG_OFF	(priv->hw_params->rdma_offset + \
 				TOTAL_DESC * DMA_DESC_SIZE)
 
+/* Tx/Rx DMA done masks: packet, buffer and multiple buffer done */
+#define UMAC_TXDMA_DONE_MASK	(UMAC_IRQ_TXDMA_BDONE | \
+				 UMAC_IRQ_TXDMA_PDONE | \
+				 UMAC_IRQ_TXDMA_MBDONE)
+
+#define UMAC_RXDMA_DONE_MASK	(UMAC_IRQ_RXDMA_BDONE | \
+				 UMAC_IRQ_RXDMA_PDONE | \
+				 UMAC_IRQ_RXDMA_MBDONE)
+
 static inline void dmadesc_set_length_status(struct bcmgenet_priv *priv,
 						void __iomem *d, u32 value)
 {
@@ -840,16 +849,14 @@ static void bcmgenet_free_cb(struct enet_cb *cb)
 static inline void bcmgenet_tx_ring16_int_disable(struct bcmgenet_priv *priv,
 						  struct bcmgenet_tx_ring *ring)
 {
-	bcmgenet_intrl2_0_writel(priv,
-			UMAC_IRQ_TXDMA_BDONE | UMAC_IRQ_TXDMA_PDONE,
+	bcmgenet_intrl2_0_writel(priv, UMAC_TXDMA_DONE_MASK,
 			INTRL2_CPU_MASK_SET);
 }
 
 static inline void bcmgenet_tx_ring16_int_enable(struct bcmgenet_priv *priv,
 						 struct bcmgenet_tx_ring *ring)
 {
-	bcmgenet_intrl2_0_writel(priv,
-			UMAC_IRQ_TXDMA_BDONE | UMAC_IRQ_TXDMA_PDONE,
+	bcmgenet_intrl2_0_writel(priv, UMAC_TXDMA_DONE_MASK
 			INTRL2_CPU_MASK_CLEAR);
 }
 
@@ -1509,7 +1516,7 @@ static int init_umac(struct bcmgenet_priv *priv)
 	bcmgenet_intrl2_0_writel(priv, 0xFFFFFFFF, INTRL2_CPU_CLEAR);
 	bcmgenet_intrl2_0_writel(priv, 0, INTRL2_CPU_MASK_CLEAR);
 
-	cpu_mask_clear = UMAC_IRQ_RXDMA_BDONE;
+	cpu_mask_clear = UMAC_RXDMA_DONE_MASK;
 
 	dev_dbg(kdev, "%s:Enabling RXDMA_BDONE interrupt\n", __func__);
 
@@ -1795,7 +1802,7 @@ static int bcmgenet_poll(struct napi_struct *napi, int budget)
 	if (work_done < budget) {
 		napi_complete(napi);
 		bcmgenet_intrl2_0_writel(priv,
-			UMAC_IRQ_RXDMA_BDONE, INTRL2_CPU_MASK_CLEAR);
+			UMAC_RXDMA_DONE_MASK, INTRL2_CPU_MASK_CLEAR);
 	}
 
 	return work_done;
@@ -1862,19 +1869,18 @@ static irqreturn_t bcmgenet_isr0(int irq, void *dev_id)
 	netif_dbg(priv, intr, priv->dev,
 		"IRQ=0x%x\n", priv->irq0_stat);
 
-	if (priv->irq0_stat & (UMAC_IRQ_RXDMA_BDONE | UMAC_IRQ_RXDMA_PDONE)) {
+	if (priv->irq0_stat & UMAC_RXDMA_DONE_MASK) {
 		/* We use NAPI(software interrupt throttling, if
 		 * Rx Descriptor throttling is not used.
 		 * Disable interrupt, will be enabled in the poll method.
 		 */
 		if (likely(napi_schedule_prep(&priv->napi))) {
 			bcmgenet_intrl2_0_writel(priv,
-				UMAC_IRQ_RXDMA_BDONE, INTRL2_CPU_MASK_SET);
+				UMAC_RXDMA_DONE_MASK, INTRL2_CPU_MASK_SET);
 			__napi_schedule(&priv->napi);
 		}
 	}
-	if (priv->irq0_stat &
-			(UMAC_IRQ_TXDMA_BDONE | UMAC_IRQ_TXDMA_PDONE)) {
+	if (priv->irq0_stat & UMAC_TXDMA_DONE_MASK) {
 		/* Tx reclaim */
 		bcmgenet_tx_reclaim(priv->dev, &priv->tx_rings[DESC_INDEX]);
 	}
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH net-next 2/4] net: bcmgenet: add support for ethtool tx-frames
  2014-04-21 15:45 [PATCH net-next 0/4] net: bcmgenet: interrupt coalescing support Florian Fainelli
  2014-04-21 15:45 ` [PATCH net-next 1/4] net: bcmgenet: prepare for MBDONE interrupts support Florian Fainelli
@ 2014-04-21 15:45 ` Florian Fainelli
  2014-04-21 19:17   ` David Miller
  2014-04-21 15:45 ` [PATCH net-next 3/4] net: bcmgenet: add support for ethtool rx-frames Florian Fainelli
  2014-04-21 15:45 ` [PATCH net-next 4/4] net: bcmgenet: do not set RX buffer length Florian Fainelli
  3 siblings, 1 reply; 8+ messages in thread
From: Florian Fainelli @ 2014-04-21 15:45 UTC (permalink / raw)
  To: netdev; +Cc: davem, Florian Fainelli

Configuring the ethtool tx-frames property, which translates into N
packets before a TX interrupt is the simplest configuration scheme
because it requires no locking neither at the softare nor hardware
level, and is completely indepedent from the link speed. Since ethtool
does not allow per-tx queue coalescing parameters, we apply the same
setting to any transmit queue.

We can no longer enable the BDONE/PDONE interrupts as those would fire
for each packet/buffer received, which would defeat the MBDONE interrupt
purpose. The MBDONE interrupt is guaranteed to correspond to a
PDONE/BDONE interrupt when the threshold is set to 1.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 43 +++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index b093a48c427f..f1b3969de10b 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -78,11 +78,10 @@
 #define GENET_RDMA_REG_OFF	(priv->hw_params->rdma_offset + \
 				TOTAL_DESC * DMA_DESC_SIZE)
 
-/* Tx/Rx DMA done masks: packet, buffer and multiple buffer done */
-#define UMAC_TXDMA_DONE_MASK	(UMAC_IRQ_TXDMA_BDONE | \
-				 UMAC_IRQ_TXDMA_PDONE | \
-				 UMAC_IRQ_TXDMA_MBDONE)
+/* TX DMA done mask: multiple buffer done */
+#define UMAC_TXDMA_DONE_MASK	(UMAC_IRQ_TXDMA_MBDONE)
 
+/* Rx DMA done masks: packet, buffer and multiple buffer done */
 #define UMAC_RXDMA_DONE_MASK	(UMAC_IRQ_RXDMA_BDONE | \
 				 UMAC_IRQ_RXDMA_PDONE | \
 				 UMAC_IRQ_RXDMA_MBDONE)
@@ -495,6 +494,38 @@ static void bcmgenet_set_msglevel(struct net_device *dev, u32 level)
 	priv->msg_enable = level;
 }
 
+static int bcmgenet_get_coalesce(struct net_device *dev,
+				 struct ethtool_coalesce *ec)
+{
+	struct bcmgenet_priv *priv = netdev_priv(dev);
+
+	ec->tx_max_coalesced_frames = bcmgenet_tdma_ring_readl(priv,
+			DESC_INDEX, DMA_MBUF_DONE_THRESH);
+
+	return 0;
+}
+
+static int bcmgenet_set_coalesce(struct net_device *dev,
+				 struct ethtool_coalesce *ec)
+{
+	struct bcmgenet_priv *priv = netdev_priv(dev);
+	unsigned int i;
+
+	if (ec->tx_max_coalesced_frames > 255)
+		return -EINVAL;
+
+	/* Program all TX queues with the same values, as there is no
+	 * ethtool knob to do coalescing on a per-queue basis
+	 */
+	for (i = 0; i < priv->hw_params->tx_queues; i++)
+		bcmgenet_tdma_ring_writel(priv, i,
+			ec->tx_max_coalesced_frames, DMA_MBUF_DONE_THRESH);
+	bcmgenet_tdma_ring_writel(priv, DESC_INDEX,
+			ec->tx_max_coalesced_frames, DMA_MBUF_DONE_THRESH);
+
+	return 0;
+}
+
 /* standard ethtool support functions. */
 enum bcmgenet_stat_type {
 	BCMGENET_STAT_NETDEV = -1,
@@ -739,6 +770,8 @@ static struct ethtool_ops bcmgenet_ethtool_ops = {
 	.get_link		= ethtool_op_get_link,
 	.get_msglevel		= bcmgenet_get_msglevel,
 	.set_msglevel		= bcmgenet_set_msglevel,
+	.get_coalesce		= bcmgenet_get_coalesce,
+	.set_coalesce		= bcmgenet_set_coalesce,
 };
 
 /* Power down the unimac, based on mode. */
@@ -856,7 +889,7 @@ static inline void bcmgenet_tx_ring16_int_disable(struct bcmgenet_priv *priv,
 static inline void bcmgenet_tx_ring16_int_enable(struct bcmgenet_priv *priv,
 						 struct bcmgenet_tx_ring *ring)
 {
-	bcmgenet_intrl2_0_writel(priv, UMAC_TXDMA_DONE_MASK
+	bcmgenet_intrl2_0_writel(priv, UMAC_TXDMA_DONE_MASK,
 			INTRL2_CPU_MASK_CLEAR);
 }
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH net-next 3/4] net: bcmgenet: add support for ethtool rx-frames
  2014-04-21 15:45 [PATCH net-next 0/4] net: bcmgenet: interrupt coalescing support Florian Fainelli
  2014-04-21 15:45 ` [PATCH net-next 1/4] net: bcmgenet: prepare for MBDONE interrupts support Florian Fainelli
  2014-04-21 15:45 ` [PATCH net-next 2/4] net: bcmgenet: add support for ethtool tx-frames Florian Fainelli
@ 2014-04-21 15:45 ` Florian Fainelli
  2014-04-21 15:45 ` [PATCH net-next 4/4] net: bcmgenet: do not set RX buffer length Florian Fainelli
  3 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2014-04-21 15:45 UTC (permalink / raw)
  To: netdev; +Cc: davem, Florian Fainelli

Add support for the ethtool rx-frames coalescing parameter which allows
defining the number of RX interrupts per frames received. Make sure that
whenever the link speed changes, we also re-program a correct push timer
value.

We can no longer enable the BDONE/PDONE interrupts as those would
fire for each packet/buffer received, which would defeat the MBDONE
interrupt purpose. The MBDONE interrupt is guaranteed to correspond to a
PDONE/BDONE interrupt when the threshold is set to 1.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 44 ++++++++++++++++++++++----
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  1 +
 drivers/net/ethernet/broadcom/genet/bcmmii.c   |  3 ++
 3 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index f1b3969de10b..4922958ec4aa 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -78,13 +78,10 @@
 #define GENET_RDMA_REG_OFF	(priv->hw_params->rdma_offset + \
 				TOTAL_DESC * DMA_DESC_SIZE)
 
-/* TX DMA done mask: multiple buffer done */
+/* RX/TX DMA done mask: multiple buffer done */
 #define UMAC_TXDMA_DONE_MASK	(UMAC_IRQ_TXDMA_MBDONE)
 
-/* Rx DMA done masks: packet, buffer and multiple buffer done */
-#define UMAC_RXDMA_DONE_MASK	(UMAC_IRQ_RXDMA_BDONE | \
-				 UMAC_IRQ_RXDMA_PDONE | \
-				 UMAC_IRQ_RXDMA_MBDONE)
+#define UMAC_RXDMA_DONE_MASK	(UMAC_IRQ_RXDMA_MBDONE)
 
 static inline void dmadesc_set_length_status(struct bcmgenet_priv *priv,
 						void __iomem *d, u32 value)
@@ -210,6 +207,7 @@ enum dma_reg {
 	DMA_ARB_CTRL,
 	DMA_PRIORITY,
 	DMA_RING_PRIORITY,
+	DMA_RING16_TIMEOUT,
 };
 
 static const u8 bcmgenet_dma_regs_v3plus[] = {
@@ -220,6 +218,7 @@ static const u8 bcmgenet_dma_regs_v3plus[] = {
 	[DMA_ARB_CTRL]		= 0x2C,
 	[DMA_PRIORITY]		= 0x30,
 	[DMA_RING_PRIORITY]	= 0x38,
+	[DMA_RING16_TIMEOUT]	= 0x6C,
 };
 
 static const u8 bcmgenet_dma_regs_v2[] = {
@@ -230,6 +229,7 @@ static const u8 bcmgenet_dma_regs_v2[] = {
 	[DMA_ARB_CTRL]		= 0x30,
 	[DMA_PRIORITY]		= 0x34,
 	[DMA_RING_PRIORITY]	= 0x3C,
+	[DMA_RING16_TIMEOUT]	= 0x6C,
 };
 
 static const u8 bcmgenet_dma_regs_v1[] = {
@@ -239,6 +239,7 @@ static const u8 bcmgenet_dma_regs_v1[] = {
 	[DMA_ARB_CTRL]		= 0x30,
 	[DMA_PRIORITY]		= 0x34,
 	[DMA_RING_PRIORITY]	= 0x3C,
+	[DMA_RING16_TIMEOUT]	= 0x6C,
 };
 
 /* Set at runtime once bcmgenet version is known */
@@ -501,17 +502,42 @@ static int bcmgenet_get_coalesce(struct net_device *dev,
 
 	ec->tx_max_coalesced_frames = bcmgenet_tdma_ring_readl(priv,
 			DESC_INDEX, DMA_MBUF_DONE_THRESH);
+	ec->rx_max_coalesced_frames = bcmgenet_rdma_ring_readl(priv,
+			DESC_INDEX, DMA_MBUF_DONE_THRESH);
 
 	return 0;
 }
 
+/* Update RX coalescing scheme based on the current link speed, caller
+ * must have updated the ring threshold before calling us.
+ */
+void bcmgenet_update_rx_coal(struct net_device *dev)
+{
+	unsigned int speeds[] = { SPEED_10, SPEED_100, SPEED_1000, SPEED_2500 };
+	struct bcmgenet_priv *priv = netdev_priv(dev);
+	u32 cur_speed;
+	u32 timeout;
+	u32 rx_coal;
+
+	rx_coal = bcmgenet_rdma_ring_readl(priv, DESC_INDEX,
+			DMA_MBUF_DONE_THRESH);
+
+	cur_speed = (bcmgenet_umac_readl(priv, UMAC_CMD) >> CMD_SPEED_SHIFT) &
+			CMD_SPEED_MASK;
+
+	timeout = 2 * (rx_coal * ENET_MAX_MTU_SIZE) / speeds[cur_speed];
+	bcmgenet_rdma_writel(priv, timeout, DMA_RING16_TIMEOUT);
+}
+
 static int bcmgenet_set_coalesce(struct net_device *dev,
 				 struct ethtool_coalesce *ec)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	unsigned int i;
 
-	if (ec->tx_max_coalesced_frames > 255)
+	if (ec->tx_max_coalesced_frames > 255 ||
+	    ec->rx_max_coalesced_frames < 1 ||
+	    ec->rx_max_coalesced_frames > 255)
 		return -EINVAL;
 
 	/* Program all TX queues with the same values, as there is no
@@ -523,6 +549,11 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
 	bcmgenet_tdma_ring_writel(priv, DESC_INDEX,
 			ec->tx_max_coalesced_frames, DMA_MBUF_DONE_THRESH);
 
+	bcmgenet_rdma_ring_writel(priv, DESC_INDEX,
+			ec->rx_max_coalesced_frames, DMA_MBUF_DONE_THRESH);
+
+	bcmgenet_update_rx_coal(dev);
+
 	return 0;
 }
 
@@ -1680,6 +1711,7 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
 			(DMA_FC_THRESH_LO << DMA_XOFF_THRESHOLD_SHIFT) |
 			DMA_FC_THRESH_HI, RDMA_XON_XOFF_THRESH);
 	bcmgenet_rdma_ring_writel(priv, index, 0, RDMA_READ_PTR);
+	bcmgenet_rdma_ring_writel(priv, index, 1, DMA_MBUF_DONE_THRESH);
 
 	return ret;
 }
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 0f117105fed1..e9c17dcbc9ab 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -624,5 +624,6 @@ int bcmgenet_mii_init(struct net_device *dev);
 int bcmgenet_mii_config(struct net_device *dev);
 void bcmgenet_mii_exit(struct net_device *dev);
 void bcmgenet_mii_reset(struct net_device *dev);
+void bcmgenet_update_rx_coal(struct net_device *dev);
 
 #endif /* __BCMGENET_H__ */
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 4608673beaff..fd2f9ba70977 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -143,6 +143,9 @@ static void bcmgenet_mii_setup(struct net_device *dev)
 			       CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE);
 		reg |= cmd_bits;
 		bcmgenet_umac_writel(priv, reg, UMAC_CMD);
+
+		/* Update coalescing scheme based on current speed */
+		bcmgenet_update_rx_coal(priv->dev);
 	}
 
 	if (status_changed)
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH net-next 4/4] net: bcmgenet: do not set RX buffer length
  2014-04-21 15:45 [PATCH net-next 0/4] net: bcmgenet: interrupt coalescing support Florian Fainelli
                   ` (2 preceding siblings ...)
  2014-04-21 15:45 ` [PATCH net-next 3/4] net: bcmgenet: add support for ethtool rx-frames Florian Fainelli
@ 2014-04-21 15:45 ` Florian Fainelli
  3 siblings, 0 replies; 8+ messages in thread
From: Florian Fainelli @ 2014-04-21 15:45 UTC (permalink / raw)
  To: netdev; +Cc: davem, Florian Fainelli

There is no need to set the buffer length during RX buffer allocation
since the hardware will fill that information when the packet is
received, so we are basically forcing a value that will get overwritten.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 4922958ec4aa..e15411431c9b 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1479,13 +1479,6 @@ static int bcmgenet_alloc_rx_buffers(struct bcmgenet_priv *priv)
 		if (cb->skb)
 			continue;
 
-		/* set the DMA descriptor length once and for all
-		 * it will only change if we support dynamically sizing
-		 * priv->rx_buf_len, but we do not
-		 */
-		dmadesc_set_length_status(priv, priv->rx_bd_assign_ptr,
-				priv->rx_buf_len << DMA_BUFLENGTH_SHIFT);
-
 		ret = bcmgenet_rx_refill(priv, cb);
 		if (ret)
 			break;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next 2/4] net: bcmgenet: add support for ethtool tx-frames
  2014-04-21 15:45 ` [PATCH net-next 2/4] net: bcmgenet: add support for ethtool tx-frames Florian Fainelli
@ 2014-04-21 19:17   ` David Miller
  2014-04-21 19:55     ` Florian Fainelli
  0 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2014-04-21 19:17 UTC (permalink / raw)
  To: f.fainelli; +Cc: netdev

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 21 Apr 2014 08:45:22 -0700

> Configuring the ethtool tx-frames property, which translates into N
> packets before a TX interrupt is the simplest configuration scheme
> because it requires no locking neither at the softare nor hardware
> level, and is completely indepedent from the link speed. Since ethtool
> does not allow per-tx queue coalescing parameters, we apply the same
> setting to any transmit queue.
> 
> We can no longer enable the BDONE/PDONE interrupts as those would fire
> for each packet/buffer received, which would defeat the MBDONE interrupt
> purpose. The MBDONE interrupt is guaranteed to correspond to a
> PDONE/BDONE interrupt when the threshold is set to 1.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Does the MBDONE scheme have a timeout?

For example if you ask for a MBDONE setting where N=4, if only 2 packets
arrive will you get an interrupt or will it wait for 2 more to arrive
no matter what?

If a timeout doesn't exist, you cannot use this.

I'm very pessimistic because I see no inspection of the timeout
parameter passed into the ethtool commands.  And in fact if the
timeout does exist, and is fixed, you should error on non-zero
values specified in the timeout field(s).

So no matter what the situation is wrt. timeouts, this series needs
either change or be completely tossed.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next 2/4] net: bcmgenet: add support for ethtool tx-frames
  2014-04-21 19:17   ` David Miller
@ 2014-04-21 19:55     ` Florian Fainelli
  2014-04-21 19:59       ` David Miller
  0 siblings, 1 reply; 8+ messages in thread
From: Florian Fainelli @ 2014-04-21 19:55 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

2014-04-21 12:17 GMT-07:00 David Miller <davem@davemloft.net>:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Mon, 21 Apr 2014 08:45:22 -0700
>
>> Configuring the ethtool tx-frames property, which translates into N
>> packets before a TX interrupt is the simplest configuration scheme
>> because it requires no locking neither at the softare nor hardware
>> level, and is completely indepedent from the link speed. Since ethtool
>> does not allow per-tx queue coalescing parameters, we apply the same
>> setting to any transmit queue.
>>
>> We can no longer enable the BDONE/PDONE interrupts as those would fire
>> for each packet/buffer received, which would defeat the MBDONE interrupt
>> purpose. The MBDONE interrupt is guaranteed to correspond to a
>> PDONE/BDONE interrupt when the threshold is set to 1.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>
> Does the MBDONE scheme have a timeout?
>
> For example if you ask for a MBDONE setting where N=4, if only 2 packets
> arrive will you get an interrupt or will it wait for 2 more to arrive
> no matter what?

There is no configurable timeout for the transmit DMA engine (receive
does have one), but we do get a ring empty interrupt as soon as the
transmit path has completed the packets, so the MBDONE interrupt cause
always makes us run the TX reclaim logic.

>
> If a timeout doesn't exist, you cannot use this.
>
> I'm very pessimistic because I see no inspection of the timeout
> parameter passed into the ethtool commands.  And in fact if the
> timeout does exist, and is fixed, you should error on non-zero
> values specified in the timeout field(s).
>
> So no matter what the situation is wrt. timeouts, this series needs
> either change or be completely tossed.

I will rework it, thanks for your feedback.
-- 
Florian

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next 2/4] net: bcmgenet: add support for ethtool tx-frames
  2014-04-21 19:55     ` Florian Fainelli
@ 2014-04-21 19:59       ` David Miller
  0 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2014-04-21 19:59 UTC (permalink / raw)
  To: f.fainelli; +Cc: netdev

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 21 Apr 2014 12:55:12 -0700

> 2014-04-21 12:17 GMT-07:00 David Miller <davem@davemloft.net>:
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Date: Mon, 21 Apr 2014 08:45:22 -0700
>>
>>> Configuring the ethtool tx-frames property, which translates into N
>>> packets before a TX interrupt is the simplest configuration scheme
>>> because it requires no locking neither at the softare nor hardware
>>> level, and is completely indepedent from the link speed. Since ethtool
>>> does not allow per-tx queue coalescing parameters, we apply the same
>>> setting to any transmit queue.
>>>
>>> We can no longer enable the BDONE/PDONE interrupts as those would fire
>>> for each packet/buffer received, which would defeat the MBDONE interrupt
>>> purpose. The MBDONE interrupt is guaranteed to correspond to a
>>> PDONE/BDONE interrupt when the threshold is set to 1.
>>>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>
>> Does the MBDONE scheme have a timeout?
>>
>> For example if you ask for a MBDONE setting where N=4, if only 2 packets
>> arrive will you get an interrupt or will it wait for 2 more to arrive
>> no matter what?
> 
> There is no configurable timeout for the transmit DMA engine (receive
> does have one), but we do get a ring empty interrupt as soon as the
> transmit path has completed the packets, so the MBDONE interrupt cause
> always makes us run the TX reclaim logic.

This latency may be too large for TCP.

In particular things like TCP Small Queues need timely feedback for
transmitted packets.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-04-21 19:59 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-21 15:45 [PATCH net-next 0/4] net: bcmgenet: interrupt coalescing support Florian Fainelli
2014-04-21 15:45 ` [PATCH net-next 1/4] net: bcmgenet: prepare for MBDONE interrupts support Florian Fainelli
2014-04-21 15:45 ` [PATCH net-next 2/4] net: bcmgenet: add support for ethtool tx-frames Florian Fainelli
2014-04-21 19:17   ` David Miller
2014-04-21 19:55     ` Florian Fainelli
2014-04-21 19:59       ` David Miller
2014-04-21 15:45 ` [PATCH net-next 3/4] net: bcmgenet: add support for ethtool rx-frames Florian Fainelli
2014-04-21 15:45 ` [PATCH net-next 4/4] net: bcmgenet: do not set RX buffer length Florian Fainelli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).