[Cerowrt-devel] Fwd: perf and BQL on the linksys 1200ac.

[Cerowrt-devel] Fwd: perf and BQL on the linksys 1200ac.

[Dave Taht]

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

this was the last patch set I saw. I seem to recall it or something
like it was reviewed later on netdev.

---------- Forwarded message ----------
From: Marcin Wojtas <mw@semihalf.com>
Date: Sun, Jun 19, 2016 at 5:30 AM
Subject: Re: perf and BQL on the linksys 1200ac.
To: Dave Taht <dave.taht@gmail.com>
Cc: Imre Kaloz <kaloz@openwrt.org>, Toke Høiland-Jørgensen
<toke@toke.dk>, Stephen Walker <stephendwalker@gmail.com>, Gregory
Clément <gregory.clement@free-electrons.com>, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>


Hi Dave,

I have some very good news! BQL in mvneta is working fine, the reason
of lock-up was trivial - coalescing threshold set to '1' meant
interrupt per 2 patckets. After setting to '0' and having real
int-per-packet on egress, BQL mechanism can adjust properly.

An easy test:
1. ping without iperf - ~0,5 - 0,7ms
2. ping with 1G TCP iperf from armada38x to host (in both tests @linerate)
WITHOUT BQL - ~1,9ms - 2,6ms
WITH BQL - ~0,65 - 0,85ms

I attach 3 patches (they apply on top of v4.7-rc3, but they should be
ok also for older releases)
- change to int-per-packet
- add bql
- add xmit_more

Can you please test it in your setup? I'm sorry it took so long, but I
hope it will eventually help for your devices. If you don't mind I
have a small request, would it be possible that you run comparison
test with and without my patches, and share results? It would help me
for a local presentation I'm preparing now (of course all tests'
credits whatsoever will be yours).

Best regards,
Marcin

2016-06-09 19:00 GMT+02:00 Marcin Wojtas <mw@semihalf.com>:
> Ok, I'll dig it in my repo and will describe the problems with TX
> interrupts that never hit.
>
> Best regards,
> Marcin
>
> 2016-06-09 18:57 GMT+02:00 Dave Taht <dave.taht@gmail.com>:
>> If you can make your lastest patches attempt available I will try to
>> find someone to pick up the slack.
>>
>> On Thu, Jun 9, 2016 at 6:27 AM, Marcin Wojtas <mw@semihalf.com> wrote:
>>> Hi Dave,
>>>
>>> Unfortunately yes - the stall seems to be permanent, I've been
>>> debugging Marvell new ARMv8 SoC's for past 5 months (mvneta as well),
>>> but no slot for BQL debug and my other mainline patches in short
>>> perspective from now.
>>>
>>> Best regards,
>>> Marcin
>>>
>>> 2016-06-07 19:27 GMT+02:00 Dave Taht <dave.taht@gmail.com>:
>>>> I imagine that your work on bql for the mvneta stalled out?
>>>>
>>>> https://lists.bufferbloat.net/pipermail/cake/2016-June/002031.html
>>>>
>>>> On Thu, Dec 17, 2015 at 7:34 AM, Marcin Wojtas <mw@semihalf.com> wrote:
>>>>> Hi Dave,
>>>>>
>>>>> Thanks for so much details.
>>>>>
>>>>>
>>>>>>
>>>>>> (that said, BQL is much better than what we had before - and what the
>>>>>> mvneta has now... so I am very enthusiastic about it getting in there
>>>>>> and promise to help.... after the new year. )
>>>>>>
>>>>>
>>>>> I hope it to be fixed and submitted by then:)
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>> given the much higher level of indirection and layering in the mvneta
>>>>>>>> driver, finding that looks hard, and I think making napi work right
>>>>>>>> first and simplifying things might be a way forward..
>>>>>>>
>>>>>>> Thanks for this point, I will take it into consideration. IMO both
>>>>>>> ingress and egress paths may seem a bit complcated at first but in
>>>>>>> fact they're not - or I know it too well and I'm not objective:)
>>>>>>
>>>>>> Well. Good. :) I don't know 'em at all, I only started playing with
>>>>>> this hardware last month in light of the turris omnia thing. I am
>>>>>> LOVING the speed of it, it's a huge jump from the mips based stuff I
>>>>>> was working with before.
>>>>>>
>>>>>> A naive question - do the rx and tx paths have to be cleaned up at the
>>>>>> same time? Does the softirq bounce or simultaneously exist for both
>>>>>> cpus?
>>>>>>
>>>>>>
>>>>>
>>>>> Of course timer-based processing for TX done in independent context.
>>>>> With new percpu irqs and XPS + RSS (with still one RX queue it's a
>>>>> preparation) commits (check out net-next), the mapping is strict and
>>>>> with per-cpu napi everything is done simultaneously on all cpus (2 on
>>>>> armada 38x and up to 4 on armada xp).
>>>>>
>>>>> As there is a common line for RXTX interrupt, rx receiving and tx
>>>>> cleaning has to be mixed in mvneta_poll and it all depends on detected
>>>>> interrupt cause.
>>>>>
>>>>> Best regards,
>>>>> Marcin
>>>>
>>>>
>>>>
>>>> --
>>>> Dave Täht
>>>> Let's go make home routers and wifi faster! With better software!
>>>> http://blog.cerowrt.org
>>
>>
>>
>> --
>> Dave Täht
>> Let's go make home routers and wifi faster! With better software!
>> http://blog.cerowrt.org


