[PATCH 0/4] net: cpsw: fix hangs and improve IRQ handling

[PATCH 0/4] net: cpsw: fix hangs and improve IRQ handling

[Felipe Balbi]

Hi folks,

patch 1 fixes the bug reported by Yegor Yefremov <yegorslists@googlemail.com>.

patches 2 - 4 improve IRQ handling a little bit.

Tested with BeagleBone Black and AM437x SK. The bug fix has been
tested for almost 3 days non-stop while the following patches
have been tested for a couple of hours with iperf and nuttcp and
NFS root.

Note that we also have a slight throughput improvement after patch 3
when running with AM437x SK (10.0.1.13 below):

===== pre-patch =====

$ iperf -c 10.0.1.13
------------------------------------------------------------
Client connecting to 10.0.1.13, TCP port 5001
TCP window size: 85.0 KByte (default)
------------------------------------------------------------
[  3] local 10.0.1.2 port 38430 connected with 10.0.1.13 port 5001
[ ID] Interval       Transfer     Bandwidth
[  3]  0.0-10.0 sec   240 MBytes   201 Mbits/sec
$ iperf -c 10.0.1.101
------------------------------------------------------------
Client connecting to 10.0.1.101, TCP port 5001
TCP window size: 85.0 KByte (default)
------------------------------------------------------------
[  3] local 10.0.1.2 port 35143 connected with 10.0.1.101 port 5001
[ ID] Interval       Transfer     Bandwidth
[  3]  0.0-10.0 sec   113 MBytes  94.7 Mbits/sec


===== post-patch =====

$ iperf -c 10.0.1.13
------------------------------------------------------------
Client connecting to 10.0.1.13, TCP port 5001
TCP window size: 85.0 KByte (default)
------------------------------------------------------------
[  3] local 10.0.1.2 port 38447 connected with 10.0.1.13 port 5001
[ ID] Interval       Transfer     Bandwidth
[  3]  0.0-10.0 sec   283 MBytes   237 Mbits/sec
$ iperf -c 10.0.1.101
------------------------------------------------------------
Client connecting to 10.0.1.101, TCP port 5001
TCP window size: 85.0 KByte (default)
------------------------------------------------------------
[  3] local 10.0.1.2 port 35157 connected with 10.0.1.101 port 5001
[ ID] Interval       Transfer     Bandwidth
[  3]  0.0-10.0 sec   113 MBytes  94.4 Mbits/sec

That minor decrease on beagleboneblack (10.0.1.101) is so small that's
likely into the error margin, but I'll run some further profiling as
I have a feeling INTC's IRQ handling can be improved.

In any case, patch 1 should go in during the -rc an get backported
all the way back to v3.9, while the other patches can (should) be
delayed for v3.20 merge window.

Felipe Balbi (4):
  net: ethernet: cpsw: fix hangs with interrupts
  net: ethernet: cpsw: unroll IRQ request loop
  net: ethernet: cpsw: split out IRQ handler
  net: ethernet: cpsw: don't requests IRQs we don't use

 drivers/net/ethernet/ti/cpsw.c | 81 +++++++++++++++++++++++++++---------------
 1 file changed, 52 insertions(+), 29 deletions(-)

-- 
2.2.0

[PATCH 1/4] net: ethernet: cpsw: fix hangs with interrupts

[Felipe Balbi]

The CPSW IP implements pulse-signaled interrupts. Due to
that we must write a correct, pre-defined value to the
CPDMA_MACEOIVECTOR register so the controller generates
a pulse on the correct IRQ line to signal the End Of
Interrupt.

The way the driver is written today, all four IRQ lines
are requested using the same IRQ handler and, because of
that, we could fall into situations where a TX IRQ fires
but we tell the controller that we ended an RX IRQ (or
vice-versa). This situation triggers an IRQ storm on the
reserved IRQ 127 of INTC which will in turn call ack_bad_irq()
which will, then, print a ton of:

	unexpected IRQ trap at vector 00

In order to fix the problem, we are moving all calls to
cpdma_ctlr_eoi() inside the IRQ handler and making sure
we *always* write the correct value to the CPDMA_MACEOIVECTOR
register. Note that the algorithm assumes that IRQ numbers and
value-to-be-written-to-EOI are proportional, meaning that a
write of value 0 would trigger an EOI pulse for the RX_THRESHOLD
Interrupt and that's the IRQ number sitting in the 0-th index
of our irqs_table array.

This, however, is safe at least for current implementations of
CPSW so we will refrain from making the check smarter (and, as
a side-effect, slower) until we actually have a platform where
IRQ lines are swapped.

This patch has been tested for several days with AM335x- and
AM437x-based platforms. AM57x was left out because there are
still pending patches to enable ethernet in mainline for that
platform. A read of the TRM confirms the statement on previous
paragraph.

