| Message ID | 1569402639-31720-3-git-send-email-wgong@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ath10k: improve throughout of 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. > > Tested with QCA6174 SDIO with firmware > WLAN.RMH.4.4.1-00017-QCARMSWPZ-1 > > Signed-off-by: Wen Gong <wgong@codeaurora.org> > --- > drivers/net/wireless/ath/ath10k/htc.h | 12 +++++++++--- > drivers/net/wireless/ath/ath10k/sdio.c | 4 ++-- > drivers/net/wireless/ath/ath10k/sdio.h | 4 ++-- > 3 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h > index f55d3ca..7055156 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 So how do I know that this change doesn't break any other hardware? I did a quick review and I think it's safe, but the commit log mentions nothing about this. Please clarify and I can update the commit log.
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-00017-QCARMSWPZ-1 > > Signed-off-by: Wen Gong <wgong@codeaurora.org> [...] > --- a/drivers/net/wireless/ath/ath10k/sdio.c > +++ b/drivers/net/wireless/ath/ath10k/sdio.c > @@ -24,7 +24,7 @@ > #include "trace.h" > #include "sdio.h" > > -#define ATH10K_SDIO_VSG_BUF_SIZE (32 * 1024) > +#define ATH10K_SDIO_VSG_BUF_SIZE (64 * 1024) Is allocating 64 kb with kmalloc() reliable, especially on smaller systems? I hope it is, but checking if someone else knows better. We only do this only once in probe(), though.
On 2019-10-24 17:25, Kalle Valo wrote: > 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-00017-QCARMSWPZ-1 >> >> Signed-off-by: Wen Gong <wgong@codeaurora.org> >> --- >> drivers/net/wireless/ath/ath10k/htc.h | 12 +++++++++--- >> drivers/net/wireless/ath/ath10k/sdio.c | 4 ++-- >> drivers/net/wireless/ath/ath10k/sdio.h | 4 ++-- >> 3 files changed, 13 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath10k/htc.h >> b/drivers/net/wireless/ath/ath10k/htc.h >> index f55d3ca..7055156 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 > > So how do I know that this change doesn't break any other hardware? I > did a quick review and I think it's safe, but the commit log mentions > nothing about this. the real max rx bundle is decided in ath10k_htc_wait_target. it is the min value of HTC_HOST_MAX_MSG_PER_RX_BUNDLE and the value reported from firmware. htc->max_msgs_per_htc_bundle = min_t(u8, msg->ready_ext.max_msgs_per_htc_bundle, HTC_HOST_MAX_MSG_PER_RX_BUNDLE); > > Please clarify and I can update the commit log.
On 2019-10-24 17:30, Kalle Valo wrote: > 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-00017-QCARMSWPZ-1 >> >> Signed-off-by: Wen Gong <wgong@codeaurora.org> > > [...] > >> --- a/drivers/net/wireless/ath/ath10k/sdio.c >> +++ b/drivers/net/wireless/ath/ath10k/sdio.c >> @@ -24,7 +24,7 @@ >> #include "trace.h" >> #include "sdio.h" >> >> -#define ATH10K_SDIO_VSG_BUF_SIZE (32 * 1024) >> +#define ATH10K_SDIO_VSG_BUF_SIZE (64 * 1024) > > Is allocating 64 kb with kmalloc() reliable, especially on smaller > systems? I hope it is, but checking if someone else knows better. We > only do this only once in probe(), though. rx packet is more than 1500 bytes for performance test, so for 32 packets, 32*1024 is not enough. yes, it is allocated only one time for probe.
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-00017-QCARMSWPZ-1 > > Signed-off-by: Wen Gong <wgong@codeaurora.org> [...] > --- 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, > @@ -48,10 +48,16 @@ enum ath10k_htc_tx_flags { > > enum ath10k_htc_rx_flags { > ATH10K_HTC_FLAGS_RECV_1MORE_BLOCK = 0x01, > - ATH10K_HTC_FLAG_TRAILER_PRESENT = 0x02, > - ATH10K_HTC_FLAG_BUNDLE_MASK = 0xF0 > + ATH10K_HTC_FLAG_TRAILER_PRESENT = 0x02 > }; I left the comma in ATH10K_HTC_FLAG_TRAILER_PRESENT to make the diff cleaner. > +#define ATH10K_HTC_FLAG_BUNDLE_MASK 0xF0 > +#define ATH10K_HTC_BUNDLE_EXTRA_MASK GENMASK(3, 2) > + > +#define ATH10K_HTC_GET_BUNDLE_COUNT(flags) \ > + (FIELD_GET(ATH10K_HTC_FLAG_BUNDLE_MASK, (flags)) + \ > + (FIELD_GET(ATH10K_HTC_BUNDLE_EXTRA_MASK, (flags)) << 4)) I think I asked you about the shift of 4 bits earlier but now I figured it out (I hope) and documented it like this: #define ATH10K_HTC_FLAG_BUNDLE_MASK GENMASK(7,4) /* bits 2-3 are for extra bundle count bits 4-5 */ #define ATH10K_HTC_BUNDLE_EXTRA_MASK GENMASK(3, 2) #define ATH10K_HTC_BUNDLE_EXTRA_SHIFT 4 static inline unsigned int ath10k_htc_get_bundle_count(u8 flags) { unsigned int count, extra_count; count = FIELD_GET(ATH10K_HTC_FLAG_BUNDLE_MASK, flags); extra_count = FIELD_GET(ATH10K_HTC_BUNDLE_EXTRA_MASK, flags) << ATH10K_HTC_BUNDLE_EXTRA_SHIFT; return count + extra_count; } As you can see I also changed the macro to a function, as I prefer C over CPP :) And changed ATH10K_HTC_FLAG_BUNDLE_MASK to use GENMASK(). But this only compiled tested, please do properly test the patches from pending branch and let me know if I broke something: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=afd85ca1b086695cfd26bf484442eaf3bccb6bdd https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=4225b4d50a4f6a1159dc3316d068398f1b5edb57 https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=911e0fc846cfc46fb4ccd1d223cb153681ff05bd
Wen Gong <wgong@codeaurora.org> writes: > On 2019-10-24 17:25, Kalle Valo wrote: >> 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-00017-QCARMSWPZ-1 >>> >>> Signed-off-by: Wen Gong <wgong@codeaurora.org> >>> --- >>> drivers/net/wireless/ath/ath10k/htc.h | 12 +++++++++--- >>> drivers/net/wireless/ath/ath10k/sdio.c | 4 ++-- >>> drivers/net/wireless/ath/ath10k/sdio.h | 4 ++-- >>> 3 files changed, 13 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/net/wireless/ath/ath10k/htc.h >>> b/drivers/net/wireless/ath/ath10k/htc.h >>> index f55d3ca..7055156 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 >> >> So how do I know that this change doesn't break any other hardware? I >> did a quick review and I think it's safe, but the commit log mentions >> nothing about this. > > the real max rx bundle is decided in ath10k_htc_wait_target. > it is the min value of HTC_HOST_MAX_MSG_PER_RX_BUNDLE and the value > reported from firmware. > htc->max_msgs_per_htc_bundle = > min_t(u8, msg->ready_ext.max_msgs_per_htc_bundle, > HTC_HOST_MAX_MSG_PER_RX_BUNDLE); And we assume that no other firmware than QCA6174 SDIO uses value bigger than 8? Because if there is a such firmware using, for example, value 9 this might cause a regression. Anyway, I added this comment to the commit log: The real max rx bundle is decided in ath10k_htc_wait_target(), it is the min value of HTC_HOST_MAX_MSG_PER_RX_BUNDLE and the value reported from firmware. So this change shouldn't cause any regressions with other hardware supported by ath10k.
On 2019-10-24 18:05, Kalle Valo wrote: > Wen Gong <wgong@codeaurora.org> writes: > As you can see I also changed the macro to a function, as I prefer C > over CPP :) And changed ATH10K_HTC_FLAG_BUNDLE_MASK to use GENMASK(). > yes. > But this only compiled tested, please do properly test the patches from > pending branch and let me know if I broke something: > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=afd85ca1b086695cfd26bf484442eaf3bccb6bdd > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=4225b4d50a4f6a1159dc3316d068398f1b5edb57 > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=911e0fc846cfc46fb4ccd1d223cb153681ff05bd I will test the 3 patches.
Wen Gong <wgong@codeaurora.org> writes: > On 2019-10-24 18:05, Kalle Valo wrote: >> Wen Gong <wgong@codeaurora.org> writes: > >> As you can see I also changed the macro to a function, as I prefer C >> over CPP :) And changed ATH10K_HTC_FLAG_BUNDLE_MASK to use GENMASK(). >> > yes. >> But this only compiled tested, please do properly test the patches from >> pending branch and let me know if I broke something: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=afd85ca1b086695cfd26bf484442eaf3bccb6bdd >> >> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=4225b4d50a4f6a1159dc3316d068398f1b5edb57 >> >> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=911e0fc846cfc46fb4ccd1d223cb153681ff05bd > > I will test the 3 patches. Did you have a chance to test them? Do note that I did one minor change today: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=28da1fe7a3ffa5c55c52328c21165d9efdf2e94c
On 2019-10-31 17:09, Kalle Valo wrote: >> I will test the 3 patches. > > Did you have a chance to test them? Do note that I did one minor change > today: Kalle, Yes, I will test the 7 patches together later. > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=28da1fe7a3ffa5c55c52328c21165d9efdf2e94c
diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h index f55d3ca..7055156 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, @@ -48,10 +48,16 @@ enum ath10k_htc_tx_flags { enum ath10k_htc_rx_flags { ATH10K_HTC_FLAGS_RECV_1MORE_BLOCK = 0x01, - ATH10K_HTC_FLAG_TRAILER_PRESENT = 0x02, - ATH10K_HTC_FLAG_BUNDLE_MASK = 0xF0 + ATH10K_HTC_FLAG_TRAILER_PRESENT = 0x02 }; +#define ATH10K_HTC_FLAG_BUNDLE_MASK 0xF0 +#define ATH10K_HTC_BUNDLE_EXTRA_MASK GENMASK(3, 2) + +#define ATH10K_HTC_GET_BUNDLE_COUNT(flags) \ + (FIELD_GET(ATH10K_HTC_FLAG_BUNDLE_MASK, (flags)) + \ + (FIELD_GET(ATH10K_HTC_BUNDLE_EXTRA_MASK, (flags)) << 4)) + struct ath10k_htc_hdr { u8 eid; /* @enum ath10k_htc_ep_id */ u8 flags; /* @enum ath10k_htc_tx_flags, ath10k_htc_rx_flags */ diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c index f545626..a510101 100644 --- a/drivers/net/wireless/ath/ath10k/sdio.c +++ b/drivers/net/wireless/ath/ath10k/sdio.c @@ -24,7 +24,7 @@ #include "trace.h" #include "sdio.h" -#define ATH10K_SDIO_VSG_BUF_SIZE (32 * 1024) +#define ATH10K_SDIO_VSG_BUF_SIZE (64 * 1024) /* inlined helper functions */ @@ -494,7 +494,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 = ATH10K_HTC_GET_BUNDLE_COUNT(htc_hdr->flags); 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 8d5b09f..00bd4ca 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-00017-QCARMSWPZ-1 Signed-off-by: Wen Gong <wgong@codeaurora.org> --- drivers/net/wireless/ath/ath10k/htc.h | 12 +++++++++--- drivers/net/wireless/ath/ath10k/sdio.c | 4 ++-- drivers/net/wireless/ath/ath10k/sdio.h | 4 ++-- 3 files changed, 13 insertions(+), 7 deletions(-)