diff mbox series

[2/7] ath10k: change max RX bundle size from 8 to 32 for sdio

Message ID 1566302108-18219-3-git-send-email-wgong@codeaurora.org (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series ath10k: improve throughout of tcp/udp TX/RX of sdio | expand

Commit Message

Wen Gong Aug. 20, 2019, 11:55 a.m. UTC
The max bundle size support by firmware is 32, change it from 8 to 32
will help performance. This results in significant performance
improvement on RX path.

Tested with QCA6174 SDIO with firmware
WLAN.RMH.4.4.1-00007-QCARMSWP-1.

Signed-off-by: Wen Gong <wgong@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/htc.h  | 2 +-
 drivers/net/wireless/ath/ath10k/sdio.c | 5 +++--
 drivers/net/wireless/ath/ath10k/sdio.h | 4 ++--
 3 files changed, 6 insertions(+), 5 deletions(-)

Comments

Toke Høiland-Jørgensen Aug. 20, 2019, 12:23 p.m. UTC | #1
Wen Gong <wgong@codeaurora.org> writes:

> The max bundle size support by firmware is 32, change it from 8 to 32
> will help performance. This results in significant performance
> improvement on RX path.

What happens when the hardware doesn't have enough data to fill a
bundle? Does it send a smaller one, or does it wait until it can fill
it?

-Toke
Wen Gong Aug. 21, 2019, 6:01 a.m. UTC | #2
> -----Original Message-----
> From: ath10k <ath10k-bounces@lists.infradead.org> On Behalf Of Toke
> Høiland-Jørgensen
> Sent: Tuesday, August 20, 2019 8:23 PM
> To: Wen Gong <wgong@codeaurora.org>; ath10k@lists.infradead.org
> Cc: linux-wireless@vger.kernel.org
> Subject: [EXT] Re: [PATCH 2/7] ath10k: change max RX bundle size from 8 to
> 32 for sdio
> 
> Wen Gong <wgong@codeaurora.org> writes:
> 
> > The max bundle size support by firmware is 32, change it from 8 to 32
> > will help performance. This results in significant performance
> > improvement on RX path.
> 
> What happens when the hardware doesn't have enough data to fill a
> bundle? Does it send a smaller one, or does it wait until it can fill
> it?
> 

The bundle is filled by firmware. 
It will not wait until it can fill it.
For example, if you do ping per second, it will have only 1 ping packet
In the bundle.

> -Toke
> 
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k
Toke Høiland-Jørgensen Aug. 21, 2019, 10:02 a.m. UTC | #3
Wen Gong <wgong@qti.qualcomm.com> writes:

>> -----Original Message-----
>> From: ath10k <ath10k-bounces@lists.infradead.org> On Behalf Of Toke
>> Høiland-Jørgensen
>> Sent: Tuesday, August 20, 2019 8:23 PM
>> To: Wen Gong <wgong@codeaurora.org>; ath10k@lists.infradead.org
>> Cc: linux-wireless@vger.kernel.org
>> Subject: [EXT] Re: [PATCH 2/7] ath10k: change max RX bundle size from 8 to
>> 32 for sdio
>> 
>> Wen Gong <wgong@codeaurora.org> writes:
>> 
>> > The max bundle size support by firmware is 32, change it from 8 to 32
>> > will help performance. This results in significant performance
>> > improvement on RX path.
>> 
>> What happens when the hardware doesn't have enough data to fill a
>> bundle? Does it send a smaller one, or does it wait until it can fill
>> it?
>> 
>
> The bundle is filled by firmware. 
> It will not wait until it can fill it.
> For example, if you do ping per second, it will have only 1 ping packet
> In the bundle.

Right, cool.

-Toke
Nicolas Boichat Aug. 27, 2019, 7:42 a.m. UTC | #4
On Tue, Aug 20, 2019 at 7:55 PM Wen Gong <wgong@codeaurora.org> wrote:
>
> The max bundle size support by firmware is 32, change it from 8 to 32
> will help performance. This results in significant performance
> improvement on RX path.
>
> Tested with QCA6174 SDIO with firmware
> WLAN.RMH.4.4.1-00007-QCARMSWP-1.
>
> Signed-off-by: Wen Gong <wgong@codeaurora.org>
> ---
>  drivers/net/wireless/ath/ath10k/htc.h  | 2 +-
>  drivers/net/wireless/ath/ath10k/sdio.c | 5 +++--
>  drivers/net/wireless/ath/ath10k/sdio.h | 4 ++--
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h
> index f55d3ca..8c79b9e 100644
> --- a/drivers/net/wireless/ath/ath10k/htc.h
> +++ b/drivers/net/wireless/ath/ath10k/htc.h
> @@ -39,7 +39,7 @@
>   * 4-byte aligned.
>   */
>
> -#define HTC_HOST_MAX_MSG_PER_RX_BUNDLE        8
> +#define HTC_HOST_MAX_MSG_PER_RX_BUNDLE        32
>
>  enum ath10k_htc_tx_flags {
>         ATH10K_HTC_FLAG_NEED_CREDIT_UPDATE = 0x01,
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
> index d9395f0..baa6051 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -24,8 +24,8 @@
>  #include "trace.h"
>  #include "sdio.h"
>
> -#define ATH10K_SDIO_DMA_BUF_SIZE       (32 * 1024)
> -#define ATH10K_SDIO_VSG_BUF_SIZE       (32 * 1024)
> +#define ATH10K_SDIO_DMA_BUF_SIZE       (64 * 1024)
> +#define ATH10K_SDIO_VSG_BUF_SIZE       (64 * 1024)
>
>  /* inlined helper functions */
>
> @@ -501,6 +501,7 @@ static int ath10k_sdio_mbox_alloc_bundle(struct ath10k *ar,
>         int ret, i;
>
>         *bndl_cnt = FIELD_GET(ATH10K_HTC_FLAG_BUNDLE_MASK, htc_hdr->flags);
> +       *bndl_cnt += (FIELD_GET(GENMASK(3, 2), htc_hdr->flags) << 4);

GENMASK(3, 2): Please define this macro somewhere.

Also, I'd merge the 2 lines in 1.

>
>         if (*bndl_cnt > HTC_HOST_MAX_MSG_PER_RX_BUNDLE) {
>                 ath10k_warn(ar,
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.h b/drivers/net/wireless/ath/ath10k/sdio.h
> index 4896eca..3ca76c7 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.h
> +++ b/drivers/net/wireless/ath/ath10k/sdio.h
> @@ -89,10 +89,10 @@
>   * to the maximum value (HTC_HOST_MAX_MSG_PER_RX_BUNDLE).
>   *
>   * in this case the driver must allocate
> - * (HTC_HOST_MAX_MSG_PER_RX_BUNDLE * HTC_HOST_MAX_MSG_PER_RX_BUNDLE) skb's.
> + * (HTC_HOST_MAX_MSG_PER_RX_BUNDLE * 2) skb's.
>   */
>  #define ATH10K_SDIO_MAX_RX_MSGS \
> -       (HTC_HOST_MAX_MSG_PER_RX_BUNDLE * HTC_HOST_MAX_MSG_PER_RX_BUNDLE)
> +       (HTC_HOST_MAX_MSG_PER_RX_BUNDLE * 2)
>
>  #define ATH10K_FIFO_TIMEOUT_AND_CHIP_CONTROL   0x00000868u
>  #define ATH10K_FIFO_TIMEOUT_AND_CHIP_CONTROL_DISABLE_SLEEP_OFF 0xFFFEFFFF
> --
> 1.9.1
>
Wen Gong Aug. 28, 2019, 2:31 a.m. UTC | #5
> -----Original Message-----
> From: ath10k <ath10k-bounces@lists.infradead.org> On Behalf Of Nicolas
> Boichat
> Sent: Tuesday, August 27, 2019 3:43 PM
> To: Wen Gong <wgong@codeaurora.org>
> Cc: open list:NETWORKING DRIVERS (WIRELESS) <linux-
> wireless@vger.kernel.org>; ath10k@lists.infradead.org
> Subject: [EXT] Re: [PATCH 2/7] ath10k: change max RX bundle size from 8 to
> 32 for sdio
> 
> > @@ -501,6 +501,7 @@ static int ath10k_sdio_mbox_alloc_bundle(struct
> ath10k *ar,
> >         int ret, i;
> >
> >         *bndl_cnt = FIELD_GET(ATH10K_HTC_FLAG_BUNDLE_MASK, htc_hdr-
> >flags);
> > +       *bndl_cnt += (FIELD_GET(GENMASK(3, 2), htc_hdr->flags) << 4);
> 
> GENMASK(3, 2): Please define this macro somewhere.
> 
> Also, I'd merge the 2 lines in 1.
Patch v2 has sent, https://patchwork.kernel.org/patch/11116677/
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k
Dave Taht Sept. 3, 2019, 4:09 p.m. UTC | #6
In terms of deeply grokking what increasing buffering to achieve high
bandwidth on a testbench, vs what it can do to clobber latency in the
real world at low bandwidths, I tend to point folk at:

https://www.youtube.com/watch?v=Rb-UnHDw02o&t=25m40s

where I got a whole bunch of hackers to stand up and act like packets
in an aggregating FIFO wifi queue.

This key section is only 8 minutes long, and I promise, y'all laugh
at least 3 times at the demonstration.

At the time, also, the ath10k was so overbuffered that on one test
I could try to start 100 flows, and only get five.

https://lwn.net/Articles/705884/

and on my slides:

https://blog.linuxplumbersconf.org/2016/ocw//system/presentations/3963/original/linuxplumbers_wifi_latency-3Nov.pdf


Wen Gong <wgong@codeaurora.org> writes:

> The max bundle size support by firmware is 32, change it from 8 to 32
> will help performance. This results in significant performance
> improvement on RX path.
>
> Tested with QCA6174 SDIO with firmware
> WLAN.RMH.4.4.1-00007-QCARMSWP-1.
>
> Signed-off-by: Wen Gong <wgong@codeaurora.org>
> ---
>  drivers/net/wireless/ath/ath10k/htc.h  | 2 +-
>  drivers/net/wireless/ath/ath10k/sdio.c | 5 +++--
>  drivers/net/wireless/ath/ath10k/sdio.h | 4 ++--
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h
> index f55d3ca..8c79b9e 100644
> --- a/drivers/net/wireless/ath/ath10k/htc.h
> +++ b/drivers/net/wireless/ath/ath10k/htc.h
> @@ -39,7 +39,7 @@
>   * 4-byte aligned.
>   */
>  
> -#define HTC_HOST_MAX_MSG_PER_RX_BUNDLE        8
> +#define HTC_HOST_MAX_MSG_PER_RX_BUNDLE        32
>  
>  enum ath10k_htc_tx_flags {
>  	ATH10K_HTC_FLAG_NEED_CREDIT_UPDATE = 0x01,
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
> index d9395f0..baa6051 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -24,8 +24,8 @@
>  #include "trace.h"
>  #include "sdio.h"
>  
> -#define ATH10K_SDIO_DMA_BUF_SIZE	(32 * 1024)
> -#define ATH10K_SDIO_VSG_BUF_SIZE	(32 * 1024)
> +#define ATH10K_SDIO_DMA_BUF_SIZE	(64 * 1024)
> +#define ATH10K_SDIO_VSG_BUF_SIZE	(64 * 1024)
>  
>  /* inlined helper functions */
>  
> @@ -501,6 +501,7 @@ static int ath10k_sdio_mbox_alloc_bundle(struct ath10k *ar,
>  	int ret, i;
>  
>  	*bndl_cnt = FIELD_GET(ATH10K_HTC_FLAG_BUNDLE_MASK, htc_hdr->flags);
> +	*bndl_cnt += (FIELD_GET(GENMASK(3, 2), htc_hdr->flags) << 4);
>  
>  	if (*bndl_cnt > HTC_HOST_MAX_MSG_PER_RX_BUNDLE) {
>  		ath10k_warn(ar,
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.h b/drivers/net/wireless/ath/ath10k/sdio.h
> index 4896eca..3ca76c7 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.h
> +++ b/drivers/net/wireless/ath/ath10k/sdio.h
> @@ -89,10 +89,10 @@
>   * to the maximum value (HTC_HOST_MAX_MSG_PER_RX_BUNDLE).
>   *
>   * in this case the driver must allocate
> - * (HTC_HOST_MAX_MSG_PER_RX_BUNDLE * HTC_HOST_MAX_MSG_PER_RX_BUNDLE) skb's.
> + * (HTC_HOST_MAX_MSG_PER_RX_BUNDLE * 2) skb's.
>   */
>  #define ATH10K_SDIO_MAX_RX_MSGS \
> -	(HTC_HOST_MAX_MSG_PER_RX_BUNDLE * HTC_HOST_MAX_MSG_PER_RX_BUNDLE)
> +	(HTC_HOST_MAX_MSG_PER_RX_BUNDLE * 2)
>  
>  #define ATH10K_FIFO_TIMEOUT_AND_CHIP_CONTROL   0x00000868u
>  #define ATH10K_FIFO_TIMEOUT_AND_CHIP_CONTROL_DISABLE_SLEEP_OFF 0xFFFEFFFF
Wen Gong Sept. 4, 2019, 4:43 a.m. UTC | #7
> -----Original Message-----
> From: Dave Taht <dave@taht.net>
> Sent: Wednesday, September 4, 2019 12:10 AM
> To: Wen Gong <wgong@qti.qualcomm.com>; ath10k@lists.infradead.org;
> linux-wireless@vger.kernel.org
> Subject: [EXT] Re: [PATCH 2/7] ath10k: change max RX bundle size from 8 to
> 32 for sdio
> 
> 
> In terms of deeply grokking what increasing buffering to achieve high
> bandwidth on a testbench, vs what it can do to clobber latency in the
> real world at low bandwidths, I tend to point folk at:
> 
> https://www.youtube.com/watch?v=Rb-UnHDw02o&t=25m40s
> 
> where I got a whole bunch of hackers to stand up and act like packets
> in an aggregating FIFO wifi queue.
> 
> This key section is only 8 minutes long, and I promise, y'all laugh
> at least 3 times at the demonstration.
> 
> At the time, also, the ath10k was so overbuffered that on one test
> I could try to start 100 flows, and only get five.
> 
> https://lwn.net/Articles/705884/
> 
> and on my slides:
> 
> https://blog.linuxplumbersconf.org/2016/ocw//system/presentations/3963/
> original/linuxplumbers_wifi_latency-3Nov.pdf
> 
Hi Dave,
So your mean is change 8  to 32 will impact latency? It will increase latency of rx?

> 
> 0xFFFEFFFF
Dave Taht Sept. 4, 2019, 1:34 p.m. UTC | #8
Wen Gong <wgong@qti.qualcomm.com> writes:

>> -----Original Message-----
>> From: Dave Taht <dave@taht.net>
>> Sent: Wednesday, September 4, 2019 12:10 AM
>> To: Wen Gong <wgong@qti.qualcomm.com>; ath10k@lists.infradead.org;
>> linux-wireless@vger.kernel.org
>> Subject: [EXT] Re: [PATCH 2/7] ath10k: change max RX bundle size from 8 to
>> 32 for sdio
>> 
>> 
>> In terms of deeply grokking what increasing buffering to achieve high
>> bandwidth on a testbench, vs what it can do to clobber latency in the
>> real world at low bandwidths, I tend to point folk at:
>> 
>> https://www.youtube.com/watch?v=Rb-UnHDw02o&t=25m40s
>> 
>> where I got a whole bunch of hackers to stand up and act like packets
>> in an aggregating FIFO wifi queue.
>> 
>> This key section is only 8 minutes long, and I promise, y'all laugh
>> at least 3 times at the demonstration.
>> 
>> At the time, also, the ath10k was so overbuffered that on one test
>> I could try to start 100 flows, and only get five.
>> 
>> https://lwn.net/Articles/705884/
>> 
>> and on my slides:
>> 
>> https://blog.linuxplumbersconf.org/2016/ocw//system/presentations/3963/
>> original/linuxplumbers_wifi_latency-3Nov.pdf
>> 
> Hi Dave,
> So your mean is change 8  to 32 will impact latency? It will increase latency of rx?

Heh. for rx, in this case, probably not!

I just get twitchy every time folk fiddle with buffer sizes. In one
recent case someone had fiddled with the interrupt polling interval on
something, going from 1ms to 10ms - it saved on cpu, but...
... just trying to make sure folk grok the tradoffs with a bit of
laughter.

carry on!

>
>> 
>> 0xFFFEFFFF
Wen Gong Sept. 5, 2019, 10:12 a.m. UTC | #9
> -----Original Message-----
> From: Dave Taht <dave@taht.net>
> Sent: Wednesday, September 4, 2019 9:34 PM
> To: Wen Gong <wgong@qti.qualcomm.com>
> Cc: ath10k@lists.infradead.org; linux-wireless@vger.kernel.org
> Subject: [EXT] Re: [PATCH 2/7] ath10k: change max RX bundle size from 8 to
> 32 for sdio
> 
> > Hi Dave,
> > So your mean is change 8  to 32 will impact latency? It will increase latency
> of rx?
> 
> Heh. for rx, in this case, probably not!
> 
> I just get twitchy every time folk fiddle with buffer sizes. In one
> recent case someone had fiddled with the interrupt polling interval on
> something, going from 1ms to 10ms - it saved on cpu, but...
> ... just trying to make sure folk grok the tradoffs with a bit of
> laughter.
> 
> carry on!
Hah, so you do not have any concern about this patch.
Patch 3 sent, https://patchwork.kernel.org/patch/11122175/
> 
> >
> >>
> >> 0xFFFEFFFF
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h
index f55d3ca..8c79b9e 100644
--- a/drivers/net/wireless/ath/ath10k/htc.h
+++ b/drivers/net/wireless/ath/ath10k/htc.h
@@ -39,7 +39,7 @@ 
  * 4-byte aligned.
  */
 
-#define HTC_HOST_MAX_MSG_PER_RX_BUNDLE        8
+#define HTC_HOST_MAX_MSG_PER_RX_BUNDLE        32
 
 enum ath10k_htc_tx_flags {
 	ATH10K_HTC_FLAG_NEED_CREDIT_UPDATE = 0x01,
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index d9395f0..baa6051 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -24,8 +24,8 @@ 
 #include "trace.h"
 #include "sdio.h"
 
-#define ATH10K_SDIO_DMA_BUF_SIZE	(32 * 1024)
-#define ATH10K_SDIO_VSG_BUF_SIZE	(32 * 1024)
+#define ATH10K_SDIO_DMA_BUF_SIZE	(64 * 1024)
+#define ATH10K_SDIO_VSG_BUF_SIZE	(64 * 1024)
 
 /* inlined helper functions */
 
@@ -501,6 +501,7 @@  static int ath10k_sdio_mbox_alloc_bundle(struct ath10k *ar,
 	int ret, i;
 
 	*bndl_cnt = FIELD_GET(ATH10K_HTC_FLAG_BUNDLE_MASK, htc_hdr->flags);
+	*bndl_cnt += (FIELD_GET(GENMASK(3, 2), htc_hdr->flags) << 4);
 
 	if (*bndl_cnt > HTC_HOST_MAX_MSG_PER_RX_BUNDLE) {
 		ath10k_warn(ar,
diff --git a/drivers/net/wireless/ath/ath10k/sdio.h b/drivers/net/wireless/ath/ath10k/sdio.h
index 4896eca..3ca76c7 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.h
+++ b/drivers/net/wireless/ath/ath10k/sdio.h
@@ -89,10 +89,10 @@ 
  * to the maximum value (HTC_HOST_MAX_MSG_PER_RX_BUNDLE).
  *
  * in this case the driver must allocate
- * (HTC_HOST_MAX_MSG_PER_RX_BUNDLE * HTC_HOST_MAX_MSG_PER_RX_BUNDLE) skb's.
+ * (HTC_HOST_MAX_MSG_PER_RX_BUNDLE * 2) skb's.
  */
 #define ATH10K_SDIO_MAX_RX_MSGS \
-	(HTC_HOST_MAX_MSG_PER_RX_BUNDLE * HTC_HOST_MAX_MSG_PER_RX_BUNDLE)
+	(HTC_HOST_MAX_MSG_PER_RX_BUNDLE * 2)
 
 #define ATH10K_FIFO_TIMEOUT_AND_CHIP_CONTROL   0x00000868u
 #define ATH10K_FIFO_TIMEOUT_AND_CHIP_CONTROL_DISABLE_SLEEP_OFF 0xFFFEFFFF