| 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 |
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
> -----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
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
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 >
> -----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
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
> -----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
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
> -----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 --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
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(-)