Reported-by: Yegor Yefremov <yegorslists@googlemail.com>
Fixes: 510a1e7 (drivers: net: davinci_cpdma: acknowledge interrupt properly)
Cc: <stable@vger.kernel.org> # v3.9+
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/net/ethernet/ti/cpsw.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index c560f9a..e61ee83 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -757,6 +757,14 @@ requeue:
 static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
 {
 	struct cpsw_priv *priv = dev_id;
+	int value = irq - priv->irqs_table[0];
+
+	/* NOTICE: Ending IRQ here. The trick with the 'value' variable above
+	 * is to make sure we will always write the correct value to the EOI
+	 * register. Namely 0 for RX_THRESH Interrupt, 1 for RX Interrupt, 2
+	 * for TX Interrupt and 3 for MISC Interrupt.
+	 */
+	cpdma_ctlr_eoi(priv->dma, value);
 
 	cpsw_intr_disable(priv);
 	if (priv->irq_enabled == true) {
@@ -786,8 +794,6 @@ static int cpsw_poll(struct napi_struct *napi, int budget)
 	int			num_tx, num_rx;
 
 	num_tx = cpdma_chan_process(priv->txch, 128);
-	if (num_tx)
-		cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX);
 
 	num_rx = cpdma_chan_process(priv->rxch, budget);
 	if (num_rx < budget) {
@@ -795,7 +801,6 @@ static int cpsw_poll(struct napi_struct *napi, int budget)
 
 		napi_complete(napi);
 		cpsw_intr_enable(priv);
-		cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_RX);
 		prim_cpsw = cpsw_get_slave_priv(priv, 0);
 		if (prim_cpsw->irq_enabled == false) {
 			prim_cpsw->irq_enabled = true;
@@ -1310,8 +1315,6 @@ static int cpsw_ndo_open(struct net_device *ndev)
 	napi_enable(&priv->napi);
 	cpdma_ctlr_start(priv->dma);
 	cpsw_intr_enable(priv);
-	cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_RX);
-	cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX);
 
 	prim_cpsw = cpsw_get_slave_priv(priv, 0);
 	if (prim_cpsw->irq_enabled == false) {
@@ -1578,9 +1581,6 @@ static void cpsw_ndo_tx_timeout(struct net_device *ndev)
 	cpdma_chan_start(priv->txch);
 	cpdma_ctlr_int_ctrl(priv->dma, true);
 	cpsw_intr_enable(priv);
-	cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_RX);
-	cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX);
-
 }
 
 static int cpsw_ndo_set_mac_address(struct net_device *ndev, void *p)
@@ -1620,9 +1620,6 @@ static void cpsw_ndo_poll_controller(struct net_device *ndev)
 	cpsw_interrupt(ndev->irq, priv);
 	cpdma_ctlr_int_ctrl(priv->dma, true);
 	cpsw_intr_enable(priv);
-	cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_RX);
-	cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX);
-
 }
 #endif
 
-- 
2.2.0

[PATCH 2/4] net: ethernet: cpsw: unroll IRQ request loop

[Felipe Balbi]

This patch is in preparation for a nicer IRQ
handling scheme where we use different IRQ
handlers for each IRQ line (as it should be).

Later, we will also drop IRQs offset 0 and 3
because they are always disabled in this driver.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/net/ethernet/ti/cpsw.c | 62 ++++++++++++++++++++++++++++++++----------
 1 file changed, 47 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index e61ee83..6e04128 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2156,7 +2156,8 @@ static int cpsw_probe(struct platform_device *pdev)
 	void __iomem			*ss_regs;
 	struct resource			*res, *ss_res;
 	u32 slave_offset, sliver_offset, slave_size;
-	int ret = 0, i, k = 0;
+	int ret = 0, i;
+	int irq;
 
 	ndev = alloc_etherdev(sizeof(struct cpsw_priv));
 	if (!ndev) {
@@ -2345,24 +2346,55 @@ static int cpsw_probe(struct platform_device *pdev)
 		goto clean_ale_ret;
 	}
 
-	while ((res = platform_get_resource(priv->pdev, IORESOURCE_IRQ, k))) {
-		if (k >= ARRAY_SIZE(priv->irqs_table)) {
-			ret = -EINVAL;
-			goto clean_ale_ret;
-		}
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		goto clean_ale_ret;
 
-		ret = devm_request_irq(&pdev->dev, res->start, cpsw_interrupt,
-				       0, dev_name(&pdev->dev), priv);
-		if (ret < 0) {
-			dev_err(priv->dev, "error attaching irq (%d)\n", ret);
-			goto clean_ale_ret;
-		}
+	priv->irqs_table[0] = irq;
+	ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
+			0, dev_name(&pdev->dev), priv);
+	if (ret < 0) {
+		dev_err(priv->dev, "error attaching irq (%d)\n", ret);
+		goto clean_ale_ret;
+	}
 
-		priv->irqs_table[k] = res->start;
-		k++;
+	irq = platform_get_irq(pdev, 1);
+	if (irq < 0)
+		goto clean_ale_ret;
+
+	priv->irqs_table[1] = irq;
+	ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
+			0, dev_name(&pdev->dev), priv);
+	if (ret < 0) {
+		dev_err(priv->dev, "error attaching irq (%d)\n", ret);
+		goto clean_ale_ret;
+	}
+
+	irq = platform_get_irq(pdev, 2);
+	if (irq < 0)
+		goto clean_ale_ret;
+
+	priv->irqs_table[2] = irq;
+	ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
+			0, dev_name(&pdev->dev), priv);
+	if (ret < 0) {
+		dev_err(priv->dev, "error attaching irq (%d)\n", ret);
+		goto clean_ale_ret;
+	}
+
+	irq = platform_get_irq(pdev, 3);
+	if (irq < 0)
+		goto clean_ale_ret;
+
+	priv->irqs_table[3] = irq;
+	ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
+			0, dev_name(&pdev->dev), priv);
+	if (ret < 0) {
+		dev_err(priv->dev, "error attaching irq (%d)\n", ret);
+		goto clean_ale_ret;
 	}
 
-	priv->num_irqs = k;
+	priv->num_irqs = 4;
 
 	ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
 
-- 
2.2.0

[PATCH 3/4] net: ethernet: cpsw: split out IRQ handler

[Felipe Balbi]