-- 
Dave Täht
Let's go make home routers and wifi faster! With better software!
http://blog.cerowrt.org

[-- Attachment #2: 0001-net-mvneta-add-BQL-support.patch --]
[-- Type: application/octet-stream, Size: 2987 bytes --]

From 86cf8044d4454ba2a3bebe0b3ea0a82b4549660d Mon Sep 17 00:00:00 2001
From: Marcin Wojtas <mw@semihalf.com>
Date: Fri, 4 Dec 2015 10:40:32 +0100
Subject: [PATCH 1/3] net: mvneta: add BQL support

This commit adds byte queue limit mechanism for the mvneta driver.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index a6d26d3..cf04c97 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -1717,8 +1717,10 @@ static struct mvneta_tx_queue *mvneta_tx_done_policy(struct mvneta_port *pp,
 
 /* Free tx queue skbuffs */
 static void mvneta_txq_bufs_free(struct mvneta_port *pp,
-				 struct mvneta_tx_queue *txq, int num)
+				 struct mvneta_tx_queue *txq, int num,
+				 struct netdev_queue *nq)
 {
+	unsigned int bytes_compl = 0, pkts_compl = 0;
 	int i;
 
 	for (i = 0; i < num; i++) {
@@ -1726,6 +1728,11 @@ static void mvneta_txq_bufs_free(struct mvneta_port *pp,
 			txq->txq_get_index;
 		struct sk_buff *skb = txq->tx_skb[txq->txq_get_index];
 
+		if (skb) {
+			bytes_compl += skb->len;
+			pkts_compl++;
+		}
+
 		mvneta_txq_inc_get(txq);
 
 		if (!IS_TSO_HEADER(txq, tx_desc->buf_phys_addr))
@@ -1736,6 +1743,8 @@ static void mvneta_txq_bufs_free(struct mvneta_port *pp,
 			continue;
 		dev_kfree_skb_any(skb);
 	}
+
+	netdev_tx_completed_queue(nq, pkts_compl, bytes_compl);
 }
 
 /* Handle end of transmission */
@@ -1749,7 +1758,7 @@ static void mvneta_txq_done(struct mvneta_port *pp,
 	if (!tx_done)
 		return;
 
-	mvneta_txq_bufs_free(pp, txq, tx_done);
+	mvneta_txq_bufs_free(pp, txq, tx_done, nq);
 
 	txq->count -= tx_done;
 
@@ -2356,6 +2365,8 @@ out:
 		struct mvneta_pcpu_stats *stats = this_cpu_ptr(pp->stats);
 		struct netdev_queue *nq = netdev_get_tx_queue(dev, txq_id);
 
+		netdev_tx_sent_queue(nq, len);
+
 		txq->count += frags;
 		mvneta_txq_pend_desc_add(pp, txq, frags);
 
@@ -2380,9 +2391,10 @@ static void mvneta_txq_done_force(struct mvneta_port *pp,
 				  struct mvneta_tx_queue *txq)
 
 {
+	struct netdev_queue *nq = netdev_get_tx_queue(pp->dev, txq->id);
 	int tx_done = txq->count;
 
-	mvneta_txq_bufs_free(pp, txq, tx_done);
+	mvneta_txq_bufs_free(pp, txq, tx_done, nq);
 
 	/* reset txq */
 	txq->count = 0;
@@ -2879,6 +2891,8 @@ static int mvneta_txq_init(struct mvneta_port *pp,
 static void mvneta_txq_deinit(struct mvneta_port *pp,
 			      struct mvneta_tx_queue *txq)
 {
+	struct netdev_queue *nq = netdev_get_tx_queue(pp->dev, txq->id);
+
 	kfree(txq->tx_skb);
 
 	if (txq->tso_hdrs)
@@ -2890,6 +2904,8 @@ static void mvneta_txq_deinit(struct mvneta_port *pp,
 				  txq->size * MVNETA_DESC_ALIGNED_SIZE,
 				  txq->descs, txq->descs_phys);
 
+	netdev_tx_reset_queue(nq);
+
 	txq->descs             = NULL;
 	txq->last_desc         = 0;
 	txq->next_desc_to_proc = 0;
-- 
1.8.3.1


[-- Attachment #3: 0002-net-mvneta-TX_DONE-interrupt-per-packet.patch --]
[-- Type: application/octet-stream, Size: 1074 bytes --]

From 1ae1a136bd53112d8b8a237149712559dd7c68ec Mon Sep 17 00:00:00 2001
From: Dmitri Epshtein <dima@marvell.com>
Date: Sun, 22 May 2016 09:41:32 +0300
Subject: [PATCH 2/3] net: mvneta: TX_DONE interrupt per packet

- when MVNETA_TXDONE_COAL_PKTS is 1
  -> interrupt per two transmitted packet

- when MVNETA_TXDONE_COAL_PKTS is 0
  -> interrupt per each transmitted packet

NAS performance strongly depends on the fact that all transmitted packets
must be released.

Signed-off-by: Dmitri Epshtein <dima@marvell.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index cf04c97..0ad2fa3 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -244,7 +244,7 @@
 /* Various constants */
 
 /* Coalescing */
-#define MVNETA_TXDONE_COAL_PKTS		1
+#define MVNETA_TXDONE_COAL_PKTS		0      /* interrupt per packet */
 #define MVNETA_RX_COAL_PKTS		32
 #define MVNETA_RX_COAL_USEC		100
 
-- 
1.8.3.1


[-- Attachment #4: 0003-net-mvneta-add-xmit_more-support.patch --]
[-- Type: application/octet-stream, Size: 1784 bytes --]

From daff2e1dbdaf24d8fa303db2b38a11bf3de8f420 Mon Sep 17 00:00:00 2001
From: Simon Guinot <simon.guinot@sequanux.org>
Date: Fri, 9 Oct 2015 00:09:53 +0200
Subject: [PATCH 3/3] net: mvneta: add xmit_more support

Basing on xmit_more flag of the skb, TX descriptors can be concatenated
before flushing. This commit delay Tx descriptor flush if the queue is
running and if there is more skb's to send.

Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
---
 drivers/net/ethernet/marvell/mvneta.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 0ad2fa3..4cad965 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -512,6 +512,7 @@ struct mvneta_tx_queue {
 	 * descriptor ring
 	 */
 	int count;
+	int pending;
 	int tx_stop_threshold;
 	int tx_wake_threshold;
 
@@ -802,8 +803,9 @@ static void mvneta_txq_pend_desc_add(struct mvneta_port *pp,
 	/* Only 255 descriptors can be added at once ; Assume caller
 	 * process TX desriptors in quanta less than 256
 	 */
-	val = pend_desc;
+	val = pend_desc + txq->pending;
 	mvreg_write(pp, MVNETA_TXQ_UPDATE_REG(txq->id), val);
+	txq->pending = 0;
 }
 
 /* Get pointer to next TX descriptor to be processed (send) by HW */
@@ -2368,11 +2370,14 @@ out:
 		netdev_tx_sent_queue(nq, len);
 
 		txq->count += frags;
-		mvneta_txq_pend_desc_add(pp, txq, frags);
-
 		if (txq->count >= txq->tx_stop_threshold)
 			netif_tx_stop_queue(nq);
 
+		if (!skb->xmit_more || netif_xmit_stopped(nq))
+			mvneta_txq_pend_desc_add(pp, txq, frags);
+		else
+			txq->pending += frags;
+
 		u64_stats_update_begin(&stats->syncp);
 		stats->tx_packets++;
 		stats->tx_bytes  += len;
-- 
1.8.3.1

Re: [Cerowrt-devel] Fwd: perf and BQL on the linksys 1200ac.

[Toke Høiland-Jørgensen]

Dave Taht <dave.taht@gmail.com> writes:

> this was the last patch set I saw. I seem to recall it or something
> like it was reviewed later on netdev.

Looks like it:

https://patchwork.kernel.org/patch/9328415/ and
https://patchwork.kernel.org/patch/9328413/

Seems the xmit_more part needs a v2; guess that's why it's still
pending? :)

-Toke

Re: [Cerowrt-devel] Fwd: perf and BQL on the linksys 1200ac.

[Marcin Wojtas]

Hi Toke,

Yes, I've been overloaded heavily with another projects and this one
is my hobby activity I haven't had time for. I should have some spare
time though in the beginning of December and will send v2.

Best regards,
Marcin

2016-11-20 22:44 GMT+01:00 Toke Høiland-Jørgensen <toke@toke.dk>:
> Dave Taht <dave.taht@gmail.com> writes:
>
>> this was the last patch set I saw. I seem to recall it or something
>> like it was reviewed later on netdev.
>
> Looks like it:
>
> https://patchwork.kernel.org/patch/9328415/ and
> https://patchwork.kernel.org/patch/9328413/
>
> Seems the xmit_more part needs a v2; guess that's why it's still
> pending? :)
>
> -Toke

Re: [Cerowrt-devel] Fwd: perf and BQL on the linksys 1200ac.

[Toke Høiland-Jørgensen]

Marcin Wojtas <mw@semihalf.com> writes:

> Hi Toke,
>
> Yes, I've been overloaded heavily with another projects and this one
> is my hobby activity I haven't had time for. I should have some spare
> time though in the beginning of December and will send v2.

Awesome!

I'm building a LEDE image with the BQL patch; will post it when it's
finished building... And then try to get the patch into LEDE and the
Turris Omnia repo once someone has tested it. Figure the xmit_more patch
can come later, once you get around to fixing that up :)

-Toke

Re: [Cerowrt-devel] Fwd: perf and BQL on the linksys 1200ac.

[Toke Høiland-Jørgensen]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> Marcin Wojtas <mw@semihalf.com> writes:
>
>> Hi Toke,
>>
>> Yes, I've been overloaded heavily with another projects and this one
>> is my hobby activity I haven't had time for. I should have some spare
>> time though in the beginning of December and will send v2.
>
> Awesome!
>
> I'm building a LEDE image with the BQL patch; will post it when it's
> finished building... And then try to get the patch into LEDE and the
> Turris Omnia repo once someone has tested it. Figure the xmit_more patch
> can come later, once you get around to fixing that up :)

WRT1200 image with BQL patch here: https://kau.toke.dk/lede/mvebu/generic/

-Toke

Re: [Cerowrt-devel] Fwd: perf and BQL on the linksys 1200ac.

[Dave Taht]

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

Better, I guess, but still weird. Now two flows get started and a few
more get started sooner in a few more tests than those attached.

Still, during the test I am basically locked out of getting in via a
new ssh until it completes.

I guess the problem here is deeper than this, perhaps higher up the stack?

Also, that it's "good" that it exists on the omnia to get more eyes on
it. Pls file bug there too?

Not so good performance for a NAS though.

On the plus side... cake CAN shape to 900mbit on this platform, only
eating 80% of cpu or so.


On Sun, Nov 20, 2016 at 2:50 PM, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> Toke Høiland-Jørgensen <toke@toke.dk> writes:
>
>> Marcin Wojtas <mw@semihalf.com> writes:
>>
>>> Hi Toke,
>>>
>>> Yes, I've been overloaded heavily with another projects and this one
>>> is my hobby activity I haven't had time for. I should have some spare
>>> time though in the beginning of December and will send v2.
>>
>> Awesome!
>>
>> I'm building a LEDE image with the BQL patch; will post it when it's
>> finished building... And then try to get the patch into LEDE and the
>> Turris Omnia repo once someone has tested it. Figure the xmit_more patch
>> can come later, once you get around to fixing that up :)
>
> WRT1200 image with BQL patch here: https://kau.toke.dk/lede/mvebu/generic/
>
> -Toke



-- 
Dave Täht
Let's go make home routers and wifi faster! With better software!
http://blog.cerowrt.org

[-- Attachment #2: better_but_still_weird.png --]
[-- Type: image/png, Size: 69373 bytes --]

[-- Attachment #3: cake_works_to_900mbit.png --]
[-- Type: image/png, Size: 93431 bytes --]

Re: [Cerowrt-devel] Fwd: perf and BQL on the linksys 1200ac.

[Toke Høiland-Jørgensen]

Dave Taht <dave.taht@gmail.com> writes:

> Better, I guess, but still weird. Now two flows get started and a few
> more get started sooner in a few more tests than those attached.
>
> Still, during the test I am basically locked out of getting in via a
> new ssh until it completes.
>
> I guess the problem here is deeper than this, perhaps higher up the
> stack?

Not a clue. Maybe the switch is to blame?

> Also, that it's "good" that it exists on the omnia to get more eyes on
> it. Pls file bug there too?

They already have it in their nightlies:
https://github.com/CZ-NIC/turris-os/blob/test/target/linux/mvebu/patches-4.4/900-mvnet-BQL.patch

Sent a patch to the LEDE list.

-Toke

Re: [Cerowrt-devel] Fwd: perf and BQL on the linksys 1200ac.

[Toke Høiland-Jørgensen]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> Dave Taht <dave.taht@gmail.com> writes:
>
>> Better, I guess, but still weird. Now two flows get started and a few
>> more get started sooner in a few more tests than those attached.
>>
>> Still, during the test I am basically locked out of getting in via a
>> new ssh until it completes.
>>
>> I guess the problem here is deeper than this, perhaps higher up the
>> stack?
>
> Not a clue. Maybe the switch is to blame?
>
>> Also, that it's "good" that it exists on the omnia to get more eyes on
>> it. Pls file bug there too?
>
> They already have it in their nightlies:
> https://github.com/CZ-NIC/turris-os/blob/test/target/linux/mvebu/patches-4.4/900-mvnet-BQL.patch
>
> Sent a patch to the LEDE list.

Which was already accepted:
https://git.lede-project.org/?p=source.git;a=commit;h=8aa9f6bd71bcfd15e953a0932ed21953ab6d6bbf  :)

-Toke