ath10k: increase rx buffer size to 2048

Commit Message

Linus Lüssing Feb. 5, 2020, 7:10 p.m. UTC

From: Linus Lüssing <[e-mail]>

Before, only frames with a maximum size of 1528 bytes could be
transmitted between two 802.11s nodes.

For batman-adv for instance, which adds its own header to each frame,
we typically need an MTU of at least 1532 bytes to be able to transmit
without fragmentation.

This patch now increases the maxmimum frame size from 1528 to 1656
bytes.

Tested with two ath10k devices in 802.11s mode, as well as with
batman-adv on top of 802.11s with forwarding disabled.

Fix originally found and developed by Ben Greear.

Link: https://github.com/greearb/ath10k-ct/issues/89
Link: https://github.com/greearb/ath10k-ct/commit/9e5ab25027e0971fa24ccf93373324c08c4e992d
Cc: Ben Greear <[e-mail]>
Signed-off-by: Linus Lüssing <[e-mail]>
---
 drivers/net/wireless/ath/ath10k/htt.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sven Eckelmann April 1, 2020, 7 a.m. UTC | #1

On Wednesday, 5 February 2020 20:10:43 CEST Linus Lüssing wrote:
> From: Linus Lüssing <[e-mail]>
> 
> Before, only frames with a maximum size of 1528 bytes could be
> transmitted between two 802.11s nodes.
> 
> For batman-adv for instance, which adds its own header to each frame,
> we typically need an MTU of at least 1532 bytes to be able to transmit
> without fragmentation.
> 
> This patch now increases the maxmimum frame size from 1528 to 1656
> bytes.
[...]

@Kalle, I saw that this patch was marked as deferred [1] but I couldn't find 
any mail why it was done so. It seems like this currently creates real world 
problems - so would be nice if you could explain shortly what is currently 
blocking its acceptance.

Kind regards,
	Sven

[1] https://patchwork.kernel.org/patch/11367055/

Sven Eckelmann April 25, 2020, 11:14 a.m. UTC | #2

On Wednesday, 1 April 2020 09:00:49 CEST Sven Eckelmann wrote:
> On Wednesday, 5 February 2020 20:10:43 CEST Linus Lüssing wrote:
> > From: Linus Lüssing <[e-mail]>
> > 
> > Before, only frames with a maximum size of 1528 bytes could be
> > transmitted between two 802.11s nodes.
> > 
> > For batman-adv for instance, which adds its own header to each frame,
> > we typically need an MTU of at least 1532 bytes to be able to transmit
> > without fragmentation.
> > 
> > This patch now increases the maxmimum frame size from 1528 to 1656
> > bytes.
> [...]
> 
> @Kalle, I saw that this patch was marked as deferred [1] but I couldn't find 
> any mail why it was done so. It seems like this currently creates real world 
> problems - so would be nice if you could explain shortly what is currently 
> blocking its acceptance.

Ping?

Kind regards,
	Sven

> [1] https://patchwork.kernel.org/patch/11367055/

Kalle Valo April 28, 2020, 12:01 p.m. UTC | #3

Sven Eckelmann <[e-mail]> writes:

> On Wednesday, 1 April 2020 09:00:49 CEST Sven Eckelmann wrote:
>> On Wednesday, 5 February 2020 20:10:43 CEST Linus Lüssing wrote:
>> > From: Linus Lüssing <[e-mail]>
>> > 
>> > Before, only frames with a maximum size of 1528 bytes could be
>> > transmitted between two 802.11s nodes.
>> > 
>> > For batman-adv for instance, which adds its own header to each frame,
>> > we typically need an MTU of at least 1532 bytes to be able to transmit
>> > without fragmentation.
>> > 
>> > This patch now increases the maxmimum frame size from 1528 to 1656
>> > bytes.
>> [...]
>> 
>> @Kalle, I saw that this patch was marked as deferred [1] but I couldn't find 
>> any mail why it was done so. It seems like this currently creates real world 
>> problems - so would be nice if you could explain shortly what is currently 
>> blocking its acceptance.
>
> Ping?

Sorry for the delay, my plan was to first write some documentation about
different hardware families but haven't managed to do that yet.

My problem with this patch is that I don't know what hardware and
firmware versions were tested, so it needs analysis before I feel safe
to apply it. The ath10k hardware families are very different that even
if a patch works perfectly on one ath10k hardware it could still break
badly on another one.

What makes me faster to apply ath10k patches is to have comprehensive
analysis in the commit log. This shows me the patch author has
considered about all hardware families, not just the one he is testing
on, and that I don't need to do the analysis myself.

Dave Taht April 28, 2020, 12:27 p.m. UTC | #4