Now we can introduce dedicated IRQ handlers
for each of the IRQ events. This helps with
cleaning up a little bit of the clutter in
cpsw_interrupt() while also making sure that
TX IRQs will try to handle TX buffers while
RX IRQs will try to handle RX buffers.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/net/ethernet/ti/cpsw.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 6e04128..c9081bd 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -754,18 +754,36 @@ requeue:
 		dev_kfree_skb_any(new_skb);
 }
 
-static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
+static irqreturn_t cpsw_dummy_interrupt(int irq, void *dev_id)
 {
 	struct cpsw_priv *priv = dev_id;
 	int value = irq - priv->irqs_table[0];
 
-	/* NOTICE: Ending IRQ here. The trick with the 'value' variable above
-	 * is to make sure we will always write the correct value to the EOI
-	 * register. Namely 0 for RX_THRESH Interrupt, 1 for RX Interrupt, 2
-	 * for TX Interrupt and 3 for MISC Interrupt.
-	 */
 	cpdma_ctlr_eoi(priv->dma, value);
 
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t cpsw_tx_interrupt(int irq, void *dev_id)
+{
+	struct cpsw_priv *priv = dev_id;
+
+	cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX);
+	cpdma_chan_process(priv->txch, 128);
+
+	priv = cpsw_get_slave_priv(priv, 1);
+	if (priv)
+		cpdma_chan_process(priv->txch, 128);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t cpsw_rx_interrupt(int irq, void *dev_id)
+{
+	struct cpsw_priv *priv = dev_id;
+
+	cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_RX);
+
 	cpsw_intr_disable(priv);
 	if (priv->irq_enabled == true) {
 		cpsw_disable_irq(priv);
@@ -1617,7 +1635,8 @@ static void cpsw_ndo_poll_controller(struct net_device *ndev)
 
 	cpsw_intr_disable(priv);
 	cpdma_ctlr_int_ctrl(priv->dma, false);
-	cpsw_interrupt(ndev->irq, priv);
+	cpsw_rx_interrupt(priv->irq[1], priv);
+	cpsw_tx_interrupt(priv->irq[2], priv);
 	cpdma_ctlr_int_ctrl(priv->dma, true);
 	cpsw_intr_enable(priv);
 }
@@ -2351,7 +2370,7 @@ static int cpsw_probe(struct platform_device *pdev)
 		goto clean_ale_ret;
 
 	priv->irqs_table[0] = irq;
-	ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
+	ret = devm_request_irq(&pdev->dev, irq, cpsw_dummy_interrupt,
 			0, dev_name(&pdev->dev), priv);
 	if (ret < 0) {
 		dev_err(priv->dev, "error attaching irq (%d)\n", ret);
@@ -2363,7 +2382,7 @@ static int cpsw_probe(struct platform_device *pdev)
 		goto clean_ale_ret;
 
 	priv->irqs_table[1] = irq;
-	ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
+	ret = devm_request_irq(&pdev->dev, irq, cpsw_rx_interrupt,
 			0, dev_name(&pdev->dev), priv);
 	if (ret < 0) {
 		dev_err(priv->dev, "error attaching irq (%d)\n", ret);
@@ -2375,7 +2394,7 @@ static int cpsw_probe(struct platform_device *pdev)
 		goto clean_ale_ret;
 
 	priv->irqs_table[2] = irq;
-	ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
+	ret = devm_request_irq(&pdev->dev, irq, cpsw_tx_interrupt,
 			0, dev_name(&pdev->dev), priv);
 	if (ret < 0) {
 		dev_err(priv->dev, "error attaching irq (%d)\n", ret);
@@ -2387,7 +2406,7 @@ static int cpsw_probe(struct platform_device *pdev)
 		goto clean_ale_ret;
 
 	priv->irqs_table[3] = irq;
-	ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
+	ret = devm_request_irq(&pdev->dev, irq, cpsw_dummy_interrupt,
 			0, dev_name(&pdev->dev), priv);
 	if (ret < 0) {
 		dev_err(priv->dev, "error attaching irq (%d)\n", ret);
-- 
2.2.0

[PATCH 4/4] net: ethernet: cpsw: don't requests IRQs we don't use

[Felipe Balbi]

CPSW never uses RX_THRESHOLD or MISC interrupts. In
fact, they are always kept masked in their appropriate
IRQ Enable register.

Instead of allocating an IRQ that never fires, it's best
to remove that code altogether and let future patches
implement it if anybody needs those.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/net/ethernet/ti/cpsw.c | 55 ++++++++++++------------------------------
 1 file changed, 15 insertions(+), 40 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index c9081bd..fd0acd9 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -754,16 +754,6 @@ requeue:
 		dev_kfree_skb_any(new_skb);
 }
 
-static irqreturn_t cpsw_dummy_interrupt(int irq, void *dev_id)
-{
-	struct cpsw_priv *priv = dev_id;
-	int value = irq - priv->irqs_table[0];
-
-	cpdma_ctlr_eoi(priv->dma, value);
-
-	return IRQ_HANDLED;
-}
-
 static irqreturn_t cpsw_tx_interrupt(int irq, void *dev_id)
 {
 	struct cpsw_priv *priv = dev_id;
@@ -1635,8 +1625,8 @@ static void cpsw_ndo_poll_controller(struct net_device *ndev)
 
 	cpsw_intr_disable(priv);
 	cpdma_ctlr_int_ctrl(priv->dma, false);
-	cpsw_rx_interrupt(priv->irq[1], priv);
-	cpsw_tx_interrupt(priv->irq[2], priv);
+	cpsw_rx_interrupt(priv->irq[0], priv);
+	cpsw_tx_interrupt(priv->irq[1], priv);
 	cpdma_ctlr_int_ctrl(priv->dma, true);
 	cpsw_intr_enable(priv);
 }
@@ -2358,30 +2348,27 @@ static int cpsw_probe(struct platform_device *pdev)
 		goto clean_dma_ret;
 	}
 
-	ndev->irq = platform_get_irq(pdev, 0);
+	ndev->irq = platform_get_irq(pdev, 1);
 	if (ndev->irq < 0) {
 		dev_err(priv->dev, "error getting irq resource\n");
 		ret = -ENOENT;
 		goto clean_ale_ret;
 	}
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		goto clean_ale_ret;
-
-	priv->irqs_table[0] = irq;
-	ret = devm_request_irq(&pdev->dev, irq, cpsw_dummy_interrupt,
-			0, dev_name(&pdev->dev), priv);
-	if (ret < 0) {
-		dev_err(priv->dev, "error attaching irq (%d)\n", ret);
-		goto clean_ale_ret;
-	}
+	/* Grab RX and TX IRQs. Note that we also have RX_THRESHOLD and
+	 * MISC IRQs which are always kept disabled with this driver so
+	 * we will not request them.
+	 *
+	 * If anyone wants to implement support for those, make sure to
+	 * first request and append them to irqs_table array.
+	 */
 
+	/* RX IRQ */
 	irq = platform_get_irq(pdev, 1);
 	if (irq < 0)
 		goto clean_ale_ret;
 
-	priv->irqs_table[1] = irq;
+	priv->irqs_table[0] = irq;
 	ret = devm_request_irq(&pdev->dev, irq, cpsw_rx_interrupt,
 			0, dev_name(&pdev->dev), priv);
 	if (ret < 0) {
@@ -2389,31 +2376,19 @@ static int cpsw_probe(struct platform_device *pdev)
 		goto clean_ale_ret;
 	}
 
+	/* TX IRQ */
 	irq = platform_get_irq(pdev, 2);
 	if (irq < 0)
 		goto clean_ale_ret;
 
-	priv->irqs_table[2] = irq;
+	priv->irqs_table[1] = irq;
 	ret = devm_request_irq(&pdev->dev, irq, cpsw_tx_interrupt,
 			0, dev_name(&pdev->dev), priv);
 	if (ret < 0) {
 		dev_err(priv->dev, "error attaching irq (%d)\n", ret);
 		goto clean_ale_ret;
 	}
-
-	irq = platform_get_irq(pdev, 3);
-	if (irq < 0)
-		goto clean_ale_ret;
-
-	priv->irqs_table[3] = irq;
-	ret = devm_request_irq(&pdev->dev, irq, cpsw_dummy_interrupt,
-			0, dev_name(&pdev->dev), priv);
-	if (ret < 0) {
-		dev_err(priv->dev, "error attaching irq (%d)\n", ret);
-		goto clean_ale_ret;
-	}
-
-	priv->num_irqs = 4;
+	priv->num_irqs = 2;
 
 	ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
 
-- 
2.2.0

Re: [PATCH 3/4] net: ethernet: cpsw: split out IRQ handler

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5700 bytes --]

Hi,

On Fri, Jan 02, 2015 at 10:49:49AM -0800, Dave Taht wrote:
> +1.
> 
> We'd had a thread on netdev (can't find it now) where we discussed
> adding BQL support and also something saner for the NAPI handling to
> this driver.

yeah, currently is completely borked. I'm on a gigabit network and I'm
getting 94Mbits/sec, total crap.

> Initial results for the beaglebone black were pretty spectacular, and
> it does look like this is way cleaner infrastructure underneat th deal
> with. Are you testing

cool, if I new more about networking I'd certainly help, but I can help
testing for sure, just keep me in Cc ;-)

> on the beaglebone black.? do you remember that convo?

yeah, testing on beagleboneblack and AM437x SK.

cheers

> On Fri, Jan 2, 2015 at 10:10 AM, Felipe Balbi <balbi@ti.com> wrote:
> > Now we can introduce dedicated IRQ handlers
> > for each of the IRQ events. This helps with
> > cleaning up a little bit of the clutter in
> > cpsw_interrupt() while also making sure that
> > TX IRQs will try to handle TX buffers while
> > RX IRQs will try to handle RX buffers.
> >
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> >  drivers/net/ethernet/ti/cpsw.c | 41 ++++++++++++++++++++++++++++++-----------
> >  1 file changed, 30 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> > index 6e04128..c9081bd 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -754,18 +754,36 @@ requeue:
> >                 dev_kfree_skb_any(new_skb);
> >  }
> >
> > -static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
> > +static irqreturn_t cpsw_dummy_interrupt(int irq, void *dev_id)
> >  {
> >         struct cpsw_priv *priv = dev_id;
> >         int value = irq - priv->irqs_table[0];
> >
> > -       /* NOTICE: Ending IRQ here. The trick with the 'value' variable above
> > -        * is to make sure we will always write the correct value to the EOI
> > -        * register. Namely 0 for RX_THRESH Interrupt, 1 for RX Interrupt, 2
> > -        * for TX Interrupt and 3 for MISC Interrupt.
> > -        */
> >         cpdma_ctlr_eoi(priv->dma, value);
> >
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t cpsw_tx_interrupt(int irq, void *dev_id)
> > +{
> > +       struct cpsw_priv *priv = dev_id;
> > +
> > +       cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX);
> > +       cpdma_chan_process(priv->txch, 128);
> > +
> > +       priv = cpsw_get_slave_priv(priv, 1);
> > +       if (priv)
> > +               cpdma_chan_process(priv->txch, 128);
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t cpsw_rx_interrupt(int irq, void *dev_id)
> > +{
> > +       struct cpsw_priv *priv = dev_id;
> > +
> > +       cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_RX);
> > +
> >         cpsw_intr_disable(priv);
> >         if (priv->irq_enabled == true) {
> >                 cpsw_disable_irq(priv);
> > @@ -1617,7 +1635,8 @@ static void cpsw_ndo_poll_controller(struct net_device *ndev)
> >
> >         cpsw_intr_disable(priv);
> >         cpdma_ctlr_int_ctrl(priv->dma, false);
> > -       cpsw_interrupt(ndev->irq, priv);
> > +       cpsw_rx_interrupt(priv->irq[1], priv);
> > +       cpsw_tx_interrupt(priv->irq[2], priv);
> >         cpdma_ctlr_int_ctrl(priv->dma, true);
> >         cpsw_intr_enable(priv);
> >  }
> > @@ -2351,7 +2370,7 @@ static int cpsw_probe(struct platform_device *pdev)
> >                 goto clean_ale_ret;
> >
> >         priv->irqs_table[0] = irq;
> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_dummy_interrupt,
> >                         0, dev_name(&pdev->dev), priv);
> >         if (ret < 0) {
> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
> > @@ -2363,7 +2382,7 @@ static int cpsw_probe(struct platform_device *pdev)
> >                 goto clean_ale_ret;
> >
> >         priv->irqs_table[1] = irq;
> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_rx_interrupt,
> >                         0, dev_name(&pdev->dev), priv);
> >         if (ret < 0) {
> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
> > @@ -2375,7 +2394,7 @@ static int cpsw_probe(struct platform_device *pdev)
> >                 goto clean_ale_ret;
> >
> >         priv->irqs_table[2] = irq;
> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_tx_interrupt,
> >                         0, dev_name(&pdev->dev), priv);
> >         if (ret < 0) {
> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
> > @@ -2387,7 +2406,7 @@ static int cpsw_probe(struct platform_device *pdev)
> >                 goto clean_ale_ret;
> >
> >         priv->irqs_table[3] = irq;
> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_dummy_interrupt,
> >                         0, dev_name(&pdev->dev), priv);
> >         if (ret < 0) {
> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
> > --
> > 2.2.0
> >
> > --
> > 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
> 
> 
> 
> -- 
> Dave Täht
> 
> thttp://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/4] net: ethernet: cpsw: split out IRQ handler

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 6806 bytes --]