On Tue, Apr 28, 2020 at 5:06 AM Kalle Valo <[e-mail]> wrote:
>
> Sven Eckelmann <[e-mail]> writes:
>
> > On Wednesday, 1 April 2020 09:00:49 CEST Sven Eckelmann wrote:
> >> On Wednesday, 5 February 2020 20:10:43 CEST Linus Lüssing wrote:
> >> > From: Linus Lüssing <[e-mail]>
> >> >
> >> > Before, only frames with a maximum size of 1528 bytes could be
> >> > transmitted between two 802.11s nodes.
> >> >
> >> > For batman-adv for instance, which adds its own header to each frame,
> >> > we typically need an MTU of at least 1532 bytes to be able to transmit
> >> > without fragmentation.
> >> >
> >> > This patch now increases the maxmimum frame size from 1528 to 1656
> >> > bytes.
> >> [...]
> >>
> >> @Kalle, I saw that this patch was marked as deferred [1] but I couldn't find
> >> any mail why it was done so. It seems like this currently creates real world
> >> problems - so would be nice if you could explain shortly what is currently
> >> blocking its acceptance.
> >
> > Ping?
>
> Sorry for the delay, my plan was to first write some documentation about
> different hardware families but haven't managed to do that yet.
>
> My problem with this patch is that I don't know what hardware and
> firmware versions were tested, so it needs analysis before I feel safe
> to apply it. The ath10k hardware families are very different that even
> if a patch works perfectly on one ath10k hardware it could still break
> badly on another one.
>
> What makes me faster to apply ath10k patches is to have comprehensive
> analysis in the commit log. This shows me the patch author has
> considered about all hardware families, not just the one he is testing
> on, and that I don't need to do the analysis myself.

I have been struggling to get the ath10k to sing and dance using
various variants
of the firmware, on this bug over here:

https://forum.openwrt.org/t/aql-and-the-ath10k-is-lovely/

The puzzling thing is the loss of bidirectional throughput at codel target 20,
and getting WAY more (but less than I expected) at codel target 5.

This doesn't quite have bearing the size of the rx ring, except that in my
experiments the rx ring is rather small!! and yet I get way more performance
out of it....

(still,  as you'll see from the bug, it's WAY better than it used to be)

is NAPI in this driver? I'm afraid to look.
> --
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Ben Greear April 28, 2020, 2:27 p.m. UTC | #5

On 04/28/2020 05:01 AM, Kalle Valo wrote:
> Sven Eckelmann <[e-mail]> writes:
>
>> On Wednesday, 1 April 2020 09:00:49 CEST Sven Eckelmann wrote:
>>> On Wednesday, 5 February 2020 20:10:43 CEST Linus Lüssing wrote:
>>>> From: Linus Lüssing <[e-mail]>
>>>>
>>>> Before, only frames with a maximum size of 1528 bytes could be
>>>> transmitted between two 802.11s nodes.
>>>>
>>>> For batman-adv for instance, which adds its own header to each frame,
>>>> we typically need an MTU of at least 1532 bytes to be able to transmit
>>>> without fragmentation.
>>>>
>>>> This patch now increases the maxmimum frame size from 1528 to 1656
>>>> bytes.
>>> [...]
>>>
>>> @Kalle, I saw that this patch was marked as deferred [1] but I couldn't find
>>> any mail why it was done so. It seems like this currently creates real world
>>> problems - so would be nice if you could explain shortly what is currently
>>> blocking its acceptance.
>>
>> Ping?
>
> Sorry for the delay, my plan was to first write some documentation about
> different hardware families but haven't managed to do that yet.
>
> My problem with this patch is that I don't know what hardware and
> firmware versions were tested, so it needs analysis before I feel safe
> to apply it. The ath10k hardware families are very different that even
> if a patch works perfectly on one ath10k hardware it could still break
> badly on another one.
>
> What makes me faster to apply ath10k patches is to have comprehensive
> analysis in the commit log. This shows me the patch author has
> considered about all hardware families, not just the one he is testing
> on, and that I don't need to do the analysis myself.

It has been in ath10k-ct for a while, and that has some fairly wide coverage
in OpenWrt, so likely if there were problems we would have seen it already.

I did not make any specific changes to firmware to support this, so upstream
firmware should behave similarly.

Seems like upstream ath10k could really benefit from having some test beds
so you can actually test code on different chips and have confidence
in your changes!

Thanks,
Ben

Kalle Valo Jan. 18, 2021, 4:13 p.m. UTC | #6

Linus Lüssing <[e-mail]> wrote:

> Before, only frames with a maximum size of 1528 bytes could be
> transmitted between two 802.11s nodes.
> 
> For batman-adv for instance, which adds its own header to each frame,
> we typically need an MTU of at least 1532 bytes to be able to transmit
> without fragmentation.
> 
> This patch now increases the maxmimum frame size from 1528 to 1656
> bytes.
> 
> Tested with two ath10k devices in 802.11s mode, as well as with
> batman-adv on top of 802.11s with forwarding disabled.
> 
> Fix originally found and developed by Ben Greear.
> 
> Link: https://github.com/greearb/ath10k-ct/issues/89
> Link: https://github.com/greearb/ath10k-ct/commit/9e5ab25027e0971fa24ccf93373324c08c4e992d
> Cc: Ben Greear <[e-mail]>
> Signed-off-by: Linus Lüssing <[e-mail]>
> Signed-off-by: Kalle Valo <[e-mail]>

Patch applied to ath-next branch of ath.git, thanks.

3e6b9cf534ca ath10k: increase rx buffer size to 2048

Patch

diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h
index 4a12564fc30e..6a2b5e10e568 100644
--- a/drivers/net/wireless/ath/ath10k/htt.h
+++ b/drivers/net/wireless/ath/ath10k/htt.h
@@ -2220,7 +2220,7 @@  struct htt_rx_chan_info {
  * Should be: sizeof(struct htt_host_rx_desc) + max rx MSDU size,
  * rounded up to a cache line size.
  */
-#define HTT_RX_BUF_SIZE 1920
+#define HTT_RX_BUF_SIZE 2048
 #define HTT_RX_MSDU_SIZE (HTT_RX_BUF_SIZE - (int)sizeof(struct htt_rx_desc))
 
 /* Refill a bunch of RX buffers for each refill round so that FW/HW can handle