Hi,

(please use reply-all to keep mailing lists in Cc, also avoid
top-posting)

On Fri, Jan 02, 2015 at 10:58:29AM -0800, Dave Taht wrote:
> The beaglebone only has a 100mbit phy, so you aren't going to get more
> than that.

very true :-) Still, with AM437x SK which is definitely GigE, I'm
getting 201Mbits/sec.

> (so do a lot of IoT devices).
> 
> So you have the two patches that went by on BQL and on NAPI for the beagle?

no, got any pointers ?

> On Fri, Jan 2, 2015 at 10:55 AM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Fri, Jan 02, 2015 at 10:49:49AM -0800, Dave Taht wrote:
> >> +1.
> >>
> >> We'd had a thread on netdev (can't find it now) where we discussed
> >> adding BQL support and also something saner for the NAPI handling to
> >> this driver.
> >
> > yeah, currently is completely borked. I'm on a gigabit network and I'm
> > getting 94Mbits/sec, total crap.
> >
> >> Initial results for the beaglebone black were pretty spectacular, and
> >> it does look like this is way cleaner infrastructure underneat th deal
> >> with. Are you testing
> >
> > cool, if I new more about networking I'd certainly help, but I can help
> > testing for sure, just keep me in Cc ;-)
> >
> >> on the beaglebone black.? do you remember that convo?
> >
> > yeah, testing on beagleboneblack and AM437x SK.
> >
> > cheers
> >
> >> On Fri, Jan 2, 2015 at 10:10 AM, Felipe Balbi <balbi@ti.com> wrote:
> >> > Now we can introduce dedicated IRQ handlers
> >> > for each of the IRQ events. This helps with
> >> > cleaning up a little bit of the clutter in
> >> > cpsw_interrupt() while also making sure that
> >> > TX IRQs will try to handle TX buffers while
> >> > RX IRQs will try to handle RX buffers.
> >> >
> >> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> >> > ---
> >> >  drivers/net/ethernet/ti/cpsw.c | 41 ++++++++++++++++++++++++++++++-----------
> >> >  1 file changed, 30 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> >> > index 6e04128..c9081bd 100644
> >> > --- a/drivers/net/ethernet/ti/cpsw.c
> >> > +++ b/drivers/net/ethernet/ti/cpsw.c
> >> > @@ -754,18 +754,36 @@ requeue:
> >> >                 dev_kfree_skb_any(new_skb);
> >> >  }
> >> >
> >> > -static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
> >> > +static irqreturn_t cpsw_dummy_interrupt(int irq, void *dev_id)
> >> >  {
> >> >         struct cpsw_priv *priv = dev_id;
> >> >         int value = irq - priv->irqs_table[0];
> >> >
> >> > -       /* NOTICE: Ending IRQ here. The trick with the 'value' variable above
> >> > -        * is to make sure we will always write the correct value to the EOI
> >> > -        * register. Namely 0 for RX_THRESH Interrupt, 1 for RX Interrupt, 2
> >> > -        * for TX Interrupt and 3 for MISC Interrupt.
> >> > -        */
> >> >         cpdma_ctlr_eoi(priv->dma, value);
> >> >
> >> > +       return IRQ_HANDLED;
> >> > +}
> >> > +
> >> > +static irqreturn_t cpsw_tx_interrupt(int irq, void *dev_id)
> >> > +{
> >> > +       struct cpsw_priv *priv = dev_id;
> >> > +
> >> > +       cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX);
> >> > +       cpdma_chan_process(priv->txch, 128);
> >> > +
> >> > +       priv = cpsw_get_slave_priv(priv, 1);
> >> > +       if (priv)
> >> > +               cpdma_chan_process(priv->txch, 128);
> >> > +
> >> > +       return IRQ_HANDLED;
> >> > +}
> >> > +
> >> > +static irqreturn_t cpsw_rx_interrupt(int irq, void *dev_id)
> >> > +{
> >> > +       struct cpsw_priv *priv = dev_id;
> >> > +
> >> > +       cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_RX);
> >> > +
> >> >         cpsw_intr_disable(priv);
> >> >         if (priv->irq_enabled == true) {
> >> >                 cpsw_disable_irq(priv);
> >> > @@ -1617,7 +1635,8 @@ static void cpsw_ndo_poll_controller(struct net_device *ndev)
> >> >
> >> >         cpsw_intr_disable(priv);
> >> >         cpdma_ctlr_int_ctrl(priv->dma, false);
> >> > -       cpsw_interrupt(ndev->irq, priv);
> >> > +       cpsw_rx_interrupt(priv->irq[1], priv);
> >> > +       cpsw_tx_interrupt(priv->irq[2], priv);
> >> >         cpdma_ctlr_int_ctrl(priv->dma, true);
> >> >         cpsw_intr_enable(priv);
> >> >  }
> >> > @@ -2351,7 +2370,7 @@ static int cpsw_probe(struct platform_device *pdev)
> >> >                 goto clean_ale_ret;
> >> >
> >> >         priv->irqs_table[0] = irq;
> >> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
> >> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_dummy_interrupt,
> >> >                         0, dev_name(&pdev->dev), priv);
> >> >         if (ret < 0) {
> >> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
> >> > @@ -2363,7 +2382,7 @@ static int cpsw_probe(struct platform_device *pdev)
> >> >                 goto clean_ale_ret;
> >> >
> >> >         priv->irqs_table[1] = irq;
> >> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
> >> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_rx_interrupt,
> >> >                         0, dev_name(&pdev->dev), priv);
> >> >         if (ret < 0) {
> >> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
> >> > @@ -2375,7 +2394,7 @@ static int cpsw_probe(struct platform_device *pdev)
> >> >                 goto clean_ale_ret;
> >> >
> >> >         priv->irqs_table[2] = irq;
> >> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
> >> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_tx_interrupt,
> >> >                         0, dev_name(&pdev->dev), priv);
> >> >         if (ret < 0) {
> >> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
> >> > @@ -2387,7 +2406,7 @@ static int cpsw_probe(struct platform_device *pdev)
> >> >                 goto clean_ale_ret;
> >> >
> >> >         priv->irqs_table[3] = irq;
> >> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
> >> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_dummy_interrupt,
> >> >                         0, dev_name(&pdev->dev), priv);
> >> >         if (ret < 0) {
> >> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
> >> > --
> >> > 2.2.0
> >> >
> >> > --
> >> > 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
> >>
> >>
> >>
> >> --
> >> Dave Täht
> >>
> >> thttp://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks
> >
> > --
> > balbi
> 
> 
> 
> -- 
> Dave Täht
> 
> thttp://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/4] net: cpsw: fix hangs and improve IRQ handling

[David Miller]

From: Felipe Balbi <balbi@ti.com>
Date: Fri, 2 Jan 2015 12:10:24 -0600

> In any case, patch 1 should go in during the -rc an get backported
> all the way back to v3.9, while the other patches can (should) be
> delayed for v3.20 merge window.

If that's what you want, the way you submitted these changes it not
the correct way to do it.

You should instead submit patch #1 all by itself, correctly targetting
'net'.

Then, after the next time I merge 'net' into 'net-next', you can submit
the rest of the changes.

Re: [PATCH 0/4] net: cpsw: fix hangs and improve IRQ handling

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 716 bytes --]

On Fri, Jan 02, 2015 at 04:45:36PM -0500, David Miller wrote:
> From: Felipe Balbi <balbi@ti.com>
> Date: Fri, 2 Jan 2015 12:10:24 -0600
> 
> > In any case, patch 1 should go in during the -rc an get backported
> > all the way back to v3.9, while the other patches can (should) be
> > delayed for v3.20 merge window.
> 
> If that's what you want, the way you submitted these changes it not
> the correct way to do it.
> 
> You should instead submit patch #1 all by itself, correctly targetting
> 'net'.
> 
> Then, after the next time I merge 'net' into 'net-next', you can submit
> the rest of the changes.

then take patch 1 and I'll resend the other in a couple weeks, no
problem.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/4] net: cpsw: fix hangs and improve IRQ handling

[David Miller]

From: Felipe Balbi <balbi@ti.com>
Date: Fri, 2 Jan 2015 15:53:35 -0600

> On Fri, Jan 02, 2015 at 04:45:36PM -0500, David Miller wrote:
>> You should instead submit patch #1 all by itself, correctly targetting
>> 'net'.
>> 
>> Then, after the next time I merge 'net' into 'net-next', you can submit
>> the rest of the changes.
> 
> then take patch 1 and I'll resend the other in a couple weeks, no
> problem.

Please resubmit the patches in the proper manner.

Re: [PATCH 3/4] net: ethernet: cpsw: split out IRQ handler

[Dave Taht]

On Fri, Jan 2, 2015 at 11:03 AM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> (please use reply-all to keep mailing lists in Cc, also avoid
> top-posting)

I am trying not to read netdev right now... and failing, obviously.

>
> On Fri, Jan 02, 2015 at 10:58:29AM -0800, Dave Taht wrote:
>> The beaglebone only has a 100mbit phy, so you aren't going to get more
>> than that.
>
> very true :-) Still, with AM437x SK which is definitely GigE, I'm
> getting 201Mbits/sec.
>
>> (so do a lot of IoT devices).
>>
>> So you have the two patches that went by on BQL and on NAPI for the beagle?
>
> no, got any pointers ?

the relevant thread was "am335x: cpsw: phy ignores max-speed setting"

and the initial very small BQL enablement patch was here:

https://patchwork.ozlabs.org/patch/407640/

(it needed a saner treatment of a failure to dma something in
cpsw_tx_packet_submit  - the patch as is has also been part of nelsons
trees for the beaglebone for a while)

But it was rightly pointed out later in the thread that this change

+#define CPSW_POLL_WEIGHT 16

made for the biggest part of the improvement, and someone else on the
thread proposed handling that more dynamically for 100mbit phys with
another patch (that I can't find at the moment)

... but the root cause of the excessive latency in this driver was the
single tx/rx dma queue, which you are addressing  in your patch set.
So if you glop on more of the above, mo better, perhaps you will win
bigger.

I will try to slice out some time to boot up a beagle on net-next next week.

>> On Fri, Jan 2, 2015 at 10:55 AM, Felipe Balbi <balbi@ti.com> wrote:
>> > Hi,
>> >
>> > On Fri, Jan 02, 2015 at 10:49:49AM -0800, Dave Taht wrote:
>> >> +1.
>> >>
>> >> We'd had a thread on netdev (can't find it now) where we discussed
>> >> adding BQL support and also something saner for the NAPI handling to
>> >> this driver.
>> >
>> > yeah, currently is completely borked. I'm on a gigabit network and I'm
>> > getting 94Mbits/sec, total crap.
>> >
>> >> Initial results for the beaglebone black were pretty spectacular, and
>> >> it does look like this is way cleaner infrastructure underneat th deal
>> >> with. Are you testing
>> >
>> > cool, if I new more about networking I'd certainly help, but I can help
>> > testing for sure, just keep me in Cc ;-)
>> >
>> >> on the beaglebone black.? do you remember that convo?
>> >
>> > yeah, testing on beagleboneblack and AM437x SK.
>> >
>> > cheers
>> >
>> >> On Fri, Jan 2, 2015 at 10:10 AM, Felipe Balbi <balbi@ti.com> wrote:
>> >> > Now we can introduce dedicated IRQ handlers
>> >> > for each of the IRQ events. This helps with
>> >> > cleaning up a little bit of the clutter in
>> >> > cpsw_interrupt() while also making sure that
>> >> > TX IRQs will try to handle TX buffers while
>> >> > RX IRQs will try to handle RX buffers.
>> >> >
>> >> > Signed-off-by: Felipe Balbi <balbi@ti.com>
>> >> > ---
>> >> >  drivers/net/ethernet/ti/cpsw.c | 41 ++++++++++++++++++++++++++++++-----------
>> >> >  1 file changed, 30 insertions(+), 11 deletions(-)
>> >> >
>> >> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>> >> > index 6e04128..c9081bd 100644
>> >> > --- a/drivers/net/ethernet/ti/cpsw.c
>> >> > +++ b/drivers/net/ethernet/ti/cpsw.c
>> >> > @@ -754,18 +754,36 @@ requeue:
>> >> >                 dev_kfree_skb_any(new_skb);
>> >> >  }
>> >> >
>> >> > -static irqreturn_t cpsw_interrupt(int irq, void *dev_id)
>> >> > +static irqreturn_t cpsw_dummy_interrupt(int irq, void *dev_id)
>> >> >  {
>> >> >         struct cpsw_priv *priv = dev_id;
>> >> >         int value = irq - priv->irqs_table[0];
>> >> >
>> >> > -       /* NOTICE: Ending IRQ here. The trick with the 'value' variable above
>> >> > -        * is to make sure we will always write the correct value to the EOI
>> >> > -        * register. Namely 0 for RX_THRESH Interrupt, 1 for RX Interrupt, 2
>> >> > -        * for TX Interrupt and 3 for MISC Interrupt.
>> >> > -        */
>> >> >         cpdma_ctlr_eoi(priv->dma, value);
>> >> >
>> >> > +       return IRQ_HANDLED;
>> >> > +}
>> >> > +
>> >> > +static irqreturn_t cpsw_tx_interrupt(int irq, void *dev_id)
>> >> > +{
>> >> > +       struct cpsw_priv *priv = dev_id;
>> >> > +
>> >> > +       cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_TX);
>> >> > +       cpdma_chan_process(priv->txch, 128);
>> >> > +
>> >> > +       priv = cpsw_get_slave_priv(priv, 1);
>> >> > +       if (priv)
>> >> > +               cpdma_chan_process(priv->txch, 128);
>> >> > +
>> >> > +       return IRQ_HANDLED;
>> >> > +}
>> >> > +
>> >> > +static irqreturn_t cpsw_rx_interrupt(int irq, void *dev_id)
>> >> > +{
>> >> > +       struct cpsw_priv *priv = dev_id;
>> >> > +
>> >> > +       cpdma_ctlr_eoi(priv->dma, CPDMA_EOI_RX);
>> >> > +
>> >> >         cpsw_intr_disable(priv);
>> >> >         if (priv->irq_enabled == true) {
>> >> >                 cpsw_disable_irq(priv);
>> >> > @@ -1617,7 +1635,8 @@ static void cpsw_ndo_poll_controller(struct net_device *ndev)
>> >> >
>> >> >         cpsw_intr_disable(priv);
>> >> >         cpdma_ctlr_int_ctrl(priv->dma, false);
>> >> > -       cpsw_interrupt(ndev->irq, priv);
>> >> > +       cpsw_rx_interrupt(priv->irq[1], priv);
>> >> > +       cpsw_tx_interrupt(priv->irq[2], priv);
>> >> >         cpdma_ctlr_int_ctrl(priv->dma, true);
>> >> >         cpsw_intr_enable(priv);
>> >> >  }
>> >> > @@ -2351,7 +2370,7 @@ static int cpsw_probe(struct platform_device *pdev)
>> >> >                 goto clean_ale_ret;
>> >> >
>> >> >         priv->irqs_table[0] = irq;
>> >> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
>> >> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_dummy_interrupt,
>> >> >                         0, dev_name(&pdev->dev), priv);
>> >> >         if (ret < 0) {
>> >> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
>> >> > @@ -2363,7 +2382,7 @@ static int cpsw_probe(struct platform_device *pdev)
>> >> >                 goto clean_ale_ret;
>> >> >
>> >> >         priv->irqs_table[1] = irq;
>> >> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
>> >> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_rx_interrupt,
>> >> >                         0, dev_name(&pdev->dev), priv);
>> >> >         if (ret < 0) {
>> >> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
>> >> > @@ -2375,7 +2394,7 @@ static int cpsw_probe(struct platform_device *pdev)
>> >> >                 goto clean_ale_ret;
>> >> >
>> >> >         priv->irqs_table[2] = irq;
>> >> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
>> >> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_tx_interrupt,
>> >> >                         0, dev_name(&pdev->dev), priv);
>> >> >         if (ret < 0) {
>> >> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
>> >> > @@ -2387,7 +2406,7 @@ static int cpsw_probe(struct platform_device *pdev)
>> >> >                 goto clean_ale_ret;
>> >> >
>> >> >         priv->irqs_table[3] = irq;
>> >> > -       ret = devm_request_irq(&pdev->dev, irq, cpsw_interrupt,
>> >> > +       ret = devm_request_irq(&pdev->dev, irq, cpsw_dummy_interrupt,
>> >> >                         0, dev_name(&pdev->dev), priv);
>> >> >         if (ret < 0) {
>> >> >                 dev_err(priv->dev, "error attaching irq (%d)\n", ret);
>> >> > --
>> >> > 2.2.0
>> >> >
>> >> > --
>> >> > 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
>> >>
>> >>
>> >>
>> >> --
>> >> Dave Täht
>> >>
>> >> thttp://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks
>> >
>> > --
>> > balbi
>>
>>
>>
>> --
>> Dave Täht
>>
>> thttp://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks
>
> --
> balbi



-- 
Dave Täht

thttp://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks

Re: [PATCH 3/4] net: ethernet: cpsw: split out IRQ handler

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2056 bytes --]

Hi,

On Fri, Jan 02, 2015 at 02:56:36PM -0800, Dave Taht wrote:
> On Fri, Jan 2, 2015 at 11:03 AM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > (please use reply-all to keep mailing lists in Cc, also avoid
> > top-posting)
> 
> I am trying not to read netdev right now... and failing, obviously.

oops :-)

> > On Fri, Jan 02, 2015 at 10:58:29AM -0800, Dave Taht wrote:
> >> The beaglebone only has a 100mbit phy, so you aren't going to get more
> >> than that.
> >
> > very true :-) Still, with AM437x SK which is definitely GigE, I'm
> > getting 201Mbits/sec.
> >
> >> (so do a lot of IoT devices).
> >>
> >> So you have the two patches that went by on BQL and on NAPI for the beagle?
> >
> > no, got any pointers ?
> 
> the relevant thread was "am335x: cpsw: phy ignores max-speed setting"
> 
> and the initial very small BQL enablement patch was here:
> 
> https://patchwork.ozlabs.org/patch/407640/

I'll test it out, sure.

> (it needed a saner treatment of a failure to dma something in
> cpsw_tx_packet_submit  - the patch as is has also been part of nelsons
> trees for the beaglebone for a while)
> 
> But it was rightly pointed out later in the thread that this change
> 
> +#define CPSW_POLL_WEIGHT 16
> 
> made for the biggest part of the improvement, and someone else on the
> thread proposed handling that more dynamically for 100mbit phys with
> another patch (that I can't find at the moment)
> 
> ... but the root cause of the excessive latency in this driver was the
> single tx/rx dma queue, which you are addressing  in your patch set.

I still think there's a lot of work pending for CPSW, the think slows to
a crawl and takes a lot of CPU for something that should be mostly
handled by DMA. I can very easily get 85% CPU usage with iperf.

> So if you glop on more of the above, mo better, perhaps you will win
> bigger.
> 
> I will try to slice out some time to boot up a beagle on net-next next week.

my patches aren't applied yet, however.

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]